* [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).