8:26 a.m.
----- 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