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

[Jens Axboe <axboe@kernel.dk>]

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