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

[Trond Myklebust <Trond.Myklebust@netapp.com>]

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.

Trond