|
> Date: Tue, 30 Jun 2015 09:26:22 -0400 > From: anderson@xxxxxxxxxx > To: crash-utility@xxxxxxxxxx > 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. :-) |
-- Crash-utility mailing list Crash-utility@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/crash-utility
