nvme: freeze IO accesses around format

If someone attempts to do IO to a drive while it is under format, we risk timing out that IO. That potentially leads to the driver triggering a controller reset, and subsequently the format is ruined and the device goes away.

Prevents this by freezing IO access to the device around a format. Without this, the following set of commands can easily make your device disappear:

parted -s /dev/nvme3n1 mklabel gpt
parted -s /dev/nvme3n1 mkpart primary 0G 100G
parted -s /dev/nvme3n1 rm 1
nvme format /dev/nvme3

since the last partition removal will trigger a udev partition reload, which happens while the format is running. If the format takes longer than the normal IO timeout, we start timing it out:

[  456.799438]  nvme3n1:
[  456.833656]  nvme3n1: p1
[  456.842025]  nvme3n1: p1
[  456.887368]  nvme3n1:
[  487.699023] nvme nvme3: I/O 879 QID 12 timeout, aborting
[  518.098840] nvme nvme3: I/O 879 QID 12 timeout, reset controller
[  571.700471] nvme nvme3: Abort status: 0x7
[  571.798306] nvme nvme3: Removing after probe failure status: -22
[  571.811330] nvme3n1: detected capacity change from 4000787030016 to 0
[  571.819189] print_req_error: I/O error, dev nvme3n1, sector 7814036992

and the device is gone, needing a driver reload or reboot to bring it back. Same thing happens if you just do a dd from the device and then start a format. Behavior is vendor agnostic, basically just timing dependent.

Signed-off-by Jens Axboe <axboe at kernel.dk>

(quoted diff) +7 -0

