[PATCH v3] nfsd: rework hashtable handling in nfsd_do_file_acquire

[PATCH v3] nfsd: rework hashtable handling in nfsd_do_file_acquire

[Jeff Layton]

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.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 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

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

[Chuck Lever III]

> On Oct 3, 2022, at 7:34 AM, Jeff Layton <jlayton@kernel.org> 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 rest of the patch looks good.


> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
> 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

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

[Jeff Layton]

On Mon, 2022-10-03 at 13:11 +0000, Chuck Lever III wrote:
> 
> > On Oct 3, 2022, at 7:34 AM, Jeff Layton <jlayton@kernel.org> 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.
> 

IDK. I'm not sold that this goto spaghetti has any value as an
optimization. I can put it back in if you like, but I think eliminating
one of the goto targets here is a good thing.

> The rest of the patch looks good.
> 
> 
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> > 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
> 
> 
> 

-- 
Jeff Layton <jlayton@kernel.org>

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

[NeilBrown]

On Tue, 04 Oct 2022, Chuck Lever III wrote:
> 
> > On Oct 3, 2022, at 7:34 AM, Jeff Layton <jlayton@kernel.org> 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@kernel.org>
> > ---
> > 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
> 
> 
> 
> 

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

[Chuck Lever III]

> On Oct 3, 2022, at 6:07 PM, NeilBrown <neilb@suse.de> wrote:
> 
> On Tue, 04 Oct 2022, Chuck Lever III wrote:
>> 
>>> On Oct 3, 2022, at 7:34 AM, Jeff Layton <jlayton@kernel.org> 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.

It's an instruction cache footprint issue.

A CPU populates its instruction cache by reading instructions from
memory in bulk (some multiple of the size of the cacheline). I
would like to keep the instructions involved with very rare cases
(like this one) out-of-line so they do not clutter the CPU's
instruction cache.

Unfortunately the compiler on my system has decided to place this
snippet of code right before the out_status: label, which defeats
my intention.


--
Chuck Lever

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

[NeilBrown]

On Wed, 05 Oct 2022, Chuck Lever III wrote:
> 
> > On Oct 3, 2022, at 6:07 PM, NeilBrown <neilb@suse.de> wrote:
> > 
> > On Tue, 04 Oct 2022, Chuck Lever III wrote:
> >> 
> >>> On Oct 3, 2022, at 7:34 AM, Jeff Layton <jlayton@kernel.org> 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.
> 
> It's an instruction cache footprint issue.
> 
> A CPU populates its instruction cache by reading instructions from
> memory in bulk (some multiple of the size of the cacheline). I
> would like to keep the instructions involved with very rare cases
> (like this one) out-of-line so they do not clutter the CPU's
> instruction cache.
> 
> Unfortunately the compiler on my system has decided to place this
> snippet of code right before the out_status: label, which defeats
> my intention.

Don't you hate that!!!!

On the unpatched code, if I put a "likely" annotation on the assumed
common case:

	if (likely(!nf)) {
		nf = new;
		goto open_file;
	}

then the open_file code is placed immediately after the
rhashtable_lookup_get_insert_key().

This supports my suggestion that likely/unlikely annotations are better
at controlling code placement than source-code placement.

I've thought for a while that those annotations should be
optimise_for() and optimise_against() or similar.  That is what is being
requested.

NeilBrown

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

[NeilBrown]

On Mon, 03 Oct 2022, Jeff Layton 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.
> 
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>  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) {

You need a "-" sign in there.

> +		retry = false;

I can see no justification for either testing or setting "retry" here.
In the original code, limiting retries is about the "open_file" branch
hitting an error.  That is totally different from a race that might
delete the file immediately after a lookup_insert failed.  At most it
should be a different retry counter.  If you are really concerned about
retries here, then we should stay with
rhashtable_lookup_get_insert_key().

Thanks,
NeilBrown


> +		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
> 
>