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

Date: Tue, 30 Jun 2015 09:26:22 -0400 From: anderson(a)redhat.com To: crash-utility(a)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.