[prev in list] [next in list] [prev in thread] [next in thread] 

List:       linux-nfsv4
Subject:    Re: [PATCH 06/17] nfsd: fix misplace fh_unlock() in nfsd_symlink()
From:       Neil Brown <neilb () suse ! de>
Date:       2006-06-27 4:59:26
Message-ID: 17568.47918.675311.141897 () cse ! unsw ! edu ! au
[Download RAW message or body]

On Tuesday June 27, bfields@fieldses.org wrote:
> From: David M. Richter <richterd@citi.umich.edu>
> 
> In the event that lookup_one_len() fails in nfsd_symlink(), fh_unlock() is
> skipped and locks are held overlong.
> 
> Patch was tested on 2.6.17-rc2 by causing lookup_one_len() to fail and
> verifying that fh_unlock() gets called appropriately.
> 
> Signed-off-by: David M. Richter <richterd@citi.umich.edu>
> Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
> ---
> 

Sorry, I cannot quite cope with this one...
The problem it is claiming to fix is that 'locks are held overlong'
yet fh_unlock is being moved down past fh_compose and dput, so in the
common case locks are now being held longer - possible a greater
increase in lock time than the decrease that was won for the error
case.

Now I agree that this code could be clearer and tidier so there is
definitely room for making changes, but let's make sure it is for the
right reasons.
You could just put an 'fh_unlock' in the 'out_nfserr' section, reason
being "it makes it more obvious that the lock is dropped", and then we
could probably take that whole section and put it at the one place
that 'out_nfserr' is 'goto'ed. 

I've taken the nfsd_link patch for now, but I'm dropping this
nfsd_symlink patch.

NeilBrown


>  fs/nfsd/vfs.c |    6 ++++--
>  1 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index 46eb652..0295cd1 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -1453,17 +1453,19 @@ nfsd_symlink(struct svc_rqst *rqstp, str
>  			err = nfsd_sync_dir(dentry);
>  	if (err)
>  		err = nfserrno(err);
> -	fh_unlock(fhp);
>  
>  	cerr = fh_compose(resfhp, fhp->fh_export, dnew, fhp);
>  	dput(dnew);
>  	if (err==0) err = cerr;
> +
> +out_unlock:
> +	fh_unlock(fhp);
>  out:
>  	return err;
>  
>  out_nfserr:
>  	err = nfserrno(err);
> -	goto out;
> +	goto out_unlock;
>  }
>  
>  /*
_______________________________________________
NFSv4 mailing list
NFSv4@linux-nfs.org
http://linux-nfs.org/cgi-bin/mailman/listinfo/nfsv4
[prev in list] [next in list] [prev in thread] [next in thread]