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

[yangoliver]

> Date: Tue, 30 Jun 2015 09:26:22 -0400
> From: anderson at redhat.com
> To: crash-utility at redhat.com
> Subject: Re: [Crash-utility] [PATCH v4] files: support dump file memory mapping
> 
> 
> 
> ----- 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.

Sure, I will. Thanks for your helps and comments.I hope I can do better for my next patch. :-) 		 	   		  
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/crash-utility/attachments/20150630/73e004d3/attachment.htm>