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 >> >