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@xxxxxxxxxx>
--
Damien Le Moal
Western Digital Research
