[PATCHv2] nvme: handle effects after freeing the request

[Chao Leng]

Reviewed-by: Chao Leng <lengchao at huawei.com>

On 2022/9/20 3:36, Keith Busch wrote:
> From: Keith Busch <kbusch at kernel.org>
> 
> If a reset occurs after the scan work attempts to issue a command, the
> reset may quisce the admin queue, which blocks the scan work's command
> from dispatching. The scan work will not be able to complete while the
> queue is quiesced.
> 
> Meanwhile, the reset work will cancel all outstanding admin tags and
> wait until all requests have transitioned to idle, which includes the
> passthrough request. But the passthrough request won't be set to idle
> until after the scan_work flushes, so we're deadlocked.
> 
> Fix this by handling the end effects after the request has been freed.
> 
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=216354
> Reported-by: Jonathan Derrick <Jonathan.Derrick at solidigm.com>
> Signed-off-by: Keith Busch <kbusch at kernel.org>
> ---
> v1->v2:
> 
>    Open code the effects end handling in each of the callers instead of
>    special casing just the 'flush_work' part.
> 
>   drivers/nvme/host/core.c       | 17 ++++++-----------
>   drivers/nvme/host/ioctl.c      |  9 ++++++++-
>   drivers/nvme/host/nvme.h       |  4 +++-
>   drivers/nvme/target/passthru.c |  7 ++++++-
>   4 files changed, 23 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 8c9c1176624d..ea6694fd550f 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -1111,8 +1111,8 @@ static u32 nvme_passthru_start(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
>   	return effects;
>   }
>   
> -static void nvme_passthru_end(struct nvme_ctrl *ctrl, u32 effects,
> -			      struct nvme_command *cmd, int status)
> +void nvme_passthru_end(struct nvme_ctrl *ctrl, u32 effects,
> +		       struct nvme_command *cmd, int status)
>   {
>   	if (effects & NVME_CMD_EFFECTS_CSE_MASK) {
>   		nvme_unfreeze(ctrl);
> @@ -1148,21 +1148,16 @@ static void nvme_passthru_end(struct nvme_ctrl *ctrl, u32 effects,
>   		break;
>   	}
>   }
> +EXPORT_SYMBOL_NS_GPL(nvme_passthru_end, NVME_TARGET_PASSTHRU);
>   
> -int nvme_execute_passthru_rq(struct request *rq)
> +int nvme_execute_passthru_rq(struct request *rq, u32 *effects)
>   {
>   	struct nvme_command *cmd = nvme_req(rq)->cmd;
>   	struct nvme_ctrl *ctrl = nvme_req(rq)->ctrl;
>   	struct nvme_ns *ns = rq->q->queuedata;
> -	u32 effects;
> -	int  ret;
>   
> -	effects = nvme_passthru_start(ctrl, ns, cmd->common.opcode);
> -	ret = nvme_execute_rq(rq, false);
> -	if (effects) /* nothing to be done for zero cmd effects */
> -		nvme_passthru_end(ctrl, effects, cmd, ret);
> -
> -	return ret;
> +	*effects = nvme_passthru_start(ctrl, ns, cmd->common.opcode);
> +	return nvme_execute_rq(rq, false);
>   }
>   EXPORT_SYMBOL_NS_GPL(nvme_execute_passthru_rq, NVME_TARGET_PASSTHRU);
>   
> diff --git a/drivers/nvme/host/ioctl.c b/drivers/nvme/host/ioctl.c
> index 27614bee7380..d3281f87cd6e 100644
> --- a/drivers/nvme/host/ioctl.c
> +++ b/drivers/nvme/host/ioctl.c
> @@ -136,9 +136,11 @@ static int nvme_submit_user_cmd(struct request_queue *q,
>   		unsigned bufflen, void __user *meta_buffer, unsigned meta_len,
>   		u32 meta_seed, u64 *result, unsigned timeout, bool vec)
>   {
> +	struct nvme_ctrl *ctrl;
>   	struct request *req;
>   	void *meta = NULL;
>   	struct bio *bio;
> +	u32 effects;
>   	int ret;
>   
>   	req = nvme_alloc_user_request(q, cmd, ubuffer, bufflen, meta_buffer,
> @@ -147,8 +149,9 @@ static int nvme_submit_user_cmd(struct request_queue *q,
>   		return PTR_ERR(req);
>   
>   	bio = req->bio;
> +	ctrl = nvme_req(req)->ctrl;
>   
> -	ret = nvme_execute_passthru_rq(req);
> +	ret = nvme_execute_passthru_rq(req, &effects);
>   
>   	if (result)
>   		*result = le64_to_cpu(nvme_req(req)->result.u64);
> @@ -158,6 +161,10 @@ static int nvme_submit_user_cmd(struct request_queue *q,
>   	if (bio)
>   		blk_rq_unmap_user(bio);
>   	blk_mq_free_request(req);
> +
> +	if (effects)
> +		nvme_passthru_end(ctrl, effects, cmd, ret);
> +
>   	return ret;
>   }
>   
> diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
> index 1bdf714dcd9e..a0bf9560cf67 100644
> --- a/drivers/nvme/host/nvme.h
> +++ b/drivers/nvme/host/nvme.h
> @@ -1023,7 +1023,9 @@ static inline void nvme_auth_free(struct nvme_ctrl *ctrl) {};
>   
>   u32 nvme_command_effects(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
>   			 u8 opcode);
> -int nvme_execute_passthru_rq(struct request *rq);
> +int nvme_execute_passthru_rq(struct request *rq, u32 *effects);
> +void nvme_passthru_end(struct nvme_ctrl *ctrl, u32 effects,
> +		       struct nvme_command *cmd, int status);
>   struct nvme_ctrl *nvme_ctrl_from_file(struct file *file);
>   struct nvme_ns *nvme_find_get_ns(struct nvme_ctrl *ctrl, unsigned nsid);
>   void nvme_put_ns(struct nvme_ns *ns);
> diff --git a/drivers/nvme/target/passthru.c b/drivers/nvme/target/passthru.c
> index 6f39a29828b1..94d3153bae54 100644
> --- a/drivers/nvme/target/passthru.c
> +++ b/drivers/nvme/target/passthru.c
> @@ -215,9 +215,11 @@ static void nvmet_passthru_execute_cmd_work(struct work_struct *w)
>   {
>   	struct nvmet_req *req = container_of(w, struct nvmet_req, p.work);
>   	struct request *rq = req->p.rq;
> +	struct nvme_ctrl *ctrl = nvme_req(rq)->ctrl;
> +	u32 effects;
>   	int status;
>   
> -	status = nvme_execute_passthru_rq(rq);
> +	status = nvme_execute_passthru_rq(rq, &effects);
>   
>   	if (status == NVME_SC_SUCCESS &&
>   	    req->cmd->common.opcode == nvme_admin_identify) {
> @@ -238,6 +240,9 @@ static void nvmet_passthru_execute_cmd_work(struct work_struct *w)
>   	req->cqe->result = nvme_req(rq)->result;
>   	nvmet_req_complete(req, status);
>   	blk_mq_free_request(rq);
> +
> +	if (effects)
> +		nvme_passthru_end(ctrl, effects, req->cmd, status);
>   }
>   
>   static void nvmet_passthru_req_done(struct request *rq,
>