-----Original Message-----
> Hi, Kazu
> Thank you for the comment.
>
> On Thu, May 12, 2022 at 1:59 PM HAGIO KAZUHITO(萩尾　一仁) <k-hagio-ab@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