From mboxrd@z Thu Jan 1 00:00:00 1970 From: Christoph Hellwig Subject: Re: [RFC] v9fs: readpage support Date: Fri, 18 Nov 2005 19:20:45 +0000 Message-ID: <20051118192045.GA5579@infradead.org> References: <20051118002438.0BC995A807B@localhost.localdomain> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: linux-fsdevel@vger.kernel.org, v9fs-developer@lists.sourceforge.net Return-path: Received: from pentafluge.infradead.org ([213.146.154.40]:4582 "EHLO pentafluge.infradead.org") by vger.kernel.org with ESMTP id S1750870AbVKRTUr (ORCPT ); Fri, 18 Nov 2005 14:20:47 -0500 To: Eric Van Hensbergen Content-Disposition: inline In-Reply-To: <20051118002438.0BC995A807B@localhost.localdomain> Sender: linux-fsdevel-owner@vger.kernel.org List-Id: linux-fsdevel.vger.kernel.org > +/* > + * v9fs_vfs_readpage - read an entire page in from 9P > + * > + * @file: file being read > + * @page: structure to page > + * > + */ this isn't actually a valid kerneldoc comment, they start with /** e.g. /** * v9fs_vfs_readpage - read an entire page in from 9P * @file: file being read * @page: structure to page */ not that I think kerneldoc comments on implementations driver / fs API methods are very useful in general.. > + loff_t offset = ((loff_t) page->index) << PAGE_CACHE_SHIFT; please use the page_offset() helper. > + struct v9fs_fcall *fcall = NULL; > + int fid = v9f->fid; > + int total = 0; > + int result = 0; > + > + page_cache_get(page); ->readpage is synchronous, so you shouldn't need to grab a reference. > + if (PageError(page)) > + ClearPageError(page); this doesn't make a lot of sense. > + retval = 0; > + > + Error: labels always at positions zero or one please. I'd call this out instead of erorr aswell as it's used for the successfull completion case aswell. > diff --git a/fs/9p/vfs_file.c b/fs/9p/vfs_file.c > index 89c849d..f332f65 100644 > --- a/fs/9p/vfs_file.c > +++ b/fs/9p/vfs_file.c > @@ -306,6 +306,9 @@ v9fs_file_write(struct file *filp, const > total += result; > } while (count); > > + if ((inode->i_mapping) && (inode->i_mapping->nrpages)) > + invalidate_inode_pages2(inode->i_mapping); > + how could inode->i_mapping be zero here? Also the parenthes around those values are superflous.