[PATCH] block: Fix inflight statistic for MQ submission with !elevator

[PATCH] block: Fix inflight statistic for MQ submission with !elevator

[Gabriel Krisman Bertazi]

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).

A more precise approach would be to only consider requests with state
MQ_RQ_IN_FLIGHT.

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 0015a1892153..997b3327eaa8 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 && rq->state == MQ_RQ_IN_FLIGHT)
 		mi->inflight[rq_data_dir(rq)]++;
 
 	return true;
-- 
2.28.0

Re: [PATCH] block: Fix inflight statistic for MQ submission with !elevator

[Jens Axboe]

On 8/31/20 9:31 AM, Gabriel Krisman Bertazi wrote:
> 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).
> 
> A more precise approach would be to only consider requests with state
> MQ_RQ_IN_FLIGHT.

We've had some churn here, last change in this area was:

commit 6131837b1de66116459ef4413e26fdbc70d066dc
Author: Omar Sandoval <osandov@fb.com>
Date:   Thu Apr 26 00:21:58 2018 -0700

    blk-mq: count allocated but not started requests in iostats inflight

which your patch basically just reverts. So more testing/explanation
needed on why it's necessary.

-- 
Jens Axboe

Re: [PATCH] block: Fix inflight statistic for MQ submission with !elevator

[Gabriel Krisman Bertazi]

Jens Axboe <axboe@kernel.dk> writes:

> On 8/31/20 9:31 AM, Gabriel Krisman Bertazi wrote:
>> 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).
>> 
>> A more precise approach would be to only consider requests with state
>> MQ_RQ_IN_FLIGHT.
>
> We've had some churn here, last change in this area was:

Hi Jens,

Thanks for the quick reply.  I wasn't aware of that patch.

I'm collecting statistics on the queue depth of a NCQ disk, and my end
goal is to have a time_in_driver clock, which is the time spent by IOs
in the driver, similarly to what is shown in diskstats, but the latter
includes the block layer time.  I stumbled this issue with inflight,
where we noticed a difference between inflight and what was actually
dispatched to the driver.  The problem is the current behavior doesn't
seem to match the documentation in Documentation/block/stat.sh.

I went back to history.git and found that this difference in behavior
from the documentation has always been there for legacy path, unless I
am misreading the documentation.  The patch I proposed consolidates
inflight in favor of documentation instead of the legacy patch.

The documentation reads:

"""
This value counts the number of I/O requests that have been issued to
the device driver but have not yet completed.  It does not include I/O
requests that are in the queue but not yet issued to the device driver.
"""

Should I patch the documentation instead?

Thinking about semantics, it seem more useful to have inflight include
only the time between dispatching and completion, but if you think there
is a concern about stable abi here, would you accept a new in_device file
tracking this metric?

-- 
Gabriel Krisman Bertazi

Re: [PATCH] block: Fix inflight statistic for MQ submission with !elevator

[Ming Lei]

On Mon, Aug 31, 2020 at 11:37 PM Gabriel Krisman Bertazi
<krisman@collabora.com> wrote:
>
> 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).
>
> A more precise approach would be to only consider requests with state
> MQ_RQ_IN_FLIGHT.
>
> 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 0015a1892153..997b3327eaa8 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 && rq->state == MQ_RQ_IN_FLIGHT)
>                 mi->inflight[rq_data_dir(rq)]++;

The fix looks fine. However, we have helper of
blk_mq_request_started() for this purpose.


Thanks,
Ming Lei

Re: [PATCH] block: Fix inflight statistic for MQ submission with !elevator

[Jens Axboe]

On 8/31/20 7:18 PM, Ming Lei wrote:
> On Mon, Aug 31, 2020 at 11:37 PM Gabriel Krisman Bertazi
> <krisman@collabora.com> wrote:
>>
>> 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).
>>
>> A more precise approach would be to only consider requests with state
>> MQ_RQ_IN_FLIGHT.
>>
>> 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 0015a1892153..997b3327eaa8 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 && rq->state == MQ_RQ_IN_FLIGHT)
>>                 mi->inflight[rq_data_dir(rq)]++;
> 
> The fix looks fine. However, we have helper of
> blk_mq_request_started() for this purpose.

Why does it look fine? Did you see the older commit I referenced? I'm
not saying the change is wrong per se, just that this is the behavior
we've always had, and making this change would deviate from that. As
Gabriel states in the follow up, it's either changing the documentation
or the patch.

