Should PAGE_CACHE_SIZE be discarded?

Should PAGE_CACHE_SIZE be discarded?

[David Howells]

Are we ever going to have PAGE_CACHE_SIZE != PAGE_SIZE?  If not, why not
discard PAGE_CACHE_SIZE as it's then redundant.

PAGE_CACHE_SIZE is also an ill-defined concept.  Just grep in Documentation/
and it only comes up in a couple of places, neither particularly informative.

If there's a possibility that it will be used, then someone who knows how it's
supposed to work needs to sit down and document what it is, what it represents,
where it applies, how it interacts with PG_compound and how the page flags
distribute over a page cache slot.

One further thing to consider: actually making PAGE_CACHE_SIZE > PAGE_SIZE
work will be an interesting problem, as I'm certain most filesystems will
break horribly without a lot of work (ramfs might be only exception).

David

Re: Should PAGE_CACHE_SIZE be discarded?

[Nick Piggin]

On Wed, Nov 14, 2007 at 01:56:53PM +0000, David Howells wrote:
> 
> Are we ever going to have PAGE_CACHE_SIZE != PAGE_SIZE?  If not, why not
> discard PAGE_CACHE_SIZE as it's then redundant.
> 

Christoph Lameter has patches exactly to make PAGE_CACHE_SIZE larger than
PAGE_SIZE, and they seem to work without much effort. I happen to hate the
patches ;) but that doesn't change the fact that PAGE_CACHE_SIZE is
relatively useful and it is not at all an ill-defined concept.


> PAGE_CACHE_SIZE is also an ill-defined concept.  Just grep in Documentation/
> and it only comes up in a couple of places, neither particularly informative.
> 
> If there's a possibility that it will be used, then someone who knows how it's
> supposed to work needs to sit down and document what it is, what it represents,
> where it applies, how it interacts with PG_compound and how the page flags
> distribute over a page cache slot.

No, this was an *example*. It has nothing to do with PG_compound upsream.
That was just an example. Basically, anything that goes in the page cache
is in units of PAGE_CACHE_SIZE, and nothing else. For filesystems it
should be pretty easy...

 
> One further thing to consider: actually making PAGE_CACHE_SIZE > PAGE_SIZE
> work will be an interesting problem, as I'm certain most filesystems will
> break horribly without a lot of work (ramfs might be only exception).

I think most filesystems actually don't have much problem with it. mm
code has bigger problems, eg. due to ptes <-> pagecache no longer being
equal size.... but why would filesystems care?

Re: Should PAGE_CACHE_SIZE be discarded?

[David Howells]

Nick Piggin <npiggin@suse.de> wrote:

> Christoph Lameter has patches exactly to make PAGE_CACHE_SIZE larger than
> PAGE_SIZE, and they seem to work without much effort. I happen to hate the
> patches ;) but that doesn't change the fact that PAGE_CACHE_SIZE is
> relatively useful and it is not at all an ill-defined concept.

Where, please? mm kernels?

> Basically, anything that goes in the page cache is in units of
> PAGE_CACHE_SIZE, and nothing else. For filesystems it should be pretty
> easy...

That depends on what the coverage of struct page is.  I don't actually know
whether this is PAGE_SIZE or PAGE_CACHE_SIZE; I assumed it to be the former,
but from what you've said, I'm not actually sure.

David

Re: Should PAGE_CACHE_SIZE be discarded?

[Nick Piggin]

On Wed, Nov 14, 2007 at 03:59:39PM +0000, David Howells wrote:
> Nick Piggin <npiggin@suse.de> wrote:
> 
> > Christoph Lameter has patches exactly to make PAGE_CACHE_SIZE larger than
> > PAGE_SIZE, and they seem to work without much effort. I happen to hate the
> > patches ;) but that doesn't change the fact that PAGE_CACHE_SIZE is
> > relatively useful and it is not at all an ill-defined concept.
> 
> Where, please? mm kernels?

Floating around. I'm not saying it will even get upstream. It's just
an example.

 
> > Basically, anything that goes in the page cache is in units of
> > PAGE_CACHE_SIZE, and nothing else. For filesystems it should be pretty
> > easy...
> 
> That depends on what the coverage of struct page is.  I don't actually know
> whether this is PAGE_SIZE or PAGE_CACHE_SIZE; I assumed it to be the former,
> but from what you've said, I'm not actually sure.

It can be pretty well any power of 2 from PAGE_SIZE upwards, with
compound pages. None of the filesystems should really care at all.
It's not even a new concept, hugetlbfs uses HPAGE_SIZE...

Re: Should PAGE_CACHE_SIZE be discarded?

[David Howells]

Nick Piggin <npiggin@suse.de> wrote:

> It can be pretty well any power of 2 from PAGE_SIZE upwards, with
> compound pages. None of the filesystems should really care at all.
> It's not even a new concept, hugetlbfs uses HPAGE_SIZE...

Ummm... The filesystem has to care.  If the VFS/VM says 'fill this page' you
do need to know how big the page is or whether it's even actually several
pages.

David

Re: Should PAGE_CACHE_SIZE be discarded?

[Benny Halevy]

