[PATCH] nvme-multipath: fix io accounting on failover

[PATCH] nvme-multipath: fix io accounting on failover

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

There are io stats accounting that needs to be handled, so don't call
blk_mq_end_request() directly. Use the existing nvme_end_req() helper
that already handles everything.

Fixes: d4d957b53d91ee ("nvme-multipath: support io stats on the mpath device")
Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 drivers/nvme/host/core.c      | 2 +-
 drivers/nvme/host/multipath.c | 3 ++-
 drivers/nvme/host/nvme.h      | 1 +
 3 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 79cdd34dfa18e..7706df2373494 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -422,7 +422,7 @@ static inline void __nvme_end_req(struct request *req)
 		nvme_mpath_end_request(req);
 }
 
-static inline void nvme_end_req(struct request *req)
+void nvme_end_req(struct request *req)
 {
 	blk_status_t status = nvme_error_status(nvme_req(req)->status);
 
diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
index 9c1e135b8df3b..1bee176fd850e 100644
--- a/drivers/nvme/host/multipath.c
+++ b/drivers/nvme/host/multipath.c
@@ -118,7 +118,8 @@ void nvme_failover_req(struct request *req)
 	blk_steal_bios(&ns->head->requeue_list, req);
 	spin_unlock_irqrestore(&ns->head->requeue_lock, flags);
 
-	blk_mq_end_request(req, 0);
+	nvme_req(req)->status = 0;
+	nvme_end_req(req);
 	kblockd_schedule_work(&ns->head->requeue_work);
 }
 
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index cacc56f4bbf44..fc31bd340a63a 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -767,6 +767,7 @@ static inline bool nvme_state_terminal(struct nvme_ctrl *ctrl)
 	}
 }
 
+void nvme_end_req(struct request *req);
 void nvme_complete_rq(struct request *req);
 void nvme_complete_batch_req(struct request *req);
 
-- 
2.43.0

Re: [PATCH] nvme-multipath: fix io accounting on failover

[John Meneghini]

Awesome. I've noticed that there is an iostat bug lurking someplace during my many days of io policy testing these last two 
weeks. I'll add this patch to my test build and let you know if it fixes the problem!

/John

On 5/21/24 14:07, Keith Busch wrote:> From: Keith Busch <kbusch@kernel.org>
> 
> There are io stats accounting that needs to be handled, so don't call
> blk_mq_end_request() directly. Use the existing nvme_end_req() helper
> that already handles everything.
> 
> Fixes: d4d957b53d91ee ("nvme-multipath: support io stats on the mpath device")
> Signed-off-by: Keith Busch <kbusch@kernel.org>
> ---
>   drivers/nvme/host/core.c      | 2 +-
>   drivers/nvme/host/multipath.c | 3 ++-
>   drivers/nvme/host/nvme.h      | 1 +
>   3 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 79cdd34dfa18e..7706df2373494 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -422,7 +422,7 @@ static inline void __nvme_end_req(struct request *req)
>   		nvme_mpath_end_request(req);
>   }
>   
> -static inline void nvme_end_req(struct request *req)
> +void nvme_end_req(struct request *req)
>   {
>   	blk_status_t status = nvme_error_status(nvme_req(req)->status);
>   
> diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
> index 9c1e135b8df3b..1bee176fd850e 100644
> --- a/drivers/nvme/host/multipath.c
> +++ b/drivers/nvme/host/multipath.c
> @@ -118,7 +118,8 @@ void nvme_failover_req(struct request *req)
>   	blk_steal_bios(&ns->head->requeue_list, req);
>   	spin_unlock_irqrestore(&ns->head->requeue_lock, flags);
>   
> -	blk_mq_end_request(req, 0);
> +	nvme_req(req)->status = 0;
> +	nvme_end_req(req);
>   	kblockd_schedule_work(&ns->head->requeue_work);
>   }
>   
> diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
> index cacc56f4bbf44..fc31bd340a63a 100644
> --- a/drivers/nvme/host/nvme.h
> +++ b/drivers/nvme/host/nvme.h
> @@ -767,6 +767,7 @@ static inline bool nvme_state_terminal(struct nvme_ctrl *ctrl)
>   	}
>   }
>   
> +void nvme_end_req(struct request *req);
>   void nvme_complete_rq(struct request *req);
>   void nvme_complete_batch_req(struct request *req);
>   

