Hi Lianbo, thank you for the review. On 2022/06/16 18:22, lijiang wrote: > Hi, Kazu > Thank you for the fix. > On Wed, Jun 15, 2022 at 8:00 PM <crash-utility-request(a)redhat.com&gt; wrote: > >> Date: Wed, 15 Jun 2022 01:50:13 +0000 >> From: HAGIO KAZUHITO(?????) <k-hagio-ab(a)nec.com&gt; >> To: &quot;lijiang(a)redhat.com&quot; <lijiang(a)redhat.com&gt;, >> &quot;crash-utility(a)redhat.com&quot; <crash-utility(a)redhat.com&gt; >> Subject: [Crash-utility] [PATCH] Fix for "dev" command on Linux 5.11 >> and later >> Message-ID: <1655257808-3245-1-git-send-email-k-hagio-ab(a)nec.com&gt; >> Content-Type: text/plain; charset="iso-2022-jp" >> >> The following kernel commits eventually removed the bdev_map array in >> Linux v5.11 kernel: >> >> e418de3abcda ("block: switch gendisk lookup to a simple xarray") >> 22ae8ce8b892 ("block: simplify bdev/disk lookup in blkdev_get") >> >> Without the patch, the "dev" command fails to dump block device data >> with the following error: >> >> crash> dev >> ... >> dev: blkdevs or all_bdevs: symbols do not exist >> >> To get block device's gendisk, search blockdev_superblock.s_inodes >> instead of bdev_map. >> >> Signed-off-by: Kazuhito Hagio <k-hagio-ab(a)nec.com&gt; >> --- >> dev.c | 76 +++++++++++++++++++++++++++++++++++++++++++++++++++++++---- >> 1 file changed, 71 insertions(+), 5 deletions(-) >> >> diff --git a/dev.c b/dev.c >> index db97f8aebdc2..238dfe0fbe3c 100644 >> --- a/dev.c >> +++ b/dev.c >> @@ -24,6 +24,7 @@ static void dump_blkdevs_v2(ulong); >> static void dump_blkdevs_v3(ulong); >> static ulong search_cdev_map_probes(char *, int, int, ulong *); >> static ulong search_bdev_map_probes(char *, int, int, ulong *); >> +static ulong search_blockdev_inodes(int, ulong *); >> > > It could be good to be consistent with the above function name, I would > suggest renaming > the "search_blockdev_inodes()" to "search_sb_inodes()". Hmm, I think that search_blockdev_inodes() is more informative than search_sb_inodes(). It says what structures the function searches for gendisk. For the list name it searches, we can see it in the code. Is the consistency important here?

> > static void do_pci(void); >> static void do_pci2(void); >> static void do_io(void); >> @@ -493,9 +494,10 @@ dump_blkdevs(ulong flags) >> ulong ops; >> } blkdevs[MAX_DEV], *bp; >> >> - if (kernel_symbol_exists("major_names") && >> - kernel_symbol_exists("bdev_map")) { >> - dump_blkdevs_v3(flags); >> + if (kernel_symbol_exists("major_names") && >> + (kernel_symbol_exists("bdev_map") || >> + kernel_symbol_exists("blockdev_superblock"))) { >> + dump_blkdevs_v3(flags); >> return; >> } >> >> @@ -717,6 +719,7 @@ dump_blkdevs_v3(ulong flags) >> char buf[BUFSIZE]; >> uint major; >> ulong gendisk, addr, fops; >> + int use_bdev_map = kernel_symbol_exists("bdev_map"); >> >> if (!(len = get_array_length("major_names", NULL, 0))) >> len = MAX_DEV; >> @@ -745,8 +748,11 @@ dump_blkdevs_v3(ulong flags) >> strncpy(buf, blk_major_name_buf + >> OFFSET(blk_major_name_name), 16); >> >> - fops = search_bdev_map_probes(buf, major == i ? major : i, >> - UNUSED, &gendisk); >> + if (use_bdev_map) >> + fops = search_bdev_map_probes(buf, major == i ? >> major : i, >> + UNUSED, &gendisk); >> + else /* v5.11 and later */ >> + fops = search_blockdev_inodes(major, &gendisk); >> >> if (CRASHDEBUG(1)) >> fprintf(fp, "blk_major_name: %lx block major: %d >> name: %s gendisk: %lx fops: %lx\n", >> @@ -829,6 +835,66 @@ search_bdev_map_probes(char *name, int major, int >> minor, ulong *gendisk) >> return fops; >> } >> >> +/* For bdev_inode. See block/bdev.c */ >> +#define I_BDEV(inode) (inode - SIZE(block_device)) >> + >> > > Good understanding, this is equivalent to the contain_of(). > > +static ulong >> +search_blockdev_inodes(int major, ulong *gendisk) >> +{ >> + struct list_data list_data, *ld; >> + ulong addr, bd_sb, disk, fops = 0; >> + int i, inode_count, gendisk_major; >> + char *gendisk_buf; >> + >> + ld = &list_data; >> + BZERO(ld, sizeof(struct list_data)); >> + >> + get_symbol_data("blockdev_superblock", sizeof(void *), &bd_sb); >> + >> + addr = bd_sb + OFFSET(super_block_s_inodes); >> + if (!readmem(addr, KVADDR, &ld->start, sizeof(ulong), >> + "blockdev_superblock.s_inodes", QUIET|RETURN_ON_ERROR)) >> + return 0; >> + >> + if (empty_list(ld->start)) >> + return 0; >> + >> + ld->flags |= LIST_ALLOCATE; >> + ld->end = bd_sb + OFFSET(super_block_s_inodes); >> + ld->list_head_offset = OFFSET(inode_i_sb_list); >> + >> + inode_count = do_list(ld); >> + >> + gendisk_buf = GETBUF(SIZE(gendisk)); >> + >> + for (i = 0; i < inode_count; i++) { >> + addr = I_BDEV(ld->list_ptr[i]) + >> OFFSET(block_device_bd_disk); >> + if (!readmem(addr, KVADDR, &disk, sizeof(ulong), >> + "block_device.bd_disk", QUIET|RETURN_ON_ERROR)) >> + continue; >> + >> + if (!disk) >> + continue; >> + >> + if (!readmem(disk, KVADDR, gendisk_buf, SIZE(gendisk), >> + "gendisk buffer", QUIET|RETURN_ON_ERROR)) >> + continue; >> + >> + gendisk_major = INT(gendisk_buf + OFFSET(gendisk_major)); >> + if (gendisk_major != major) >> + continue; >> + >> + fops = ULONG(gendisk_buf + OFFSET(gendisk_fops)); >> + if (fops) { >> + *gendisk = disk; >> + break; >> + } >> + } >> + >> > > Because the ld->flags is set to LIST_ALLOCATE, here need to free it as > below: > FREEBUF(ld->list_ptr); Oh, good catch. I referred to nr_blockdev_pages_v2() to implement, but missed it. Will add.

Kazu