On Fri, Jun 17, 2022 at 8:10 AM HAGIO KAZUHITO(萩尾 一仁) <[email protected]> wrote:
> 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 <[email protected]> > wrote: > > > >> Date: Wed, 15 Jun 2022 01:50:13 +0000 > >> From: HAGIO KAZUHITO(?????) <[email protected]> > >> To: "[email protected]" <[email protected]>, > >> "[email protected]" <[email protected]> > >> Subject: [Crash-utility] [PATCH] Fix for "dev" command on Linux 5.11 > >> and later > >> Message-ID: <[email protected]> > >> 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 <[email protected]> > >> --- > >> 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? > > Not very important, It doesn't affect actual results. Anyway, the "search_sb_inodes()" doesn't make much sense to me.. > > > > > 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. > > Thanks. Other changes look good to me. Lianbo Thanks, > Kazu >
-- Crash-utility mailing list [email protected] https://listman.redhat.com/mailman/listinfo/crash-utility Contribution Guidelines: https://github.com/crash-utility/crash/wiki
