> Date: Fri, 19 Jun 2015 15:40:22 -0400
> From: anderson@redhat.com
> To: crash-utility@redhat.com
> Subject: Re: [Crash-utility] [PATCH v2] files: support dump file pages from its address space
>
>
> 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>

I fixed line up problem in v3 patch, please check v3 patch in my another mail.

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

I can't reproduce this issue on my 32bit kernel.

Could you verify whether you can get the same number of pages by tree command?

First step, get the page_tree address,

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

I removed page_tree address in patch v3.

>
> 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:

In my v3 patch, I files -a accept inode as parameter, instead of address space address.

The benefit of this change is, files -a is more separate with files -m. 

We decoupled two commands here.

In many cases, when we get a file from kernel core, we have the requirements that dump its

memory pages by using files -a with this file inode.

>
> 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:

Yes.

I used -m and -a now.

But how about using -p instead of -a for page dump?

> 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.

Give an update in v3.

I didn't add examples at this time point. Will add it after CLI got confirmed.

Anyway, please review all related changes in v3 patch.

Thanks.