12:59 a.m.
Date: Mon, 29 Jun 2015 15:16:45 -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 Dave,
>
> This is v4 patch for files memory mapping dump support.
>
> The major changes in this version are,
>
> 1. Your alignment patch for NRPAGES
>
> 2. Changed files -a to files -p
>
> Changed output and displayed INODE, ADDRESS_SPACE, NRPAGES
> at beginning.
>
> 3. Updated help.c and added exmaple outputs for new options.
>
> 4. Some minor code cleanup, for function name defined in defs.h
>
> Here is my patch,
>
> Added two options in files command,
>
> 1. -m option, which allows dump file mapping and
> page count for each files
> 2. -p option, which could dump each pages within
> the mapping for given inode address
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.
File address space is different with task address space. The users who are using
filescommand 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"?
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.
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_spacestruct 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
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.