Re: [PATCH] nvme-multipath: fix io accounting on failover

[Keith Busch]

On Tue, May 21, 2024 at 02:35:44PM -0400, John Meneghini wrote:
> Awesome. I've noticed that there is an iostat bug lurking someplace during
> my many days of io policy testing these last two weeks. I'll add this patch
> to my test build and let you know if it fixes the problem!

I was testing CMIC capable PCI devices. The nvme-pci driver is currently
the only one batching completions, and that had a different multipath
accounting bug. I had to fix that before I could validate this patch,
and this one has a dependency on that first fix, which is here:

  https://lore.kernel.org/linux-nvme/20240521170537.2029233-1-kbusch@meta.com/T/#u

Re: [PATCH] nvme-multipath: fix io accounting on failover

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH] nvme-multipath: fix io accounting on failover

[Sagi Grimberg]

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

Re: [PATCH] nvme-multipath: fix io accounting on failover

[Nilay Shroff]

On 5/21/24 23:37, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
> 
> There are io stats accounting that needs to be handled, so don't call
> blk_mq_end_request() directly. Use the existing nvme_end_req() helper
> that already handles everything.
> 
The changes look good however I have a question about why do we retry an IO
when that IO is cancelled? For instance, when a multipath IO request is cancelled 
(from nvme_cancel_request()) we re-queue the bio in nvme_failover_req().
Similarly, for non-multipath request, we do retry request in nvme_retry_req()
until retries for a request are maxed out by nvme_max_retries. So wouldn't it be 
appropriate to drop the cancelled request instead of retrying? 

However, I do understand retrying a request on a different path when we got the 
request completion status specifying the path related error.

Thanks,
--Nilay

Re: [PATCH] nvme-multipath: fix io accounting on failover

[Keith Busch]

On Wed, May 22, 2024 at 06:32:11PM +0530, Nilay Shroff wrote:
> 
> 
> On 5/21/24 23:37, Keith Busch wrote:
> > From: Keith Busch <kbusch@kernel.org>
> > 
> > There are io stats accounting that needs to be handled, so don't call
> > blk_mq_end_request() directly. Use the existing nvme_end_req() helper
> > that already handles everything.
> > 
> The changes look good however I have a question about why do we retry an IO
> when that IO is cancelled? For instance, when a multipath IO request is cancelled 
> (from nvme_cancel_request()) we re-queue the bio in nvme_failover_req().
> Similarly, for non-multipath request, we do retry request in nvme_retry_req()
> until retries for a request are maxed out by nvme_max_retries. So wouldn't it be 
> appropriate to drop the cancelled request instead of retrying? 
> 
> However, I do understand retrying a request on a different path when we got the 
> request completion status specifying the path related error.

A cancelled request just means the host thinks the target failed to
produce a response. It doesn't mean the host stopped caring about the
command; the host still wants it to succeed, but determined corrective
action is needed to reclaim and resubmit the command.

Re: [PATCH] nvme-multipath: fix io accounting on failover

[Nilay Shroff]

On 5/22/24 19:48, Keith Busch wrote:
> On Wed, May 22, 2024 at 06:32:11PM +0530, Nilay Shroff wrote:
>>
>>
>> On 5/21/24 23:37, Keith Busch wrote:
>>> From: Keith Busch <kbusch@kernel.org>
>>>
>>> There are io stats accounting that needs to be handled, so don't call
>>> blk_mq_end_request() directly. Use the existing nvme_end_req() helper
>>> that already handles everything.
>>>
>> The changes look good however I have a question about why do we retry an IO
>> when that IO is cancelled? For instance, when a multipath IO request is cancelled 
>> (from nvme_cancel_request()) we re-queue the bio in nvme_failover_req().
>> Similarly, for non-multipath request, we do retry request in nvme_retry_req()
>> until retries for a request are maxed out by nvme_max_retries. So wouldn't it be 
>> appropriate to drop the cancelled request instead of retrying? 
>>
>> However, I do understand retrying a request on a different path when we got the 
>> request completion status specifying the path related error.
> 
> A cancelled request just means the host thinks the target failed to
> produce a response. It doesn't mean the host stopped caring about the
> command; the host still wants it to succeed, but determined corrective
> action is needed to reclaim and resubmit the command.
> 
Thank Keith, got it!

--Nilay