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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, 2010-03-17 at 13:52 -0400, Jeff Layton wrote: 
> On Wed, 17 Mar 2010 13:26:59 -0400
> Trond Myklebust <Trond.Myklebust@xxxxxxxxxx> 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?
> 

The above 2 functions are the ones that lock and unlock the request.
Once locked by means of the call to nfs_lock_request_dontget(req), no
other threads can change the contents of wb_page and wb_context.

IOW: yes, there is definitely such a guarantee...

Trond
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Index of Archives]     [Linux Filesystem Development]     [Linux USB Development]     [Linux Media Development]     [Video for Linux]     [Linux NILFS]     [Linux Audio Users]     [Yosemite Info]     [Linux SCSI]

  Powered by Linux