[PATCH] dm-table: check BLK_FEAT_ATOMIC_WRITES inside limits_lock

[PATCH] dm-table: check BLK_FEAT_ATOMIC_WRITES inside limits_lock

[Benjamin Marzinski]

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>
---
 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: "
-- 
2.48.1

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

[John Garry]

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.

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

> ---
>   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: "

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

[Benjamin Marzinski]

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: "

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

[John Garry]

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?

> 
>>
>> 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

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

[Benjamin Marzinski]

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