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@xxxxxxxxxx> wrote:
>
>> Date: Wed, 15 Jun 2022 01:50:13 +0000
>> From: HAGIO KAZUHITO(?????) <k-hagio-ab@xxxxxxx>
>> To: "lijiang@xxxxxxxxxx" <lijiang@xxxxxxxxxx>,
>> "crash-utility@xxxxxxxxxx" <crash-utility@xxxxxxxxxx>
>> Subject: [PATCH] Fix for "dev" command on Linux 5.11
>> and later
>> Message-ID: <1655257808-3245-1-git-send-email-k-hagio-ab@xxxxxxx>
>> 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@xxxxxxx>
>> ---
>> 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?
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,
Kazu
--
Crash-utility mailing list
Crash-utility@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/crash-utility
Contribution Guidelines: https://github.com/crash-utility/crash/wiki