When replying to patches that have had previous discussion, please
reference that when making followups. That makes for a more productive
discussion on how to proceed.

-- 
Jens Axboe

Re: [PATCH] block: Fix inflight statistic for MQ submission with !elevator

[Ming Lei]

Hi Jens,

On Mon, Aug 31, 2020 at 09:42:05PM -0600, Jens Axboe wrote:
> On 8/31/20 7:18 PM, Ming Lei wrote:
> > On Mon, Aug 31, 2020 at 11:37 PM Gabriel Krisman Bertazi
> > <krisman@collabora.com> wrote:
> >>
> >> 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).
> >>
> >> A more precise approach would be to only consider requests with state
> >> MQ_RQ_IN_FLIGHT.
> >>
> >> 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 0015a1892153..997b3327eaa8 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 && rq->state == MQ_RQ_IN_FLIGHT)
> >>                 mi->inflight[rq_data_dir(rq)]++;
> > 
> > The fix looks fine. However, we have helper of
> > blk_mq_request_started() for this purpose.
> 
> Why does it look fine? Did you see the older commit I referenced? I'm

Looks my gmail inbox has problem, and I didn't see your referenced commit.
but I can see your reply just now in my redhat email box, sorry for that.

BTW, commit 6131837b1de6 ("blk-mq: count allocated but not started requests
in iostats inflight") didn't does what it claimed. blk_mq_queue_tag_busy_iter()
iterates over driver tags, so for real io scheduler, blk_mq_check_inflight()
basically returns count of inflight request, instead of allocated request.

Even worse, since commit 6131837b1de6 blk_mq_in_flight() behaves inconsistently
between q->elevator and !q->elevator.

> not saying the change is wrong per se, just that this is the behavior
> we've always had, and making this change would deviate from that. As
> Gabriel states in the follow up, it's either changing the documentation
> or the patch.

Looks iostat doesn't use the 'inflight' count, so what is the userspace's
expectation on this counter?

If it counts allocated request, it is easy for userspace to observe different
statistics if someone updates nr_requests via sysfs.


Thanks,
Ming

Re: [PATCH] block: Fix inflight statistic for MQ submission with !elevator

[Gabriel Krisman Bertazi]

Ming Lei <tom.leiming@gmail.com> writes:

> On Mon, Aug 31, 2020 at 11:37 PM Gabriel Krisman Bertazi
> <krisman@collabora.com> wrote:

>> -       if (rq->part == mi->part)
>> +       if (rq->part == mi->part && rq->state == MQ_RQ_IN_FLIGHT)
>>                 mi->inflight[rq_data_dir(rq)]++;
>
> The fix looks fine. However, we have helper of
> blk_mq_request_started() for this purpose.

Thanks for the review.

I was aware of blk_mq_request_started, but it forces a READ_ONCE which
on Alpha includes a mb() for every tagged request, which doesn't seem
necessary or desired here.  I might be wrong though, memory barriers
are hard. :)

let's see what Jens says about the other points, so I don't spin this
unnecessarily.

-- 
Gabriel Krisman Bertazi

Re: [PATCH] block: Fix inflight statistic for MQ submission with !elevator

[Jens Axboe]

On 9/1/20 12:36 AM, Ming Lei wrote:
> Hi Jens,
> 
> On Mon, Aug 31, 2020 at 09:42:05PM -0600, Jens Axboe wrote:
>> On 8/31/20 7:18 PM, Ming Lei wrote:
>>> On Mon, Aug 31, 2020 at 11:37 PM Gabriel Krisman Bertazi
>>> <krisman@collabora.com> wrote:
>>>>
>>>> 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).
>>>>
>>>> A more precise approach would be to only consider requests with state
>>>> MQ_RQ_IN_FLIGHT.
>>>>
>>>> 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 0015a1892153..997b3327eaa8 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 && rq->state == MQ_RQ_IN_FLIGHT)
>>>>                 mi->inflight[rq_data_dir(rq)]++;
>>>
>>> The fix looks fine. However, we have helper of
>>> blk_mq_request_started() for this purpose.
>>
>> Why does it look fine? Did you see the older commit I referenced? I'm
> 
> Looks my gmail inbox has problem, and I didn't see your referenced
> commit. but I can see your reply just now in my redhat email box,
> sorry for that.

Ah ok, that explains it, just felt like it was being ignored.

