| Message ID | 20230105121512.21484-4-jlayton@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | nfsd: filecache cleanups and optimizations | expand |
On Thu, 2023-01-05 at 07:15 -0500, Jeff Layton wrote: > An error from break_lease is non-fatal, so we needn't destroy the > nfsd_file in that case. Just put the reference like we normally would > and return the error. > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > --- > fs/nfsd/filecache.c | 29 +++++++++++++++-------------- > 1 file changed, 15 insertions(+), 14 deletions(-) > > diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c > index a67b22579c6e..f0ca9501edb2 100644 > --- a/fs/nfsd/filecache.c > +++ b/fs/nfsd/filecache.c > @@ -1113,7 +1113,7 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp, > nf = nfsd_file_alloc(&key, may_flags); > if (!nf) { > status = nfserr_jukebox; > - goto out_status; > + goto out; > } > > ret = rhashtable_lookup_insert_key(&nfsd_file_rhash_tbl, > @@ -1122,13 +1122,11 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp, > if (likely(ret == 0)) > goto open_file; > > - nfsd_file_slab_free(&nf->nf_rcu); > - nf = NULL; > if (ret == -EEXIST) > goto retry; > trace_nfsd_file_insert_err(rqstp, key.inode, may_flags, ret); > status = nfserr_jukebox; > - goto out_status; > + goto construction_err; > > wait_for_construction: > wait_on_bit(&nf->nf_flags, NFSD_FILE_PENDING, TASK_UNINTERRUPTIBLE); > @@ -1138,28 +1136,24 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp, > trace_nfsd_file_cons_err(rqstp, key.inode, may_flags, nf); > if (!open_retry) { > status = nfserr_jukebox; > - goto out; > + goto construction_err; > } > open_retry = false; > - if (refcount_dec_and_test(&nf->nf_ref)) > - nfsd_file_free(nf); > goto retry; > } > - > this_cpu_inc(nfsd_file_cache_hits); > > status = nfserrno(nfsd_open_break_lease(file_inode(nf->nf_file), may_flags)); > + if (status != nfs_ok) { > + nfsd_file_put(nf); > + nf = NULL; > + } > + > out: > if (status == nfs_ok) { > this_cpu_inc(nfsd_file_acquisitions); > *pnf = nf; > - } else { > - if (refcount_dec_and_test(&nf->nf_ref)) > - nfsd_file_free(nf); > - nf = NULL; > } > - > -out_status: > put_cred(key.cred); > trace_nfsd_file_acquire(rqstp, key.inode, may_flags, nf, status); > return status; > @@ -1189,6 +1183,13 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp, > if (status != nfs_ok) > nfsd_file_unhash(nf); > clear_and_wake_up_bit(NFSD_FILE_PENDING, &nf->nf_flags); > + if (status == nfs_ok) > + goto out; > + > +construction_err: > + if (refcount_dec_and_test(&nf->nf_ref)) > + nfsd_file_free(nf); > + nf = NULL; > goto out; > } > Chuck, ping? You never responded to this patch and I don't see it in your tree. Any thoughts?
> On Jan 18, 2023, at 12:54 PM, Jeff Layton <jlayton@kernel.org> wrote: > > On Thu, 2023-01-05 at 07:15 -0500, Jeff Layton wrote: >> An error from break_lease is non-fatal, so we needn't destroy the >> nfsd_file in that case. Just put the reference like we normally would >> and return the error. >> >> Signed-off-by: Jeff Layton <jlayton@kernel.org> >> --- >> fs/nfsd/filecache.c | 29 +++++++++++++++-------------- >> 1 file changed, 15 insertions(+), 14 deletions(-) >> >> diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c >> index a67b22579c6e..f0ca9501edb2 100644 >> --- a/fs/nfsd/filecache.c >> +++ b/fs/nfsd/filecache.c >> @@ -1113,7 +1113,7 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp, >> nf = nfsd_file_alloc(&key, may_flags); >> if (!nf) { >> status = nfserr_jukebox; >> - goto out_status; >> + goto out; >> } >> >> ret = rhashtable_lookup_insert_key(&nfsd_file_rhash_tbl, >> @@ -1122,13 +1122,11 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp, >> if (likely(ret == 0)) >> goto open_file; >> >> - nfsd_file_slab_free(&nf->nf_rcu); >> - nf = NULL; >> if (ret == -EEXIST) >> goto retry; >> trace_nfsd_file_insert_err(rqstp, key.inode, may_flags, ret); >> status = nfserr_jukebox; >> - goto out_status; >> + goto construction_err; >> >> wait_for_construction: >> wait_on_bit(&nf->nf_flags, NFSD_FILE_PENDING, TASK_UNINTERRUPTIBLE); >> @@ -1138,28 +1136,24 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp, >> trace_nfsd_file_cons_err(rqstp, key.inode, may_flags, nf); >> if (!open_retry) { >> status = nfserr_jukebox; >> - goto out; >> + goto construction_err; >> } >> open_retry = false; >> - if (refcount_dec_and_test(&nf->nf_ref)) >> - nfsd_file_free(nf); >> goto retry; >> } >> - >> this_cpu_inc(nfsd_file_cache_hits); >> >> status = nfserrno(nfsd_open_break_lease(file_inode(nf->nf_file), may_flags)); >> + if (status != nfs_ok) { >> + nfsd_file_put(nf); >> + nf = NULL; >> + } >> + >> out: >> if (status == nfs_ok) { >> this_cpu_inc(nfsd_file_acquisitions); >> *pnf = nf; >> - } else { >> - if (refcount_dec_and_test(&nf->nf_ref)) >> - nfsd_file_free(nf); >> - nf = NULL; >> } >> - >> -out_status: >> put_cred(key.cred); >> trace_nfsd_file_acquire(rqstp, key.inode, may_flags, nf, status); >> return status; >> @@ -1189,6 +1183,13 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp, >> if (status != nfs_ok) >> nfsd_file_unhash(nf); >> clear_and_wake_up_bit(NFSD_FILE_PENDING, &nf->nf_flags); >> + if (status == nfs_ok) >> + goto out; >> + >> +construction_err: >> + if (refcount_dec_and_test(&nf->nf_ref)) >> + nfsd_file_free(nf); >> + nf = NULL; >> goto out; >> } >> > > Chuck, ping? > > You never responded to this patch and I don't see it in your tree. Any > thoughts? It's been in nfsd-next for a bit, but I had to drop a bunch of patches from nfsd-next until yesterday's -rc PR was applied upstream because they depended on one of Dai's crasher fixes. That's been sorted, and I've restored these to nfsd-next. -- Chuck Lever
diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c index a67b22579c6e..f0ca9501edb2 100644 --- a/fs/nfsd/filecache.c +++ b/fs/nfsd/filecache.c @@ -1113,7 +1113,7 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp, nf = nfsd_file_alloc(&key, may_flags); if (!nf) { status = nfserr_jukebox; - goto out_status; + goto out; } ret = rhashtable_lookup_insert_key(&nfsd_file_rhash_tbl, @@ -1122,13 +1122,11 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp, if (likely(ret == 0)) goto open_file; - nfsd_file_slab_free(&nf->nf_rcu); - nf = NULL; if (ret == -EEXIST) goto retry; trace_nfsd_file_insert_err(rqstp, key.inode, may_flags, ret); status = nfserr_jukebox; - goto out_status; + goto construction_err; wait_for_construction: wait_on_bit(&nf->nf_flags, NFSD_FILE_PENDING, TASK_UNINTERRUPTIBLE); @@ -1138,28 +1136,24 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp, trace_nfsd_file_cons_err(rqstp, key.inode, may_flags, nf); if (!open_retry) { status = nfserr_jukebox; - goto out; + goto construction_err; } open_retry = false; - if (refcount_dec_and_test(&nf->nf_ref)) - nfsd_file_free(nf); goto retry; } - this_cpu_inc(nfsd_file_cache_hits); status = nfserrno(nfsd_open_break_lease(file_inode(nf->nf_file), may_flags)); + if (status != nfs_ok) { + nfsd_file_put(nf); + nf = NULL; + } + out: if (status == nfs_ok) { this_cpu_inc(nfsd_file_acquisitions); *pnf = nf; - } else { - if (refcount_dec_and_test(&nf->nf_ref)) - nfsd_file_free(nf); - nf = NULL; } - -out_status: put_cred(key.cred); trace_nfsd_file_acquire(rqstp, key.inode, may_flags, nf, status); return status; @@ -1189,6 +1183,13 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp, if (status != nfs_ok) nfsd_file_unhash(nf); clear_and_wake_up_bit(NFSD_FILE_PENDING, &nf->nf_flags); + if (status == nfs_ok) + goto out; + +construction_err: + if (refcount_dec_and_test(&nf->nf_ref)) + nfsd_file_free(nf); + nf = NULL; goto out; }
An error from break_lease is non-fatal, so we needn't destroy the nfsd_file in that case. Just put the reference like we normally would and return the error. Signed-off-by: Jeff Layton <jlayton@kernel.org> --- fs/nfsd/filecache.c | 29 +++++++++++++++-------------- 1 file changed, 15 insertions(+), 14 deletions(-)