[PATCH V7 4/6] nvmet: add ZBD over ZNS backend support

[Chaitanya Kulkarni]

On 12/15/20 7:43 PM, Damien Le Moal wrote:
> On 2020/12/16 12:13, Chaitanya Kulkarni wrote:
>> On 12/15/20 15:13, Damien Le Moal wrote:
>>> On 2020/12/16 8:06, Chaitanya Kulkarni wrote:
>>> [...]
>>>>>> +void nvmet_bdev_execute_zone_mgmt_recv(struct nvmet_req *req)
>>>>>> +{
>>>>>> +	sector_t sect = nvmet_lba_to_sect(req->ns, req->cmd->zmr.slba);
>>>>>> +	u32 bufsize = (le32_to_cpu(req->cmd->zmr.numd) + 1) << 2;
>>>>>> +	struct nvmet_report_zone_data data = { .ns = req->ns };
>>>>>> +	unsigned int nr_zones;
>>>>>> +	int reported_zones;
>>>>>> +	u16 status;
>>>>>> +
>>>>>> +	nr_zones = (bufsize - sizeof(struct nvme_zone_report)) /
>>>>>> +			sizeof(struct nvme_zone_descriptor);
>>>>> You could move this right before the vmalloc call since it is first used there.
>>>> There are only three lines between the this and the vmalloc, does that
>>>> really
>>>> going to make any difference ?
>>> It makes the code far easier to read and understand at a quick glance without
>>> having to go up and down reading to be reminded what nr_zones was. That also
>>> would avoid changes to sneak in between these related statements, making things
>>> even harder to read.
>>>
>>> I personally like to think of code as a natural language text: if statements
>>> related to each other are not grouped in a single paragraph, the text is really
>>> hard to understand...
>>>
>> hmmm, why can't we use a macro and like everywhere else in zns.c
>> we can declare-init the nr_zones which will make nr_zones initialization
>> uniform withall the code with a meaningful name.
>>
>> How about following (untested) ?
>>
>> diff --git a/drivers/nvme/target/zns.c b/drivers/nvme/target/zns.c
>> index 149bc8ce7010..6c497b60cd98 100644
>> --- a/drivers/nvme/target/zns.c
>> +++ b/drivers/nvme/target/zns.c
>> @@ -201,18 +201,19 @@ static int nvmet_bdev_report_zone_cb(struct
>> blk_zone *z, unsigned int idx,
>>         return 0;
>>  }
>>  
>> +#define NVMET_NR_ZONES_FROM_BUFSIZE(bufsize) \
>> +       ((bufsize - sizeof(struct nvme_zone_report)) / \
>> +                       sizeof(struct nvme_zone_descriptor))
>> +
>>  void nvmet_bdev_execute_zone_mgmt_recv(struct nvmet_req *req)
>>  {
>>         sector_t sect = nvmet_lba_to_sect(req->ns, req->cmd->zmr.slba);
>>         u32 bufsize = (le32_to_cpu(req->cmd->zmr.numd) + 1) << 2;
>>         struct nvmet_report_zone_data data = { .ns = req->ns };
>> -       unsigned int nr_zones;
>> +       unsigned int nr_zones = NVMET_NR_ZONES_FROM_BUFSIZE(bufsize);
> Hiding calculations in a macro does not help readability. And I do not see the
> point since this is used in one place only. If you want to isolate the report
> buffer allocation & nr zones calculation, then something like what scsi does in
> sd_zbc_alloc_report_buffer() (in drivers/scsi/sd_zbc.c) is in my opinion much
> cleaner.

This is what I was thinking about it since we also do similar buffer
allocation calculation

on the host side. Let me see if I can add that to V8.

>>         int reported_zones;
>>         u16 status;
>>  
>> -       nr_zones = (bufsize - sizeof(struct nvme_zone_report)) /
>> -                       sizeof(struct nvme_zone_descriptor);
>> -
>>         status = nvmet_bdev_zns_checks(req);
>>         if (status)
>>                 goto out;
>>
>>> -- Damien Le Moal Western Digital Research
>>
>