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

[Benjamin Marzinski <bmarzins@redhat.com>]

On Tue, Jun 03, 2025 at 08:57:32AM +0100, John Garry wrote:
> On 02/06/2025 17:25, Benjamin Marzinski wrote:
> > 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.
> 
> Do you have a pointer for that?

https://lore.kernel.org/dm-devel/ee66a4f2-ecc4-68d2-4594-a0bcba7ffe9c@redhat.com/

Specifically, in that thread Ming Lei suggests it here:
https://lore.kernel.org/dm-devel/Z9t709DZs-Flq1qS@fedora/
and Jens agrees.

-Ben

> 
> > 
> > > 
> > > 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?
> 
> No, I would not think so.
> 
> thanks,
> John