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@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/crash-utility
>
--
Crash-utility mailing list
Crash-utility@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/crash-utility
