[V2,0/2] block: track per requests type merged count

Message

Chaitanya Kulkarni Sept. 18, 2019, 12:54 a.m. UTC

Hi,

With current debugfs block layer infrastructure, we only get the total
merge count which includes all the requests types, but we don't get 
the per request type merge count.

This patch-series replaces the rq_merged variable into the 
rq_merged array so that we can track the per request type merged stats.

Instead of having one number for all the requests which are merged,
with this now we can get the detailed number of the merged requests                                    
per request type which is mergeable.

This is helpful in the understanding merging of the requests under
different workloads and for the special requests such as discard which
implements request specific merging mechanism.

* Changes since V1 :-

 1. Add a reusable helper.
 2. Only print merged stats for the requests which are mergeable.
 3. Adjust the code for the latest block/for-next branch.

Chaitanya Kulkarni (2):
  block: add helper to check mergeable req op
  block: track per requests type merged count

 block/blk-mq-debugfs.c | 12 ++++++++++--
 block/blk-mq-sched.c   |  2 +-
 block/blk-mq.h         |  2 +-
 include/linux/blkdev.h | 24 +++++++++++++++++-------
 4 files changed, 29 insertions(+), 11 deletions(-)

Comments

Jens Axboe Sept. 18, 2019, 2:32 a.m. UTC | #1

On 9/17/19 6:54 PM, Chaitanya Kulkarni wrote:
> Hi,
> 
> With current debugfs block layer infrastructure, we only get the total
> merge count which includes all the requests types, but we don't get
> the per request type merge count.
> 
> This patch-series replaces the rq_merged variable into the
> rq_merged array so that we can track the per request type merged stats.
> 
> Instead of having one number for all the requests which are merged,
> with this now we can get the detailed number of the merged requests
> per request type which is mergeable.
> 
> This is helpful in the understanding merging of the requests under
> different workloads and for the special requests such as discard which
> implements request specific merging mechanism.

Regular io stats get merging stats for read/write/trim. Why do we need
something else for this?

Chaitanya Kulkarni Sept. 18, 2019, 7:28 p.m. UTC | #2

On 09/17/2019 07:32 PM, Jens Axboe wrote:
> Regular io stats get merging stats for read/write/trim. Why do we need
> something else for this?
>
> -- Jens Axboe

Yes, but we don't have a mechanism through debug-fs to have such per
request type information which makes debugging much easier.

One such a scenario where we want to add a new request type and
examine rq_merged not as a whole number, which is not covered by
the io stats (since it only covers read/write/trim).

Jens Axboe Sept. 18, 2019, 7:49 p.m. UTC | #3

On 9/18/19 1:28 PM, Chaitanya Kulkarni wrote:
> On 09/17/2019 07:32 PM, Jens Axboe wrote:
>> Regular io stats get merging stats for read/write/trim. Why do we need
>> something else for this?
>>
>> -- Jens Axboe
> 
> Yes, but we don't have a mechanism through debug-fs to have such per
> request type information which makes debugging much easier.
> 
> One such a scenario where we want to add a new request type and
> examine rq_merged not as a whole number, which is not covered by
> the io stats (since it only covers read/write/trim).

What is this new request type that supports merging?

My point is that adding stats like this isn't free, it's a runtime cost.
And if it's just for debugging, there better be a damn good reason for
why we'd want to pay this cost the 99.999% of the time where nobody is
looking at those counters.

So why isn't the iostat stuff good enough? There's also the option of
having blktrace tell you this information, it would not be hard to add a
blktrace-stats type of tool that'll just give you the last second of
stats so you wouldn't have to store the data, for example.

What I'm asking for is justification for adding these stats, so far I
don't see any outside of perhaps convenience, it's easier to just add
them to blk-mq and retrieve them through debugfs.

Chaitanya Kulkarni Sept. 18, 2019, 9:05 p.m. UTC | #4

On 09/18/2019 12:49 PM, Jens Axboe wrote:
> What is this new request type that supports merging?
>
> My point is that adding stats like this isn't free, it's a runtime cost.
> And if it's just for debugging, there better be a damn good reason for
> why we'd want to pay this cost the 99.999% of the time where nobody is
> looking at those counters.
>
> So why isn't the iostat stuff good enough? There's also the option of
> having blktrace tell you this information, it would not be hard to add a
> blktrace-stats type of tool that'll just give you the last second of
> stats so you wouldn't have to store the data, for example.
>
> What I'm asking for is justification for adding these stats, so far I
> don't see any outside of perhaps convenience, it's easier to just add
> them to blk-mq and retrieve them through debugfs.

Thanks for the explanation, make sense to drop these patches and rely
on other tools.