nvme-cli: Add ioctl retry support for connect-all

PATCH Superseded James Smart 3 participants 3 messages Apr 27, 2018 – Apr 30, 2018 NVMe

Share & tools

James Smart

author

2018-04-27 17:29 UTC #0 lore raw

PATCH 1/1 nvme-cli: Add ioctl retry support for connect-all

Currently, if an ioctl command to a controller fails, the routines within nvme-cli don't do retries and just fail. In the case of operations such as "connect-all" which depend on the ioctl to read the discovery log to give it the list of controller to then "connect" to, this failure can be catastrophic as there's no guarantee that the event/admin action that performed the connect-all will be re-triggered, resulting in a loss of connectivity to storage subsystems.

As to why the ioctl may have failed in the first place: if the controller is in the middle of a reset or reconnect due to an error or temporary loss of connectivity (which can happen even on a discovery controller), ioctl commands will fail rather than suspend for the full duration of the controller state change (say to live or delete), which could be a minute or more.

This patch makes the following changes:
1) As it's expected other cases will run into the need to do retries, Implement wrappers for the ioctl routine. The wrappers have a retry loop. The ioctl will only retry if the failure status was EAGAIN.

2) Modify the ioctl callees to use the wrappers and specify a retry count parameter.

3) Leave all ioctl requests for no retries except the command that obtains log pages. The command that obtains log pages qualifies the log page being queried: if reading the discovery log, then it requests retries for up to 60s (default controller loss tmo); if not the discovery log page, then no retries.

Signed-off-by James Smart <james.smart at broadcom.com>

---
 intel-nvme.c    |   3 +-
 nvme-ioctl.c    | 103 ++++++++++++++++++++++++++++++++++++--------------------
 nvme-ioctl.h    |  11 +++++-
 nvme-lightnvm.c |   8 +++--
 wdc-nvme.c      |  26 +++++++++-----
 5 files changed, 101 insertions(+), 50 deletions(-)

intel-nvme.c +2 -1

