Thank you for the comment, Kazu.
On Fri, May 27, 2022 at 5:11 PM HAGIO KAZUHITO(萩尾 一仁) <k-hagio-ab@xxxxxxx> wrote:
On 2022/05/27 15:32, lijiang wrote:
> >> + /* static_rqs */
> >> + ret = 0;
> >> + addr = tag + OFFSET(blk_mq_tags_static_rqs);
> >> + if (!readmem(addr, KVADDR, &rqs_addr, sizeof(void *), "blk_mq_tags.static_rqs", RETURN_ON_ERROR))
> >> + return ret;
> >> + addr = rqs_addr + bitnr * sizeof(ulong); /* static_rqs[bitnr] */
> >> + if (!readmem(addr, KVADDR, &rq, sizeof(ulong), "blk_mq_tags.static_rqs[]", RETURN_ON_ERROR))
> >> + return ret;
> >> + ret = tag_iter->fn(rq, tag_iter->data);
> >> +
> >> + return ret;
> >
> > I'm not familiar with blk-mq, I'd like some information.
> >
> > I think that the "dev -d" displays the same inflight stats in /proc/diskstats
> > for devices without mq:
> >
> > diskstats_show
> > part_in_flight
> > count up per_cpu disk_stats->in_flight
> >
> > so, if we do the same thing for mq devices, we should imitate this path:
> >
> > diskstats_show
> > blk_mq_in_flight
> > blk_mq_queue_tag_busy_iter
> > queue_for_each_hw_ctx
> > bt_for_each
> > sbitmap_for_each_set
> > bt_iter
> > blk_mq_check_inflight
> >
> > On the path, I cannot see the static_rqs being counted.
> > Why does it need to be counted?
> >
>
> You are right. Basically, the implementation imitates the above path in the kernel. But not exactly, there are
> two changes in this patch:
> [1] add additional statistics for the static_rqs[] and sched_tags
> [2] the calculate_disk_io() imitates the blk_mq_check_inflight(), but doesn't check if the status of rq is in flight.
>
> For the [1], because the rqs[] only contains the requests from the driver tag, once the io scheduler is used, crash
> may miss the check of sched_tags and can not watch its value . You mean that crash shouldn't cover this case,
> or do I misunderstand the sched_tags and static_rqs[]?
hmm, if so, I would like to know the reason why kernel doesn't count them.
> For the [2], the intent is to calculate all requests in the current queue from the bitmap(not only inflight, but also handling
> rq), that can help to get actual read and write request counts. But, to be honest, I'm not sure whether this is a reasonable
> method. Are you concerned that it might have repeat read/write request counts? Or should the crash follow the above
> kernel path exactly? Any idea about this?
Yes, it's one of my concerns. It does not count a request twice?
I didn't watch them, seems that the blk-mq bypass the io scheduler?
IMHO, basically crash should follow the kernel path exactly, including the
check for MQ_RQ_IN_FLIGHT. The "dev -d" option displays the same stats as
/proc/diskstats for non-mq devices, so displaying different stats for mq
devices will be confusing. And it will make tracking kernel changes easier.
The various scenarios of the block layer are complicated, it might be safe to keep exactly the same as the kernel path.
Let me adjust it a bit. Thank you for the suggestions.
Thanks.
Lianbo
Thanks,
Kazu
-- Crash-utility mailing list Crash-utility@xxxxxxxxxx https://listman.redhat.com/mailman/listinfo/crash-utility Contribution Guidelines: https://github.com/crash-utility/crash/wiki
