[PATCH] nvme-fabrics: Use shutdown timeout on disconnect

[PATCH] nvme-fabrics: Use shutdown timeout on disconnect

[Martin Belanger]

From: Martin Belanger <martin.belanger@dell.com>

Before disconnecting from a controller the driver may send commands
such as setting the NVME_CC_SHN_NORMAL bit.

However, if the controller cannot be reached (e.g. network down),
these commands will block for 1 minute, which is the default timeout
for a write opration. We should not be waiting for this long when
we're actually trying to disconnect.

The driver already specifies a shorter 5-second timeout to be used
during shutdowns (ctrl->shutdown_timeout).

This patch checks the state of the controller before performing a
write command, and if the state is NVME_CTRL_DELETING,
NVME_CTRL_DELETING_NOIO, or NVME_CTRL_DEAD, the shorter timeout
ctrl->shutdown_timeout is used instead of the default.

Signed-off-by: Martin Belanger <martin.belanger@dell.com>
---
 drivers/nvme/host/fabrics.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
index 668c6bb7a567..fa1ef8b5fd02 100644
--- a/drivers/nvme/host/fabrics.c
+++ b/drivers/nvme/host/fabrics.c
@@ -237,6 +237,19 @@ int nvmf_reg_write32(struct nvme_ctrl *ctrl, u32 off, u32 val)
 {
 	struct nvme_command cmd = { };
 	int ret;
+	unsigned int timeout = 0;
+
+	/* Use shutdown timeout when the controller is being deleted */
+	switch (ctrl->state) {
+	case NVME_CTRL_DELETING:
+	case NVME_CTRL_DELETING_NOIO:
+	case NVME_CTRL_DEAD:
+		timeout = jiffies + (ctrl->shutdown_timeout * HZ);
+		break;
+	default:
+		timeout = 0;
+		break;
+	}
 
 	cmd.prop_set.opcode = nvme_fabrics_command;
 	cmd.prop_set.fctype = nvme_fabrics_type_property_set;
@@ -244,8 +257,8 @@ int nvmf_reg_write32(struct nvme_ctrl *ctrl, u32 off, u32 val)
 	cmd.prop_set.offset = cpu_to_le32(off);
 	cmd.prop_set.value = cpu_to_le64(val);
 
-	ret = __nvme_submit_sync_cmd(ctrl->fabrics_q, &cmd, NULL, NULL, 0, 0,
-			NVME_QID_ANY, 0, 0);
+	ret = __nvme_submit_sync_cmd(ctrl->fabrics_q, &cmd, NULL, NULL, 0,
+				     timeout, NVME_QID_ANY, 0, 0);
 	if (unlikely(ret))
 		dev_err(ctrl->device,
 			"Property Set error: %d, offset %#x\n",
-- 
2.31.1


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvme-fabrics: Use shutdown timeout on disconnect

[Daniel Wagner]

On Thu, Sep 30, 2021 at 10:56:34AM -0400, Martin Belanger wrote:
> @@ -244,8 +257,8 @@ int nvmf_reg_write32(struct nvme_ctrl *ctrl, u32 off, u32 val)
>  	cmd.prop_set.offset = cpu_to_le32(off);
>  	cmd.prop_set.value = cpu_to_le64(val);
>  
> -	ret = __nvme_submit_sync_cmd(ctrl->fabrics_q, &cmd, NULL, NULL, 0, 0,
> -			NVME_QID_ANY, 0, 0);
> +	ret = __nvme_submit_sync_cmd(ctrl->fabrics_q, &cmd, NULL, NULL, 0,
> +				     timeout, NVME_QID_ANY, 0, 0);

If I am not mistaken nvmf_connect_io_queue() suffers from the same
problem.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvme-fabrics: Use shutdown timeout on disconnect

[Martin Belanger]

From: Daniel Wagner <dwagner@suse.de>
Sent: Friday, October 1, 2021 7:11 AM
To: Martin Belanger <nitram_67@hotmail.com>
Cc: linux-nvme@lists.infradead.org <linux-nvme@lists.infradead.org>; kbusch@kernel.org <kbusch@kernel.org>; axboe@fb.com <axboe@fb.com>; hch@lst.de <hch@lst.de>; sagi@grimberg.me <sagi@grimberg.me>; Martin Belanger <martin.belanger@dell.com>
Subject: Re: [PATCH] nvme-fabrics: Use shutdown timeout on disconnect 
 
On Thu, Sep 30, 2021 at 10:56:34AM -0400, Martin Belanger wrote:
> @@ -244,8 +257,8 @@ int nvmf_reg_write32(struct nvme_ctrl *ctrl, u32 off, u32 val)
>        cmd.prop_set.offset = cpu_to_le32(off);
>        cmd.prop_set.value = cpu_to_le64(val);
>  
> -     ret = __nvme_submit_sync_cmd(ctrl->fabrics_q, &cmd, NULL, NULL, 0, 0,
> -                     NVME_QID_ANY, 0, 0);
> +     ret = __nvme_submit_sync_cmd(ctrl->fabrics_q, &cmd, NULL, NULL, 0,
> +                                  timeout, NVME_QID_ANY, 0, 0);

If I am not mistaken nvmf_connect_io_queue() suffers from the same
problem.

What I'm trying to fix is the timeout when disconnecting. The disconnect timeout should be short so that we don't get blocked for 1 minute. nvmf_connect_io_queue() is used when establishing a connection, in which case we do want to use the default (longer) timeout.
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvme-fabrics: Use shutdown timeout on disconnect

[Hannes Reinecke]

On 10/1/21 1:11 PM, Daniel Wagner wrote:
> On Thu, Sep 30, 2021 at 10:56:34AM -0400, Martin Belanger wrote:
>> @@ -244,8 +257,8 @@ int nvmf_reg_write32(struct nvme_ctrl *ctrl, u32 off, u32 val)
>>   	cmd.prop_set.offset = cpu_to_le32(off);
>>   	cmd.prop_set.value = cpu_to_le64(val);
>>   
>> -	ret = __nvme_submit_sync_cmd(ctrl->fabrics_q, &cmd, NULL, NULL, 0, 0,
>> -			NVME_QID_ANY, 0, 0);
>> +	ret = __nvme_submit_sync_cmd(ctrl->fabrics_q, &cmd, NULL, NULL, 0,
>> +				     timeout, NVME_QID_ANY, 0, 0);
> 
> If I am not mistaken nvmf_connect_io_queue() suffers from the same
> problem.
> 
Hmm. Not a big fan, I must admit.

If the controller is _not_ live we cannot send commands. And especially 
if the controller is in DELETING I really can't see how we _could_ send 
commands at all.

Wouldn't it be better to ensure that __nvme_submit_sync_cmd() returns an 
appropriate error?

Cheers,

Hannes

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme