Re: [PATCH 4/4] dm-zoned: allow for device size smaller than the capacity

[Damien Le Moal <Damien.LeMoal@wdc.com>]

On 2020/03/31 17:53, Hannes Reinecke wrote:
> On 3/31/20 2:49 AM, Damien Le Moal wrote:
>> On 2020/03/27 16:15, Hannes Reinecke wrote:
>>> dm-zoned requires several zones for metadata and chunk bitmaps,
>>> so it cannot expose the entire capacity as the device size.
>>> Originally the code would check for the capacity being equal to
>>> the device size, which is arguably wrong.
>>> So relax this check and increase the interface version number
>>> to signal to userspace that it can set a smaller device size.
>>>
>>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>>> ---
>>>   drivers/md/dm-zoned-target.c | 4 ++--
>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/md/dm-zoned-target.c b/drivers/md/dm-zoned-target.c
>>> index 7ec9dde24516..89a825d1034e 100644
>>> --- a/drivers/md/dm-zoned-target.c
>>> +++ b/drivers/md/dm-zoned-target.c
>>> @@ -715,7 +715,7 @@ static int dmz_get_zoned_device(struct dm_target *ti, char *path)
>>>   	aligned_capacity = dev->capacity &
>>>   				~((sector_t)blk_queue_zone_sectors(q) - 1);
>>>   	if (ti->begin ||
>>> -	    ((ti->len != dev->capacity) && (ti->len != aligned_capacity))) {
>>> +	    ((ti->len > dev->capacity) && (ti->len > aligned_capacity))) {
>>>   		ti->error = "Partial mapping not supported";
>>
>> The message is now wrong. Also, the condition can now be simplified to:
>>
>> if ((ti->begin + ti->len) > aligned_capacity) {
>>
>> Since aligned capacity is equal or smaller than dev capacity. And we have to
>> account for the potential non-zero begin.
>>
> _Actually_ I would forbid for 'ti->begin' to be anything other than 0.
> For a zoned device there is no point in allowing for partial handling at 
> all.
> Problem is that 'dev->capacity' is the capacity of the zoned block 
> device, whereas 'ti-len' is the exported capacity of the device-mapper 
> device, which is smaller than the device capacity by the number of 
> metadata blocks/zones.

OK. Got it. I misunderstood that. Sounds good to me.

> 
>>>   		ret = -EINVAL;
>>>   		goto err;
>>> @@ -1008,7 +1008,7 @@ static int dmz_message(struct dm_target *ti, unsigned int argc, char **argv,
>>>   
>>>   static struct target_type dmz_type = {
>>>   	.name		 = "zoned",
>>> -	.version	 = {1, 2, 0},
>>> +	.version	 = {1, 3, 0},
>>>   	.features	 = DM_TARGET_SINGLETON | DM_TARGET_ZONED_HM,
>>>   	.module		 = THIS_MODULE,
>>>   	.ctr		 = dmz_ctr,
>>>
>>
>> I do not think this is nearly enough: dmz_init_zones() is still considering the
>> entire drive and does a zone report from 0 on the backend bdev. It should start
>> at ti->begin and up to ti->begin+ti->len, no ?
>>
> Yes, and no. I want to disallow 'ti->begin' to be anything else than 0 
> as ti->begin and ti->len are relative to exported device-mapper device, 
> and we always want to have block 0 mapped :-)
> 
> And as such dmz_init_zones() needs to cover all zones, as this is 
> relative to the zoned block device.

Yes. Which is already the case since we need to see the metatdata and reserved
zones too. Makes sense.

> 
>> Furthermore, this introduce a change in the meaning of the zone ID. Since this
>> is set to the index of the zone in the report (patch 1), if the mapping is
>> partial and the zone report does not start at 0, then zone ID is not zone number
>> on the device anymore. So dmz_start_block() needs to be offset by ti->begin
>> otherwise IOs will go to the wrong zones.
>>
> As I said: We will never do a partial mapping.
> What this patch does it to bring the device-mapper mapping in-line with 
> the exported device size.

Got it !

> Originally we would export a device-mapper mapping for blocks up to the 
> zone-device capacity. As the resulting device-mapper block device has a 
> smaller capacity than the mapping would allow for those 'spare' blocks 
> would never been used, thus the invalid mapping was never triggered.
> 
> What this patch does is to bring the device-mapper mapping in-line with 
> the exported block device capacity, so that we don't have an invalid 
> mapping. Nothing else has (and should) be changed.
> 
> Especially not the partial zoned device handling :-)

OK. Thanks for doing that ! I  totally missed these points when I wrote the
mapper :)

Cheers.

-- 
Damien Le Moal
Western Digital Research