Re: [patch 01/19] Define functions for page cache handling

[Andrew Morton <akpm@linux-foundation.org>]

On Fri, 30 Nov 2007 09:34:49 -0800
Christoph Lameter <clameter@sgi.com> wrote:

> We use the macros PAGE_CACHE_SIZE PAGE_CACHE_SHIFT PAGE_CACHE_MASK
> and PAGE_CACHE_ALIGN in various places in the kernel. Many times
> common operations like calculating the offset or the index are coded
> using shifts and adds. This patch provides inline functions to
> get the calculations accomplished without having to explicitly
> shift and add constants.
> 
> All functions take an address_space pointer. The address space pointer
> will be used in the future to eventually support a variable size
> page cache. Information reachable via the mapping may then determine
> page size.
> 
> New function                    Related base page constant
> ====================================================================
> page_cache_shift(a)             PAGE_CACHE_SHIFT
> page_cache_size(a)              PAGE_CACHE_SIZE
> page_cache_mask(a)              PAGE_CACHE_MASK
> page_cache_index(a, pos)        Calculate page number from position
> page_cache_next(addr, pos)      Page number of next page
> page_cache_offset(a, pos)       Calculate offset into a page
> page_cache_pos(a, index, offset)
>                                 Form position based on page number
>                                 and an offset.
> 
> This provides a basis that would allow the conversion of all page cache
> handling in the kernel and ultimately allow the removal of the PAGE_CACHE_*
> constants.
> 
> ...
>
> +/*
> + * Functions that are currently setup for a fixed PAGE_SIZEd. The use of
> + * these will allow the user of largere page sizes in the future.
> + */
> +static inline int mapping_order(struct address_space *a)
> +{
> +	return 0;
> +}
> +
> +static inline int page_cache_shift(struct address_space *a)
> +{
> +	return PAGE_SHIFT;
> +}
> +
> +static inline unsigned int page_cache_size(struct address_space *a)
> +{
> +	return PAGE_SIZE;
> +}
> +
> +static inline unsigned int page_cache_offset(struct address_space *a,
> +		loff_t pos)
> +{
> +	return pos & ~PAGE_MASK;
> +}
> +
> +static inline pgoff_t page_cache_index(struct address_space *a,
> +		loff_t pos)
> +{
> +	return pos >> page_cache_shift(a);
> +}

These will of course all work OK as they are presently implemented.

But you have callsites doing things like

	page_cache_size(page_mapping(page));

which is a whole different thing.  Once page_cache_size() is changed to
look inside the address_space we need to handle races against truncation
and we need to handle the address_space getting reclaimed, etc.

So I think it would be misleading to merge these changes at present - they
make it _look_ like we can have variable PAGE_CACHE_SIZE just by tweaking a
bit of core code, but we in fact cannot do that without a careful review of
all callsites and perhaps the addition of new locking and null-checking.

Now, one possible way around this is to rework all these functions so they
take only a page*, and to create (and assert) the requirement that the caller
has locked the page.  That's a little bit inefficient (additional calls to
page_mapping()) but it does mean that we can now confidently change the
implementation of these functions as you intend.


And a coding nit: when you implement the out-of-line versions of these
functions you're going to stick with VFS conventions and use the identifier
`mapping' to identify the address_space*.  So I think it would be better to
also call in `mapping' in these inlined stubbed functions, rather than `a'.
No?