Re: [PATCH] dm-table: check BLK_FEAT_ATOMIC_WRITES inside limits_lock

[Benjamin Marzinski <bmarzins@redhat.com>]

On Mon, Jun 02, 2025 at 11:08:46AM +0100, John Garry wrote:
> On 30/05/2025 15:50, Benjamin Marzinski wrote:
> 
> +
> 
> > dm_set_device_limits() should check q->limits.features for
> > BLK_FEAT_ATOMIC_WRITES while holding q->limits_lock, like it does for
> > the rest of the queue limits.
> > 
> > Fixes: b7c18b17a173 ("dm-table: Set BLK_FEAT_ATOMIC_WRITES for target queue limits")
> > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> 
> In itself, the change seems fine, but I have doubts whether it's preferred
> to even grab the q->limits_lock outside block layer / its helpers.

I'm pretty sure Mikulas added the q->limits_lock around DM's queue
limits accesses as the result of a discussion with some block layer
developers.

> 
> And, apart from this, if the bottom device limits change later, do we
> actually trigger a top device limits evaluation update?

DM will obviously re-evaluate the limits if you reload the table. In
some cases, DM will also disable features if turns out that they aren't
supported when it actually tries to use them. Dumb question: Is there
much chance of a SCSI device's atomic write support changing while it's
in-use?

-Ben
 
> > ---
> >   drivers/md/dm-table.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
> > index 57573e8b5aa9..9f95f77687ef 100644
> > --- a/drivers/md/dm-table.c
> > +++ b/drivers/md/dm-table.c
> > @@ -430,13 +430,13 @@ static int dm_set_device_limits(struct dm_target *ti, struct dm_dev *dev,
> >   		return 0;
> >   	}
> > +	mutex_lock(&q->limits_lock);
> >   	/*
> >   	 * BLK_FEAT_ATOMIC_WRITES is not inherited from the bottom device in
> >   	 * blk_stack_limits(), so do it manually.
> >   	 */
> >   	limits->features |= (q->limits.features & BLK_FEAT_ATOMIC_WRITES);
> > -	mutex_lock(&q->limits_lock);
> >   	if (blk_stack_limits(limits, &q->limits,
> >   			get_start_sect(bdev) + start) < 0)
> >   		DMWARN("%s: adding target device %pg caused an alignment inconsistency: "