Re: introduce a new command to display the disk's information

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



----- Original Message -----
> 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

Hello Wen,

This second patch looks good -- I did make a few small changes.
I re-worded the help page:
  
  crash> help dev
  
  NAME
    dev - device data
  
  SYNOPSIS
    dev [-i | -p | -d]
  
  DESCRIPTION
    If no argument is entered, this command dumps character and block
    device data.
  
      -i  display I/O port usage; on 2.4 kernels, also display I/O memory usage.
      -p  display PCI device data.
      -d  display disk I/O statistics:
           TOTAL: total number of allocated I/O requests that are in-progress
            SYNC: I/O requests that are synchronous
           ASYNC: I/O requests that are asynchronous
            READ: I/O requests that are reads (older kernels)
           WRITE: I/O requests that are writes (older kernels)
             DRV: I/O requests that are in-flight in the device driver
  
  EXAMPLES
  ...
  
I reformatted the output to allow for larger disk name strings,
such as "cciss/c0d0" in this example:
  
  crash> dev -d
  MAJOR GENDISK            NAME       REQUEST QUEUE      TOTAL  READ WRITE   DRV
    104 0xffff81007e422800 cciss/c0d0 0xffff81007f6d4048     1     0     1     1
    253 0xffff81007e89fa00 dm-0       0xffff81007f6d4c98     0     0     0     0
    253 0xffff81007eace200 dm-1       0xffff81007f6d4670     0     0     0     0
      9 0xffff81007eb43400 md0        0xffff81007d397928     0     0     0     0
  crash>
  
And I created a dev_table.flags DISKIO_INIT bit instead of using
the static "initialized" variable.
  
Queued for crash-6.0.3.

Thanks,
  Dave





--
Crash-utility mailing list
Crash-utility@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/crash-utility


[Index of Archives]     [Fedora Development]     [Fedora Desktop]     [Fedora SELinux]     [Yosemite News]     [KDE Users]     [Fedora Tools]

 

Powered by Linux