Re: [PATCH v2 2/7] md/raid1: advertise atomic write limits and handle runtime constraints

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Atomic write limits are improperly configured and completely overwritten during limit stacking.
--

commit c2837eff05b0ac2b80653d924f40f1bef7bad6d3
Author: Abd-Alrhman Masalkhi <[email protected]>

md/raid1: advertise atomic write limits and handle runtime constraints

Atomic writes in RAID1 must fit within a single barrier unit. Advertise
this restriction through the queue limits by setting
atomic_write_hw_unit_max to BARRIER_UNIT_SECTOR_SIZE so that bios which
would cross a barrier-unit boundary are rejected by the block layer
before reaching MD.

A bio that passes block-layer validation may still become unserviceable
within RAID1 due to bad blocks or write-behind constraints. In the former
case, complete the bio with EIO. In the latter case, disable
write-behind rather than failing the bio with EIO.

> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> --- a/drivers/md/raid1.c
> +++ b/drivers/md/raid1.c
> @@ -1633,10 +1620,24 @@ static bool raid1_write_request(struct mddev *mddev, struct bio *bio,
[ ... ]
>  	if (max_sectors < bio_sectors(bio)) {
> +		if (atomic) {
> +			bio_io_error(bio);
> +			goto err_dec_pending;
> +		}
> +

[Severity: High]
Will this result in atomic writes dynamically failing with unexpected I/O
errors instead of being properly rejected early by the block layer?

Because the limits configured in raid1_set_limits() appear to get overwritten
during stacking, boundary-crossing bios will bypass block layer validation and
trigger an error dynamically here.

>  		bio = bio_submit_split_bioset(bio, max_sectors,
>  					      &conf->bio_split);
[ ... ]
> @@ -3229,6 +3230,7 @@ static int raid1_set_limits(struct mddev *mddev)
>  	lim.max_write_zeroes_sectors = 0;
>  	lim.max_hw_wzeroes_unmap_sectors = 0;
>  	lim.logical_block_size = mddev->logical_block_size;
> +	lim.atomic_write_hw_unit_max = BARRIER_UNIT_SECTOR_SIZE;
> +	lim.features |= BLK_FEAT_ATOMIC_WRITES;
> +	lim.features |= BLK_FEAT_PCI_P2PDMA;
>  	err = mddev_stack_rdev_limits(mddev, &lim, MDDEV_STACK_INTEGRITY);

[Severity: High]
Is BARRIER_UNIT_SECTOR_SIZE being assigned in sectors while the
atomic_write_hw_unit_max limit expects bytes? If so, this might artificially
cap atomic writes to mismatched units.

Additionally, will these custom RAID1 atomic limits be completely overwritten
when stacking?

Because atomic_write_hw_max is left unmodified from its default of UINT_MAX,
when mddev_stack_rdev_limits() is called:

mddev_stack_rdev_limits() ->
   queue_limits_stack_bdev() ->
      blk_stack_atomic_writes_limits() ->
         blk_stack_atomic_writes_head()

The stacking code will see UINT_MAX and unconditionally overwrite the newly
configured RAID1 limit with the first underlying device's limits.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2