> Date: Mon, 29 Jun 2015 15:16:45 -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 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 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"?

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_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

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.