| Message ID | 20221003113436.24161-1-jlayton@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] nfsd: rework hashtable handling in nfsd_do_file_acquire | expand |
> 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
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 > > >
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 > >
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 > > > >
> 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
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
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; } /**
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(-)