On Mon, Dec 03, 2007 at 02:10:20PM -0800, Andrew Morton wrote:
> 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.
Hmmmm. Many of the places where these functions are called will have
the page locked and the mapping protected against truncate.
A quick pass through the patches indicates the changes to rmap.c,
migrate.c, alloc_page_buffers(), and drivers/block/rd.c seem to be
the only ones that are suspect. Almost everywhere else we either
use the inode->i_mapping or the page comes in locked (i.e. would
crash on struct inode * inode = page->mapping->host; at function entry
otherwise).
It seems the exposure here is not that great. I'm ambivalent, though; I
don'tmind what interface there is just so long as it cleans up this mess ;)
> 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?
Definitely an improvement.
Cheers,
Dave.
--
Dave Chinner
Principal Engineer
SGI Australian Software Group
-
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
