From mboxrd@z Thu Jan 1 00:00:00 1970 From: Nick Piggin Subject: Re: [rfc][patch 3/5] afs: new aops Date: Wed, 14 Nov 2007 16:18:25 +0100 Message-ID: <20071114151825.GA20973@wotan.suse.de> References: <20071114042420.GF557@wotan.suse.de> <20071113004459.GE30650@wotan.suse.de> <20071113001548.GA30650@wotan.suse.de> <20071112071448.GE22953@wotan.suse.de> <20071112071245.GB22953@wotan.suse.de> <6161.1194881354@redhat.com> <17445.1194913805@redhat.com> <18637.1194951385@redhat.com> <30440.1195042723@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Andrew Morton , linux-fsdevel@vger.kernel.org, mhalcrow@us.ibm.com, phillip@hellewell.homeip.net, sfrench@samba.org To: David Howells Return-path: Received: from ns2.suse.de ([195.135.220.15]:54749 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753538AbXKNPSb (ORCPT ); Wed, 14 Nov 2007 10:18:31 -0500 Content-Disposition: inline In-Reply-To: <30440.1195042723@redhat.com> Sender: linux-fsdevel-owner@vger.kernel.org List-Id: linux-fsdevel.vger.kernel.org On Wed, Nov 14, 2007 at 12:18:43PM +0000, David Howells wrote: > Nick Piggin 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.