Re: [PATCH 10/12] NFS: Simplify nfs_wb_page()

From: Jeff Layton <jlayton@redhat.com>
To: Trond Myklebust <Trond.Myklebust@netapp.com>
Cc: Christoph Hellwig <hch@infradead.org>,
	"J. R. Okajima" <hooanon05@yahoo.co.jp>,
	linux-nfs@vger.kernel.org, Wu Fengguang <fengguang.wu@intel.com>,
	Peter Zijlstra <peterz@infradead.org>, Jan Kara <jack@suse.cz>,
	Steve Rago <sar@nec-labs.com>, Jens Axboe <jens.axboe@oracle.com>,
	Arjan van de Ven <arjan@infradead.org>,
	Ingo Molnar <mingo@elte.hu>,
	linux-fsdevel@vger.kernel.org, Christoph Hellwig <hch@lst.de>,
	Al Viro <viro@ZenIV.linux.org.uk>
Subject: Re: [PATCH 10/12] NFS: Simplify nfs_wb_page()
Date: Wed, 17 Mar 2010 13:52:05 -0400	[thread overview]
Message-ID: <20100317135205.58731824@tlielax.poochiereds.net> (raw)
In-Reply-To: <1268846819.8335.3.camel@localhost.localdomain>

On Wed, 17 Mar 2010 13:26:59 -0400
Trond Myklebust <Trond.Myklebust@netapp.com> wrote:

> On Wed, 2010-03-17 at 12:49 -0400, Christoph Hellwig wrote: 
> > On Thu, Mar 11, 2010 at 09:26:22AM -0500, Trond Myklebust wrote:
> > > @@ -112,12 +112,10 @@ void nfs_unlock_request(struct nfs_page *req)
> > >   */
> > >  int nfs_set_page_tag_locked(struct nfs_page *req)
> > >  {
> > > -	struct nfs_inode *nfsi = NFS_I(req->wb_context->path.dentry->d_inode);
> > > -
> > >  	if (!nfs_lock_request_dontget(req))
> > >  		return 0;
> > >  	if (req->wb_page != NULL)
> > > -		radix_tree_tag_set(&nfsi->nfs_page_tree, req->wb_index, NFS_PAGE_TAG_LOCKED);
> > > +		radix_tree_tag_set(&NFS_I(req->wb_context->path.dentry->d_inode)->nfs_page_tree, req->wb_index, NFS_PAGE_TAG_LOCKED);
> > >  	return 1;
> > >  }
> > >  
> > 
> > This hunk is not only entirely unrealted to the fix, but also osbfucates
> > the code.
> > 
> > > @@ -126,10 +124,10 @@ int nfs_set_page_tag_locked(struct nfs_page *req)
> > >   */
> > >  void nfs_clear_page_tag_locked(struct nfs_page *req)
> > >  {
> > > -	struct inode *inode = req->wb_context->path.dentry->d_inode;
> > > -	struct nfs_inode *nfsi = NFS_I(inode);
> > > -
> > >  	if (req->wb_page != NULL) {
> > > +		struct inode *inode = req->wb_context->path.dentry->d_inode;
> > > +		struct nfs_inode *nfsi = NFS_I(inode);
> > > +
> > >  		spin_lock(&inode->i_lock);
> > >  		radix_tree_tag_clear(&nfsi->nfs_page_tree, req->wb_index, NFS_PAGE_TAG_LOCKED);
> > >  		nfs_unlock_request(req);
> > 
> > Another one unrelated to the fix.
> > 
> 
> No. They are needed because we don't want to dereference req->wb_context
> after it (and req->wb_page) have been cleared. Without these 2 hunks,
> the resulting kernel Oopses.
> 

It seems like that just tightens up the race window without actually
closing it.

Just because wb_page was non-NULL when you checked it gives no
guarantee that wb_context won't be NULL when you go to dereference it,
right?

-- 
Jeff Layton <jlayton@redhat.com>