[Crash-utility] introduce a new command to display the disk's information
Wen Congyang
wency at cn.fujitsu.com
Mon Jan 30 05:32:21 UTC 2012
Hi, Dave
At 01/21/2012 04:24 AM, Dave Anderson Wrote:
>
>
> ----- Original Message -----
>> Hi, Dave
>>
>> When we investigate the problems of disk I/O, we want to get the disk's
>> gendisk address and request queue's address easily, and the requests num
>> is also important.
>>
>> Tha attached patch introduce a new command diskio to display such
>> information.
>>
>> Thanks
>> Wen Congyang
>
> Hello Wen,
>
> I built and tested this patch, and it certainly contains some useful
> information. However, I do have a several suggestions.
>
> First, there's no need to make it a unique command. It would be
> far more appropriate to make it a "dev" command option, which for
> example, already shows gendisk structure addresses for each of the
> block devices.
>
> So please move all of your functions from kernel.c to dev.c.
> Then, for example, use "dev -d", and have cmd_dev() call your
> command like this:
>
> while ((c = getopt(argcnt, args, "dpi")) != EOF) {
> switch(c)
> {
> case 'd':
> diskio_option();
> return;
> ...
>
> Since all of the new offset_table and size_table entries are only
> used by this command, you can avoid unnecessarily initializing
> everything in kernel_init() by doing something like this:
>
> static void
> diskio_option(void)
> {
> if (INVALID_MEMBER(class_devices)) {
> MEMBER_OFFSET_INIT(class_devices, "class", "class_devices");
> if (INVALID_MEMBER(class_devices))
> MEMBER_OFFSET_INIT(class_devices, "class", "devices");
> MEMBER_OFFSET_INIT(class_p, "class", "p");
> MEMBER_OFFSET_INIT(class_private_devices, "class_private",
> "class_devices");
> MEMBER_OFFSET_INIT(device_knode_class, "device", "knode_class");
> MEMBER_OFFSET_INIT(device_node, "device", "node");
> MEMBER_OFFSET_INIT(device_type, "device", "type");
> MEMBER_OFFSET_INIT(gendisk_dev, "gendisk", "dev");
> if (INVALID_MEMBER(gendisk_dev))
> MEMBER_OFFSET_INIT(gendisk_dev, "gendisk", "__dev");
> MEMBER_OFFSET_INIT(gendisk_kobj, "gendisk", "kobj");
> MEMBER_OFFSET_INIT(gendisk_part0, "gendisk", "part0");
> MEMBER_OFFSET_INIT(gendisk_queue, "gendisk", "queue");
> MEMBER_OFFSET_INIT(hd_struct_dev, "hd_struct", "__dev");
> MEMBER_OFFSET_INIT(klist_k_list, "klist", "k_list");
> MEMBER_OFFSET_INIT(klist_node_n_klist, "klist_node", "n_klist");
> MEMBER_OFFSET_INIT(klist_node_n_node, "klist_node", "n_node");
> MEMBER_OFFSET_INIT(kobject_entry, "kobject", "entry");
> MEMBER_OFFSET_INIT(kset_list, "kset", "list");
> MEMBER_OFFSET_INIT(request_list_count, "request_list", "count");
> MEMBER_OFFSET_INIT(request_queue_in_flight, "request_queue",
> "in_flight");
> MEMBER_OFFSET_INIT(request_queue_rq, "request_queue", "rq");
> MEMBER_OFFSET_INIT(subsys_private_klist_devices, "subsys_private",
> "klist_devices");
> MEMBER_OFFSET_INIT(subsystem_kset, "subsystem", "kset");
> STRUCT_SIZE_INIT(subsystem, "subsystem");
> STRUCT_SIZE_INIT(class_private, "class_private");
> MEMBER_SIZE_INIT(rq_in_flight, "request_queue", "in_flight");
> MEMBER_SIZE_INIT(class_private_devices, "class_private",
> "class_devices");
> }
>
> display_all_diskio();
> }
>
> Secondly, the whole READ/WRITE issue is confusing to the uninitiated
> (like myself). After speaking with Jeff Moyer, we believe the output
> of your command could be clarified.
>
> Your help page indicates:
>
> +" This command dumps I/O statistics of all disks:",
> +" TOTAL: The total requests that have not been ended",
> +" READ: The total read requests that have not been ended",
> +" WRITE: The total write requests that have not been ended",
> +" DRV: The total requests that have been in the driver, but not end",
> +"",
> +" Note: some kernel does not contain read/write requests, and the command",
> +" will output '-----'"
> +"\nEXAMPLES",
> +" %s> diskio",
> +" MAJOR GENDISK NAME RUQUEST QUEUE TOTAL READ WRITE DRV",
> +" 008 0xe00000010773ea80 sda 0x3000000109c9fbf0 12 0 12 0",
> +" 008 0xe00000010773e680 sdb 0x3000000109c9f8a0 2 2 0 0",
> +" 008 0xe000000107781d80 sdc 0x300000010c268050 6 0 6 6",
> +" 008 0xe00000010773e080 sdd 0x300000010c26bbf0 0 0 0 0",
> +" 008 0xe00000010773dc80 sde 0x300000010c3dd780 0 0 0 0",
> +NULL
>
> As Jeff explained to me, this is how it works:
>
> (1) In older kernels, this enum did not exist:
>
> enum {
> BLK_RW_ASYNC = 0,
> BLK_RW_SYNC = 1,
> };
>
> and in that case, the request_list.count[2] values are the
> READ/WRITE values, as you show in the help page example above.
>
> (2) In newer kernels, the enum does exist, and the meaning of the
> request_list.count[2] values are ASYNC/SYNC requests. In that
> case, you show "-----" under the READ and WRITE columns, which
> I found *very* confusing.
>
> What Jeff Moyer suggested is that -- in the case of new kernels -- you
> should alternatively show the counts with ASYNC and SYNC columns like
> this:
>
> MAJOR GENDISK NAME REQUEST QUEUE TOTAL ASYNC SYNC DRV",
> ...
>
> And the READ/WRITE vs. ASYNC/SYNC output display difference should be
> referenced in the help page output.
>
> Third, a minor nit -- note that you misspelled it as "RUQUEST" both in
> the command and in the help page. Also, it appears that you used an IA64 as
> the example, given the GENDISK addresses. But aren't the REQUEST QUEUE
> addresses below beginning with "0x300..." user-space addresses for IA64?
>
> +" MAJOR GENDISK NAME RUQUEST QUEUE TOTAL READ WRITE DRV",
> +" 008 0xe00000010773ea80 sda 0x3000000109c9fbf0 12 0 12 0",
> +" 008 0xe00000010773e680 sdb 0x3000000109c9f8a0 2 2 0 0",
> +" 008 0xe000000107781d80 sdc 0x300000010c268050 6 0 6 6",
> +" 008 0xe00000010773e080 sdd 0x300000010c26bbf0 0 0 0 0",
> +" 008 0xe00000010773dc80 sde 0x300000010c3dd780 0 0 0 0",
>
> In any case, can you change it to use either x86_64 or x86 as an example?
>
> Fourth, in testing this with a 2.6.25 kernel, the command hangs:
>
> crash> dev -d
> MAJOR GENDISK NAME REQUEST QUEUE TOTAL READ WRITE DRV
> 2 0xffff81012d8a5000 fd0 0xffff81012dc053c0 0 0 0 0
> 22 0xffff81012dc6b000 hdc 0xffff81012d8ae340 0 0 0 0
> 8 0xffff81012dd71000 sda 0xffff81012d8af040 0 0 0 0
> < hangs here forever >
>
> I'm not absolutely sure, but if I hit CTRL-C under gdb, it seems that it's
> spinning in next_disk_list() in that "goto again" loop?
>
> Fifth, I tried it on a much older RHEL3 kernel, which shows:
>
> crash> dev -d
> dev: invalid request_queue.in_flight's size
> crash>
>
> It's not really invalid size, but it's more the case that you
> don't support that old a kernel version. In that case, instead
> of doing this:
>
> error(FATAL, "invalid request_queue.in_flight's size\n");
>
> you should do this instead:
>
> option_not_supported('d');
>
> And finally, whenever adding fields to the offset_table or size_table,
> please display their values in dump_offset_table() for the "help -o"
> command.
I have updated the patch.
Thanks
Wen Congyang
>
> Thanks,
> Dave
>
>
>
>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-display-all-disk-I-O-statistics.patch
Type: text/x-patch
Size: 20292 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/crash-utility/attachments/20120130/db92be86/attachment.bin>
More information about the Crash-utility
mailing list