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

[Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>]

On Fri, Jul 03, 2026 at 09:28 +0100, John Garry wrote:
> On 30/06/2026 09:39, Abd-Alrhman Masalkhi wrote:
>> 
>> Hi John,
>> 
>> On Mon, Jun 29, 2026 at 15:48 +0100, John Garry wrote:
>>> On 28/06/2026 15:24, Abd-Alrhman Masalkhi wrote:
>>>> 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.
>>>>
>>>> Fixes: f2a38abf5f1c ("md/raid1: Atomic write support")
>>>> Fixes: a4c55c902670 ("md/raid1: simplify raid1_write_request() error handling")
>>>> Signed-off-by: Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>
>>>> ---
>>>> Changes in v2:
>>>>    - Drop the early atomic write split check from raid1_write_request().
>>>>    - Advertise the atomic write size limit via queue limits.
>>>>    - Disable write-behind instead of failing atomic writes when the
>>>>      BIO_MAX_VECS limit is encountered.
>>>>    - Link to v1: https://urldefense.com/v3/__https://lore.kernel.org/linux-raid/20260623072456.333437-3-abd.masalkhi@gmail.com/__;!!ACWV5N9M2RV99hQ!LbMSGSClRi0PNBqQti5ZNWGDVjDd34-7saYEAwNyBNjpNTjEA7veqM5RHG8KB1QiscarW4UaIefjm19ywSImtIgh$
>>>> ---
>>>>    drivers/md/raid1.c | 36 +++++++++++++++++++-----------------
>>>>    1 file changed, 19 insertions(+), 17 deletions(-)
>>>>
>>>> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
>>>> index afe2ca96ad8c..f322048ab3c2 100644
>>>> --- a/drivers/md/raid1.c
>>>> +++ b/drivers/md/raid1.c
>>>> @@ -1522,6 +1522,7 @@ static bool raid1_write_request(struct mddev *mddev, struct bio *bio,
>>>>    	int first_clone;
>>>>    	bool write_behind = false;
>>>>    	bool nowait = bio->bi_opf & REQ_NOWAIT;
>>>> +	bool atomic = bio->bi_opf & REQ_ATOMIC;
>>>>    	bool is_discard = op_is_discard(bio->bi_opf);
>>>>    	sector_t sector = bio->bi_iter.bi_sector;
>>>>    
>>>> @@ -1603,20 +1604,6 @@ static bool raid1_write_request(struct mddev *mddev, struct bio *bio,
>>>>    			}
>>>>    			if (is_bad) {
>>>>    				int good_sectors;
>>>> -
>>>> -				/*
>>>> -				 * We cannot atomically write this, so just
>>>> -				 * error in that case. It could be possible to
>>>> -				 * atomically write other mirrors, but the
>>>> -				 * complexity of supporting that is not worth
>>>> -				 * the benefit.
>>>> -				 */
>>>> -				if (bio->bi_opf & REQ_ATOMIC) {
>>>> -					bio->bi_status = BLK_STS_NOTSUPP;
>>>> -					bio_endio(bio);
>>>> -					goto err_dec_pending;
>>>> -				}
>>>> -
>>>>    				good_sectors = first_bad - sector;
>>>>    				if (good_sectors < max_sectors)
>>>>    					max_sectors = good_sectors;
>>>> @@ -1633,10 +1620,24 @@ static bool raid1_write_request(struct mddev *mddev, struct bio *bio,
>>>>    	 * at a time and thus needs a new bio that can fit the whole payload
>>>>    	 * this bio in page sized chunks.
>>>>    	 */
>>>> -	if (write_behind && mddev->bitmap)
>>>> -		max_sectors = min_t(int, max_sectors,
>>>> -				    BIO_MAX_VECS * (PAGE_SIZE >> 9));
>>>> +	if (write_behind && mddev->bitmap) {
>>>> +		if (atomic && max_sectors > BIO_MAX_VECS * (PAGE_SIZE >> 9))
>>>
>>> where does BIO_MAX_VECS * (PAGE_SIZE >> 9) even come from?
>>>
>> 
>> BIO_MAX_VECS * (PAGE_SIZE >> 9) defines the maximum size supported by
>> write-behind. The write-behind copy (alloc_behind_master_bio) uses a
>> single bio, which can hold at most BIO_MAX_VECS pages, making this the
>> largest payload it can carry. With a 4 KiB PAGE_SIZE, that corresponds
>> to 256 pages, or 1 MiB (2048 sectors).
>> This patch changes the behavior for atomic writes that exceed this
>> limit. Instead of failing the write with -EIO when the number of sectors
>> must be reduced, it disables write-behind and proceeds with the atomic
>> write.
>> 
>>>> +			/*
>>>> +			 * Atomic writes cannot be split, so disable
>>>> +			 * write-behind.
>>>> +			 */
>>>> +			write_behind = false;
>
> It's a bit poor to have write_behind initially = false, then allow it to 
> be set = true, and then later be set = false. Can this be improved?
>

Yes, I agree that setting write_behind to true and then resetting it to
false later is not ideal. I will rework this.


>>>> +		else
>>>> +			max_sectors = min_t(int, max_sectors,
>>>> +					    BIO_MAX_VECS * (PAGE_SIZE >> 9));
>>>> +	}
>>>> +
>>>>    	if (max_sectors < bio_sectors(bio)) {
>>>> +		if (atomic) {
>>>> +			bio_io_error(bio);
>>>> +			goto err_dec_pending;
>>>> +		}
>>>> +
>>>>    		bio = bio_submit_split_bioset(bio, max_sectors,
>>>>    					      &conf->bio_split);
>>>>    		if (!bio)
>>>> @@ -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;
>>>
>>> This BARRIER_UNIT_SECTOR_SIZE is a bit like chunk sectors, no? I am just
>>> wondering if we just should set it to chunk sectors =
>>> BARRIER_UNIT_SECTOR_SIZE
>>>
>>> I assume that it affects more than Reads and writes, e.g. discard also.
>>>
>> 
>> BARRIER_UNIT_SECTOR_SIZE is the resync barrier-bucket, not the layout
>> chunk size. Unless I'm missing something, using
>> atomic_write_hw_unit_max seems more appropriate than using the chunk
>> size. That way, the limit only applies to atomic writes instead of
>> affecting other operations such.
>
> well it seems to be that everything in the driver is split over 
> BARRIER_UNIT_SECTOR_SIZE, so is in effect a chunk size.
>
> Note that atomic_write_hw_unit_max is going to be small always compared 
> to BARRIER_UNIT_SECTOR_SIZE.
>
> However, can you check the blk stacking code to make sure that this does 
> as you want? As I remember, for stacking we take the atomic write limits 
> of the first bottom device and then stack the other bottom devices and I 
> don't think that setting atomic_write_hw_unit_max in this way has an 
> impact - see blk_stack_atomic_writes_limits()
>

I checked blk_stack_atomic_writes_limits(), and you are right: setting
atomic_write_hw_unit_max here alone does not have the intended effect on
the final stacked device.

Also, since atomic_write_hw_unit_max is expected to always be smaller
than BARRIER_UNIT_SECTOR_SIZE, it seems that there is no need to set an
additional atomic write limit in raid1, or even to set
atomic_write_hw_unit_max at all. Is that what you mean?

>> 
>>>>    	lim.features |= BLK_FEAT_ATOMIC_WRITES;
>>>>    	lim.features |= BLK_FEAT_PCI_P2PDMA;
>>>>    	err = mddev_stack_rdev_limits(mddev, &lim, MDDEV_STACK_INTEGRITY);
>>>
>> 
>

-- 
Best Regards,
Abd-Alrhman