[PATCH] nvme: don't flush scan work with non-idle request

[PATCH] nvme: don't flush scan work with non-idle request

[Keith Busch]

From: Keith Busch <kbusch@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 moving the flush_work after the request has been freed.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=216354
Reported-by: Jonathan Derrick <Jonathan.Derrick@solidigm.com>
Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 drivers/nvme/host/core.c  |  5 ++---
 drivers/nvme/host/ioctl.c | 12 ++++++++++++
 2 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index af367b22871b..1143f625e195 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1121,12 +1121,11 @@ static void nvme_passthru_end(struct nvme_ctrl *ctrl, u32 effects,
 		nvme_remove_invalid_namespaces(ctrl, NVME_NSID_ALL);
 		mutex_unlock(&ctrl->scan_lock);
 	}
+
 	if (effects & NVME_CMD_EFFECTS_CCC)
 		nvme_init_ctrl_finish(ctrl);
-	if (effects & (NVME_CMD_EFFECTS_NIC | NVME_CMD_EFFECTS_NCC)) {
+	if (effects & (NVME_CMD_EFFECTS_NIC | NVME_CMD_EFFECTS_NCC))
 		nvme_queue_scan(ctrl);
-		flush_work(&ctrl->scan_work);
-	}
 
 	switch (cmd->common.opcode) {
 	case nvme_admin_set_features:
diff --git a/drivers/nvme/host/ioctl.c b/drivers/nvme/host/ioctl.c
index 27614bee7380..97febd5f41a3 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,6 +149,8 @@ static int nvme_submit_user_cmd(struct request_queue *q,
 		return PTR_ERR(req);
 
 	bio = req->bio;
+	ctrl = nvme_req(req)->ctrl;
+	effects = nvme_command_effects(ctrl, q->queuedata, cmd->common.opcode);
 
 	ret = nvme_execute_passthru_rq(req);
 
@@ -158,6 +162,14 @@ static int nvme_submit_user_cmd(struct request_queue *q,
 	if (bio)
 		blk_rq_unmap_user(bio);
 	blk_mq_free_request(req);
+
+	/*
+	 * Ensure the namespace inventory is up-to-date before returning if
+	 * this command can change it.
+	 */
+	if (ret >= 0 && effects & (NVME_CMD_EFFECTS_NIC | NVME_CMD_EFFECTS_NCC))
+		flush_work(&ctrl->scan_work);
+
 	return ret;
 }
 
-- 
2.30.2

Re: [PATCH] nvme: don't flush scan work with non-idle request

[Jonathan Derrick]

On 8/12/2022 12:21 PM, Keith Busch wrote:
> From: Keith Busch <kbusch@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 moving the flush_work after the request has been freed.
> 
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=216354
> Reported-by: Jonathan Derrick <Jonathan.Derrick@solidigm.com>
> Signed-off-by: Keith Busch <kbusch@kernel.org>

Seems to fix the repro case. Thanks Keith.

> ---
>   drivers/nvme/host/core.c  |  5 ++---
>   drivers/nvme/host/ioctl.c | 12 ++++++++++++
>   2 files changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index af367b22871b..1143f625e195 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -1121,12 +1121,11 @@ static void nvme_passthru_end(struct nvme_ctrl *ctrl, u32 effects,
>   		nvme_remove_invalid_namespaces(ctrl, NVME_NSID_ALL);
>   		mutex_unlock(&ctrl->scan_lock);
>   	}
> +
>   	if (effects & NVME_CMD_EFFECTS_CCC)
>   		nvme_init_ctrl_finish(ctrl);
> -	if (effects & (NVME_CMD_EFFECTS_NIC | NVME_CMD_EFFECTS_NCC)) {
> +	if (effects & (NVME_CMD_EFFECTS_NIC | NVME_CMD_EFFECTS_NCC))
>   		nvme_queue_scan(ctrl);
> -		flush_work(&ctrl->scan_work);
> -	}
>   
>   	switch (cmd->common.opcode) {
>   	case nvme_admin_set_features:
> diff --git a/drivers/nvme/host/ioctl.c b/drivers/nvme/host/ioctl.c
> index 27614bee7380..97febd5f41a3 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,6 +149,8 @@ static int nvme_submit_user_cmd(struct request_queue *q,
>   		return PTR_ERR(req);
>   
>   	bio = req->bio;
> +	ctrl = nvme_req(req)->ctrl;
> +	effects = nvme_command_effects(ctrl, q->queuedata, cmd->common.opcode);
>   
>   	ret = nvme_execute_passthru_rq(req);
>   
> @@ -158,6 +162,14 @@ static int nvme_submit_user_cmd(struct request_queue *q,
>   	if (bio)
>   		blk_rq_unmap_user(bio);
>   	blk_mq_free_request(req);
> +
> +	/*
> +	 * Ensure the namespace inventory is up-to-date before returning if
> +	 * this command can change it.
> +	 */
> +	if (ret >= 0 && effects & (NVME_CMD_EFFECTS_NIC | NVME_CMD_EFFECTS_NCC))
> +		flush_work(&ctrl->scan_work);
> +
>   	return ret;
>   }
>   

Re: [PATCH] nvme: don't flush scan work with non-idle request

[Chao Leng]

Looks good to me.
Reviewed-by: Chao Leng <lengchao@huawei.com>

On 2022/8/13 2:21, Keith Busch wrote:
> From: Keith Busch <kbusch@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 moving the flush_work after the request has been freed.
> 
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=216354
> Reported-by: Jonathan Derrick <Jonathan.Derrick@solidigm.com>
> Signed-off-by: Keith Busch <kbusch@kernel.org>
> ---
>   drivers/nvme/host/core.c  |  5 ++---
>   drivers/nvme/host/ioctl.c | 12 ++++++++++++
>   2 files changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index af367b22871b..1143f625e195 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -1121,12 +1121,11 @@ static void nvme_passthru_end(struct nvme_ctrl *ctrl, u32 effects,
>   		nvme_remove_invalid_namespaces(ctrl, NVME_NSID_ALL);
>   		mutex_unlock(&ctrl->scan_lock);
>   	}
> +
>   	if (effects & NVME_CMD_EFFECTS_CCC)
>   		nvme_init_ctrl_finish(ctrl);
> -	if (effects & (NVME_CMD_EFFECTS_NIC | NVME_CMD_EFFECTS_NCC)) {
> +	if (effects & (NVME_CMD_EFFECTS_NIC | NVME_CMD_EFFECTS_NCC))
>   		nvme_queue_scan(ctrl);
> -		flush_work(&ctrl->scan_work);
> -	}
>   
>   	switch (cmd->common.opcode) {
>   	case nvme_admin_set_features:
> diff --git a/drivers/nvme/host/ioctl.c b/drivers/nvme/host/ioctl.c
> index 27614bee7380..97febd5f41a3 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,6 +149,8 @@ static int nvme_submit_user_cmd(struct request_queue *q,
>   		return PTR_ERR(req);
>   
>   	bio = req->bio;
> +	ctrl = nvme_req(req)->ctrl;
> +	effects = nvme_command_effects(ctrl, q->queuedata, cmd->common.opcode);
>   
>   	ret = nvme_execute_passthru_rq(req);
>   
> @@ -158,6 +162,14 @@ static int nvme_submit_user_cmd(struct request_queue *q,
>   	if (bio)
>   		blk_rq_unmap_user(bio);
>   	blk_mq_free_request(req);
> +
> +	/*
> +	 * Ensure the namespace inventory is up-to-date before returning if
> +	 * this command can change it.
> +	 */
> +	if (ret >= 0 && effects & (NVME_CMD_EFFECTS_NIC | NVME_CMD_EFFECTS_NCC))
> +		flush_work(&ctrl->scan_work);
Maybe it is better to move nvme_queue_scan together here.
> +
>   	return ret;
>   }
>   
> 

Re: [PATCH] nvme: don't flush scan work with non-idle request

[Sagi Grimberg]

>> +
>> +    /*
>> +     * Ensure the namespace inventory is up-to-date before returning if
>> +     * this command can change it.
>> +     */
>> +    if (ret >= 0 && effects & (NVME_CMD_EFFECTS_NIC | 
>> NVME_CMD_EFFECTS_NCC))
>> +        flush_work(&ctrl->scan_work);
> Maybe it is better to move nvme_queue_scan together here.

I agree, why not move the scan here as well?

>> +
>>       return ret;
>>   }
>>
> 

Re: [PATCH] nvme: don't flush scan work with non-idle request

[Keith Busch]

On Wed, Aug 17, 2022 at 01:29:37PM +0300, Sagi Grimberg wrote:
> 
> > > +
> > > +    /*
> > > +     * Ensure the namespace inventory is up-to-date before returning if
> > > +     * this command can change it.
> > > +     */
> > > +    if (ret >= 0 && effects & (NVME_CMD_EFFECTS_NIC |
> > > NVME_CMD_EFFECTS_NCC))
> > > +        flush_work(&ctrl->scan_work);
> > Maybe it is better to move nvme_queue_scan together here.
> 
> I agree, why not move the scan here as well?

Then I'd need to duplicate this to nvmet_passthru_execute_cmd_work(), and I
didn't want to do that. :)

Re: [PATCH] nvme: don't flush scan work with non-idle request

[Sagi Grimberg]

>>>> +    /*
>>>> +     * Ensure the namespace inventory is up-to-date before returning if
>>>> +     * this command can change it.
>>>> +     */
>>>> +    if (ret >= 0 && effects & (NVME_CMD_EFFECTS_NIC |
>>>> NVME_CMD_EFFECTS_NCC))
>>>> +        flush_work(&ctrl->scan_work);
>>> Maybe it is better to move nvme_queue_scan together here.
>>
>> I agree, why not move the scan here as well?
> 
> Then I'd need to duplicate this to nvmet_passthru_execute_cmd_work(), and I
> didn't want to do that. :)

Got it,

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

Re: [PATCH] nvme: don't flush scan work with non-idle request

[Christoph Hellwig]

With this the flush_work is lost for the target passthrough case.

I also don't really like the double call to nvme_command_effects().

What about just removing nvme_execute_passthru_rq() and open coding
it in the two calles, with the nvme_passthru_end call moved until
after freeing the request?

Re: [PATCH] nvme: don't flush scan work with non-idle request

[Keith Busch]

On Sun, Aug 21, 2022 at 04:40:39PM +0200, Christoph Hellwig wrote:
> With this the flush_work is lost for the target passthrough case.
> 
> I also don't really like the double call to nvme_command_effects().
> 
> What about just removing nvme_execute_passthru_rq() and open coding
> it in the two calles, with the nvme_passthru_end call moved until
> after freeing the request?

I didn't think it mattered for passthrough because the host needs to flush the
scan work on its side before safely using the changed namespace. It doesn't
matter if the target side has flushed out the new namespace format because it's
not the one creating LBA command parameters.

Never-the-less, if you do prefer to have each caller open code the sequence, I
have that patch ready to send.

Re: [PATCH] nvme: don't flush scan work with non-idle request

[Christoph Hellwig]

On Mon, Aug 22, 2022 at 09:09:10AM -0600, Keith Busch wrote:
> On Sun, Aug 21, 2022 at 04:40:39PM +0200, Christoph Hellwig wrote:
> > With this the flush_work is lost for the target passthrough case.
> > 
> > I also don't really like the double call to nvme_command_effects().
> > 
> > What about just removing nvme_execute_passthru_rq() and open coding
> > it in the two calles, with the nvme_passthru_end call moved until
> > after freeing the request?
> 
> I didn't think it mattered for passthrough because the host needs to flush the
> scan work on its side before safely using the changed namespace. It doesn't
> matter if the target side has flushed out the new namespace format because it's
> not the one creating LBA command parameters.

Well, a passthrough controller is still also available locally, so I don't
think we can just skip the scan.

> Never-the-less, if you do prefer to have each caller open code the sequence, I
> have that patch ready to send.

It's not that I really like the open coding, but it was the first
thing that came to mind on how to fix the passthrough case.