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 <[email protected]>
---
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
> On Oct 4, 2022, at 3:41 PM, Jeff Layton <[email protected]> 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 <[email protected]>
> ---
> 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