Re: [rfc][patch 3/5] afs: new aops

[Nick Piggin <npiggin@suse.de>]

On Wed, Nov 14, 2007 at 12:18:43PM +0000, David Howells wrote:
> Nick Piggin <npiggin@suse.de> wrote:
> 
> > > The problem is that the code called assumes that the struct page *
> > > argument points to a single page, not an array of pages as would
> > > presumably be the case if PAGE_CACHE_SIZE > PAGE_SIZE.
> > 
> > Incorrect. Christoph's patch for example does this by using compound pages.
> > Now I personally don't like the patch or see the point in PAGE_CACHE_SIZE /
> > PAGE_SIZE distinction, but I'm just telling you what the convention is. There
> > is no point you arguing against it, that's simply how it is.
> 
> No!  You are wrong.  I wrote the AFS code.  I *know* it can only deal with

No I'm talking about core code. In core code, the PAGE_CACHE_SIZE is
for page cache struct pages. Single struct pages (not page arrays).
Take a look at generic mapping read or something.

There is nothing to deal with page arrays there either, but that's simply
the convention.


> > > So: you may not change the assertion unless you also fix the lower
> > > functions.
> > 
> > I won't change the assertion, because you haven't been following said
> > convention, so just changing it in one place is stupider than not changing
> > it at all, but not for the reason cited.
> 
> The convention is not precisely clear.  Just grep for PAGE_CACHE_SIZE in
> Documentation/.  It's only mentioned twice, and in neither case does it give
> any information about what PAGE_CACHE_SIZE is, what it represents, or where it
> applies.  Therefore it's an ill-defined concept.
> 
> If you look in Documentation/filesystems/vfs.txt, you'll see that it almost
> always talks about 'pages'.  It only mentions 'pagecache pages' once - in the
> description of write_begin(), but it's not clear whether that means anything.

Documentation is the opposite of convention ;) Look in mm/.

 
> However, I've now noted that I need to fix my code, so just keep the assertion
> for now and I'll fix my code to handle multipage blocks.

I'm not saying you need to do that. Leave it at PAGE_SIZE, really it
doesn't matter that much at present. This has just blown out of proportion.