Re: [PATCH -mm 05/25] define page_file_cache() function

[Rik van Riel <riel@redhat.com>]

On Fri, 6 Jun 2008 18:04:34 -0700
Andrew Morton <akpm@linux-foundation.org> wrote:

> > Unfortunately this needs to use a page flag, since the
> > PG_swapbacked state needs to be preserved all the way
> > to the point where the page is last removed from the
> > LRU.  Trying to derive the status from other info in
> > the page resulted in wrong VM statistics in earlier
> > split VM patchsets.
> 
> argh.  How many are left?

After this patch, 19 are in use.  I believe the way we
keep track of zones means we have 24 total on i386, but
the exact value depends on CONFIG_NODES_SHIFT, which
determines the maximum number of NUMA nodes supported.

The code in include/linux/mm.h makes sure the user
cannot compile the kernel with too high a NODES_SHIFT
value.

On 64 bits, we have more than enough flags.

> > +static inline int page_file_cache(struct page *page)
> > +{
> > +	if (PageSwapBacked(page))
> > +		return 0;
> > +
> > +	/* The page is page cache backed by a normal filesystem. */
> > +	return 2;
> 
> 2?
> 
> Maybe bool would suit here.

It will be replaced with LRU_FILE in a later patch.  I'll change it
to 1 in this patch.
 
> Maybe a better name would be page_is_file_cache().  The gnu (gcc?)
> convention of putting _p at the end of predicate functions' names makes
> heaps of sense.

I'll change it to page_is_file_cache()

> This function doesn't do enough stuff to do that which it says it does.
> There must be a whole pile of preconditions which the caller must
> evaluate before this function can be usefully used.  I mean, it would
> be a bug to pass an anonymous page or a slab page or whatever into
> here?

Passing in a slab page would indeed not give a useful result.  This
function is meant to help functions that manipulate the LRU lists
(which already know the page is or should be an LRU page) sort the
page onto the right list.

I have amended the comment to reflect this.

-- 
All rights reversed.