> BTW, commit 6131837b1de6 ("blk-mq: count allocated but not started requests
> in iostats inflight") didn't does what it claimed. blk_mq_queue_tag_busy_iter()
> iterates over driver tags, so for real io scheduler, blk_mq_check_inflight()
> basically returns count of inflight request, instead of allocated request.
> 
> Even worse, since commit 6131837b1de6 blk_mq_in_flight() behaves inconsistently
> between q->elevator and !q->elevator.

I agree, that is definitely a problem, and I've run into this internally
at FB in fact.

>> not saying the change is wrong per se, just that this is the behavior
>> we've always had, and making this change would deviate from that. As
>> Gabriel states in the follow up, it's either changing the documentation
>> or the patch.
> 
> Looks iostat doesn't use the 'inflight' count, so what is the
> userspace's expectation on this counter?
> 
> If it counts allocated request, it is easy for userspace to observe
> different statistics if someone updates nr_requests via sysfs.

I don't think there's necessarily an expectation, but if we change the
scope then that might cause problems for folks. All of a sudden the
iowait looks less, for example, and that can be hard to explain.

That said, I do think the patch makes sense, obviously, since that's how
I originally did the mq accounting. But it's hard to argue with history
and expectations, and that is my worry. I don't want to apply this
patch, then have to revert the behavior a few revisions later. And then
we end up with different kernels that have different metrics, and that's
somewhat of a mess.

-- 
Jens Axboe

Re: [PATCH] block: Fix inflight statistic for MQ submission with !elevator

[Jens Axboe]

On 9/1/20 12:37 PM, Gabriel Krisman Bertazi wrote:
> Ming Lei <tom.leiming@gmail.com> writes:
> 
>> On Mon, Aug 31, 2020 at 11:37 PM Gabriel Krisman Bertazi
>> <krisman@collabora.com> wrote:
> 
>>> -       if (rq->part == mi->part)
>>> +       if (rq->part == mi->part && rq->state == MQ_RQ_IN_FLIGHT)
>>>                 mi->inflight[rq_data_dir(rq)]++;
>>
>> The fix looks fine. However, we have helper of
>> blk_mq_request_started() for this purpose.
> 
> Thanks for the review.
> 
> I was aware of blk_mq_request_started, but it forces a READ_ONCE which
> on Alpha includes a mb() for every tagged request, which doesn't seem
> necessary or desired here.  I might be wrong though, memory barriers
> are hard. :)
> 
> let's see what Jens says about the other points, so I don't spin this
> unnecessarily.

On the READ_ONCE() part, I don't really care about alpha to the extent
that I'll sacrifice readability and code unification to cater to that
specific architecture :-)

We just need to decide if this makes sense or not. I think we should
apply this for 5.10, with Ming's suggestion of using
blk_mq_request_started(). Then I guess we'll see what happens...

-- 
Jens Axboe

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

[Gabriel Krisman Bertazi]

Jens Axboe <axboe@kernel.dk> writes:

> We just need to decide if this makes sense or not. I think we should
> apply this for 5.10, with Ming's suggestion of using
> blk_mq_request_started(). Then I guess we'll see what happens...

Hello,

Here is the second version, then.  But, instead of
blk_mq_request_started as suggested on the review, this uses
blk_mq_rq_state to access the state attribute, since we don't want to
include MQ_RQ_COMPLETE.

Also, improved the commit message a bit.

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.

Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com>
---
Changes Since v1:
  - Use blk_mq_rq_state to fetch req->state
  - Improve commit message

 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 0015a1892153..bee55f80fb69 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: [PATCH v2] block: Consider only dispatched requests for inflight statistic

[Gabriel Krisman Bertazi]

Gabriel Krisman Bertazi <krisman@collabora.com> writes:

> Jens Axboe <axboe@kernel.dk> writes:
>
>> We just need to decide if this makes sense or not. I think we should
>> apply this for 5.10, with Ming's suggestion of using
>> blk_mq_request_started(). Then I guess we'll see what happens...
>
> Hello,
>
> Here is the second version, then.  But, instead of
> blk_mq_request_started as suggested on the review, this uses
> blk_mq_rq_state to access the state attribute, since we don't want to
> include MQ_RQ_COMPLETE.
>
> Also, improved the commit message a bit.
>

Hi Jens,

Sorry for the ping.  Have you made a decision here?

Thanks,

-- 
Gabriel Krisman Bertazi