Re: [PATCH 2/2] dm-crypt: enable DM_TARGET_ATOMIC_WRITES

[Benjamin Marzinski <bmarzins@redhat.com>]

On Wed, Nov 05, 2025 at 04:02:36PM +0100, Mikulas Patocka wrote:
> Allow handling of bios with REQ_ATOMIC flag set.
> 
> Don't split these bios and fail them if they overrun the hard limit
> "BIO_MAX_VECS << PAGE_SHIFT".
> 
> This commit joins the logic that avoids splitting emulated zone append
> bios with atomic write bios.
> 
> Signed-off-by: John Garry <john.g.garry@oracle.com>
> Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> 
> ---
>  drivers/md/dm-crypt.c |   39 ++++++++++++++++++++++++---------------
>  1 file changed, 24 insertions(+), 15 deletions(-)
> 
> Index: linux-2.6/drivers/md/dm-crypt.c
> ===================================================================
> --- linux-2.6.orig/drivers/md/dm-crypt.c	2025-11-05 14:50:28.000000000 +0100
> +++ linux-2.6/drivers/md/dm-crypt.c	2025-11-05 14:50:28.000000000 +0100
> @@ -254,22 +254,15 @@ static unsigned int max_write_size = 0;
>  module_param(max_write_size, uint, 0644);
>  MODULE_PARM_DESC(max_write_size, "Maximum size of a write request");
>  
> -static unsigned get_max_request_sectors(struct dm_target *ti, struct bio *bio)
> +static unsigned get_max_request_sectors(struct dm_target *ti, struct bio *bio, bool no_split)
>  {
>  	struct crypt_config *cc = ti->private;
>  	unsigned val, sector_align;
>  	bool wrt = op_is_write(bio_op(bio));
>  
> -	if (wrt) {
> -		/*
> -		 * For zoned devices, splitting write operations creates the
> -		 * risk of deadlocking queue freeze operations with zone write
> -		 * plugging BIO work when the reminder of a split BIO is
> -		 * issued. So always allow the entire BIO to proceed.
> -		 */
> -		if (ti->emulate_zone_append)
> -			return bio_sectors(bio);
> -
> +	if (no_split) {
> +		val = -1;
> +	} else if (wrt) {
>  		val = min_not_zero(READ_ONCE(max_write_size),
>  				   DM_CRYPT_DEFAULT_MAX_WRITE_SIZE);
>  	} else {
> @@ -3496,6 +3489,7 @@ static int crypt_map(struct dm_target *t
>  	struct dm_crypt_io *io;
>  	struct crypt_config *cc = ti->private;
>  	unsigned max_sectors;
> +	bool no_split;
>  
>  	/*
>  	 * If bio is REQ_PREFLUSH or REQ_OP_DISCARD, just bypass crypt queues.
> @@ -3513,10 +3507,20 @@ static int crypt_map(struct dm_target *t
>  
>  	/*
>  	 * Check if bio is too large, split as needed.
> -	 */
> -	max_sectors = get_max_request_sectors(ti, bio);
> -	if (unlikely(bio_sectors(bio) > max_sectors))
> +	 *
> +	 * For zoned devices, splitting write operations creates the
> +	 * risk of deadlocking queue freeze operations with zone write
> +	 * plugging BIO work when the reminder of a split BIO is
> +	 * issued. So always allow the entire BIO to proceed.
> +	 */
> +	no_split = (ti->emulate_zone_append && op_is_write(bio_op(bio))) ||
> +		   (bio->bi_opf & REQ_ATOMIC);
> +	max_sectors = get_max_request_sectors(ti, bio, no_split);
> +	if (unlikely(bio_sectors(bio) > max_sectors)) {
> +		if (unlikely(no_split))
> +			return DM_MAPIO_KILL;
>  		dm_accept_partial_bio(bio, max_sectors);
> +	}
>  
>  	/*
>  	 * Ensure that bio is a multiple of internal sector encryption size
> @@ -3762,6 +3766,11 @@ static void crypt_io_hints(struct dm_tar
>  	if (ti->emulate_zone_append)
>  		limits->max_hw_sectors = min(limits->max_hw_sectors,
>  					     BIO_MAX_VECS << PAGE_SECTORS_SHIFT);
> +
> +	limits->atomic_write_hw_unit_max = min(limits->atomic_write_hw_unit_max,
> +					       BIO_MAX_VECS << PAGE_SHIFT);
> +	limits->atomic_write_hw_max = min(limits->atomic_write_hw_max,
> +					  BIO_MAX_VECS << PAGE_SHIFT);
>  }

Do we need to cap these limits, instead of just accepting the underlying
device limits? Neither of them are really used for IO.
atomic_write_unit_max, which is used for IO, will already get a capped
value from atomic_write_hw_unit_max in
blk_atomic_writes_update_limits(). And capping atomic_write_hw_max seems
wrong, since atomic_write_hw_max == UINT_MAX is used
blk_validate_atomic_write_limits() to indicate that atomic writes were
never set up, because no underlying device supported them. I don't think
these caps will actually break things, but in my mind they make some
already confusing limits even more confusing. 

Or am I missing some reason why this is needed?

-Ben

>  
>  static struct target_type crypt_target = {
> @@ -3770,7 +3779,7 @@ static struct target_type crypt_target =
>  	.module = THIS_MODULE,
>  	.ctr    = crypt_ctr,
>  	.dtr    = crypt_dtr,
> -	.features = DM_TARGET_ZONED_HM,
> +	.features = DM_TARGET_ZONED_HM | DM_TARGET_ATOMIC_WRITES,
>  	.report_zones = crypt_report_zones,
>  	.map    = crypt_map,
>  	.status = crypt_status,
>