On 12/15/20 7:43 PM, Damien Le Moal wrote:

On 2020/12/16 12:13, Chaitanya Kulkarni wrote:

quoted

On 12/15/20 15:13, Damien Le Moal wrote:

quoted

On 2020/12/16 8:06, Chaitanya Kulkarni wrote:
[...]

quoted

quoted

quoted

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

quoted

        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;

quoted

-- Damien Le Moal Western Digital Research