[Crash-utility] [PATCH v4] files: support dump file memory mapping

[Dave Anderson]

----- Original Message -----
> > Hi Oliver,
> > 
> > The more I play with this patch, the more I wonder why they should show
> > the "MAPPING" address at all. 
> > 
> > Your "help files" patch indicates this:
> > 
> > -p inode given a hexadecimal inode address, dump all memory pages in
> > its address space.
> > -m show inode memory mapping information, including mapping
> > address, page counts within the mapping.
> > 
> > The output of the "files -m" command could more accurately be described as: 
> > 
> > For each open file, how many pages does that file currently have in the page cache?
> > 
> > and "files -p" could be described as:
> > 
> > For a given inode, list the pages that are currently in the page cache.
> > 
> > The NRPAGES count shown by "files -m" does not reflect that the pages are 
> > "mapped" into the task's address space. So why bother showing the embedded
> > address_space inode.i_mapping address? It somewhat confuses the issue, 
> > leading the user to think that all of those pages are mapped into to the
> > task's address space. If the user is interested in pages that are actually
> > mapped into its address space, the "vm -p" command does just that. 
> 
> Should we use I_MAPPING or IMAPPING instead of MAPPING? 
> 
> IMAPPING means inode memory mapping, that could make it more clear.

Yes -- in fact I already made that change to your v4 patch.  In the case of "kmem -p"
output, "MAPPING" implies "page->mapping".  In this case, using "I_MAPPING" would 
shift the focus to "inode->i_mapping".  (even though they point to the same thing)

> 
> File address space is different with task address space. The users who are using files
> command should have this knowledge. 
> 
> The help information could be,
> 
> -m show inode memory mapping information. 
>       I_MAPPING is the file address space for page/buffer cache. 
>       NRPAGES the number of pages in page/buffer caches in the file address space.
> 
> > For that matter, in case of this new option, a better option letter might be "files -c"
> > for "page cache".
> 
> Did you mean I should use "files -c" to replace "files -m"?

Right.  Going back to your very first patch-post, you stated:

  This patch add -M and -m option for file commands, which allow to dump
  page cache for a file.

I would think "-m" implies "mapping" or "mapped", whereas the NRPAGES value
is typically not a count of open file pages that are mapped into the task's
address space.  

> 
> I'm ok with this new option. 
> 
> > 
> > And for that matter, I'm wondering what benefit there is to have the
> > "files -p" option show the inode's i_mapping address_space address? 
> > And yes, I realize it was *my* suggestion to do so, but I'm having 
> > second thoughts about the output of both commands.
> > 
> > So again, as I understand it, all we care about in "files -m" is the number
> > of pages each open file has in the page cache, and that "files -p" verbosely
> > dumps the pages a file has in the page cache. 
> > 
> > Do you have a compelling reason to show the i_mapping address in either
> > command?
> 
> 1. We can remove the i_mapping from files -p output.
> 
>     Because i_mapping address showed in page dump output as well. 
>         
>     But we may still need INODE, NRPAGES as the output header at the beginning.
> 
>     The header is useful when we save the output to a file, and let a scripts to process the output.    

OK good, especially given the page->mapping in each page's line.

> 2. We may need keep i_mapping in "file -m" output
> 
> The typical debug scenario is, 
>        a. use foreach files -m to find who hole max page caches.
>        b. Then use files -p to dump the pages in inode address space.
> 
> But it is also a good case we want to debug file write back behaviors by checking the 
> struct address_space.  Both files -m and files -p have the i_mapping, means we can directly check
> struct address_space without walking the data structure from inode to i_mapping.
> 
> I highlighted the members we may want to check
> 
> crash> struct address_space
> struct address_space {
>     struct inode *host;
>     struct radix_tree_root page_tree;
>     spinlock_t tree_lock;
>     unsigned int i_mmap_writable;
>     struct rb_root i_mmap;
>     struct list_head i_mmap_nonlinear;
>     struct mutex i_mmap_mutex;
>     unsigned long nrpages;
>     unsigned long writeback_index;   >>>>>>>>>>>>>>Next writeback starts point
>     const struct address_space_operations *a_ops;>>>>>>address space ops, depending on file system type
>     unsigned long flags;
>     struct backing_dev_info *backing_dev_info; >>>>>>>>backing device
>     spinlock_t private_lock;
>     struct list_head private_list;>>>>>>>>>>>>>>>>>>>could be buffer head struct, depending the context
>     void *private_data;
> }
> SIZE: 168
> 

OK, I'll buy that.  And as I think you mentioned earlier, you could also run a reference
search on an address_space address with "foreach files -cR <address_space address>".

So in the interest of expediency, what I will do is this:

  (1) change "files -m" to "files -c"
  (2) drop the MAPPING column from "files -p"
  (3) reword the description of the two options in the help page to emphasize
      that the NRPAGES count and page dumps are page cache counts/page-dumps
  (4) either figure out a way to compress the help page example outputs into
      80 columns, or drop the files -p example completely

I'll post the patch this afternoon and you can verify it tonight.

Thanks,
  Dave




> If you really think we need remove the i_mapping from files -m output, I could do that.
> Under "files -m"  output context, user still can access address_space data structure by
> walking from inode struct