From: Caleb Sander <csander@purestorage.com>
Date: Tue, 1 Aug 2023 01:57:54 +0000 (-0600)
Subject: fabrics: avoid redundant Get Log Page on retry
X-Git-Tag: v1.7~34
X-Git-Url: https://www.infradead.org/git/?a=commitdiff_plain;h=115cb9f5e1adae05a28c4f2795f44dcc254ae632;p=users%2Fsagi%2Flibnvme.git

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 49a01e21..c406acf8 100644
--- 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 f2e11a75..c62aff65 100644
--- 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),