Re: [PATCH v3] nfsd: rework hashtable handling in nfsd_do_file_acquire

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

 



On Tue, 04 Oct 2022, Chuck Lever III wrote:
> 
> > On Oct 3, 2022, at 7:34 AM, Jeff Layton <jlayton@xxxxxxxxxx> wrote:
> > 
> > nfsd_file is RCU-freed, so we need to hold the rcu_read_lock long enough
> > to get a reference after finding it in the hash. Take the
> > rcu_read_lock() and call rhashtable_lookup directly.
> > 
> > Switch to using rhashtable_lookup_insert_key as well, and use the usual
> > retry mechanism if we hit an -EEXIST. Eliminiate the insert_err goto
> > target as well.
> 
> The insert_err goto is there to remove a very rare case from
> the hot path. I'd kinda like to keep that feature of this code.

????
The fast path in the new code looks quite clean - what concerns you?
Maybe a "likely()" annotation can be used to encourage the compiler to
optimise for the non-error path so the error-handling gets moved
out-of-line (assuming it isn't already), but don't think the new code
needs that goto.

NeilBrown


> 
> The rest of the patch looks good.
> 
> 
> > Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
> > ---
> > fs/nfsd/filecache.c | 46 ++++++++++++++++++++-------------------------
> > 1 file changed, 20 insertions(+), 26 deletions(-)
> > 
> > diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
> > index be152e3e3a80..63955f3353ed 100644
> > --- a/fs/nfsd/filecache.c
> > +++ b/fs/nfsd/filecache.c
> > @@ -1043,9 +1043,10 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp,
> > 		.need	= may_flags & NFSD_FILE_MAY_MASK,
> > 		.net	= SVC_NET(rqstp),
> > 	};
> > -	struct nfsd_file *nf, *new;
> > +	struct nfsd_file *nf;
> > 	bool retry = true;
> > 	__be32 status;
> > +	int ret;
> > 
> > 	status = fh_verify(rqstp, fhp, S_IFREG,
> > 				may_flags|NFSD_MAY_OWNER_OVERRIDE);
> > @@ -1055,35 +1056,35 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp,
> > 	key.cred = get_current_cred();
> > 
> > retry:
> > -	/* Avoid allocation if the item is already in cache */
> > -	nf = rhashtable_lookup_fast(&nfsd_file_rhash_tbl, &key,
> > -				    nfsd_file_rhash_params);
> > +	rcu_read_lock();
> > +	nf = rhashtable_lookup(&nfsd_file_rhash_tbl, &key,
> > +			       nfsd_file_rhash_params);
> > 	if (nf)
> > 		nf = nfsd_file_get(nf);
> > +	rcu_read_unlock();
> > 	if (nf)
> > 		goto wait_for_construction;
> > 
> > -	new = nfsd_file_alloc(&key, may_flags);
> > -	if (!new) {
> > +	nf = nfsd_file_alloc(&key, may_flags);
> > +	if (!nf) {
> > 		status = nfserr_jukebox;
> > 		goto out_status;
> > 	}
> > 
> > -	nf = rhashtable_lookup_get_insert_key(&nfsd_file_rhash_tbl,
> > -					      &key, &new->nf_rhash,
> > -					      nfsd_file_rhash_params);
> > -	if (!nf) {
> > -		nf = new;
> > -		goto open_file;
> > -	}
> > -	if (IS_ERR(nf))
> > -		goto insert_err;
> > -	nf = nfsd_file_get(nf);
> > -	if (nf == NULL) {
> > -		nf = new;
> > +	ret = rhashtable_lookup_insert_key(&nfsd_file_rhash_tbl,
> > +					   &key, &nf->nf_rhash,
> > +					   nfsd_file_rhash_params);
> > +	if (ret == 0)
> > 		goto open_file;
> > +
> > +	nfsd_file_slab_free(&nf->nf_rcu);
> > +	if (retry && ret == EEXIST) {
> > +		retry = false;
> > +		goto retry;
> > 	}
> > -	nfsd_file_slab_free(&new->nf_rcu);
> > +	trace_nfsd_file_insert_err(rqstp, key.inode, may_flags, ret);
> > +	status = nfserr_jukebox;
> > +	goto out_status;
> > 
> > wait_for_construction:
> > 	wait_on_bit(&nf->nf_flags, NFSD_FILE_PENDING, TASK_UNINTERRUPTIBLE);
> > @@ -1143,13 +1144,6 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp,
> > 	smp_mb__after_atomic();
> > 	wake_up_bit(&nf->nf_flags, NFSD_FILE_PENDING);
> > 	goto out;
> > -
> > -insert_err:
> > -	nfsd_file_slab_free(&new->nf_rcu);
> > -	trace_nfsd_file_insert_err(rqstp, key.inode, may_flags, PTR_ERR(nf));
> > -	nf = NULL;
> > -	status = nfserr_jukebox;
> > -	goto out_status;
> > }
> > 
> > /**
> > -- 
> > 2.37.3
> > 
> 
> --
> Chuck Lever
> 
> 
> 
> 




[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