[PATCH] nvme-tcp: handle failing req immediately for REQ_FAILFAST_DRIVER
Sagi Grimberg
sagi at grimberg.me
Mon Apr 11 02:58:09 PDT 2022
>>> But in your case the queue is live...
>>
>> Could we not just test for NVME_CTRL_ADMIN_Q_STOPPED instead as it is
>> set in nvme_stop_admin_queue:
>>
>> nvme_tcp_teardown_ctrl
>> nvme_stop_admin_queue
>> nvme_shutdown_ctrl
>>
>> instead?
>>
>>> I'm thinking that we maybe need a register access timeout value for
>>> fabrics...
>>
>> I see there is a shutdown_timeout which is 5 seconds on default. Seems
>> not to trigger though.
>
> Obviously, the shutdown_timeout is not connected to the register
> writes. So we would need something like:
Looking at this, its probably not what we want...
I've inspected the code and I don't think that we should check
the return code at all, we should simply fail the request. If
the io_work context is running in parallel with the the error
recovery handler we have a bug that we need to fix.
How about this?
--
diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
index 10fc45d95b86..6d59867bb275 100644
--- a/drivers/nvme/host/tcp.c
+++ b/drivers/nvme/host/tcp.c
@@ -1147,8 +1147,7 @@ static int nvme_tcp_try_send(struct nvme_tcp_queue
*queue)
} else if (ret < 0) {
dev_err(queue->ctrl->ctrl.device,
"failed to send request %d\n", ret);
- if (ret != -EPIPE && ret != -ECONNRESET)
- nvme_tcp_fail_request(queue->request);
+ nvme_tcp_fail_request(req);
nvme_tcp_done_send_req(queue);
}
return ret;
--
Adding Anton,
Anton, do you see an issue with this change? io_work is safely
fenced from the cancellation iter so there is no reason for
us not to fail the request here when the socket closes (or
any other reason for that matter).
>
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index f204c6f78b5b..a146ea25f386 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -2133,7 +2133,8 @@ static int nvme_wait_ready(struct nvme_ctrl *ctrl, u64 cap, bool enabled)
> u32 csts, bit = enabled ? NVME_CSTS_RDY : 0;
> int ret;
>
> - while ((ret = ctrl->ops->reg_read32(ctrl, NVME_REG_CSTS, &csts)) == 0) {
> + while ((ret = ctrl->ops->reg_read32(ctrl, NVME_REG_CSTS,
> + &csts, timeout)) == 0) {
> if (csts == ~0)
> return -ENODEV;
> if ((csts & NVME_CSTS_RDY) == bit)
> @@ -2166,7 +2167,8 @@ int nvme_disable_ctrl(struct nvme_ctrl *ctrl)
> ctrl->ctrl_config &= ~NVME_CC_SHN_MASK;
> ctrl->ctrl_config &= ~NVME_CC_ENABLE;
>
> - ret = ctrl->ops->reg_write32(ctrl, NVME_REG_CC, ctrl->ctrl_config);
> + ret = ctrl->ops->reg_write32(ctrl, NVME_REG_CC, ctrl->ctrl_config,
> + NVME_REG_ACCESS_TIMEOUT);
> if (ret)
> return ret;
>
> @@ -2182,7 +2184,8 @@ int nvme_enable_ctrl(struct nvme_ctrl *ctrl)
> unsigned dev_page_min;
> int ret;
>
> - ret = ctrl->ops->reg_read64(ctrl, NVME_REG_CAP, &ctrl->cap);
> + ret = ctrl->ops->reg_read64(ctrl, NVME_REG_CAP, &ctrl->cap,
> + NVME_REG_ACCESS_TIMEOUT);
> if (ret) {
> dev_err(ctrl->device, "Reading CAP failed (%d)\n", ret);
> return ret;
> @@ -2205,7 +2208,8 @@ int nvme_enable_ctrl(struct nvme_ctrl *ctrl)
> ctrl->ctrl_config |= NVME_CC_IOSQES | NVME_CC_IOCQES;
> ctrl->ctrl_config |= NVME_CC_ENABLE;
>
> - ret = ctrl->ops->reg_write32(ctrl, NVME_REG_CC, ctrl->ctrl_config);
> + ret = ctrl->ops->reg_write32(ctrl, NVME_REG_CC, ctrl->ctrl_config,
> + NVME_REG_ACCESS_TIMEOUT);
> if (ret)
> return ret;
> return nvme_wait_ready(ctrl, ctrl->cap, true);
> @@ -2221,11 +2225,13 @@ int nvme_shutdown_ctrl(struct nvme_ctrl *ctrl)
> ctrl->ctrl_config &= ~NVME_CC_SHN_MASK;
> ctrl->ctrl_config |= NVME_CC_SHN_NORMAL;
>
> - ret = ctrl->ops->reg_write32(ctrl, NVME_REG_CC, ctrl->ctrl_config);
> + ret = ctrl->ops->reg_write32(ctrl, NVME_REG_CC, ctrl->ctrl_config,
> + NVME_REG_ACCESS_TIMEOUT);
> if (ret)
> return ret;
>
> - while ((ret = ctrl->ops->reg_read32(ctrl, NVME_REG_CSTS, &csts)) == 0) {
> + while ((ret = ctrl->ops->reg_read32(ctrl, NVME_REG_CSTS, &csts,
> + NVME_REG_ACCESS_TIMEOUT)) == 0) {
> if ((csts & NVME_CSTS_SHST_MASK) == NVME_CSTS_SHST_CMPLT)
> break;
>
> @@ -3084,7 +3090,8 @@ int nvme_init_ctrl_finish(struct nvme_ctrl *ctrl)
> {
> int ret;
>
> - ret = ctrl->ops->reg_read32(ctrl, NVME_REG_VS, &ctrl->vs);
> + ret = ctrl->ops->reg_read32(ctrl, NVME_REG_VS, &ctrl->vs,
> + NVME_REG_ACCESS_TIMEOUT);
> if (ret) {
> dev_err(ctrl->device, "Reading VS failed (%d)\n", ret);
> return ret;
> @@ -4386,7 +4393,8 @@ static bool nvme_ctrl_pp_status(struct nvme_ctrl *ctrl)
>
> u32 csts;
>
> - if (ctrl->ops->reg_read32(ctrl, NVME_REG_CSTS, &csts))
> + if (ctrl->ops->reg_read32(ctrl, NVME_REG_CSTS, &csts,
> + NVME_REG_ACCESS_TIMEOUT))
> return false;
>
> if (csts == ~0)
> diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
> index ee79a6d639b4..7882559e89bc 100644
> --- a/drivers/nvme/host/fabrics.c
> +++ b/drivers/nvme/host/fabrics.c
> @@ -142,7 +142,7 @@ EXPORT_SYMBOL_GPL(nvmf_get_address);
> * > 0: NVMe error status code
> * < 0: Linux errno error code
> */
> -int nvmf_reg_read32(struct nvme_ctrl *ctrl, u32 off, u32 *val)
> +int nvmf_reg_read32(struct nvme_ctrl *ctrl, u32 off, u32 *val, int timeout)
> {
> struct nvme_command cmd = { };
> union nvme_result res;
> @@ -152,8 +152,8 @@ int nvmf_reg_read32(struct nvme_ctrl *ctrl, u32 off, u32 *val)
> cmd.prop_get.fctype = nvme_fabrics_type_property_get;
> cmd.prop_get.offset = cpu_to_le32(off);
>
> - ret = __nvme_submit_sync_cmd(ctrl->fabrics_q, &cmd, &res, NULL, 0, 0,
> - NVME_QID_ANY, 0, 0);
> + ret = __nvme_submit_sync_cmd(ctrl->fabrics_q, &cmd, &res, NULL, 0,
> + timeout, NVME_QID_ANY, 0, 0);
>
> if (ret >= 0)
> *val = le64_to_cpu(res.u64);
> @@ -187,7 +187,7 @@ EXPORT_SYMBOL_GPL(nvmf_reg_read32);
> * > 0: NVMe error status code
> * < 0: Linux errno error code
> */
> -int nvmf_reg_read64(struct nvme_ctrl *ctrl, u32 off, u64 *val)
> +int nvmf_reg_read64(struct nvme_ctrl *ctrl, u32 off, u64 *val, int timeout)
> {
> struct nvme_command cmd = { };
> union nvme_result res;
> @@ -198,8 +198,8 @@ int nvmf_reg_read64(struct nvme_ctrl *ctrl, u32 off, u64 *val)
> cmd.prop_get.attrib = 1;
> cmd.prop_get.offset = cpu_to_le32(off);
>
> - ret = __nvme_submit_sync_cmd(ctrl->fabrics_q, &cmd, &res, NULL, 0, 0,
> - NVME_QID_ANY, 0, 0);
> + ret = __nvme_submit_sync_cmd(ctrl->fabrics_q, &cmd, &res, NULL, 0,
> + timeout, NVME_QID_ANY, 0, 0);
>
> if (ret >= 0)
> *val = le64_to_cpu(res.u64);
> @@ -232,7 +232,7 @@ EXPORT_SYMBOL_GPL(nvmf_reg_read64);
> * > 0: NVMe error status code
> * < 0: Linux errno error code
> */
> -int nvmf_reg_write32(struct nvme_ctrl *ctrl, u32 off, u32 val)
> +int nvmf_reg_write32(struct nvme_ctrl *ctrl, u32 off, u32 val, int timeout)
> {
> struct nvme_command cmd = { };
> int ret;
> @@ -243,8 +243,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",
> diff --git a/drivers/nvme/host/fabrics.h b/drivers/nvme/host/fabrics.h
> index c3203ff1c654..c2ae0c04cd4a 100644
> --- a/drivers/nvme/host/fabrics.h
> +++ b/drivers/nvme/host/fabrics.h
> @@ -186,9 +186,9 @@ static inline char *nvmf_ctrl_subsysnqn(struct nvme_ctrl *ctrl)
> return ctrl->subsys->subnqn;
> }
>
> -int nvmf_reg_read32(struct nvme_ctrl *ctrl, u32 off, u32 *val);
> -int nvmf_reg_read64(struct nvme_ctrl *ctrl, u32 off, u64 *val);
> -int nvmf_reg_write32(struct nvme_ctrl *ctrl, u32 off, u32 val);
> +int nvmf_reg_read32(struct nvme_ctrl *ctrl, u32 off, u32 *val, int timeout);
> +int nvmf_reg_read64(struct nvme_ctrl *ctrl, u32 off, u64 *val, int timeout);
> +int nvmf_reg_write32(struct nvme_ctrl *ctrl, u32 off, u32 val, int timeout);
> int nvmf_connect_admin_queue(struct nvme_ctrl *ctrl);
> int nvmf_connect_io_queue(struct nvme_ctrl *ctrl, u16 qid);
> int nvmf_register_transport(struct nvmf_transport_ops *ops);
> diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
> index a1f8403ffd78..f17a4df17e11 100644
> --- a/drivers/nvme/host/nvme.h
> +++ b/drivers/nvme/host/nvme.h
> @@ -27,6 +27,8 @@ extern unsigned int admin_timeout;
>
> #define NVME_DEFAULT_KATO 5
>
> +#define NVME_REG_ACCESS_TIMEOUT NVME_DEFAULT_KATO
> +
> #ifdef CONFIG_ARCH_NO_SG_CHAIN
> #define NVME_INLINE_SG_CNT 0
> #define NVME_INLINE_METADATA_SG_CNT 0
> @@ -489,9 +491,12 @@ struct nvme_ctrl_ops {
> #define NVME_F_FABRICS (1 << 0)
> #define NVME_F_METADATA_SUPPORTED (1 << 1)
> #define NVME_F_PCI_P2PDMA (1 << 2)
> - int (*reg_read32)(struct nvme_ctrl *ctrl, u32 off, u32 *val);
> - int (*reg_write32)(struct nvme_ctrl *ctrl, u32 off, u32 val);
> - int (*reg_read64)(struct nvme_ctrl *ctrl, u32 off, u64 *val);
> + int (*reg_read32)(struct nvme_ctrl *ctrl, u32 off, u32 *val,
> + int timeout);
> + int (*reg_write32)(struct nvme_ctrl *ctrl, u32 off, u32 val,
> + int timoeut);
> + int (*reg_read64)(struct nvme_ctrl *ctrl, u32 off, u64 *val,
> + int timeout);
> void (*free_ctrl)(struct nvme_ctrl *ctrl);
> void (*submit_async_event)(struct nvme_ctrl *ctrl);
> void (*delete_ctrl)(struct nvme_ctrl *ctrl);
> @@ -561,7 +566,8 @@ static inline int nvme_reset_subsystem(struct nvme_ctrl *ctrl)
> {
> if (!ctrl->subsystem)
> return -ENOTTY;
> - return ctrl->ops->reg_write32(ctrl, NVME_REG_NSSR, 0x4E564D65);
> + return ctrl->ops->reg_write32(ctrl, NVME_REG_NSSR, 0x4E564D65,
> + NVME_REG_ACCESS_TIMEOUT);
> }
>
> /*
> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> index 66f1eee2509a..b7246fee99ef 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -2943,19 +2943,22 @@ static void nvme_remove_dead_ctrl_work(struct work_struct *work)
> nvme_put_ctrl(&dev->ctrl);
> }
>
> -static int nvme_pci_reg_read32(struct nvme_ctrl *ctrl, u32 off, u32 *val)
> +static int nvme_pci_reg_read32(struct nvme_ctrl *ctrl, u32 off, u32 *val,
> + int timeout)
> {
> *val = readl(to_nvme_dev(ctrl)->bar + off);
> return 0;
> }
>
> -static int nvme_pci_reg_write32(struct nvme_ctrl *ctrl, u32 off, u32 val)
> +static int nvme_pci_reg_write32(struct nvme_ctrl *ctrl, u32 off, u32 val,
> + int timeout)
> {
> writel(val, to_nvme_dev(ctrl)->bar + off);
> return 0;
> }
>
> -static int nvme_pci_reg_read64(struct nvme_ctrl *ctrl, u32 off, u64 *val)
> +static int nvme_pci_reg_read64(struct nvme_ctrl *ctrl, u32 off, u64 *val,
> + int timeout)
> {
> *val = lo_hi_readq(to_nvme_dev(ctrl)->bar + off);
> return 0;
More information about the Linux-nvme
mailing list