| Message ID | 171253979210.17212.5851835299179227478@noble.neil.brown.name (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | nfsd: optimise recalculate_deny_mode() for a common case | expand |
On Mon, 2024-04-08 at 11:29 +1000, NeilBrown wrote: > recalculate_deny_mode() takes time that is linear in the number of > stateids active on the file. > > When called from > release_openowner -> free_ol_stateid_reaplist ->nfs4_free_ol_stateid > -> release_all_access > > the number of times it is called is linear in the number of stateids. > The net result is that time taken by release_openowner is quadratic in > the number of stateids. > > When the nfsd server is shut down while there are many active stateids > this can result in a soft lockup. ("CPU stuck for 302s" seen in one case). > > In many cases all the states have the same deny modes and there is no > need to examine the entire list in recalculate_deny_mode(). In > particular, recalculate_deny_mode() will only reduce the deny mode, > never increase it. So if some prefix of the list causes the original > deny mode to be required, there is no need to examine the remainder of > the list. > > So we can improve recalculate_deny_mode() to usually run in constant > time, so release_openowner will typically be only linear in the number > of states. > > Signed-off-by: NeilBrown <neilb@suse.de> > --- > fs/nfsd/nfs4state.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c > index 1824a30e7dd4..a46f5230bc9b 100644 > --- a/fs/nfsd/nfs4state.c > +++ b/fs/nfsd/nfs4state.c > @@ -1409,11 +1409,16 @@ static void > recalculate_deny_mode(struct nfs4_file *fp) > { > struct nfs4_ol_stateid *stp; > + u32 old_deny; > > spin_lock(&fp->fi_lock); > + old_deny = fp->fi_share_deny; > fp->fi_share_deny = 0; > - list_for_each_entry(stp, &fp->fi_stateids, st_perfile) > + list_for_each_entry(stp, &fp->fi_stateids, st_perfile) { > fp->fi_share_deny |= bmap_to_share_mode(stp->st_deny_bmap); > + if (fp->fi_share_deny == old_deny) > + break; > + } > spin_unlock(&fp->fi_lock); > } > Clever! Reviewed-by: Jeff Layton <jlayton@kernel.org>
On Sun, Apr 07, 2024 at 09:29:52PM -0400, NeilBrown wrote: > > recalculate_deny_mode() takes time that is linear in the number of > stateids active on the file. > > When called from > release_openowner -> free_ol_stateid_reaplist ->nfs4_free_ol_stateid > -> release_all_access > > the number of times it is called is linear in the number of stateids. > The net result is that time taken by release_openowner is quadratic in > the number of stateids. > > When the nfsd server is shut down while there are many active stateids > this can result in a soft lockup. ("CPU stuck for 302s" seen in one case). > > In many cases all the states have the same deny modes and there is no > need to examine the entire list in recalculate_deny_mode(). In > particular, recalculate_deny_mode() will only reduce the deny mode, > never increase it. So if some prefix of the list causes the original > deny mode to be required, there is no need to examine the remainder of > the list. > > So we can improve recalculate_deny_mode() to usually run in constant > time, so release_openowner will typically be only linear in the number > of states. > > Signed-off-by: NeilBrown <neilb@suse.de> Applied to nfsd-next. Thanks! > --- > fs/nfsd/nfs4state.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c > index 1824a30e7dd4..a46f5230bc9b 100644 > --- a/fs/nfsd/nfs4state.c > +++ b/fs/nfsd/nfs4state.c > @@ -1409,11 +1409,16 @@ static void > recalculate_deny_mode(struct nfs4_file *fp) > { > struct nfs4_ol_stateid *stp; > + u32 old_deny; > > spin_lock(&fp->fi_lock); > + old_deny = fp->fi_share_deny; > fp->fi_share_deny = 0; > - list_for_each_entry(stp, &fp->fi_stateids, st_perfile) > + list_for_each_entry(stp, &fp->fi_stateids, st_perfile) { > fp->fi_share_deny |= bmap_to_share_mode(stp->st_deny_bmap); > + if (fp->fi_share_deny == old_deny) > + break; > + } > spin_unlock(&fp->fi_lock); > } > > -- > 2.44.0 >
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c index 1824a30e7dd4..a46f5230bc9b 100644 --- a/fs/nfsd/nfs4state.c +++ b/fs/nfsd/nfs4state.c @@ -1409,11 +1409,16 @@ static void recalculate_deny_mode(struct nfs4_file *fp) { struct nfs4_ol_stateid *stp; + u32 old_deny; spin_lock(&fp->fi_lock); + old_deny = fp->fi_share_deny; fp->fi_share_deny = 0; - list_for_each_entry(stp, &fp->fi_stateids, st_perfile) + list_for_each_entry(stp, &fp->fi_stateids, st_perfile) { fp->fi_share_deny |= bmap_to_share_mode(stp->st_deny_bmap); + if (fp->fi_share_deny == old_deny) + break; + } spin_unlock(&fp->fi_lock); }
recalculate_deny_mode() takes time that is linear in the number of stateids active on the file. When called from release_openowner -> free_ol_stateid_reaplist ->nfs4_free_ol_stateid -> release_all_access the number of times it is called is linear in the number of stateids. The net result is that time taken by release_openowner is quadratic in the number of stateids. When the nfsd server is shut down while there are many active stateids this can result in a soft lockup. ("CPU stuck for 302s" seen in one case). In many cases all the states have the same deny modes and there is no need to examine the entire list in recalculate_deny_mode(). In particular, recalculate_deny_mode() will only reduce the deny mode, never increase it. So if some prefix of the list causes the original deny mode to be required, there is no need to examine the remainder of the list. So we can improve recalculate_deny_mode() to usually run in constant time, so release_openowner will typically be only linear in the number of states. Signed-off-by: NeilBrown <neilb@suse.de> --- fs/nfsd/nfs4state.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)