On Fri, Dec 24, 2021 at 3:34 PM HAGIO KAZUHITO(萩尾 一仁)
<k-hagio-ab(a)nec.com> wrote:
> -----Original Message-----
> On Fri, Dec 24, 2021 at 1:14 PM HAGIO KAZUHITO(萩尾 一仁)
> <k-hagio-ab(a)nec.com> wrote:
> >
> > Hi Lianbo,
> >
> > -----Original Message-----
> > > Kernel commit <9a14d6ce4135> ("block: remove debugfs
blk_mq_ctx
> > > dispatched/merged/completed attributes") removed the member
> > > rq_dispatched and rq_completed from struct blk_mq_ctx. Without
> > > this patch, crash will fail with the following error:
> > >
> > > crash> dev -d
> > > MAJOR GENDISK NAME REQUEST_QUEUE TOTAL ASYNC SYNC
> > >
> > > dev: invalid structure member offset: blk_mq_ctx_rq_dispatched
> > > FILE: dev.c LINE: 4229 FUNCTION: get_one_mctx_diskio()
> >
> > Thank you for working on this.
> > IIUC, crash will need sbitmap functionality to support this fully.
> >
> > Meanwhile, I think it's OK to skip the count, but it should say
"actually zero"
> > or "not supported" so that users can know it, for example:
> >
> Thank you for the comment, Kazu.
>
> > crash> dev -d
> > MAJOR GENDISK NAME REQUEST_QUEUE TOTAL ASYNC SYNC
> > 8 ffff92bbd102e400 sdb ffff92bbf04e3678 0 0 0
> > dev: sdb: Disk I/O statistics is not supported in this kernel
> > 8 ffff92bbd1014400 sdc ffff92bbf04e26e8 0 0 0
> > dev: sdc: Disk I/O statistics is not supported in this kernel
> > 11 ffff92c347da5e00 sr0 ffff92bbd9c3e528 0 0 0
> > 253 ffff92bbca726e00 dm-0 ffff92bbcad7dd60 0 0 0
> >
> > What do you think?
> >
>
> I agree with you that it should be good to have the above information
> for users, but it looks like the statistics result is redundant and
> the readability is not very good. What do you think of removing that
> statistical result and only outputting prompt information? For
> example:
>
> crash> dev -d
> MAJOR GENDISK NAME REQUEST_QUEUE TOTAL ASYNC SYNC
> dev: sda: Disk I/O statistics is not supported in this kernel
> dev: sr0: Disk I/O statistics is not supported in this kernel
> dev: dm-0: Disk I/O statistics is not supported in this kernel
> dev: dm-1: Disk I/O statistics is not supported in this kernel
> dev: dm-2: Disk I/O statistics is not supported in this kernel
I would prefer to display information like the addresses of request_queue
as far as we can. No statistics, but still we will often want to see the
structures.
So for example:
crash> dev -d
MAJOR GENDISK NAME REQUEST_QUEUE TOTAL ASYNC SYNC
8 ffff92bbd102e400 sdb ffff92bbf04e3678 (not supported)
8 ffff92bbd1014400 sdc ffff92bbf04e26e8 0 0 0
...
crash> request_queue ffff92bbf04e3678 | head -n 3
struct request_queue {
last_merge = 0x0,
elevator = 0xffff92bbc449b000,
Is this possible?
Yes, thanks for the suggestion, it looks good. Will post v2 later.
crash> dev -d
MAJOR GENDISK NAME REQUEST_QUEUE TOTAL ASYNC SYNC
8 ffff996f81fb1800 sda ffff996f82545750
(not supported)
11 ffff99722a8d1000 sr0 ffff996f82540fe0
(not supported)
253 ffff9970b4671c00 dm-0 ffff9970b6505750
(not supported)
253 ffff9970b3493200 dm-1 ffff9970b5d8e730
(not supported)
253 ffff99722a8d5c00 dm-2 ffff996f851a5750
(not supported)
>
> How about the following changes?
> ---
> dev.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/dev.c b/dev.c
> index effe789f38d8..3cf70c41cd17 100644
> --- a/dev.c
> +++ b/dev.c
> @@ -4465,6 +4465,12 @@ display_one_diskio(struct iter *i, unsigned
> long gendisk, ulong flags)
> if (is_skipped_disk(disk_name))
> return;
>
> + if (!MEMBER_EXISTS("blk_mq_ctx", "rq_dispatched")
&&
> + !MEMBER_EXISTS("blk_mq_ctx", "rq_completed")) {
> + fprintf(fp, "dev: %s: Disk I/O statistics is not
> supported in this kernel\n", disk_name);
> + return;
> + }
> +
> readmem(gendisk + OFFSET(gendisk_queue), KVADDR, &queue_addr,
> sizeof(ulong), "gen_disk.queue", FAULT_ON_ERROR);
> readmem(gendisk + OFFSET(gendisk_major), KVADDR, &major, sizeof(int),
> --
>
>
> Thanks.
> Lianbo
>
> >
> > >
> > > Signed-off-by: Lianbo Jiang <lijiang(a)redhat.com>
> > > ---
> > > dev.c | 4 ++++
> > > 1 file changed, 4 insertions(+)
> > >
> > > diff --git a/dev.c b/dev.c
> > > index effe789f38d8..dd21511e5dfc 100644
> > > --- a/dev.c
> > > +++ b/dev.c
> > > @@ -4246,6 +4246,10 @@ get_mq_diskio(unsigned long q, unsigned long
*mq_count)
> > > unsigned long mctx_addr;
> > > struct diskio tmp;
> > >
> > > + if (!MEMBER_EXISTS("blk_mq_ctx",
"rq_dispatched") &&
> > > + !MEMBER_EXISTS("blk_mq_ctx",
"rq_completed"))
> > > + return;
> > > +
> > > memset(&tmp, 0x00, sizeof(struct diskio));
> > >
> > > readmem(q + OFFSET(request_queue_queue_ctx), KVADDR,
&queue_ctx,
> > > --
> > > 2.20.1
> >