Re: [PATCH v3 5/6] dm: limit swapping tables for devices with zone write plugs

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

On 4/10/25 5:15 AM, Benjamin Marzinski wrote:
>>> @@ -1873,11 +1898,17 @@ int dm_table_set_restrictions(struct dm_table *t, struct request_queue *q,
>>>  		limits->features &= ~BLK_FEAT_DAX;
>>>  
>>>  	/* For a zoned table, setup the zone related queue attributes. */
>>> -	if (IS_ENABLED(CONFIG_BLK_DEV_ZONED) &&
>>> -	    (limits->features & BLK_FEAT_ZONED)) {
>>> -		r = dm_set_zones_restrictions(t, q, limits);
>>> -		if (r)
>>> -			return r;
>>> +	if (IS_ENABLED(CONFIG_BLK_DEV_ZONED)) {
>>> +		if (limits->features & BLK_FEAT_ZONED) {
>>> +			r = dm_set_zones_restrictions(t, q, limits);
>>> +			if (r)
>>> +				return r;
>>> +		} else if (dm_has_zone_plugs(t->md)) {
>>> +			DMWARN("%s: device has zone write plug resources. "
>>> +			       "Cannot switch to non-zoned table.",
>>> +			       dm_device_name(t->md));
>>> +			return -EINVAL;
>>> +		}
>>
>> I am confused with this one. We can only have zone write plugs if the device is
>> zoned. So it seems that the check for "if (dm_has_zone_plugs(t->md)" should
>> really be inside dm_set_zones_restrictions(). Or is this trying to detect a
>> table change that would switch the device from zoned to non-zoned ? If that is
>> the case, regardless of the zone write plug initialization state, we should
>> never allow such change.

And I was wrong on this one: using dm-linear to map only conventional zones of
an SMR HDD, the DM device will *not* be zoned but the underlying device is. So
this check is fine since the dm device will not have wplugs in that case.

> I don't think that, for instance, allowing a linear device stacked on
> top of a zoned device to get remapped to stack on top of a non-zoned
> device runs much more risk than allowing a linear device to get remapped
> in any other case? This is currently allowed, and I don't believe anyone
> has complained about it. I would rather assume that the user knows what
> they are doing, instead of disallowing things that aren't obviously
> wrong, and won't cause system problems if they are (aside from the
> problems that naturally result from putting the wrong device in your
> table line). But if Mikulas and Mike think it would be better to
> disallow this, then I'm fine with that.

OK. Let's leave things as you have now.

>>> +	if (get_capacity(disk) && dm_has_zone_plugs(t->md)) {
>>> +		if (q->limits.chunk_sectors != lim->chunk_sectors) {
>>> +			DMWARN("%s: device has zone write plug resources. "
>>> +			       "Cannot change zone size",
>>> +			       disk->disk_name);
>>> +			return -EINVAL;
>>> +		}
>>> +		if (lim->max_hw_zone_append_sectors != 0 &&
>>> +		    !dm_table_is_wildcard(t)) {
>>> +			DMWARN("%s: device has zone write plug resources. "
>>> +			       "New table must emulate zone append",
>>> +			       disk->disk_name);
>>> +			return -EINVAL;
>>> +		}
>>> +	}
>>
>> I have some concerns about this: what if the new table has the same capacity
>> and the same zone size but the types of zones changed ? We are not checking
>> this here and we cannot actually check that without doing a report zones. So I
>> really think we should just use the big hammer here and simply only allow the
>> wildcard target and no other change.
> 
> I don't see how this could lead to accessing invalid memory, which was
> my big concern, with these patches. The worst that could happen in an IO
> error, AFAICS. Disallowing all table loads except for the error target
> would keep people from being able from things like changing options on
> the dm-crypt target. Again, that is just my preference, and I'll defer
> to Mike and Mikulas on how this should be handled.

Yeah, but these IO errors that can happen would be hard to debug/understand...
But as you said, given that this has never been checked/prevented before and
that no one complained, let's keep this as is. Your example for dm-crypt is
indeed a valid case.

>>>  	/*
>>>  	 * Warn once (when the capacity is not yet set) if the mapped device is
>>>  	 * partially using zone resources of the target devices as that leads to
>>> diff --git a/drivers/md/dm.c b/drivers/md/dm.c
>>> index 292414da871d..240f6dab8dda 100644
>>> --- a/drivers/md/dm.c
>>> +++ b/drivers/md/dm.c
>>> @@ -2429,6 +2429,12 @@ static struct dm_table *__bind(struct mapped_device *md, struct dm_table *t,
>>>  	size = dm_table_get_size(t);
>>>  
>>>  	old_size = dm_get_size(md);
>>> +
>>> +	if (!dm_table_supports_size_change(t, old_size, size)) {
>>> +		old_map = ERR_PTR(-EINVAL);
>>> +		goto out;
>>> +	}
>>
>> And I guess we could probably move the "wildcard-only is allowed" change check
>> here as that would further simplify the revalidation code. No ?
> 
> If we are disallowing any zoned device to reload its table to something
> other than an error target, then yes. It can go here. If we only care
> about zoned devices that emulate zoned append, when we need to wait till
> dm_set_zones_restrictions() where we determine that. Since we already
> need to handle failures in dm_table_set_restrictions(), moving it
> doesn't simplify the code much.

OK. So with the commit message typos fixed, feel free to add:

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>


-- 
Damien Le Moal
Western Digital Research