Re: [PATCH] block: fix atomic write limits for stacked devices

[John Garry <john.g.garry@xxxxxxxxxx>]

On 03/06/2025 16:16, Nilay Shroff wrote:
> > > diff --git a/block/blk-settings.c b/block/blk-settings.c
> > > index a000daafbfb4..35c1354dd5ae 100644
> > > --- a/block/blk-settings.c
> > > +++ b/block/blk-settings.c
> > > @@ -598,8 +598,14 @@ static bool blk_stack_atomic_writes_head(struct queue_limits *t,
> > >            !blk_stack_atomic_writes_boundary_head(t, b))
> > >            return false;
> > >    -    if (t->io_min <= SECTOR_SIZE) {
> > > -        /* No chunk sectors, so use bottom device values directly */
> > > +    if (t->io_min <= SECTOR_SIZE ||
> > > +        (!(t->atomic_write_hw_unit_max % t->io_min) &&
> > > +         !(t->atomic_write_hw_unit_min % t->io_min))) {
> > So will this now break md raid0/10 or dm stripe when t->io_min is set (> SECTOR_SIZE)? I mean, for md raid0/10 or dm-stripe, we should be taking the chunk size into account there and we now don't seem to be doing so now.
> Shouldn't it be work good if we ensure that a bottom device atomic write unit min/max are
> aligned with the top device chunk sectors then top device could simply copy and use the
> bottom device atomic write limits directly? 

You need to be more specific when you say "aligned".

Consider chunk sectors for md raid0 is 16KB and 
b->atomic_write_hw_unit_max is 32KB. Then we must reduce 
t->atomic_write_hw_unit_max to 16KB (so cannot use the value in 
b->atomic_write_hw_unit_max directly).

> Or do we have a special case for raid0/10 and
> dm-strip which can't handle atomic write if chunk size for stacked device is greater than
> SECTOR_SIZE?
>
> BTW there's a typo in the above change, we should have the above if check written as below
> (my bad):
>      if (t->io_min <= SECTOR_SIZE ||
>          (!(b->atomic_write_hw_unit_max % t->io_min) &&
>           !(b->atomic_write_hw_unit_min % t->io_min))) {
>      ...
>      ...
>
> > What is the value of top device io_min and physical_block_size in your example?
> The NVme disk which I am using has both t->io_min and t->physical_block_size set
> to 4096.

I need to test further, but maybe we can change the check to this:

if (t->io_min <= SECTOR_SIZE || t->io_min == t->physical_block_size) {
		/* No chunk sectors, so use bottom device values directly */
		t->atomic_write_hw_unit_max = b->atomic_write_hw_unit_max;
		t->atomic_write_hw_unit_min = b->atomic_write_hw_unit_min;
		t->atomic_write_hw_max = b->atomic_write_hw_max;
		return true;
}