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