[PATCH v2 0/2] nvme-mpath: Add IO stats support

[PATCH v2 0/2] nvme-mpath: Add IO stats support

[Sagi Grimberg]

I've been hearing complaints about the fact that the nvme mpath stack
device does not expose IO stats just like any normal block device,
instead people need to check the bottom namespaces hidden devices,
mapping back to the mpath device node.

This really sucks, especially for observability hooks/plugins that
I've seen people do.

This series make the nvme mpath device expose normal IO stats. Given
that nvme-mpath doesn't have any context after submitting the bio, we
use the core completion path to start/end stats accounting on its
behalf, a similar practice that we use for other multipath related stuff.

While we are "double" accounting every request, this is a preferable
approach as opposed to try and locally collect/combine accurate stats
from multiple bottom hidden devices in the driver, or teach the block
layer to do so.

Local tests with null-blk + nvme-loop did not show any noticeable
performance degradation (within 1%).

Feedback is welcome.

Changes from v1:
- split into 2 patches, one prep and second is nvme-mpath stats
- fix possible use-after-free when ending request and accounting mpath io
  stats
- mark REQ_NVME_MPATH_IO_STATS to allow user to disable stats on the mpath
  device (also in the middle of a request).

Sagi Grimberg (2):
  nvme: introduce nvme_start_request
  nvme: support io stats on the mpath device

 drivers/nvme/host/apple.c     |  2 +-
 drivers/nvme/host/core.c      | 10 ++++++++++
 drivers/nvme/host/fc.c        |  2 +-
 drivers/nvme/host/multipath.c | 25 +++++++++++++++++++++++++
 drivers/nvme/host/nvme.h      | 13 +++++++++++++
 drivers/nvme/host/pci.c       |  2 +-
 drivers/nvme/host/rdma.c      |  2 +-
 drivers/nvme/host/tcp.c       |  2 +-
 drivers/nvme/target/loop.c    |  2 +-
 9 files changed, 54 insertions(+), 6 deletions(-)

-- 
2.34.1

[PATCH v2 1/2] nvme: introduce nvme_start_request

[Sagi Grimberg]

In preparation for nvme-multipath IO stats accounting, we want the
accounting to happen in a centralized place. The request completion
is already centralized, but we need a common helper to request I/O
start.

Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
---
 drivers/nvme/host/apple.c  | 2 +-
 drivers/nvme/host/core.c   | 6 ++++++
 drivers/nvme/host/fc.c     | 2 +-
 drivers/nvme/host/nvme.h   | 1 +
 drivers/nvme/host/pci.c    | 2 +-
 drivers/nvme/host/rdma.c   | 2 +-
 drivers/nvme/host/tcp.c    | 2 +-
 drivers/nvme/target/loop.c | 2 +-
 8 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/drivers/nvme/host/apple.c b/drivers/nvme/host/apple.c
index 5fc5ea196b40..6df4b8a5d8ab 100644
--- a/drivers/nvme/host/apple.c
+++ b/drivers/nvme/host/apple.c
@@ -763,7 +763,7 @@ static blk_status_t apple_nvme_queue_rq(struct blk_mq_hw_ctx *hctx,
 			goto out_free_cmd;
 	}
 
-	blk_mq_start_request(req);
+	nvme_start_request(req);
 	apple_nvme_submit_cmd(q, cmnd);
 	return BLK_STS_OK;
 
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 9bacfd014e3d..64fd772de817 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -419,6 +419,12 @@ void nvme_complete_rq(struct request *req)
 }
 EXPORT_SYMBOL_GPL(nvme_complete_rq);
 
