diff mbox series

[v4] nfsd: rework hashtable handling in nfsd_do_file_acquire

Message ID 20221004194110.120599-1-jlayton@kernel.org (mailing list archive)
State New, archived
Headers show
Series [v4] nfsd: rework hashtable handling in nfsd_do_file_acquire | expand

Commit Message

Jeff Layton Oct. 4, 2022, 7:41 p.m. UTC 
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. Rename the "retry" bool to
open_retry, and eliminiate the insert_err goto target.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/nfsd/filecache.c | 52 +++++++++++++++++++--------------------------
 1 file changed, 22 insertions(+), 30 deletions(-)

v4: fix sign on -EEXIST comparison
    don't clear the retry flag on an insertion race

Comments

Chuck Lever Oct. 4, 2022, 7:59 p.m. UTC | #1 
> On Oct 4, 2022, at 3:41 PM, 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. Rename the "retry" bool to
> open_retry, and eliminiate the insert_err goto target.

This needs to explain "why", not "what"... I can look at the diff
and see exactly what it's doing. But OK, we've been painting this
shed for far too long. I'll apply this one for testing.


> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
> fs/nfsd/filecache.c | 52 +++++++++++++++++++--------------------------
> 1 file changed, 22 insertions(+), 30 deletions(-)
> 
> v4: fix sign on -EEXIST comparison
>    don't clear the retry flag on an insertion race
> 
> diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
> index be152e3e3a80..5399d8b44c45 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;
> -	bool retry = true;
> +	struct nfsd_file *nf;
> +	bool open_retry = true;
> 	__be32 status;
> +	int ret;
> 
> 	status = fh_verify(rqstp, fhp, S_IFREG,
> 				may_flags|NFSD_MAY_OWNER_OVERRIDE);
> @@ -1055,35 +1056,33 @@ 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 (likely(ret == 0))
> 		goto open_file;
> -	}
> -	nfsd_file_slab_free(&new->nf_rcu);
> +
> +	nfsd_file_slab_free(&nf->nf_rcu);
> +	if (ret == -EEXIST)
> +		goto retry;
> +	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);
> @@ -1091,11 +1090,11 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp,
> 	/* Did construction of this file fail? */
> 	if (!test_bit(NFSD_FILE_HASHED, &nf->nf_flags)) {
> 		trace_nfsd_file_cons_err(rqstp, key.inode, may_flags, nf);
> -		if (!retry) {
> +		if (!open_retry) {
> 			status = nfserr_jukebox;
> 			goto out;
> 		}
> -		retry = false;
> +		open_retry = false;
> 		nfsd_file_put_noref(nf);
> 		goto retry;
> 	}
> @@ -1143,13 +1142,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
diff mbox series

Patch

diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
index be152e3e3a80..5399d8b44c45 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;
-	bool retry = true;
+	struct nfsd_file *nf;
+	bool open_retry = true;
 	__be32 status;
+	int ret;
 
 	status = fh_verify(rqstp, fhp, S_IFREG,
 				may_flags|NFSD_MAY_OWNER_OVERRIDE);
@@ -1055,35 +1056,33 @@  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 (likely(ret == 0))
 		goto open_file;
-	}
-	nfsd_file_slab_free(&new->nf_rcu);
+
+	nfsd_file_slab_free(&nf->nf_rcu);
+	if (ret == -EEXIST)
+		goto retry;
+	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);
@@ -1091,11 +1090,11 @@  nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	/* Did construction of this file fail? */
 	if (!test_bit(NFSD_FILE_HASHED, &nf->nf_flags)) {
 		trace_nfsd_file_cons_err(rqstp, key.inode, may_flags, nf);
-		if (!retry) {
+		if (!open_retry) {
 			status = nfserr_jukebox;
 			goto out;
 		}
-		retry = false;
+		open_retry = false;
 		nfsd_file_put_noref(nf);
 		goto retry;
 	}
@@ -1143,13 +1142,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;
 }
 
 /**