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