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

[Nick Piggin <npiggin@suse.de>]

On Tue, Nov 13, 2007 at 10:56:25AM +0000, David Howells wrote:
> Nick Piggin <npiggin@suse.de> wrote:
> 
> > It takes a pagecache page, yes. If you follow convention, you use
> > PAGE_CACHE_SIZE for that guy. You don't have to allow PAGE_CACHE_SIZE !=
> > PAGE_SIZE, and if all the rest of your code is in units of PAGE_SIZE, then
> > obviously my changing of just the one unit is even more confusing than
> > the current arrangement ;)
> 
> 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.


> If I should allow for an array of pages then
> the lower functions (specifically afs_deliver_fs_fetch_data()) need to change,
> and until that time occurs, the assertion *must* remain as it is now.  It
> defends the lower functions against being asked to do something they weren't
> designed to do.
> 
> 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.