[Crash-utility] [PATCH v2] files: support dump file pages from its address space

[Dave Anderson]

And a couple other things I forgot to mention...

The vast majority of the "files -M" output consists of either the file
descriptors for CHR, FIFO, SOCK, etc., files, or REG file descriptors 
that don't have any pages mapped into the task's address space.

(1) Wouldn't it make more sense to show *only* the file descriptors
    that have mapped pages?  And if the task has no mapped pages, 
    just display "(no mapped pages)".

(2) Is there any good reason to show the ROOT and CWD directories?

Dave

----- Original Message -----
> 
> Hi Oliver,
> 
> A few more comments and suggestions regarding your patch.
> 
> A couple things I noted when testing on a 32-bit x86.
> First, the columns don't line up correctly:
>   
>   crash> files -M 3804
>   PID: 3804   TASK: f466a5e0  CPU: 0   COMMAND: "crash"
>   ROOT: /    CWD: /root/crash-5.1.8
>    FD  ADDR-SPACE  PAGE-COUNT   INODE    TYPE  PATH
>     0  e6d6f63c      0     e6d6f570  CHR   /dev/pts/0
>     1  e6d6f63c      0     e6d6f570  CHR   /dev/pts/0
>     2  e6d6f63c      0     e6d6f570  CHR   /dev/pts/0
>     3  f53ec874      0     f53ec7a8  CHR   /dev/null
>     4  f02944d4      0     f0294408  CHR   /dev/crash
>     5  e6c6a294      0     e6c6a1c8  REG   /tmp/tmpfvd9PjN
>     6  e6ca9e54      0     e6ca9d88  FIFO
>     7  e6ca9e54      0     e6ca9d88  FIFO
>     8  e6c6a754   147034   e6c6a688  REG
>     /root/crash-5.1.8/snapshot-2.6.40.4-5.fc15.i686.PAE
>     9  e6caa3f4      0     e6caa328  FIFO
>   crash>
>   
> And secondly, taking the address_space e6c6a754 from the task above,
> again, shouldn't the page count above be reflected in the number of
> shown by the address_space tree dump, where the page dump seems to
> be missing about 20000 pages?:
>   
>   crash> files -m e6c6a754 | wc -l
>   128825
>   crash>
>   
> For the address_space page tree dump, I don't see any point in displaying the
> page_tree member address:
>   
>   crash> files -m ffff810218e31220
>   Address Space ffff810218e31220, page tree ffff810218e31228, 12 pages
>   
>         PAGE        PHYSICAL      MAPPING       INDEX CNT FLAGS
>   ffff81010774d1f8 221609000 ffff810218e31220        0  1 22010000001006c
>   ffff81010485b378 14ac59000 ffff810218e31220        1  1 14810000001006c
>   ffff810107993a78 22bc79000 ffff810218e31220        2  1 228100000010028
>   ffff8101049d3660 1517d4000 ffff810218e31220        3  1 150100000010028
>   ffff810103b29670 10e742000 ffff810218e31220        4  1 108100000010028
>   ffff810106d51ba0 1f3bec000 ffff810218e31220        5  1 1f0100000010028
>   ffff810103ac95f0 10cbd2000 ffff810218e31220        6  1 108100000010028
>   ffff810106c8cc18 1f03a5000 ffff810218e31220        7  1 1f0100000010028
>   ffff8101077b1028 223293000 ffff810218e31220        8  1 220100000010028
>   ffff810106cc03b0 1f125a000 ffff810218e31220        9  1 1f0100000010028
>   ffff810107a04cd8 22dccd000 ffff810218e31220        a  1 228100000010028
>   ffff8101078741b8 226a51000 ffff810218e31220        b  1 220100000010028
>   crash>
> 
> For that matter, displaying the address_space address is redundant
> since (1) it has to be entered as the command argument, and (2) it gets
> shown in every page line "MAPPING".  On the other hand, perhaps the inode
> that contains the address_space structure would be helpful, say, like
> this:
> 
>   crash> files -m ffff810218e31220
>   ADDRESS_SPACE     INODE             PAGES
>   ffff810218e31220  ffff810218e310e0  12
>  
>         PAGE        PHYSICAL      MAPPING       INDEX CNT FLAGS
>   ffff81010774d1f8 221609000 ffff810218e31220        0  1 22010000001006c
>   ffff81010485b378 14ac59000 ffff810218e31220        1  1 14810000001006c
>   ffff810107993a78 22bc79000 ffff810218e31220        2  1 228100000010028
>   ffff8101049d3660 1517d4000 ffff810218e31220        3  1 150100000010028
>   ffff810103b29670 10e742000 ffff810218e31220        4  1 108100000010028
>   ffff810106d51ba0 1f3bec000 ffff810218e31220        5  1 1f0100000010028
>   ffff810103ac95f0 10cbd2000 ffff810218e31220        6  1 108100000010028
>   ffff810106c8cc18 1f03a5000 ffff810218e31220        7  1 1f0100000010028
>   ffff8101077b1028 223293000 ffff810218e31220        8  1 220100000010028
>   ffff810106cc03b0 1f125a000 ffff810218e31220        9  1 1f0100000010028
>   ffff810107a04cd8 22dccd000 ffff810218e31220        a  1 228100000010028
>   ffff8101078741b8 226a51000 ffff810218e31220        b  1 220100000010028
>   crash>
> 
> Also, I usually try to avoid using upper-case capital letters as arguments
> unless there aren't any more logical lower-case letters left to use.
> 
> How about changing your "files -M" and "files -m" to something like:
> 
>   crash> files -m [pid|task]   (for your current files -M)
> 
> and
> 
>   crash> files -a <address_space>  (for your current "files -m");
> 
> And don't forget to update the help_files[] array with your new
> functionality.
> 
> Thanks,
>   Dave
> 
>   
> 
> --
> Crash-utility mailing list
> Crash-utility at redhat.com
> https://www.redhat.com/mailman/listinfo/crash-utility
>