On Nov. 15, 2007, 14:05 +0200, David Howells <dhowells@redhat.com> wrote:
> Nick Piggin <npiggin@suse.de> wrote:
> 
>> It can be pretty well any power of 2 from PAGE_SIZE upwards, with
>> compound pages. None of the filesystems should really care at all.
>> It's not even a new concept, hugetlbfs uses HPAGE_SIZE...
> 
> Ummm... The filesystem has to care.  If the VFS/VM says 'fill this page' you
> do need to know how big the page is or whether it's even actually several
> pages.

I think that what Nick was trying to say is that PAGE_CACHE_SIZE should always
be used properly as the size of the memory struct Page covers (while PAGE_SIZE
is the hardware page size and the constraint is that PAGE_CACHE_SIZE == (PAGE_SIZE << k)
for some k >= 0).  If everybody does that then "None of the filesystems should really
care at all". That said, it doesn't seem like the current usage in fs/ and drivers/
is consistent with this convention.

> 
> David
> -

Re: Should PAGE_CACHE_SIZE be discarded?

[David Howells]

Benny Halevy <bhalevy@panasas.com> wrote:

> I think that what Nick was trying to say is that PAGE_CACHE_SIZE should
> always be used properly as the size of the memory struct Page covers (while
> PAGE_SIZE is the hardware page size and the constraint is that
> PAGE_CACHE_SIZE == (PAGE_SIZE << k) for some k >= 0).  If everybody does
> that then "None of the filesystems should really care at all". That said, it
> doesn't seem like the current usage in fs/ and drivers/ is consistent with
> this convention.

Indeed.  One thing you have to consider is kmap().  I would expect it to
present an area of PAGE_SIZE for access.  However, if the filesystem gets an
area of PAGE_CACHE_SIZE to fill, then I would have to do multiple kmap() calls
in the process of filling that 'pagecache page' in AFS.

Furthermore, if a page struct covers a PAGE_CACHE_SIZE chunk of memory, then I
suspect the page allocator is also wrong, as it I believe it deals with
PAGE_SIZE chunks of memory, assuming a struct page for each.

David

Re: Should PAGE_CACHE_SIZE be discarded?

[Nick Piggin]

On Thu, Nov 15, 2007 at 02:46:46PM +0000, David Howells wrote:
> Benny Halevy <bhalevy@panasas.com> wrote:
> 
> > I think that what Nick was trying to say is that PAGE_CACHE_SIZE should
> > always be used properly as the size of the memory struct Page covers (while
> > PAGE_SIZE is the hardware page size and the constraint is that
> > PAGE_CACHE_SIZE == (PAGE_SIZE << k) for some k >= 0).  If everybody does
> > that then "None of the filesystems should really care at all". That said, it
> > doesn't seem like the current usage in fs/ and drivers/ is consistent with
> > this convention.
> 
> Indeed.  One thing you have to consider is kmap().  I would expect it to
> present an area of PAGE_SIZE for access.  However, if the filesystem gets an
> area of PAGE_CACHE_SIZE to fill, then I would have to do multiple kmap() calls
> in the process of filling that 'pagecache page' in AFS.
> 
> Furthermore, if a page struct covers a PAGE_CACHE_SIZE chunk of memory, then I
> suspect the page allocator is also wrong, as it I believe it deals with
> PAGE_SIZE chunks of memory, assuming a struct page for each.

That's because you're thinking about all these difficulties outside the
filesystem. Really, pagecache is in PAGE_CACHE_SIZE. Nobody said pagecache
> PAGE_SIZE will work by changing a single define, but filesystems are
about the least problematic here.

Re: Should PAGE_CACHE_SIZE be discarded?

[Jörn Engel]

On Wed, 14 November 2007 13:56:53 +0000, David Howells wrote:
> 
> Are we ever going to have PAGE_CACHE_SIZE != PAGE_SIZE?  If not, why not
> discard PAGE_CACHE_SIZE as it's then redundant.

I did some digging.  The code was merged between 28.4.1999 and
11.5.1999, as it first appears in 2.2.8 and 2.3.0:

/*
 * The page cache can done in larger chunks than
 * one page, because it allows for more efficient
 * throughput (it can then be mapped into user
 * space in smaller chunks for same flexibility).
 *
 * Or rather, it _will_ be done in larger chunks.
 */
#define PAGE_CACHE_SHIFT        PAGE_SHIFT
#define PAGE_CACHE_SIZE         PAGE_SIZE
#define PAGE_CACHE_MASK         PAGE_MASK

#define page_cache_alloc()      __get_free_page(GFP_USER)
#define page_cache_free(x)      free_page(x)
#define page_cache_release(x)   __free_page(x)

Looks like PAGE_CACHE_SIZE>PAGE_SIZE was planned before 2.3 opened, but
never went very far.  So judging by the last eight years of history,
PAGE_CACHE_SIZE is dead.

Completely discarding it might be a bad idea, as it serves as code
annotation.  Christoph Lameters "Large Blocksize Support" patches seem
to globally replace PAGE_CACHE_SIZE with a different macro.  Having
PAGE_CACHE_SIZE makes such an effort easier.  Dropping existing
annotation seems useless, when maintaining them is zero effort.

Large code audits for the PAGE_SIZE/PAGE_CACHE_SIZE distinction seem
completely useless, though.  For as long as the macro existed, it never
ever mattered.  My take is to keep it and let it bitrot until someone
actually cares.

Jörn

-- 
Joern's library part 6:
http://www.gzip.org/zlib/feldspar.html
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html