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

[David Howells <dhowells@redhat.com>]

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
single pages.  It has no knowledge of compound pages and does not handle page
arrays.  This may be a flaw in my code, but it's there nonetheless.  The
assertion is a guard against that.  *That* was the point of my statement.
Perhaps I should've said 'my code' rather than 'the code'.

If Christoph has a patch to deal with that, it's either not upstream yet or it
hasn't altered AFS.

> > 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.

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.

David