Linux-NVME Archive on lore.kernel.org
 help / color / Atom feed
* [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	[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	[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, back to index

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

Linux-NVME Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-nvme/0 linux-nvme/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-nvme linux-nvme/ https://lore.kernel.org/linux-nvme \
		linux-nvme@lists.infradead.org linux-nvme@archiver.kernel.org
	public-inbox-index linux-nvme

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-nvme


AGPL code for this site: git clone https://public-inbox.org/ public-inbox