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

[yangoliver]

> Date: Fri, 19 Jun 2015 15:40:22 -0400
> From: anderson at redhat.com
> To: crash-utility at 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.The major problem here is ADDR-SPACE and PAGE-COUNT width is greater than long size on 32 bit kernel.
> 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,
address_space.page_tree e6c6a754
Second step, using tree command dump pages,
tree -t radix -N <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 itsmemory 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. 		 	   		  
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/crash-utility/attachments/20150624/31bb482a/attachment.htm>