Re: [Crash-utility] [PATCH 1/2] Support for "dev -d|-D" options by parsing bitmap in blk-mq layer
-----Original Message-----
Hi, Kazu
Thank you for the comment.
On Thu, May 12, 2022 at 1:59 PM HAGIO KAZUHITO(萩尾 一仁) <k-hagio-ab(a)nec.com
<mailto:k-hagio-ab@nec.com>
> wrote:
Hi Lianbo,
Thank you for working on this, we know the stats are very useful but
it's a tough work to support :-(
I have a few questions about basic design:
- Is it possible to re-use sbitmap_for_each_set() etc. in sbitmap.c?
They were implemented like the sbitmap functions in kernel, maybe
we can imitate bt_for_each() in kernel. Otherwise, we will have to
update sbitmap.c and dev.c when a change in sbitmap structure occurs.
or did you find any reason that they cannot be used?
The main reason is that they have different code logic, and not sure if that
is easy to reuse, just like the sbitmap_bitmap_show(), which doesn't reuse
the sbitmap_for_each_set() in the sbitmap.c. ^^
Got it.
crash's sbitmap_bitmap_show() imitates kernel's sbitmap_bitmap_show(),
they print the whole bitmap:
# cat /sys/kernel/debug/block/sda/hctx0/tags_bitmap
00000000: 0000 0000
crash's sbitmap_for_each_set() imitates kernel's sbitmap_for_each_set(),
they do callback on each set bit. This time we want to check this like
blk_mq_queue_tag_busy_iter(), it should be used.
But if it can be easily reused, that would be a good idea.
Yes, if we can imitate bt_for_each(), it will make tracking changes easier.
Note: the following kernel commit(mentioned in the cover-letter) may
make
that the sbitmap can not work as expected.
[3] commit <3301bc53358a> ("lib/sbitmap: kill 'depth' from
sbitmap_word")
Thanks for the info. so do you mean we need to fix sbitmap.c for 5.18?
I think this can be done later.
- Is it possible to use the new logic when blk-mq has sbitmap, even if
a kernel has rq_dispatched and rq_completed?
If we would like to replace the rq_dispatched/rq_completed with the sbitmap
to calculate the disk I/O statistics, that might be extra work.
But anyway, if it's worth doing, we might consider doing it at the next stage.
What do you think?
sorry, cannot get it. I just mean this flow:
if blk-mq uses sbitmap then
get_mq_diskio_from_hw_queues(q, &tmp);
return;
else if rq_dispatched/rq_completed exists then
...
get_one_mctx_diskio(mctx_addr, &tmp);
What is the extra work?
BTW: I will improve the v1 and post v2 later, because I have got some feedback
from Nitin and John pittman. Also discussed with you in another email thread.
Thanks, I see.
Kazu