Re: [PATCH 3/7] dm: handle failures in dm_table_set_restrictions

[Damien Le Moal <dlemoal@kernel.org>]

On 3/11/25 03:15, Benjamin Marzinski wrote:
>>>> @@ -1883,6 +1879,7 @@ int dm_table_set_restrictions(struct dm_table *t, struct request_queue *q,
>>>>  	if (dm_table_supports_atomic_writes(t))
>>>>  		limits->features |= BLK_FEAT_ATOMIC_WRITES;
>>>>  
>>>> +	old_limits = q->limits;
>>>
>>> I am not sure this is safe to do like this since the user may be simultaneously
>>> changing attributes, which would result in the old_limits struct being in an
>>> inconsistent state. So shouldn't we hold q->limits_lock here ? We probably want
>>> a queue_limits_get() helper for that though.
>>>
>>>>  	r = queue_limits_set(q, limits);
>>>
>>> ...Or, we could modify queue_limits_set() to also return the old limit struct
>>> under the q limits_lock. That maybe easier.
>>
>> If we disallow switching between zoned devices then this is unnecssary.
> 
> Err.. nevermind that last line. There are still multiple cases where we
> could still fail here and need to fail back to the earlier limits. But
> I'm less sure that it's really necessary to lock the limits before
> reading them. For DM devices, I don't see a place where a bunch of
> limits could be updated at the same time, while we are swapping tables.
> An individual limit could get updated by things like the sysfs
> interface. But since that could happen at any time, I don't see what
> locking gets us. And if it's not safe to simply read a limit without
> locking them, then there are lots of places where we have unsafe code.
> Am I missing something here?

Yes, for simple scalar limits, I do not think there is any issue. But there are
some cases where changing one limit implies a change to other limits when the
limits are committed (under the limits lock). So my concern was that if the
above runs simultaneously with a queue limits commit, we may endup with the
limits struct copy grabbing part of the new limits and thus resulting in an
inconsistent limits struct. Not entirely sure that can actually happen though.
But given that queue_limits_commit_update() does:

	q->limits = *lim;

and this code does:

	old_limits = q->limits;

we may endup depending on how the compiler handles the struct copy ?


-- 
Damien Le Moal
Western Digital Research