Re: [PATCH 2/3] block: cap atomic write size by PI buffer size constraints

[John Garry <john.garry@linux.dev>]

On 9/7/26 08:40, Christoph Hellwig wrote:
> As Sashiko points out, limiting the I/O size by the size of the available
> PI buffer can cause inconsistencies for atomic writes.  This also limit
> atomic_write_max_sectors in blk_validate_integrity_limits.
> 
> Fixes: ec7f31b2a2d3 ("block: make bio auto-integrity deadlock safe")
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>   block/blk-settings.c | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/block/blk-settings.c b/block/blk-settings.c
> index 8274631290db..e469baa1f08b 100644
> --- a/block/blk-settings.c
> +++ b/block/blk-settings.c
> @@ -206,6 +206,12 @@ static int blk_validate_integrity_limits(struct queue_limits *lim)
>   	lim->max_sectors = min(lim->max_sectors,
>   		max_integrity_io_size(lim) >> SECTOR_SHIFT);
>   
> +	if (lim->features & BLK_FEAT_ATOMIC_WRITES) {
> +		lim->atomic_write_max_sectors =
> +			min(lim->atomic_write_max_sectors,
> +				max_integrity_io_size(lim) >> SECTOR_SHIFT);
> +	}
> +

A note about the sashiko comment from earlier:
"
The block layer calculates atomic write limits (lim->atomic_write_unit_max)
before blk_validate_integrity_limits() restricts lim->max_sectors. As a
result, the filesystem permits atomic writes up to the unconstrained atomic
limit.
"

atomic writes are not limited by lim->max_sectors, but rather by 
max_hw_sectors. That decision was taken as the atomic limits for a block 
device should be fixed, and not update-able when userspace changes 
max_sectors via sysfs. See this in get_max_io_size():

	if (bio_op(bio) == REQ_OP_WRITE_ZEROES)
		max_sectors = lim->max_write_zeroes_sectors;
	else if (is_atomic)
		max_sectors = lim->atomic_write_max_sectors;
	else
		max_sectors = lim->max_sectors;

Furthermore, I don't think that this is the change above is correct. We 
have 4x atomic limits and I think that 3x would need updating:
- atomic_write_max_sectors
- atomic_write_unit_min
- atomic_write_unit_max

The expectation is that the driver sets atomic_write_hw_max, 
atomic_write_hw_unit_min, and atomic_write_hw_unit_max individually. We 
don't assume that atomic_write_hw_unit_max == 
rounddown_pow_of_two(atomic_write_hw_max). The is because of SCSI and 
it's granularity and alignment atomic limits :(

Anyway, maybe this is a better change:

diff --git a/block/blk-settings.c b/block/blk-settings.c
index 8274631290db..b88744d42719 100644
--- a/block/blk-settings.c
+++ b/block/blk-settings.c
@@ -233,11 +233,15 @@ static void blk_atomic_writes_update_limits(struct 
queue_limits *lim)
  	unsigned int unit_limit = min(lim->max_hw_sectors << SECTOR_SHIFT, 
blk_queue_max_guaranteed_bio(lim));

+	unit_limit = min_not_zero(unit_limit, max_integrity_io_size(lim));
  	unit_limit = rounddown_pow_of_two(unit_limit);

  	lim->atomic_write_max_sectors =
  		min(lim->atomic_write_hw_max >> SECTOR_SHIFT,
  			lim->max_hw_sectors);
+	lim->atomic_write_max_sectors =
+		min_not_zero(lim->atomic_write_max_sectors,
+			max_integrity_io_size(lim));
  	lim->atomic_write_unit_min =
  		min(lim->atomic_write_hw_unit_min, unit_limit);
  	lim->atomic_write_unit_max =
@@ -505,11 +509,12 @@ int blk_validate_limits(struct queue_limits *lim)
  	if (!(lim->features & BLK_FEAT_WRITE_CACHE))
  		lim->features &= ~BLK_FEAT_FUA;

-	blk_validate_atomic_write_limits(lim);

  	err = blk_validate_integrity_limits(lim);
  	if (err)
  		return err;
+
+	blk_validate_atomic_write_limits(lim);
  	return blk_validate_zoned_limits(lim);
  }
  EXPORT_SYMBOL_GPL(blk_validate_limits);

Side note:
I would (naively) assume that if the disk has integrity, then it's 
atomic limits reported would update accordingly and 
lim->atomic_write_hw_max et al would already have any integrity limits 
built in (so that we don't need to do any capping of atomic limits by 
integrity limits).