[RESEND PATCH v2] block: Consider only dispatched requests for inflight statistic

[RESEND PATCH v2] block: Consider only dispatched requests for inflight statistic

[Gabriel Krisman Bertazi]

Hi Jens,

Reviving this, hopefully for the upcoming merge window, following the
suggestion that we let it sit there and see if people complain about the
metric fix.  As I mentioned, I'm also happy to change the documentation
plus create a new in_device metric to track only requests submitted, if
you prefer.

Thanks,

>8

According to Documentation/block/stat.rst, inflight should not include
I/O requests that are in the queue but not yet dispatched to the device,
but blk-mq identifies as inflight any request that has a tag allocated,
which, for queues without elevator, happens at request allocation time
and before it is queued in the ctx (default case in blk_mq_submit_bio).

In addition, current behavior is different for queues with elevator from
queues without it, since for the former the driver tag is allocated at
dispatch time.  A more precise approach would be to only consider
requests with state MQ_RQ_IN_FLIGHT.

This effectively reverts commit 6131837b1de6 ("blk-mq: count allocated
but not started requests in iostats inflight") to consolidate blk-mq
behavior with itself (elevator case) and with original documentation,
but it differs from the behavior used by the legacy path.

This version differs from v1 by using blk_mq_rq_state to access the
state attribute.  Avoid using blk_mq_request_started, which was
suggested, since we don't want to include MQ_RQ_COMPLETE.

Cc: Omar Sandoval <osandov@fb.com>
Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com>
---
 block/blk-mq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index 2e4b3cad2a61..c5fefd39d0c0 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
-- 
2.28.0

[RESEND PATCH v2] block: Consider only dispatched requests for inflight statistic

[Gabriel Krisman Bertazi]

Oops, I have no idea what happened, but something ate the hunk at the
last submission.  My apologies.  Please find it below.

>8

According to Documentation/block/stat.rst, inflight should not include
I/O requests that are in the queue but not yet dispatched to the device,
but blk-mq identifies as inflight any request that has a tag allocated,
which, for queues without elevator, happens at request allocation time
and before it is queued in the ctx (default case in blk_mq_submit_bio).

In addition, current behavior is different for queues with elevator from
queues without it, since for the former the driver tag is allocated at
dispatch time.  A more precise approach would be to only consider
requests with state MQ_RQ_IN_FLIGHT.

This effectively reverts commit 6131837b1de6 ("blk-mq: count allocated
but not started requests in iostats inflight") to consolidate blk-mq
behavior with itself (elevator case) and with original documentation,
but it differs from the behavior used by the legacy path.

This version differs from v1 by using blk_mq_rq_state to access the
state attribute.  Avoid using blk_mq_request_started, which was
suggested, since we don't want to include MQ_RQ_COMPLETE.

Cc: Omar Sandoval <osandov@fb.com>
Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com>
---
 block/blk-mq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index 2e4b3cad2a61..c5fefd39d0c0 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -105,7 +105,7 @@ static bool blk_mq_check_inflight(struct blk_mq_hw_ctx *hctx,
 {
 	struct mq_inflight *mi = priv;
 
-	if (rq->part == mi->part)
+	if (rq->part == mi->part && blk_mq_rq_state(rq) == MQ_RQ_IN_FLIGHT)
 		mi->inflight[rq_data_dir(rq)]++;
 
 	return true;
-- 
2.28.0

Re: [RESEND PATCH v2] block: Consider only dispatched requests for inflight statistic

[Jens Axboe]

On 10/6/20 1:20 PM, Gabriel Krisman Bertazi wrote:
> 
> Oops, I have no idea what happened, but something ate the hunk at the
> last submission.  My apologies.  Please find it below.

Care to just resend a fixed up one? Saves me the time from fixing
things up.

I guess we'll just try and see if this flies, not sure how else to
make progress on it.

-- 
Jens Axboe

Re: [RESEND PATCH v2] block: Consider only dispatched requests for inflight statistic

[Gabriel Krisman Bertazi]

Jens Axboe <axboe@kernel.dk> writes:

> On 10/6/20 1:20 PM, Gabriel Krisman Bertazi wrote:
>> 
>> Oops, I have no idea what happened, but something ate the hunk at the
>> last submission.  My apologies.  Please find it below.
>
> Care to just resend a fixed up one? Saves me the time from fixing
> things up.

hm, the first submission had an empty patch and the email you quoted had
the entire fixed patch ready to apply with scissors.  It should be good
to apply it, I think.  Or, what do you mean?

-- 
Gabriel Krisman Bertazi

Re: [RESEND PATCH v2] block: Consider only dispatched requests for inflight statistic

[Jens Axboe]

On 10/6/20 1:33 PM, Gabriel Krisman Bertazi wrote:
> Jens Axboe <axboe@kernel.dk> writes:
> 
>> On 10/6/20 1:20 PM, Gabriel Krisman Bertazi wrote:
>>>
>>> Oops, I have no idea what happened, but something ate the hunk at the
>>> last submission.  My apologies.  Please find it below.
>>
>> Care to just resend a fixed up one? Saves me the time from fixing
>> things up.
> 
> hm, the first submission had an empty patch and the email you quoted had
> the entire fixed patch ready to apply with scissors.  It should be good
> to apply it, I think.  Or, what do you mean?

The point is that I need to manually fiddle with it.

-- 
Jens Axboe

Re: [RESEND PATCH v2] block: Consider only dispatched requests for inflight statistic

[Gabriel Krisman Bertazi]

Jens Axboe <axboe@kernel.dk> writes:

> On 10/6/20 1:33 PM, Gabriel Krisman Bertazi wrote:
>> Jens Axboe <axboe@kernel.dk> writes:
>> 
>>> On 10/6/20 1:20 PM, Gabriel Krisman Bertazi wrote:
>>>>
>>>> Oops, I have no idea what happened, but something ate the hunk at the
>>>> last submission.  My apologies.  Please find it below.
>>>
>>> Care to just resend a fixed up one? Saves me the time from fixing
>>> things up.
>> 
>> hm, the first submission had an empty patch and the email you quoted had
>> the entire fixed patch ready to apply with scissors.  It should be good
>> to apply it, I think.  Or, what do you mean?
>
> The point is that I need to manually fiddle with it.

Done, sorry for the noise.  The v3 should apply on top of your for-next
branch cleanly.

-- 
Gabriel Krisman Bertazi