@@ -1013,10 +1013,23 @@ static int nvme_user_cmd(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
 	if (cmd.timeout_ms)
 		timeout = msecs_to_jiffies(cmd.timeout_ms);
 
+	/*
+	 * Freeze current access to the device, and prevent new ones, around
+	 * a format operation.
+	 */
+	if (cmd.opcode == nvme_admin_format_nvm) {
+		nvme_start_freeze(ctrl);
+		nvme_wait_freeze(ctrl);

Quoted 11 lines · Jens Axboe

drivers/nvme/host/core.c +7 -0

@@ -1013,10 +1013,23 @@ static int nvme_user_cmd(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
 	if (cmd.timeout_ms)
 		timeout = msecs_to_jiffies(cmd.timeout_ms);
 
+	/*
+	 * Freeze current access to the device, and prevent new ones, around
+	 * a format operation.
+	 */
+	if (cmd.opcode == nvme_admin_format_nvm) {
+		nvme_start_freeze(ctrl);
+		nvme_wait_freeze(ctrl);

drivers/nvme/host/core.c static int nvme_user_cmd(struct nvme_ctrl *ctrl, struct nvme_ns *ns, · line 1032

Quoted 10 lines

drivers/nvme/host/core.c +6 -0

+	}
+
 	status = nvme_submit_user_cmd(ns ? ns->queue : ctrl->admin_q, &c,
 			(void __user *)(uintptr_t)cmd.addr, cmd.data_len,
 			(void __user *)(uintptr_t)cmd.metadata, cmd.metadata,
 			0, &cmd.result, timeout);
+
+	if (cmd.opcode == nvme_admin_format_nvm)
+		nvme_unfreeze(ctrl);
+

Testing some obscure vendor specific IO commands, I noticed this will deadlock if an IO command opcode happens to be the same as an nvme_admin_format_nvm command since the IO queues are all frozen.

You can check 'ns == NULL' before checking the opcode.

I also have this patch that's more generic and will perform actions based on the controller's command effects:

drivers/nvme/host/core.c +11 -0

+static void nvme_get_admin_effects(struct nvme_ctrl *ctrl, u8 opcode, u32 *effects)
+{
+	if (ctrl->effects)
+		*effects = le32_to_cpu(ctrl->effects->acs[opcode]);
+
+	if (*effects != 0)
+		return;
+
+	/*
+	 * Set known effects for opcodes if the controller doesn't support
+	 * reporting the command's effects.

If the controller does not support the command effects log page, the driver will define the effects for known opcodes. The nvme format is the only such command in this patch with known effects.

+static void nvme_get_admin_effects(struct nvme_ctrl *ctrl, u8 opcode, u32 *effects)

drivers/nvme/host/core.c +10 -0

+{
+	if (ctrl->effects)
+		*effects = le32_to_cpu(ctrl->effects->acs[opcode]);
+
+	if (*effects != 0)
+		return;
+
+	/*
+	 * Set known effects for opcodes if the controller doesn't support
+	 * reporting the command's effects.

+static int nvme_get_log(struct nvme_ctrl *ctrl, u8 log_page, void *log, size_t size)

drivers/nvme/host/core.c +1 -2

+static void nvme_scan(struct nvme_ctrl *ctrl)
 {
-	struct nvme_ctrl *ctrl =
-		container_of(work, struct nvme_ctrl, scan_work);
 	struct nvme_id_ctrl *id;
 	unsigned nn;

Quoted 3 lines

If the controller does not support the command effects log page, the driver will define the effects for known opcodes. The nvme format is the only such command in this patch with known effects.

Sanitize is another one. Also Format might either affect a single namespace or the whole controller depending on what it advertises in FNA.

static u32 nvme_get_admin_effects((struct nvme_ctrl *ctrl, u8 opcode)

{
	if (ctrl->effects)
		return le32_to_cpu(ctrl->effects->acs[opcode]);

	switch (opcode) {
		...
	}
}

That is: a) return the value instead of pass by reference, and
b) if the controller supports the log page rely on it.

Also don't we need to also handle this for I/O commands? While non of the currently defined I/O commands would need anything, the spec defines the mechanism? and it might be useful for vendor specific commands

drivers/nvme/host/core.c static int nvme_submit_io(struct nvme_ns *ns, struct nvme_user_io __user *uio) · line 1017

Quoted 7 lines

drivers/nvme/host/core.c +7 -0

+static void nvme_passthru_start(struct nvme_ctrl *ctrl, u32 effects)
+{
+	if (effects & (NVME_CMD_FX_LBCC | NVME_CMD_FX_CSE_MASK)) {
+		nvme_start_freeze(ctrl);
+		nvme_wait_freeze(ctrl);
+	}
+}

I'd move the nvme_get_admin_effects call into this, and return the value from this function to keep the caller a little more uncluttered.

Quoted 1 line

+static int nvme_get_log(struct nvme_ctrl *ctrl, u8 log_page, void *log, size_t size)

To loong line again. Also I think adding this helper should probably be a preparation patch.

drivers/nvme/host/core.c static void nvme_scan_work(struct work_struct *work) · line 2659

Quoted 7 lines

drivers/nvme/host/core.c +7 -0

+static void nvme_scan_work(struct work_struct *work)
+{
+	struct nvme_ctrl *ctrl =
+		container_of(work, struct nvme_ctrl, scan_work);
+
+	nvme_scan(ctrl);
+}

Why do we do the scan inline here, but not in any other place?

include/linux/nvme.h struct nvme_fw_slot_info_log { · line 405

Quoted 7 lines

enum {

include/linux/nvme.h +6 -0

+	NVME_CMD_FX_CSUPP	= 1 << 0,
+	NVME_CMD_FX_LBCC	= 1 << 1,
+	NVME_CMD_FX_NCC		= 1 << 2,
+	NVME_CMD_FX_NIC		= 1 << 3,
+	NVME_CMD_FX_CCC		= 1 << 4,
+	NVME_CMD_FX_CSE_MASK	= 3 << 16,

s/NVME_CMD_FX/NVME_CMD_EFFECTS/g ?

Okay, we can freeze single nvme namespace's request queues if desired. It's only a little more code to do that.

Quoted 4 lines

Also don't we need to also handle this for I/O commands? While non of the currently defined I/O commands would need anything, the spec defines the mechanism? and it might be useful for vendor specific commands

That gets tricky. What if the IO command effects says it needs to run exclusively? We don't have a way to quiesce IO queues and then issue our exclusive IO through the frozen queue. We'd deadlock trying to allocate a request from it.

If it's okay, I'd like to handle the IO command effects separately for future work.

And what about sanitize? I guess instead of looking at opcodes directly we'd better look at the indications in the Commands Supported and Effects log. That's what the Windows driver does, btw.

Quoted 3 lines · Christoph Hellwig

And what about sanitize? I guess instead of looking at opcodes directly we'd better look at the indications in the Commands Supported and Effects log. That's what the Windows driver does, btw.

Isn't that exactly what Keith's patch did?

Quoted 6 lines · Jens Axboe

Quoted 3 lines · Christoph Hellwig

And what about sanitize? I guess instead of looking at opcodes directly we'd better look at the indications in the Commands Supported and Effects log. That's what the Windows driver does, btw.

Isn't that exactly what Keith's patch did?

Ooops I had missed his patch was a completely different approach.

Prevents this by freezing IO access to the device around a format. Without this, the following set of commands can easily make your device disappear:

parted -s /dev/nvme3n1 mklabel gpt
parted -s /dev/nvme3n1 mkpart primary 0G 100G
parted -s /dev/nvme3n1 rm 1
nvme format /dev/nvme3

Quoted 8 lines · Jens Axboe

Prevents this by freezing IO access to the device around a format. Without this, the following set of commands can easily make your device disappear:

parted -s /dev/nvme3n1 mklabel gpt
parted -s /dev/nvme3n1 mkpart primary 0G 100G
parted -s /dev/nvme3n1 rm 1
nvme format /dev/nvme3

Can you wire this test case up for blktests?