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 05:24:20 +0100 Message-ID: <20071114042420.GF557@wotan.suse.de> References: <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> 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]:42636 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754266AbXKNEYY (ORCPT ); Tue, 13 Nov 2007 23:24:24 -0500 Content-Disposition: inline In-Reply-To: <18637.1194951385@redhat.com> Sender: linux-fsdevel-owner@vger.kernel.org List-Id: linux-fsdevel.vger.kernel.org On Tue, Nov 13, 2007 at 10:56:25AM +0000, David Howells wrote: > Nick Piggin 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.