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

[Gabriel Krisman Bertazi <krisman@collabora.com>]

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