index 5e407df..474bdc1 100644
@@ -544,7 +544,8 @@ static int read_entire_cmd(struct nvme_passthru_cmd *cmd, int total_size,
 
 	dword_tfer = min(max_tfer, total_size);
 	while (total_size > 0) {
-		err = nvme_submit_passthru(ioctl_fd, NVME_IOCTL_ADMIN_CMD, cmd);
+		err = nvme_submit_passthru(ioctl_fd, NVME_IOCTL_ADMIN_CMD, cmd,
+				NO_RETRIES);
 		if (err) {
 			fprintf(stderr, "failed on cmd.data_len %u cmd.cdw13 %u cmd.cdw12 %x cmd.cdw10 %u err %x remaining size %d\n", cmd->data_len, cmd->cdw13, cmd->cdw12, cmd->cdw10, err, total_size);
 			goto out;

nvme-ioctl.c +67 ~~-36~~

290 lines not on this page · raw message

nvme-ioctl.h +10 -1

index 9bf5459..2997bd7 100644
@@ -6,10 +6,19 @@
 #include "linux/nvme_ioctl.h"
 #include "nvme.h"
 
+/* rate of ioctl retries */
+#define IOCTL_TIMESPERSEC	4
+/* delay between retries. Units in us */
+#define IOCTL_DELAY		(1000000 / IOCTL_TIMESPERSEC)	/* 250ms */
+
+#define NO_RETRIES		0
+#define DISCOVERY_RETRIES	(IOCTL_TIMESPERSEC * 60)	/* 60s */
+
 int nvme_get_nsid(int fd);
 
 /* Generic passthrough */
-int nvme_submit_passthru(int fd, int ioctl_cmd, struct nvme_passthru_cmd *cmd);
+int nvme_submit_passthru(int fd, int ioctl_cmd, struct nvme_passthru_cmd *cmd,
+		int maxretry);
 
 int nvme_passthru(int fd, int ioctl_cmd, __u8 opcode, __u8 flags,
 		  __u16 rsvd, __u32 nsid, __u32 cdw2, __u32 cdw3,

nvme-lightnvm.c +5 -3

index 0fdc74d..52456a0 100644
@@ -383,7 +383,7 @@ static int lnvm_get_identity(int fd, int nsid, struct nvme_nvm_id *nvm_id)
 		.data_len	= 0x1000,
 	};
 
-	return nvme_submit_passthru(fd, NVME_IOCTL_ADMIN_CMD, &cmd);
+	return nvme_submit_passthru(fd, NVME_IOCTL_ADMIN_CMD, &cmd, NO_RETRIES);
 }
 
 int lnvm_do_id_ns(int fd, int nsid, unsigned int flags)

@@ -440,7 +440,8 @@ static int __lnvm_do_get_bbtbl(int fd, struct nvme_nvm_id *id,
 	};
 
 	err = nvme_submit_passthru(fd, NVME_IOCTL_ADMIN_CMD,
-					(struct nvme_passthru_cmd *)&cmd);
+					(struct nvme_passthru_cmd *)&cmd,
+					NO_RETRIES);
 	if (err > 0) {
 		fprintf(stderr, "NVMe Status:%s(%x)\n",
 			nvme_status_to_string(err), err);

@@ -500,7 +501,8 @@ static int __lnvm_do_set_bbtbl(int fd, struct ppa_addr ppa, __u8 value)
 	};
 
 	err = nvme_submit_passthru(fd, NVME_IOCTL_ADMIN_CMD,
-					(struct nvme_passthru_cmd *)&cmd);
+					(struct nvme_passthru_cmd *)&cmd,
+					NO_RETRIES);
 	if (err > 0) {
 		fprintf(stderr, "NVMe Status:%s(%x)\n",
 			nvme_status_to_string(err), err);

wdc-nvme.c +17 -9

index 0f4375c..7c769d1 100644
@@ -650,7 +650,8 @@ static int wdc_do_clear_dump(int fd, __u8 opcode, __u32 cdw12)
 	memset(&admin_cmd, 0, sizeof (struct nvme_admin_cmd));
 	admin_cmd.opcode = opcode;
 	admin_cmd.cdw12 = cdw12;
-	ret = nvme_submit_passthru(fd, NVME_IOCTL_ADMIN_CMD, &admin_cmd);
+	ret = nvme_submit_passthru(fd, NVME_IOCTL_ADMIN_CMD, &admin_cmd,
+				NO_RETRIES);
 	if (ret != 0) {
 		fprintf(stdout, "ERROR : WDC : Crash dump erase failed\n");
 	}

@@ -673,7 +674,8 @@ static __u32 wdc_dump_length(int fd, __u32 opcode, __u32 cdw10, __u32 cdw12, __u
 	admin_cmd.cdw10 = cdw10;
 	admin_cmd.cdw12 = cdw12;
 
-	ret = nvme_submit_passthru(fd, NVME_IOCTL_ADMIN_CMD, &admin_cmd);
+	ret = nvme_submit_passthru(fd, NVME_IOCTL_ADMIN_CMD, &admin_cmd,
+				NO_RETRIES);
 	if (ret != 0) {
 		l->log_size = 0;
 		ret = -1;

@@ -709,7 +711,8 @@ static int wdc_do_dump(int fd, __u32 opcode,__u32 data_len, __u32 cdw10,
 	admin_cmd.cdw10 = cdw10;
 	admin_cmd.cdw12 = cdw12;
 
-	ret = nvme_submit_passthru(fd, NVME_IOCTL_ADMIN_CMD, &admin_cmd);
+	ret = nvme_submit_passthru(fd, NVME_IOCTL_ADMIN_CMD, &admin_cmd,
+			NO_RETRIES);
 	fprintf(stderr, "NVMe Status:%s(%x)\n", nvme_status_to_string(ret), ret);
 	if (ret == 0) {
 		ret = wdc_create_log_file(file, dump_data, dump_length);

@@ -853,7 +856,8 @@ static int wdc_do_drive_log(int fd, char *file)
 	admin_cmd.cdw12 = ((WDC_NVME_DRIVE_LOG_SUBCMD <<
 				WDC_NVME_SUBCMD_SHIFT) | WDC_NVME_DRIVE_LOG_SIZE_CMD);
 
-	ret = nvme_submit_passthru(fd, NVME_IOCTL_ADMIN_CMD, &admin_cmd);
+	ret = nvme_submit_passthru(fd, NVME_IOCTL_ADMIN_CMD, &admin_cmd,
+			NO_RETRIES);
 	fprintf(stderr, "NVMe Status:%s(%x)\n", nvme_status_to_string(ret),
 			ret);
 	if (ret == 0) {

@@ -1002,7 +1006,8 @@ static int wdc_purge(int argc, char **argv,
 		return fd;
 
 	wdc_check_device(fd);
-	ret = nvme_submit_passthru(fd, NVME_IOCTL_ADMIN_CMD, &admin_cmd);
+	ret = nvme_submit_passthru(fd, NVME_IOCTL_ADMIN_CMD, &admin_cmd,
+			NO_RETRIES);
 	if (ret > 0) {
 		switch (ret) {
 		case WDC_NVME_PURGE_CMD_SEQ_ERR:

@@ -1049,7 +1054,8 @@ static int wdc_purge_monitor(int argc, char **argv,
 		return fd;
 
 	wdc_check_device(fd);
-	ret = nvme_submit_passthru(fd, NVME_IOCTL_ADMIN_CMD, &admin_cmd);
+	ret = nvme_submit_passthru(fd, NVME_IOCTL_ADMIN_CMD, &admin_cmd,
+			NO_RETRIES);
 	if (ret == 0) {
 		mon = (struct wdc_nvme_purge_monitor_data *) output;
 		printf("Purge state = 0x%0x\n", admin_cmd.result);

@@ -1515,7 +1521,8 @@ static int wdc_clear_pcie_corr(int argc, char **argv, struct command *command,
 	admin_cmd.cdw12 = ((WDC_NVME_CLEAR_PCIE_CORR_SUBCMD << WDC_NVME_SUBCMD_SHIFT) |
 			WDC_NVME_CLEAR_PCIE_CORR_CMD);
 
-	ret = nvme_submit_passthru(fd, NVME_IOCTL_ADMIN_CMD, &admin_cmd);
+	ret = nvme_submit_passthru(fd, NVME_IOCTL_ADMIN_CMD, &admin_cmd,
+			NO_RETRIES);
 	fprintf(stderr, "NVMe Status:%s(%x)\n", nvme_status_to_string(ret), ret);
 	return ret;
 }

@@ -1584,7 +1591,7 @@ int wdc_de_VU_read_size(int fd, __u32 fileId, __u16 spiDestn, __u32* logSize)
 	cmd.cdw13 = fileId<<16;
 	cmd.cdw14 = spiDestn;
 
-	ret = nvme_submit_passthru(fd, NVME_IOCTL_ADMIN_CMD, &cmd);
+	ret = nvme_submit_passthru(fd, NVME_IOCTL_ADMIN_CMD, &cmd, NO_RETRIES);
 
 	if (!ret && logSize)
 		*logSize = cmd.result;

@@ -1619,7 +1626,8 @@ int wdc_de_VU_read_buffer(int fd, __u32 fileId, __u16 spiDestn, __u32 offsetInDw
 	cmd.addr = (__u64)(__u64)(uintptr_t)dataBuffer;
 	cmd.data_len = *bufferSize;
 
-	ret = nvme_submit_passthru(fd, NVME_IOCTL_ADMIN_CMD, &cmd);
+	ret = nvme_submit_passthru(fd, NVME_IOCTL_ADMIN_CMD, &cmd,
+			NO_RETRIES);
 
 	if( ret != WDC_STATUS_SUCCESS)
 		fprintf(stderr, "ERROR : WDC : VUReadBuffer() failed, status:%s(0x%x)\n", nvme_status_to_string(ret), ret);

Signature · 1 line

2.13.1

Keith Busch

2018-04-27 21:39 UTC #1 lore raw

PATCH 1/1 nvme-cli: Add ioctl retry support for connect-all

nvme-ioctl.h line 21

Quoted 4 lines · James Smart

/* Generic passthrough */

nvme-ioctl.h +2 -1

-int nvme_submit_passthru(int fd, int ioctl_cmd, struct nvme_passthru_cmd *cmd);
+int nvme_submit_passthru(int fd, int ioctl_cmd, struct nvme_passthru_cmd *cmd,
+		int maxretry);

Hi James,

Since the far majority of the callers don't care for retries, let's leave this API alone and create a new one that takes the retries. The old API can call the new one with the default value of NO_RETRIES, and that should simplify this patch significantly.

Thanks,
Keith

James Smart

2018-04-30 16:45 UTC #2 lore raw

PATCH 1/1 nvme-cli: Add ioctl retry support for connect-all

OK. I still believe we'll keep adding it in, but I'm good with a reduced patch.

-- james

Quoted 14 lines · Keith Busch

nvme-ioctl.h line 21
Quoted 4 lines · James Smart
/* Generic passthrough */
nvme-ioctl.h +2 -1
-int nvme_submit_passthru(int fd, int ioctl_cmd, struct nvme_passthru_cmd *cmd);
+int nvme_submit_passthru(int fd, int ioctl_cmd, struct nvme_passthru_cmd *cmd,
+		int maxretry);
Hi James,
Since the far majority of the callers don't care for retries, let's leave this API alone and create a new one that takes the retries. The old API can call the new one with the default value of NO_RETRIES, and that should simplify this patch significantly.
Thanks,
Keith