> Date: Tue, 30 Jun 2015 09:26:22 -0400
> From: anderson@redhat.com
> To: crash-utility@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.
I hope I can do better for my next patch. :-)