[PATCH] nvme: if_ready checks to fail io to deleting controller

[emilne@redhat.com (Ewan D. Milne)]

On Fri, 2018-07-20@15:49 -0700, James Smart wrote:
> The revised if_ready checks skipped over the case of returning
> error when the controller is being deleted. Instead it was returning
> BUSY, which caused the ios to retry, which caused the ns delete to
> hang waiting for the ios to drain.
> 
> Stack trace of hang looks like:
>  kworker/u64:2   D    0    74      2 0x80000000
>  Workqueue: nvme-delete-wq nvme_delete_ctrl_work [nvme_core]
>  Call Trace:
>   ? __schedule+0x26d/0x820
>   schedule+0x32/0x80
>   blk_mq_freeze_queue_wait+0x36/0x80
>   ? remove_wait_queue+0x60/0x60
>   blk_cleanup_queue+0x72/0x160
>   nvme_ns_remove+0x106/0x140 [nvme_core]
>   nvme_remove_namespaces+0x7e/0xa0 [nvme_core]
>   nvme_delete_ctrl_work+0x4d/0x80 [nvme_core]
>   process_one_work+0x160/0x350
>   worker_thread+0x1c3/0x3d0
>   kthread+0xf5/0x130
>   ? process_one_work+0x350/0x350
>   ? kthread_bind+0x10/0x10
>   ret_from_fork+0x1f/0x30
> 
> Extend nvmf_fail_nonready_command() to supply the controller pointer
> so that the controller state can be looked at. Fail any io to a
> controller that is deleting.
> 
> Fixes: 3bc32bb1186c  ("nvme-fabrics: refactor queue ready check")
> Fixes: 35897b920c8a  ("nvme-fabrics: fix and refine state checks in __nvmf_check_ready")
> Signed-off-by: James Smart <james.smart at broadcom.com>
> 
> ---
> This should go into 4.18 if possible, which is where the prior
> two patches were merged.
> ---
>  drivers/nvme/host/fabrics.c | 10 +++++++---
>  drivers/nvme/host/fabrics.h |  3 ++-
>  drivers/nvme/host/fc.c      |  2 +-
>  drivers/nvme/host/rdma.c    |  2 +-
>  drivers/nvme/target/loop.c  |  2 +-
>  5 files changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
> index 903eb4545e26..f7efe5a58cc7 100644
> --- a/drivers/nvme/host/fabrics.c
> +++ b/drivers/nvme/host/fabrics.c
> @@ -539,14 +539,18 @@ static struct nvmf_transport_ops *nvmf_lookup_transport(
>  /*
>   * For something we're not in a state to send to the device the default action
>   * is to busy it and retry it after the controller state is recovered.  However,
> - * anything marked for failfast or nvme multipath is immediately failed.
> + * if the controller is deleting or if anything is marked for failfast or
> + * nvme multipath it is immediately failed.
>   *
>   * Note: commands used to initialize the controller will be marked for failfast.
>   * Note: nvme cli/ioctl commands are marked for failfast.
>   */
> -blk_status_t nvmf_fail_nonready_command(struct request *rq)
> +blk_status_t nvmf_fail_nonready_command(struct nvme_ctrl *ctrl,
> +		struct request *rq)
>  {
> -	if (!blk_noretry_request(rq) && !(rq->cmd_flags & REQ_NVME_MPATH))
> +	if (ctrl->state != NVME_CTRL_DELETING &&
> +	    ctrl->state != NVME_CTRL_DEAD &&
> +	    !blk_noretry_request(rq) && !(rq->cmd_flags & REQ_NVME_MPATH))
>  		return BLK_STS_RESOURCE;
>  	nvme_req(rq)->status = NVME_SC_ABORT_REQ;
>  	return BLK_STS_IOERR;
> diff --git a/drivers/nvme/host/fabrics.h b/drivers/nvme/host/fabrics.h
> index e1818a27aa2d..aa2fdb2a2e8f 100644
> --- a/drivers/nvme/host/fabrics.h
> +++ b/drivers/nvme/host/fabrics.h
> @@ -162,7 +162,8 @@ void nvmf_unregister_transport(struct nvmf_transport_ops *ops);
>  void nvmf_free_options(struct nvmf_ctrl_options *opts);
>  int nvmf_get_address(struct nvme_ctrl *ctrl, char *buf, int size);
>  bool nvmf_should_reconnect(struct nvme_ctrl *ctrl);
> -blk_status_t nvmf_fail_nonready_command(struct request *rq);
> +blk_status_t nvmf_fail_nonready_command(struct nvme_ctrl *ctrl,
> +		struct request *rq);
>  bool __nvmf_check_ready(struct nvme_ctrl *ctrl, struct request *rq,
>  		bool queue_live);
>  
> diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
> index 9cc33752539a..611e70cae754 100644
> --- a/drivers/nvme/host/fc.c
> +++ b/drivers/nvme/host/fc.c
> @@ -2273,7 +2273,7 @@ nvme_fc_queue_rq(struct blk_mq_hw_ctx *hctx,
>  
>  	if (ctrl->rport->remoteport.port_state != FC_OBJSTATE_ONLINE ||
>  	    !nvmf_check_ready(&queue->ctrl->ctrl, rq, queue_ready))
> -		return nvmf_fail_nonready_command(rq);
> +		return nvmf_fail_nonready_command(&queue->ctrl->ctrl, rq);
>  
>  	ret = nvme_setup_cmd(ns, rq, sqe);
>  	if (ret)
> diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
> index 13a6064e4794..0805fa6215ee 100644
> --- a/drivers/nvme/host/rdma.c
> +++ b/drivers/nvme/host/rdma.c
> @@ -1702,7 +1702,7 @@ static blk_status_t nvme_rdma_queue_rq(struct blk_mq_hw_ctx *hctx,
>  	WARN_ON_ONCE(rq->tag < 0);
>  
>  	if (!nvmf_check_ready(&queue->ctrl->ctrl, rq, queue_ready))
> -		return nvmf_fail_nonready_command(rq);
> +		return nvmf_fail_nonready_command(&queue->ctrl->ctrl, rq);
>  
>  	dev = queue->device->dev;
>  	ib_dma_sync_single_for_cpu(dev, sqe->dma,
> diff --git a/drivers/nvme/target/loop.c b/drivers/nvme/target/loop.c
> index af7fbf4132b0..9908082b32c4 100644
> --- a/drivers/nvme/target/loop.c
> +++ b/drivers/nvme/target/loop.c
> @@ -162,7 +162,7 @@ static blk_status_t nvme_loop_queue_rq(struct blk_mq_hw_ctx *hctx,
>  	blk_status_t ret;
>  
>  	if (!nvmf_check_ready(&queue->ctrl->ctrl, req, queue_ready))
> -		return nvmf_fail_nonready_command(req);
> +		return nvmf_fail_nonready_command(&queue->ctrl->ctrl, req);
>  
>  	ret = nvme_setup_cmd(ns, req, &iod->cmd);
>  	if (ret)

Yes, that fixes it.  The problem is fairly easy to reproduce with actual NVMe/FC hardware:

while (true) do
  nvme connect -t fc -a "nn-0x20000090fac7dbce:pn-0x10000090fac7dbce" -w "nn-0x20000090fac7dacc:pn-0x10000090fac7dacc" -n testnqn
  nvme disconnect -d nvme1n1
done

would typically fail in 30 or so iterations on my machines.

Tested-by: Ewan D. Milne <emilne at redhat.com>
Reviewed-by: Ewan D. Milne <emilne at redhat.com>