From mboxrd@z Thu Jan 1 00:00:00 1970 From: David Howells Subject: Re: [rfc][patch 3/5] afs: new aops Date: Mon, 12 Nov 2007 15:29:14 +0000 Message-ID: <6161.1194881354@redhat.com> References: <20071112071448.GE22953@wotan.suse.de> <20071112071245.GB22953@wotan.suse.de> Cc: dhowells@redhat.com, Andrew Morton , linux-fsdevel@vger.kernel.org, mhalcrow@us.ibm.com, phillip@hellewell.homeip.net, sfrench@samba.org To: Nick Piggin Return-path: Received: from mx1.redhat.com ([66.187.233.31]:52815 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751114AbXKLP30 (ORCPT ); Mon, 12 Nov 2007 10:29:26 -0500 In-Reply-To: <20071112071448.GE22953@wotan.suse.de> Sender: linux-fsdevel-owner@vger.kernel.org List-Id: linux-fsdevel.vger.kernel.org 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. 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? > + 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:-/ I notice you removed the stuff that clears holes in the page to be written. Is this is now done by the caller? 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. David