Re: [PATCH v3 002/114] nfsd: Protect addition to the file_hashtbl

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

 



On Mon, 30 Jun 2014 16:28:19 -0400
"J. Bruce Fields" <bfields@xxxxxxxxxxxx> wrote:

> On Mon, Jun 30, 2014 at 11:48:31AM -0400, Jeff Layton wrote:
> > From: Trond Myklebust <trond.myklebust@xxxxxxxxxxxxxxx>
> > 
> > Ensure that we only can have a single struct nfs4_file per inode
> > in the file_hashtbl and make addition atomic with respect to lookup.
> 
> This changelog gives the impression it's a bugfix, and it isn't.  I
> understand it's tedious in this series, but really I think at a minimum
> a changelog should always make it clear whether a patch is a bugfix or
> not.  Applying with changelog rewritten to:
> 
> 	Current code depends on the client_mutex to guarantee a single
> 	struct nfs4_file per inode in the file_hashtbl and make addition
> 	atomic with respect to lookup.  Rely instead on the state_Lock,
> 	to make it easier to stop taking the client_mutex here later.
> 	...
> 
> --b.
> 

Thanks and sorry, it's definitely correct that it's not a bugfix wrt
the existing code. I thought I had updated that patch to spell it out,
but I guess I missed that one...

> > 
> > To prevent an i_lock/state_lock inversion, change nfsd4_init_file to
> > use ihold instead if igrab. That's also more efficient anyway as we
> > definitely hold a reference to the inode at that point.
> > 
> > Signed-off-by: Trond Myklebust <trond.myklebust@xxxxxxxxxxxxxxx>
> > Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxxxxxxx>
> > Reviewed-by: Christoph Hellwig <hch@xxxxxx>
> > ---
> >  fs/nfsd/nfs4state.c | 49 +++++++++++++++++++++++++++++++++++++------------
> >  1 file changed, 37 insertions(+), 12 deletions(-)
> > 
> > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > index c473bd6d52c8..29788fd0da24 100644
> > --- a/fs/nfsd/nfs4state.c
> > +++ b/fs/nfsd/nfs4state.c
> > @@ -2611,17 +2611,18 @@ static void nfsd4_init_file(struct nfs4_file *fp, struct inode *ino)
> >  {
> >  	unsigned int hashval = file_hashval(ino);
> >  
> > +	lockdep_assert_held(&state_lock);
> > +
> >  	atomic_set(&fp->fi_ref, 1);
> >  	INIT_LIST_HEAD(&fp->fi_stateids);
> >  	INIT_LIST_HEAD(&fp->fi_delegations);
> > -	fp->fi_inode = igrab(ino);
> > +	ihold(ino);
> > +	fp->fi_inode = ino;
> >  	fp->fi_had_conflict = false;
> >  	fp->fi_lease = NULL;
> >  	memset(fp->fi_fds, 0, sizeof(fp->fi_fds));
> >  	memset(fp->fi_access, 0, sizeof(fp->fi_access));
> > -	spin_lock(&state_lock);
> >  	hlist_add_head(&fp->fi_hash, &file_hashtbl[hashval]);
> > -	spin_unlock(&state_lock);
> >  }
> >  
> >  void
> > @@ -2787,23 +2788,49 @@ find_openstateowner_str(unsigned int hashval, struct nfsd4_open *open,
> >  
> >  /* search file_hashtbl[] for file */
> >  static struct nfs4_file *
> > -find_file(struct inode *ino)
> > +find_file_locked(struct inode *ino)
> >  {
> >  	unsigned int hashval = file_hashval(ino);
> >  	struct nfs4_file *fp;
> >  
> > -	spin_lock(&state_lock);
> > +	lockdep_assert_held(&state_lock);
> > +
> >  	hlist_for_each_entry(fp, &file_hashtbl[hashval], fi_hash) {
> >  		if (fp->fi_inode == ino) {
> >  			get_nfs4_file(fp);
> > -			spin_unlock(&state_lock);
> >  			return fp;
> >  		}
> >  	}
> > -	spin_unlock(&state_lock);
> >  	return NULL;
> >  }
> >  
> > +static struct nfs4_file *
> > +find_file(struct inode *ino)
> > +{
> > +	struct nfs4_file *fp;
> > +
> > +	spin_lock(&state_lock);
> > +	fp = find_file_locked(ino);
> > +	spin_unlock(&state_lock);
> > +	return fp;
> > +}
> > +
> > +static struct nfs4_file *
> > +find_or_add_file(struct inode *ino, struct nfs4_file *new)
> > +{
> > +	struct nfs4_file *fp;
> > +
> > +	spin_lock(&state_lock);
> > +	fp = find_file_locked(ino);
> > +	if (fp == NULL) {
> > +		nfsd4_init_file(new, ino);
> > +		fp = new;
> > +	}
> > +	spin_unlock(&state_lock);
> > +
> > +	return fp;
> > +}
> > +
> >  /*
> >   * Called to check deny when READ with all zero stateid or
> >   * WRITE with all zero or all one stateid
> > @@ -3325,21 +3352,19 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nf
> >  	 * and check for delegations in the process of being recalled.
> >  	 * If not found, create the nfs4_file struct
> >  	 */
> > -	fp = find_file(ino);
> > -	if (fp) {
> > +	fp = find_or_add_file(ino, open->op_file);
> > +	if (fp != open->op_file) {
> >  		if ((status = nfs4_check_open(fp, open, &stp)))
> >  			goto out;
> >  		status = nfs4_check_deleg(cl, open, &dp);
> >  		if (status)
> >  			goto out;
> >  	} else {
> > +		open->op_file = NULL;
> >  		status = nfserr_bad_stateid;
> >  		if (nfsd4_is_deleg_cur(open))
> >  			goto out;
> >  		status = nfserr_jukebox;
> > -		fp = open->op_file;
> > -		open->op_file = NULL;
> > -		nfsd4_init_file(fp, ino);
> >  	}
> >  
> >  	/*
> > -- 
> > 1.9.3
> > 


-- 
Jeff Layton <jlayton@xxxxxxxxxxxxxxx>
--
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