From mboxrd@z Thu Jan 1 00:00:00 1970 From: Nick Piggin Subject: Re: [rfc][patch 3/5] afs: new aops Date: Tue, 13 Nov 2007 01:15:48 +0100 Message-ID: <20071113001548.GA30650@wotan.suse.de> References: <20071112071448.GE22953@wotan.suse.de> <20071112071245.GB22953@wotan.suse.de> <6161.1194881354@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]:33743 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756340AbXKMAPy (ORCPT ); Mon, 12 Nov 2007 19:15:54 -0500 Content-Disposition: inline In-Reply-To: <6161.1194881354@redhat.com> Sender: linux-fsdevel-owner@vger.kernel.org List-Id: linux-fsdevel.vger.kernel.org On Mon, Nov 12, 2007 at 03:29:14PM +0000, David Howells wrote: > Nick Piggin wrote: > > > - ASSERTCMP(start + len, <=, PAGE_SIZE); > > + ASSERTCMP(len, <=, PAGE_CACHE_SIZE); > > Do you guarantee this will work if PAGE_CACHE_SIZE != PAGE_SIZE? If not, you > can't make this particular change. PAGE_CACHE_SIZE should be used to address the pagecache. > Do we ever intend to have PAGE_CACHE_SIZE != PAGE_SIZE? If not, then surely > the former is redundant and should scrapped to avoid confusion? Maybe. > > + i_size = i_size_read(&vnode->vfs_inode); > > + if (pos + len > i_size) > > + eof = i_size; > > + else > > + eof = PAGE_CACHE_SIZE; > > + > > + ret = afs_vnode_fetch_data(vnode, key, 0, eof, page); > > That can't be right, surely. Either 'eof' is the size of the file or it's the > length of the data to be read. It can't be both. The first case needs eof > masking off. Also, 'eof' isn't a good choice of name. 'len' would be better > were it not already taken:-/ Yeah, just missed the mask. > I notice you removed the stuff that clears holes in the page to be written. Is > this is now done by the caller? It is supposed to bring the page uptodate first. So, no need to clear AFAIKS? > I notice also that you use afs_fill_page() in place of afs_prepare_page() to > prepare a page. You can't do this if the region to be filled currently lies > beyond the server's idea of EOF. > > I'll try and get a look at fixing this patch tomorrow. No rush, it won't get into 2.6.24 obviously. But that would be nice, thanks.