+void nvme_start_request(struct request *rq)
+{
+	blk_mq_start_request(rq);
+}
+EXPORT_SYMBOL_GPL(nvme_start_request);
+
 void nvme_complete_batch_req(struct request *req)
 {
 	trace_nvme_complete_rq(req);
diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
index 127abaf9ba5d..2cdcc7f5d0a9 100644
--- a/drivers/nvme/host/fc.c
+++ b/drivers/nvme/host/fc.c
@@ -2744,7 +2744,7 @@ nvme_fc_start_fcp_op(struct nvme_fc_ctrl *ctrl, struct nvme_fc_queue *queue,
 	atomic_set(&op->state, FCPOP_STATE_ACTIVE);
 
 	if (!(op->flags & FCOP_FLAGS_AEN))
-		blk_mq_start_request(op->rq);
+		nvme_start_request(op->rq);
 
 	cmdiu->csn = cpu_to_be32(atomic_inc_return(&queue->csn));
 	ret = ctrl->lport->ops->fcp_io(&ctrl->lport->localport,
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 2d5d44a73f26..c4d1a4e9b961 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -753,6 +753,7 @@ static inline enum req_op nvme_req_op(struct nvme_command *cmd)
 }
 
 #define NVME_QID_ANY -1
+void nvme_start_request(struct request *rq);
 void nvme_init_request(struct request *req, struct nvme_command *cmd);
 void nvme_cleanup_cmd(struct request *req);
 blk_status_t nvme_setup_cmd(struct nvme_ns *ns, struct request *req);
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 3bdb97205699..e898b9e4e6e0 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -929,7 +929,7 @@ static blk_status_t nvme_prep_rq(struct nvme_dev *dev, struct request *req)
 			goto out_unmap_data;
 	}
 
-	blk_mq_start_request(req);
+	nvme_start_request(req);
 	return BLK_STS_OK;
 out_unmap_data:
 	nvme_unmap_data(dev, req);
diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
index 8e52d2362fa1..ab9d5a17704b 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -2089,7 +2089,7 @@ static blk_status_t nvme_rdma_queue_rq(struct blk_mq_hw_ctx *hctx,
 	if (ret)
 		goto unmap_qe;
 
-	blk_mq_start_request(rq);
+	nvme_start_request(rq);
 
 	if (IS_ENABLED(CONFIG_BLK_DEV_INTEGRITY) &&
 	    queue->pi_support &&
diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
index 2524b5304bfb..a1df405de7f1 100644
--- a/drivers/nvme/host/tcp.c
+++ b/drivers/nvme/host/tcp.c
@@ -2461,7 +2461,7 @@ static blk_status_t nvme_tcp_queue_rq(struct blk_mq_hw_ctx *hctx,
 	if (unlikely(ret))
 		return ret;
 
-	blk_mq_start_request(rq);
+	nvme_start_request(rq);
 
 	nvme_tcp_queue_request(req, true, bd->last);
 
diff --git a/drivers/nvme/target/loop.c b/drivers/nvme/target/loop.c
index 9750a7fca268..c327615decc2 100644
--- a/drivers/nvme/target/loop.c
+++ b/drivers/nvme/target/loop.c
@@ -145,7 +145,7 @@ static blk_status_t nvme_loop_queue_rq(struct blk_mq_hw_ctx *hctx,
 	if (ret)
 		return ret;
 
-	blk_mq_start_request(req);
+	nvme_start_request(req);
 	iod->cmd.common.flags |= NVME_CMD_SGL_METABUF;
 	iod->req.port = queue->ctrl->port;
 	if (!nvmet_req_init(&iod->req, &queue->nvme_cq,
-- 
2.34.1

Re: [PATCH v2 1/2] nvme: introduce nvme_start_request

[Hannes Reinecke]

On 10/3/22 11:43, Sagi Grimberg wrote:
> In preparation for nvme-multipath IO stats accounting, we want the
> accounting to happen in a centralized place. The request completion
> is already centralized, but we need a common helper to request I/O
> start.
> 
> Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
> ---
>   drivers/nvme/host/apple.c  | 2 +-
>   drivers/nvme/host/core.c   | 6 ++++++
>   drivers/nvme/host/fc.c     | 2 +-
>   drivers/nvme/host/nvme.h   | 1 +
>   drivers/nvme/host/pci.c    | 2 +-
>   drivers/nvme/host/rdma.c   | 2 +-
>   drivers/nvme/host/tcp.c    | 2 +-
>   drivers/nvme/target/loop.c | 2 +-
>   8 files changed, 13 insertions(+), 6 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Ivo Totev, Andrew
Myers, Andrew McDonald, Martje Boudien Moerman

Re: [PATCH v2 1/2] nvme: introduce nvme_start_request

[Keith Busch]

On Mon, Oct 03, 2022 at 12:43:43PM +0300, Sagi Grimberg wrote:
>  
> +void nvme_start_request(struct request *rq)
> +{
> +	blk_mq_start_request(rq);
> +}
> +EXPORT_SYMBOL_GPL(nvme_start_request);

Looks good, other than I feel this should be a static inline function in
nvme.h instead.

Reviewed-by: Keith Busch <kbusch@kernel.org>

Re: [PATCH v2 1/2] nvme: introduce nvme_start_request

[Jens Axboe]

On 11/29/22 7:28 AM, Keith Busch wrote:
> On Mon, Oct 03, 2022 at 12:43:43PM +0300, Sagi Grimberg wrote:
>>  
>> +void nvme_start_request(struct request *rq)
>> +{
>> +	blk_mq_start_request(rq);
>> +}
>> +EXPORT_SYMBOL_GPL(nvme_start_request);
> 
> Looks good, other than I feel this should be a static inline function in
> nvme.h instead.

Hah, mid-air collision. But at least we agree :-)

-- 
Jens Axboe

Re: [PATCH v2 1/2] nvme: introduce nvme_start_request

[Jens Axboe]

On 10/3/22 3:43?AM, Sagi Grimberg wrote:
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 9bacfd014e3d..64fd772de817 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -419,6 +419,12 @@ void nvme_complete_rq(struct request *req)
>  }
>  EXPORT_SYMBOL_GPL(nvme_complete_rq);
>  
> +void nvme_start_request(struct request *rq)
> +{
> +	blk_mq_start_request(rq);
> +}
> +EXPORT_SYMBOL_GPL(nvme_start_request);
> +
>  void nvme_complete_batch_req(struct request *req)
>  {
>  	trace_nvme_complete_rq(req);

This really should be an inline. Apart from that, looks fine to me.

-- 
Jens Axboe

[PATCH v2 2/2] nvme: support io stats on the mpath device

[Sagi Grimberg]

Our mpath stack device is just a shim that selects a bottom namespace
and submits the bio to it without any fancy splitting. This also means
that we don't clone the bio or have any context to the bio beyond
submission. However it really sucks that we don't see the mpath device
io stats.

Given that the mpath device can't do that without adding some context
to it, we let the bottom device do it on its behalf (somewhat similar
to the approach taken in nvme_trace_bio_complete).

When the IO starts, we account the request for multipath IO stats using
REQ_NVME_MPATH_IO_STATS nvme_request flag to avoid queue io stats disable
in the middle of the request.

Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
---
 drivers/nvme/host/core.c      |  4 ++++
 drivers/nvme/host/multipath.c | 25 +++++++++++++++++++++++++
 drivers/nvme/host/nvme.h      | 12 ++++++++++++
 3 files changed, 41 insertions(+)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 64fd772de817..d5a54ddf73f2 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -384,6 +384,8 @@ static inline void nvme_end_req(struct request *req)
 		nvme_log_error(req);
 	nvme_end_req_zoned(req);
 	nvme_trace_bio_complete(req);
+	if (req->cmd_flags & REQ_NVME_MPATH)
+		nvme_mpath_end_request(req);
 	blk_mq_end_request(req, status);
 }
 
@@ -421,6 +423,8 @@ EXPORT_SYMBOL_GPL(nvme_complete_rq);
 
 void nvme_start_request(struct request *rq)
 {
+	if (rq->cmd_flags & REQ_NVME_MPATH)
+		nvme_mpath_start_request(rq);
 	blk_mq_start_request(rq);
 }
 EXPORT_SYMBOL_GPL(nvme_start_request);
diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
index b9cf17cbbbd5..5ef43f54aab6 100644
--- a/drivers/nvme/host/multipath.c
+++ b/drivers/nvme/host/multipath.c
@@ -114,6 +114,30 @@ void nvme_failover_req(struct request *req)
 	kblockd_schedule_work(&ns->head->requeue_work);
 }
 
+void nvme_mpath_start_request(struct request *rq)
+{
+	struct nvme_ns *ns = rq->q->queuedata;
+	struct gendisk *disk = ns->head->disk;
+
+	if (!blk_queue_io_stat(disk->queue) || blk_rq_is_passthrough(rq))
+		return;
+
+	nvme_req(rq)->flags |= NVME_MPATH_IO_STATS;
+	nvme_req(rq)->start_time = bdev_start_io_acct(disk->part0,
+					blk_rq_bytes(rq) >> SECTOR_SHIFT,
+					req_op(rq), jiffies);
+}
+
+void nvme_mpath_end_request(struct request *rq)
+{
+	struct nvme_ns *ns = rq->q->queuedata;
+
+	if (!(nvme_req(rq)->flags & NVME_MPATH_IO_STATS))
+		return;
+	bdev_end_io_acct(ns->head->disk->part0, req_op(rq),
+		nvme_req(rq)->start_time);
+}
+
 void nvme_kick_requeue_lists(struct nvme_ctrl *ctrl)
 {
 	struct nvme_ns *ns;
@@ -502,6 +526,7 @@ int nvme_mpath_alloc_disk(struct nvme_ctrl *ctrl, struct nvme_ns_head *head)
 
 	blk_queue_flag_set(QUEUE_FLAG_NONROT, head->disk->queue);
 	blk_queue_flag_set(QUEUE_FLAG_NOWAIT, head->disk->queue);
+	blk_queue_flag_set(QUEUE_FLAG_IO_STAT, head->disk->queue);
 	/*
 	 * This assumes all controllers that refer to a namespace either
 	 * support poll queues or not.  That is not a strict guarantee,
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index c4d1a4e9b961..c4edc91b1358 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -162,6 +162,9 @@ struct nvme_request {
 	u8			retries;
 	u8			flags;
 	u16			status;
+#ifdef CONFIG_NVME_MULTIPATH
+	unsigned long		start_time;
+#endif
 	struct nvme_ctrl	*ctrl;
 };
 
@@ -173,6 +176,7 @@ struct nvme_request {
 enum {
 	NVME_REQ_CANCELLED		= (1 << 0),
 	NVME_REQ_USERCMD		= (1 << 1),
+	NVME_MPATH_IO_STATS		= (1 << 2),
 };
 
 static inline struct nvme_request *nvme_req(struct request *req)
@@ -862,6 +866,8 @@ bool nvme_mpath_clear_current_path(struct nvme_ns *ns);
 void nvme_mpath_revalidate_paths(struct nvme_ns *ns);
 void nvme_mpath_clear_ctrl_paths(struct nvme_ctrl *ctrl);
 void nvme_mpath_shutdown_disk(struct nvme_ns_head *head);
+void nvme_mpath_start_request(struct request *rq);
+void nvme_mpath_end_request(struct request *rq);
 
 static inline void nvme_trace_bio_complete(struct request *req)
 {
@@ -947,6 +953,12 @@ static inline void nvme_mpath_start_freeze(struct nvme_subsystem *subsys)
 static inline void nvme_mpath_default_iopolicy(struct nvme_subsystem *subsys)
 {
 }
+static inline void nvme_mpath_start_request(struct request *rq)
+{
+}
+static inline void nvme_mpath_end_request(struct request *rq)
+{
+}
 #endif /* CONFIG_NVME_MULTIPATH */
 
 int nvme_revalidate_zones(struct nvme_ns *ns);
-- 
2.34.1

Re: [PATCH v2 2/2] nvme: support io stats on the mpath device

[Hannes Reinecke]

On 10/3/22 11:43, Sagi Grimberg wrote:
> Our mpath stack device is just a shim that selects a bottom namespace
> and submits the bio to it without any fancy splitting. This also means
> that we don't clone the bio or have any context to the bio beyond
> submission. However it really sucks that we don't see the mpath device
> io stats.
> 
> Given that the mpath device can't do that without adding some context
> to it, we let the bottom device do it on its behalf (somewhat similar
> to the approach taken in nvme_trace_bio_complete).
> 
> When the IO starts, we account the request for multipath IO stats using
> REQ_NVME_MPATH_IO_STATS nvme_request flag to avoid queue io stats disable
> in the middle of the request.
> 
> Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
> ---
>   drivers/nvme/host/core.c      |  4 ++++
>   drivers/nvme/host/multipath.c | 25 +++++++++++++++++++++++++
>   drivers/nvme/host/nvme.h      | 12 ++++++++++++
>   3 files changed, 41 insertions(+)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 64fd772de817..d5a54ddf73f2 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -384,6 +384,8 @@ static inline void nvme_end_req(struct request *req)
>   		nvme_log_error(req);
>   	nvme_end_req_zoned(req);
>   	nvme_trace_bio_complete(req);
> +	if (req->cmd_flags & REQ_NVME_MPATH)
> +		nvme_mpath_end_request(req);
>   	blk_mq_end_request(req, status);
>   }
>   
> @@ -421,6 +423,8 @@ EXPORT_SYMBOL_GPL(nvme_complete_rq);
>   
>   void nvme_start_request(struct request *rq)
>   {
> +	if (rq->cmd_flags & REQ_NVME_MPATH)
> +		nvme_mpath_start_request(rq);
>   	blk_mq_start_request(rq);
>   }
>   EXPORT_SYMBOL_GPL(nvme_start_request);

Why don't you move the check for REQ_NVME_MPATH into 
nvme_mpath_{start,end}_request?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Ivo Totev, Andrew
Myers, Andrew McDonald, Martje Boudien Moerman

Re: [PATCH v2 2/2] nvme: support io stats on the mpath device

[Sagi Grimberg]

On 10/4/22 09:11, Hannes Reinecke wrote:
> On 10/3/22 11:43, Sagi Grimberg wrote:
>> Our mpath stack device is just a shim that selects a bottom namespace
>> and submits the bio to it without any fancy splitting. This also means
>> that we don't clone the bio or have any context to the bio beyond
>> submission. However it really sucks that we don't see the mpath device
>> io stats.
>>
>> Given that the mpath device can't do that without adding some context
>> to it, we let the bottom device do it on its behalf (somewhat similar
>> to the approach taken in nvme_trace_bio_complete).
>>
>> When the IO starts, we account the request for multipath IO stats using
>> REQ_NVME_MPATH_IO_STATS nvme_request flag to avoid queue io stats disable
>> in the middle of the request.
>>
>> Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
>> ---
>>   drivers/nvme/host/core.c      |  4 ++++
>>   drivers/nvme/host/multipath.c | 25 +++++++++++++++++++++++++
>>   drivers/nvme/host/nvme.h      | 12 ++++++++++++
>>   3 files changed, 41 insertions(+)
>>
>> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
>> index 64fd772de817..d5a54ddf73f2 100644
>> --- a/drivers/nvme/host/core.c
>> +++ b/drivers/nvme/host/core.c
>> @@ -384,6 +384,8 @@ static inline void nvme_end_req(struct request *req)
>>           nvme_log_error(req);
>>       nvme_end_req_zoned(req);
>>       nvme_trace_bio_complete(req);
>> +    if (req->cmd_flags & REQ_NVME_MPATH)
>> +        nvme_mpath_end_request(req);
>>       blk_mq_end_request(req, status);
>>   }
>> @@ -421,6 +423,8 @@ EXPORT_SYMBOL_GPL(nvme_complete_rq);
>>   void nvme_start_request(struct request *rq)
>>   {
>> +    if (rq->cmd_flags & REQ_NVME_MPATH)
>> +        nvme_mpath_start_request(rq);
>>       blk_mq_start_request(rq);
>>   }
>>   EXPORT_SYMBOL_GPL(nvme_start_request);
> 
> Why don't you move the check for REQ_NVME_MPATH into 
> nvme_mpath_{start,end}_request?

I'm less fond of calling a function that may or may not
do anything...

But it is a pattern that exists in the code, if people prefer
it I can change it.

Re: [PATCH v2 2/2] nvme: support io stats on the mpath device

[Jens Axboe]

On 10/4/22 2:19 AM, Sagi Grimberg wrote:
> 
> 
> On 10/4/22 09:11, Hannes Reinecke wrote:
>> On 10/3/22 11:43, Sagi Grimberg wrote:
>>> Our mpath stack device is just a shim that selects a bottom namespace
>>> and submits the bio to it without any fancy splitting. This also means
>>> that we don't clone the bio or have any context to the bio beyond
>>> submission. However it really sucks that we don't see the mpath device
>>> io stats.
>>>
>>> Given that the mpath device can't do that without adding some context
>>> to it, we let the bottom device do it on its behalf (somewhat similar
>>> to the approach taken in nvme_trace_bio_complete).
>>>
>>> When the IO starts, we account the request for multipath IO stats using
>>> REQ_NVME_MPATH_IO_STATS nvme_request flag to avoid queue io stats disable
>>> in the middle of the request.
>>>
>>> Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
>>> ---
>>>   drivers/nvme/host/core.c      |  4 ++++
>>>   drivers/nvme/host/multipath.c | 25 +++++++++++++++++++++++++
>>>   drivers/nvme/host/nvme.h      | 12 ++++++++++++
>>>   3 files changed, 41 insertions(+)
>>>
>>> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
>>> index 64fd772de817..d5a54ddf73f2 100644
>>> --- a/drivers/nvme/host/core.c
>>> +++ b/drivers/nvme/host/core.c
>>> @@ -384,6 +384,8 @@ static inline void nvme_end_req(struct request *req)
>>>           nvme_log_error(req);
>>>       nvme_end_req_zoned(req);
>>>       nvme_trace_bio_complete(req);
>>> +    if (req->cmd_flags & REQ_NVME_MPATH)
>>> +        nvme_mpath_end_request(req);
>>>       blk_mq_end_request(req, status);
>>>   }
>>> @@ -421,6 +423,8 @@ EXPORT_SYMBOL_GPL(nvme_complete_rq);
>>>   void nvme_start_request(struct request *rq)
>>>   {
>>> +    if (rq->cmd_flags & REQ_NVME_MPATH)
>>> +        nvme_mpath_start_request(rq);
>>>       blk_mq_start_request(rq);
>>>   }
>>>   EXPORT_SYMBOL_GPL(nvme_start_request);
>>
>> Why don't you move the check for REQ_NVME_MPATH into nvme_mpath_{start,end}_request?
> 
> I'm less fond of calling a function that may or may not
> do anything...
> 
> But it is a pattern that exists in the code, if people prefer
> it I can change it.

I prefer it the way that you have it, avoids a function call for
the hot path of not being multipath.

-- 
Jens Axboe

Re: [PATCH v2 2/2] nvme: support io stats on the mpath device

[Jens Axboe]

On 10/3/22 3:43 AM, Sagi Grimberg wrote:
> Our mpath stack device is just a shim that selects a bottom namespace
> and submits the bio to it without any fancy splitting. This also means
> that we don't clone the bio or have any context to the bio beyond
> submission. However it really sucks that we don't see the mpath device
> io stats.
> 
> Given that the mpath device can't do that without adding some context
> to it, we let the bottom device do it on its behalf (somewhat similar
> to the approach taken in nvme_trace_bio_complete).
> 
> When the IO starts, we account the request for multipath IO stats using
> REQ_NVME_MPATH_IO_STATS nvme_request flag to avoid queue io stats disable
> in the middle of the request.

Reviewed-by: Jens Axboe <axboe@kernel.dk>

-- 
Jens Axboe

Re: [PATCH v2 2/2] nvme: support io stats on the mpath device

[Keith Busch]

On Mon, Oct 03, 2022 at 12:43:44PM +0300, Sagi Grimberg wrote:
> Our mpath stack device is just a shim that selects a bottom namespace
> and submits the bio to it without any fancy splitting. This also means
> that we don't clone the bio or have any context to the bio beyond
> submission. However it really sucks that we don't see the mpath device
> io stats.
> 
> Given that the mpath device can't do that without adding some context
> to it, we let the bottom device do it on its behalf (somewhat similar
> to the approach taken in nvme_trace_bio_complete).
> 
> When the IO starts, we account the request for multipath IO stats using
> REQ_NVME_MPATH_IO_STATS nvme_request flag to avoid queue io stats disable
> in the middle of the request.

An unfortunate side effect is that a successful error failover will get
accounted for twice in the mpath device, but cloning to create a
separate context just to track iostats for that unusual condition is
much worse.

Reviewed-by: Keith Busch <kbusch@kernel.org>

> +void nvme_mpath_start_request(struct request *rq)
> +{
> +	struct nvme_ns *ns = rq->q->queuedata;
> +	struct gendisk *disk = ns->head->disk;
> +
> +	if (!blk_queue_io_stat(disk->queue) || blk_rq_is_passthrough(rq))
> +		return;
> +
> +	nvme_req(rq)->flags |= NVME_MPATH_IO_STATS;
> +	nvme_req(rq)->start_time = bdev_start_io_acct(disk->part0,
> +					blk_rq_bytes(rq) >> SECTOR_SHIFT,
> +					req_op(rq), jiffies);
> +}
> +void nvme_mpath_end_request(struct request *rq)
> +{
> +	struct nvme_ns *ns = rq->q->queuedata;
> +
> +	if (!(nvme_req(rq)->flags & NVME_MPATH_IO_STATS))
> +		return;
> +	bdev_end_io_acct(ns->head->disk->part0, req_op(rq),
> +		nvme_req(rq)->start_time);
> +}

I think these also can be static inline.

Re: [PATCH v2 0/2] nvme-mpath: Add IO stats support

[Christoph Hellwig]

I've applied this with nvme_start_request changed to an inline
funtion.

Re: [PATCH v2 0/2] nvme-mpath: Add IO stats support

[Keith Busch]

On Tue, Nov 29, 2022 at 03:45:10PM +0100, Christoph Hellwig wrote:
> I've applied this with nvme_start_request changed to an inline
> funtion.

You'll also need to change patch 2 with this inlined. That patch needs
to either inline nvme_mpath_start_request(), or add an
EXPORT_SYMBOL_GPL.