fabrics: avoid redundant Get Log Page on retry

In case of a GENCTR mismatch, nvme_discovery_log() currently fetches
the Discovery Log Page header again before fetching the entries.
This is unnecessary since it was just fetched to determine GENCTR.
So only fetch the header once and re-use it for the next loop iteration.

Remove a couple of unnecessary variables in the process.

Signed-off-by: Caleb Sander <csander@purestorage.com>

diff --git a/src/nvme/fabrics.c b/src/nvme/fabrics.c
index 49a01e21ae4d84665fbbdd879205bce36ca8d146..c406acf8ca82c2dc42b9f1762310955e3a982bb5 100644 (file)
--- a/src/nvme/fabrics.c
+++ b/src/nvme/fabrics.c
@@ -1059,39 +1059,34 @@ static struct nvmf_discovery_log *nvme_discovery_log(nvme_ctrl_t c,
                                                     int max_retries)
 {
        nvme_root_t r = c->s && c->s->h ? c->s->h->r : NULL;
-       struct nvmf_discovery_log *log = NULL;
-       int ret, retries = 0;
+       struct nvmf_discovery_log *log;
+       int retries = 0;
        const char *name = nvme_ctrl_get_name(c);
        uint64_t genctr, numrec;
-       unsigned int size;
        int fd = nvme_ctrl_get_fd(c);
 
-       args->fd = fd;
-
-       do {
-               size = sizeof(struct nvmf_discovery_log);
+       log = __nvme_alloc(sizeof(*log));
+       if (!log) {
+               nvme_msg(r, LOG_ERR,
+                        "could not allocate memory for discovery log header\n");
+               errno = ENOMEM;
+               return NULL;
+       }
 
-               free(log);
-               log = __nvme_alloc(size);
-               if (!log) {
-                       nvme_msg(r, LOG_ERR,
-                                "could not allocate memory for discovery log header\n");
-                       errno = ENOMEM;
-                       return NULL;
-               }
+       nvme_msg(r, LOG_DEBUG, "%s: get header (try %d/%d)\n",
+                name, retries, max_retries);
+       args->lpo = 0;
+       args->log = log;
+       args->len = sizeof(*log);
+       if (nvme_get_log_page(fd, NVME_LOG_PAGE_PDU_SIZE, args)) {
+               nvme_msg(r, LOG_INFO,
+                        "%s: discover try %d/%d failed, error %d\n",
+                        name, retries, max_retries, errno);
+               goto out_free_log;
+       }
 
-               nvme_msg(r, LOG_DEBUG, "%s: get header (try %d/%d)\n",
-                       name, retries, max_retries);
-               args->lpo = 0;
-               args->len = size;
-               args->log = log;
-               ret = nvme_get_log_page(fd, NVME_LOG_PAGE_PDU_SIZE, args);
-               if (ret) {
-                       nvme_msg(r, LOG_INFO,
-                                "%s: discover try %d/%d failed, error %d\n",
-                                name, retries, max_retries, errno);
-                       goto out_free_log;
-               }
+       do {
+               size_t entries_size;
 
                numrec = le64_to_cpu(log->numrec);
                genctr = le64_to_cpu(log->genctr);
@@ -1099,11 +1094,9 @@ static struct nvmf_discovery_log *nvme_discovery_log(nvme_ctrl_t c,
                if (numrec == 0)
                        break;
 
-               size = sizeof(struct nvmf_discovery_log) +
-                       sizeof(struct nvmf_disc_log_entry) * numrec;
-
                free(log);
-               log = __nvme_alloc(size);
+               entries_size = sizeof(*log->entries) * numrec;
+               log = __nvme_alloc(sizeof(*log) + entries_size);
                if (!log) {
                        nvme_msg(r, LOG_ERR,
                                 "could not alloc memory for discovery log page\n");
@@ -1112,15 +1105,13 @@ static struct nvmf_discovery_log *nvme_discovery_log(nvme_ctrl_t c,
                }
 
                nvme_msg(r, LOG_DEBUG,
-                        "%s: get %" PRIu64
-                        " records (length %d genctr %" PRIu64 ")\n",
-                        name, numrec, size, genctr);
+                        "%s: get %" PRIu64 " records (genctr %" PRIu64 ")\n",
+                        name, numrec, genctr);
 
-               args->lpo = sizeof(struct nvmf_discovery_log);
-               args->len = size - sizeof(struct nvmf_discovery_log);
+               args->lpo = sizeof(*log);
                args->log = log->entries;
-               ret = nvme_get_log_page(fd, NVME_LOG_PAGE_PDU_SIZE, args);
-               if (ret) {
+               args->len = entries_size;
+               if (nvme_get_log_page(fd, NVME_LOG_PAGE_PDU_SIZE, args)) {
                        nvme_msg(r, LOG_INFO,
                                 "%s: discover try %d/%d failed, error %d\n",
                                 name, retries, max_retries, errno);
@@ -1134,10 +1125,9 @@ static struct nvmf_discovery_log *nvme_discovery_log(nvme_ctrl_t c,
                nvme_msg(r, LOG_DEBUG, "%s: get header again\n", name);
 
                args->lpo = 0;
-               args->len = sizeof(struct nvmf_discovery_log);
                args->log = log;
-               ret = nvme_get_log_page(fd, NVME_LOG_PAGE_PDU_SIZE, args);
-               if (ret) {
+               args->len = sizeof(*log);
+               if (nvme_get_log_page(fd, NVME_LOG_PAGE_PDU_SIZE, args)) {
                        nvme_msg(r, LOG_INFO,
                                 "%s: discover try %d/%d failed, error %d\n",
                                 name, retries, max_retries, errno);

diff --git a/test/ioctl/discovery.c b/test/ioctl/discovery.c
index f2e11a75432951e358274733430623abc13a710b..c62aff65ed914eb3074942cc52cdc7ec6729765b 100644 (file)
--- a/test/ioctl/discovery.c
+++ b/test/ioctl/discovery.c
@@ -195,7 +195,7 @@ static void test_genctr_change(nvme_ctrl_t c)
        };
        /*
         * genctr changes after the entries are fetched the first time,
-        * so the log page fetch is retried
+        * so the log page entries are refetched
         */
        struct mock_cmd mock_admin_cmds[] = {
                {
@@ -220,13 +220,6 @@ static void test_genctr_change(nvme_ctrl_t c)
                               | NVME_LOG_LID_DISCOVER, /* LID */
                        .out_data = &header2,
                },
-               {
-                       .opcode = nvme_admin_get_log_page,
-                       .data_len = sizeof(header2),
-                       .cdw10 = (sizeof(header2) / 4 - 1) << 16 /* NUMDL */
-                              | NVME_LOG_LID_DISCOVER, /* LID */
-                       .out_data = &header2,
-               },
                {
                        .opcode = nvme_admin_get_log_page,
                        .data_len = sizeof(entries2),
@@ -291,13 +284,6 @@ static void test_max_retries(nvme_ctrl_t c)
                               | NVME_LOG_LID_DISCOVER, /* LID */
                        .out_data = &header2,
                },
-               {
-                       .opcode = nvme_admin_get_log_page,
-                       .data_len = sizeof(header2),
-                       .cdw10 = (sizeof(header2) / 4 - 1) << 16 /* NUMDL */
-                              | NVME_LOG_LID_DISCOVER, /* LID */
-                       .out_data = &header2,
-               },
                {
                        .opcode = nvme_admin_get_log_page,
                        .data_len = sizeof(entry),