On Fri, 30 Nov 2007 09:34:49 -0800
Christoph Lameter <clameter@xxxxxxx> 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?
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
