Re: [patch 01/19] Define functions for page cache handling
On Mon, 3 Dec 2007, Andrew Morton wrote: > 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. Right. The page must be locked for that to work right. I tried to avoid the above construct as much as possible by relying on the inode mapping. I can go over this again to make sure that there is nothing amiss after the recent changes. > 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. The mapping is generally available in some form if you cannot get it from the page. In some cases I added a new parameter to functions to pass the mapping so that we do not have to use page->mapping. I can recheck that all is fine on that level. > 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? Ok. A trivial change. But a is shorter and made the functions more concise. - To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 01/19] Define functions for page cache handling
On Mon, Dec 03, 2007 at 02:10:20PM -0800, Andrew Morton wrote:
> On Fri, 30 Nov 2007 09:34:49 -0800
> Christoph Lameter <[EMAIL PROTECTED]> 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 functionRelated 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.
H. 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 [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 01/19] Define functions for page cache handling
On Fri, 30 Nov 2007 09:34:49 -0800
Christoph Lameter <[EMAIL PROTECTED]> 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 functionRelated 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 [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
[patch 01/19] Define functions for page cache handling
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 functionRelated 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.
Reviewed-by: Dave Chinner <[EMAIL PROTECTED]>
Signed-off-by: Christoph Lameter <[EMAIL PROTECTED]>
---
include/linux/pagemap.h | 53 +++-
1 file changed, 52 insertions(+), 1 deletion(-)
Index: mm/include/linux/pagemap.h
===
--- mm.orig/include/linux/pagemap.h 2007-11-29 12:27:53.878812858 -0800
+++ mm/include/linux/pagemap.h 2007-11-29 12:28:42.063283655 -0800
@@ -52,12 +52,61 @@ static inline void mapping_set_gfp_mask(
* space in smaller chunks for same flexibility).
*
* Or rather, it _will_ be done in larger chunks.
+ *
+ * The following constants can be used if a filesystem only supports a single
+ * page size.
*/
#define PAGE_CACHE_SHIFT PAGE_SHIFT
#define PAGE_CACHE_SIZEPAGE_SIZE
#define PAGE_CACHE_MASKPAGE_MASK
#define PAGE_CACHE_ALIGN(addr) (((addr)+PAGE_CACHE_SIZE-1)&PAGE_CACHE_MASK)
+/*
+ * 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);
+}
+
+/*
+ * Index of the page starting on or after the given position.
+ */
+static inline pgoff_t page_cache_next(struct address_space *a,
+ loff_t pos)
+{
+ return page_cache_index(a, pos + page_cache_size(a) - 1);
+}
+
+static inline loff_t page_cache_pos(struct address_space *a,
+ pgoff_t index, unsigned long offset)
+{
+ return ((loff_t)index << page_cache_shift(a)) + offset;
+}
+
#define page_cache_get(page) get_page(page)
#define page_cache_release(page) put_page(page)
void release_pages(struct page **pages, int nr, int cold);
@@ -142,10 +191,12 @@ extern void __remove_from_page_cache(str
/*
* Return byte-offset into filesystem object for page.
+ * Note: Assumes a mapping of order 0.
+ * Use page_cache_pos to allow larger pages.
*/
static inline loff_t page_offset(struct page *page)
{
- return ((loff_t)page->index) << PAGE_CACHE_SHIFT;
+ return page->index << PAGE_CACHE_SHIFT;
}
static inline pgoff_t linear_page_index(struct vm_area_struct *vma,
--
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 01/19] Define functions for page cache handling
On Thu, 29 Nov 2007, Fengguang Wu wrote:
> On Wed, Nov 28, 2007 at 05:10:53PM -0800, Christoph Lameter wrote:
> > +static inline loff_t page_cache_mask(struct address_space *a)
> > +{
> > + return (loff_t)PAGE_MASK;
> > +}
>
> A tiny question: Why choose loff_t instead of 'unsigned long'?
>
> It's not obvious because page_cache_mask() is not referenced in this
> patchset at all ;-)
Ok Then lets drop page_cache_mask completely.
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 01/19] Define functions for page cache handling
On Wed, Nov 28, 2007 at 05:10:53PM -0800, Christoph Lameter wrote:
> +static inline loff_t page_cache_mask(struct address_space *a)
> +{
> + return (loff_t)PAGE_MASK;
> +}
A tiny question: Why choose loff_t instead of 'unsigned long'?
It's not obvious because page_cache_mask() is not referenced in this
patchset at all ;-)
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 01/19] Define functions for page cache handling
On Thu, 29 Nov 2007, David Chinner wrote:
> On Wed, Nov 28, 2007 at 05:10:53PM -0800, Christoph Lameter wrote:
> > +/*
> > + * Index of the page starting on or after the given position.
> > + */
> > +static inline pgoff_t page_cache_next(struct address_space *a,
> > + loff_t pos)
> > +{
> > + return page_cache_index(a, pos + page_cache_size(a) - 1);
>
> return page_cache_index(a, pos + page_cache_mask(a));
Na that is confusing. We really want to go to one byte before the end of
the page.
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 01/19] Define functions for page cache handling
On Wed, Nov 28, 2007 at 05:10:53PM -0800, Christoph Lameter wrote:
> +/*
> + * Index of the page starting on or after the given position.
> + */
> +static inline pgoff_t page_cache_next(struct address_space *a,
> + loff_t pos)
> +{
> + return page_cache_index(a, pos + page_cache_size(a) - 1);
return page_cache_index(a, pos + page_cache_mask(a));
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 [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
[patch 01/19] Define functions for page cache handling
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 larger buffers.
Information reachable via the mapping may then determine page size.
New functionRelated 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.
Signed-off-by: Christoph Lameter <[EMAIL PROTECTED]>
---
include/linux/pagemap.h | 54 +++
1 files changed, 54 insertions(+), 0 deletions(-)
diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h
index 8a83537..836e9dd 100644
--- a/include/linux/pagemap.h
+++ b/include/linux/pagemap.h
@@ -52,12 +52,66 @@ static inline void mapping_set_gfp_mask(struct
address_space *m, gfp_t mask)
* space in smaller chunks for same flexibility).
*
* Or rather, it _will_ be done in larger chunks.
+ *
+ * The following constants can be used if a filesystem only supports a single
+ * page size.
*/
#define PAGE_CACHE_SHIFT PAGE_SHIFT
#define PAGE_CACHE_SIZEPAGE_SIZE
#define PAGE_CACHE_MASKPAGE_MASK
#define PAGE_CACHE_ALIGN(addr) (((addr)+PAGE_CACHE_SIZE-1)&PAGE_CACHE_MASK)
+/*
+ * Functions that are currently setup for a fixed PAGE_SIZEd. The use of
+ * these will allow a variable page size pagecache 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 loff_t page_cache_mask(struct address_space *a)
+{
+ return (loff_t)PAGE_MASK;
+}
+
+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);
+}
+
+/*
+ * Index of the page starting on or after the given position.
+ */
+static inline pgoff_t page_cache_next(struct address_space *a,
+ loff_t pos)
+{
+ return page_cache_index(a, pos + page_cache_size(a) - 1);
+}
+
+static inline loff_t page_cache_pos(struct address_space *a,
+ pgoff_t index, unsigned long offset)
+{
+ return ((loff_t)index << page_cache_shift(a)) + offset;
+}
+
#define page_cache_get(page) get_page(page)
#define page_cache_release(page) put_page(page)
void release_pages(struct page **pages, int nr, int cold);
--
1.5.2.5
--
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
