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@redhat.com> wrote:
>
>> Date: Wed, 15 Jun 2022 01:50:13 +0000
>> From: HAGIO KAZUHITO(?????)  <k-hagio-ab@nec.com>
>> To: "lijiang@redhat.com" <lijiang@redhat.com>,
>>          "crash-utility@redhat.com" <crash-utility@redhat.com>
>> 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@nec.com>
>> 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@nec.com>
>> ---
>>   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