On Fri, Jun 17, 2022 at 8:10 AM HAGIO KAZUHITO(萩尾 一仁) <k-hagio-ab@xxxxxxx> 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 <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?
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 Crash-utility@xxxxxxxxxx https://listman.redhat.com/mailman/listinfo/crash-utility Contribution Guidelines: https://github.com/crash-utility/crash/wiki
