* [PATCH] nvme: set NVME_REQ_CANCELLED flag when cancelling a request
@ 2019-09-20 18:19 Sagi Grimberg
2019-09-22 4:26 ` Chaitanya Kulkarni
0 siblings, 1 reply; 5+ messages in thread
From: Sagi Grimberg @ 2019-09-20 18:19 UTC (permalink / raw)
To: linux-nvme; +Cc: Keith Busch, Christoph Hellwig
This will prevent userspace to wrongly get the completion result
when we cancelled the command. Also, make sure to clear it from
the request flags in nvme_setup_cmd.
Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
---
drivers/nvme/host/core.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index f85e7b09d1d4..fc3ed75ef80b 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -298,6 +298,7 @@ bool nvme_cancel_request(struct request *req, void *data, bool reserved)
if (blk_mq_request_completed(req))
return true;
+ nvme_req(req)->flags |= NVME_REQ_CANCELLED;
nvme_req(req)->status = NVME_SC_HOST_PATH_ERROR;
blk_mq_complete_request(req);
return true;
@@ -434,6 +435,7 @@ static inline void nvme_clear_nvme_request(struct request *req)
nvme_req(req)->flags = 0;
req->rq_flags |= RQF_DONTPREP;
}
+ nvme_req(req)->flags &= ~NVME_REQ_CANCELLED;
}
struct request *nvme_alloc_request(struct request_queue *q,
--
2.17.1
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] nvme: set NVME_REQ_CANCELLED flag when cancelling a request
2019-09-20 18:19 [PATCH] nvme: set NVME_REQ_CANCELLED flag when cancelling a request Sagi Grimberg
@ 2019-09-22 4:26 ` Chaitanya Kulkarni
2019-09-23 17:04 ` Sagi Grimberg
0 siblings, 1 reply; 5+ messages in thread
From: Chaitanya Kulkarni @ 2019-09-22 4:26 UTC (permalink / raw)
To: Sagi Grimberg, linux-nvme; +Cc: Keith Busch, Christoph Hellwig
There are couple of places where we explicitly set the
NVME_REQ_CANCALLED which is followed by the call to
nvme_dev_disable(). Since :-
nvme_dev_disable()
blk_mq_tagset_busy_iter()
nvme_cancel_requets()
with your patch nvme_cancel_request() will duplicate
NVME_REQ_CANCELLED assignment, we need to avoid duplicate
assignment. Following patch is on the top of yours, please
have a look :-
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 6447702..4c71bbd 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -1286,7 +1286,6 @@ static enum blk_eh_timer_return
nvme_timeout(struct request *req, bool reserved
"I/O %d QID %d timeout, disable controller\n",
req->tag, nvmeq->qid);
nvme_dev_disable(dev, true);
- nvme_req(req)->flags |= NVME_REQ_CANCELLED;
return BLK_EH_DONE;
case NVME_CTRL_RESETTING:
return BLK_EH_RESET_TIMER;
@@ -1306,7 +1305,6 @@ static enum blk_eh_timer_return
nvme_timeout(struct request *req, bool reserved
nvme_dev_disable(dev, false);
nvme_reset_ctrl(&dev->ctrl);
- nvme_req(req)->flags |= NVME_REQ_CANCELLED;
return BLK_EH_DONE;
}
If you are okay I can send a separate patch on the top yours.
On 09/20/2019 11:19 AM, Sagi Grimberg wrote:
> This will prevent userspace to wrongly get the completion result
> when we cancelled the command. Also, make sure to clear it from
> the request flags in nvme_setup_cmd.
>
> Signed-off-by: Sagi Grimberg<sagi@grimberg.me>
> ---
> drivers/nvme/host/core.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index f85e7b09d1d4..fc3ed75ef80b 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -298,6 +298,7 @@ bool nvme_cancel_request(struct request *req, void *data, bool reserved)
> if (blk_mq_request_completed(req))
> return true;
>
> + nvme_req(req)->flags |= NVME_REQ_CANCELLED;
> nvme_req(req)->status = NVME_SC_HOST_PATH_ERROR;
> blk_mq_complete_request(req);
> return true;
> @@ -434,6 +435,7 @@ static inline void nvme_clear_nvme_request(struct request *req)
> nvme_req(req)->flags = 0;
> req->rq_flags |= RQF_DONTPREP;
> }
> + nvme_req(req)->flags &= ~NVME_REQ_CANCELLED;
> }
>
> struct request *nvme_alloc_request(struct request_queue *q,
> -- 2.17.1 _______________________________________________ Linux-nvme
> mailing list Linux-nvme@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-nvme
>
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] nvme: set NVME_REQ_CANCELLED flag when cancelling a request
2019-09-22 4:26 ` Chaitanya Kulkarni
@ 2019-09-23 17:04 ` Sagi Grimberg
2019-09-27 21:32 ` Christoph Hellwig
0 siblings, 1 reply; 5+ messages in thread
From: Sagi Grimberg @ 2019-09-23 17:04 UTC (permalink / raw)
To: Chaitanya Kulkarni, linux-nvme; +Cc: Keith Busch, Christoph Hellwig
> There are couple of places where we explicitly set the
> NVME_REQ_CANCALLED which is followed by the call to
> nvme_dev_disable(). Since :-
>
> nvme_dev_disable()
> blk_mq_tagset_busy_iter()
> nvme_cancel_requets()
>
> with your patch nvme_cancel_request() will duplicate
> NVME_REQ_CANCELLED assignment, we need to avoid duplicate
> assignment. Following patch is on the top of yours, please
> have a look :-
>
> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> index 6447702..4c71bbd 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -1286,7 +1286,6 @@ static enum blk_eh_timer_return
> nvme_timeout(struct request *req, bool reserved
> "I/O %d QID %d timeout, disable controller\n",
> req->tag, nvmeq->qid);
> nvme_dev_disable(dev, true);
> - nvme_req(req)->flags |= NVME_REQ_CANCELLED;
> return BLK_EH_DONE;
> case NVME_CTRL_RESETTING:
> return BLK_EH_RESET_TIMER;
> @@ -1306,7 +1305,6 @@ static enum blk_eh_timer_return
> nvme_timeout(struct request *req, bool reserved
> nvme_dev_disable(dev, false);
> nvme_reset_ctrl(&dev->ctrl);
>
> - nvme_req(req)->flags |= NVME_REQ_CANCELLED;
> return BLK_EH_DONE;
> }
>
> If you are okay I can send a separate patch on the top yours.
That is fine with me.
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] nvme: set NVME_REQ_CANCELLED flag when cancelling a request
2019-09-23 17:04 ` Sagi Grimberg
@ 2019-09-27 21:32 ` Christoph Hellwig
2019-09-27 22:15 ` Chaitanya Kulkarni
0 siblings, 1 reply; 5+ messages in thread
From: Christoph Hellwig @ 2019-09-27 21:32 UTC (permalink / raw)
To: Sagi Grimberg
Cc: Keith Busch, linux-nvme, Chaitanya Kulkarni, Christoph Hellwig
On Mon, Sep 23, 2019 at 10:04:19AM -0700, Sagi Grimberg wrote:
>> If you are okay I can send a separate patch on the top yours.
>
> That is fine with me.
I think we really should have the whole thing in one patch one way or
another.
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] nvme: set NVME_REQ_CANCELLED flag when cancelling a request
2019-09-27 21:32 ` Christoph Hellwig
@ 2019-09-27 22:15 ` Chaitanya Kulkarni
0 siblings, 0 replies; 5+ messages in thread
From: Chaitanya Kulkarni @ 2019-09-27 22:15 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: Keith Busch, Sagi Grimberg, linux-nvme
Okay. We can add missing changes at the time of apply I am fine anyway.
> On Sep 27, 2019, at 2:32 PM, Christoph Hellwig <hch@lst.de> wrote:
>
> On Mon, Sep 23, 2019 at 10:04:19AM -0700, Sagi Grimberg wrote:
>>> If you are okay I can send a separate patch on the top yours.
>>
>> That is fine with me.
>
> I think we really should have the whole thing in one patch one way or
> another.
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-09-27 22:15 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-20 18:19 [PATCH] nvme: set NVME_REQ_CANCELLED flag when cancelling a request Sagi Grimberg
2019-09-22 4:26 ` Chaitanya Kulkarni
2019-09-23 17:04 ` Sagi Grimberg
2019-09-27 21:32 ` Christoph Hellwig
2019-09-27 22:15 ` Chaitanya Kulkarni
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).