Re: [PATCH v2] dm-crypt: refactor buffer allocation retry handling

[Mikulas Patocka <mpatocka@redhat.com>]

On Tue, 11 Aug 2026, Runyu Xiao wrote:

> crypt_alloc_buffer() conditionally acquires bio_alloc_lock around the
> allocation path and also contains the retry logic for the reclaim
> fallback.
> 
> Move one allocation attempt into crypt_alloc_buffer_try() so that
> bio_alloc_lock is always acquired and released in crypt_alloc_buffer(),
> and the retry decision is made only after the mutex has been dropped.
> 
> This keeps the retry path outside the locked region and makes the
> locking context easier to analyze.
> 
> Signed-off-by: Runyu Xiao <runyu.xiao@seu.edu.cn>

Hi

I wouldn't do this. The patch just moves code around and increases code 
size with no benefit.

Mikulas

> ---
> Changes in v2:
> - Move one allocation attempt into crypt_alloc_buffer_try(), as suggested
>   by Bart Van Assche.
> - Keep bio_alloc_lock acquisition and release in crypt_alloc_buffer(), and
>   make the retry decision only after releasing the lock.
> - Preserve the distinction between a page-pool retry and a final integrity
>   allocation failure.
> 
> v1: https://lore.kernel.org/r/20260809051839.3504080-1-runyu.xiao@seu.edu.cn
> 
> diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
> index 608b617fb817..7c68fad12960 100644
> --- a/drivers/md/dm-crypt.c
> +++ b/drivers/md/dm-crypt.c
> @@ -1627,18 +1627,16 @@ static void crypt_free_buffer_pages(struct crypt_config *cc, struct bio *clone);
>   * In order to reduce allocation overhead, we try to allocate compound pages in
>   * the first pass. If they are not available, we fall back to the mempool.
>   */
> -static struct bio *crypt_alloc_buffer(struct dm_crypt_io *io, unsigned int size)
> +static struct bio *crypt_alloc_buffer_try(struct dm_crypt_io *io,
> +					  unsigned int size, gfp_t gfp_mask,
> +					  unsigned int order, bool *retry)
>  {
>  	struct crypt_config *cc = io->cc;
>  	struct bio *clone;
>  	unsigned int nr_iovecs = (size + PAGE_SIZE - 1) >> PAGE_SHIFT;
> -	gfp_t gfp_mask = GFP_NOWAIT | __GFP_HIGHMEM;
>  	unsigned int remaining_size;
> -	unsigned int order = MAX_PAGE_ORDER;
>  
> -retry:
> -	if (unlikely(gfp_mask & __GFP_DIRECT_RECLAIM))
> -		mutex_lock(&cc->bio_alloc_lock);
> +	*retry = false;
>  
>  	clone = bio_alloc_bioset(cc->dev->bdev, nr_iovecs, io->base_bio->bi_opf,
>  				 GFP_NOIO, &cc->bs);
> @@ -1674,9 +1672,8 @@ static struct bio *crypt_alloc_buffer(struct dm_crypt_io *io, unsigned int size)
>  		if (!pages) {
>  			crypt_free_buffer_pages(cc, clone);
>  			bio_put(clone);
> -			gfp_mask |= __GFP_DIRECT_RECLAIM;
> -			order = 0;
> -			goto retry;
> +			*retry = true;
> +			return NULL;
>  		}
>  
>  have_pages:
> @@ -1692,12 +1689,34 @@ static struct bio *crypt_alloc_buffer(struct dm_crypt_io *io, unsigned int size)
>  		clone = NULL;
>  	}
>  
> -	if (unlikely(gfp_mask & __GFP_DIRECT_RECLAIM))
> -		mutex_unlock(&cc->bio_alloc_lock);
> -
>  	return clone;
>  }
>  
> +static struct bio *crypt_alloc_buffer(struct dm_crypt_io *io, unsigned int size)
> +{
> +	struct crypt_config *cc = io->cc;
> +	struct bio *clone;
> +	gfp_t gfp_mask = GFP_NOWAIT | __GFP_HIGHMEM;
> +	unsigned int order = MAX_PAGE_ORDER;
> +	bool retry;
> +
> +	for (;;) {
> +		if (unlikely(gfp_mask & __GFP_DIRECT_RECLAIM))
> +			mutex_lock(&cc->bio_alloc_lock);
> +
> +		clone = crypt_alloc_buffer_try(io, size, gfp_mask, order, &retry);
> +
> +		if (unlikely(gfp_mask & __GFP_DIRECT_RECLAIM))
> +			mutex_unlock(&cc->bio_alloc_lock);
> +
> +		if (!retry)
> +			return clone;
> +
> +		gfp_mask |= __GFP_DIRECT_RECLAIM;
> +		order = 0;
> +	}
> +}
> +
>  static void crypt_free_buffer_pages(struct crypt_config *cc, struct bio *clone)
>  {
>  	struct folio_iter fi;
> -- 
> 2.34.1
>