| Message ID | 20250127012257.1803314-2-neilb@suse.de (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Chuck Lever |
| Headers | show |
| Series | nfsd: filecache: change garbage collection lists | expand |
On Mon, 2025-01-27 at 12:20 +1100, NeilBrown wrote: > The race that this code tries to protect against is not interesting. > The code is problematic as we access the "nf" after we have given our > reference to the lru system. While that take 2+ seconds to free things > it is still poor form. > > The only interesting race I can find would be with > nfsd_file_close_inode_sync(); > This is the only place that really doesn't want the file to stay on the > LRU when unhashed (which is the direct consequence of the race). > > However for the race to happen, some other thread must own a reference > to a file and be putting in while nfsd_file_close_inode_sync() is trying > to close all files for an inode. If this is possible, that other thread > could simply call nfsd_file_put() a little bit later and the result > would be the same: not all files are closed when > nfsd_file_close_inode_sync() completes. > > If this was really a problem, we would need to wait in close_inode_sync > for the other references to be dropped. We probably don't want to do > that. > > So it is best to simply remove this code. > > Signed-off-by: NeilBrown <neilb@suse.de> > --- > fs/nfsd/filecache.c | 16 +++------------- > 1 file changed, 3 insertions(+), 13 deletions(-) > > diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c > index e342b2e76882..f5a92ac3f16f 100644 > --- a/fs/nfsd/filecache.c > +++ b/fs/nfsd/filecache.c > @@ -371,20 +371,10 @@ nfsd_file_put(struct nfsd_file *nf) > > /* Try to add it to the LRU. If that fails, decrement. */ > if (nfsd_file_lru_add(nf)) { > - /* If it's still hashed, we're done */ > - if (test_bit(NFSD_FILE_HASHED, &nf->nf_flags)) { > - nfsd_file_schedule_laundrette(); > - return; > - } > - > - /* > - * We're racing with unhashing, so try to remove it from > - * the LRU. If removal fails, then someone else already > - * has our reference. > - */ > - if (!nfsd_file_lru_remove(nf)) > - return; > + nfsd_file_schedule_laundrette(); > + return; > } > + > } > if (refcount_dec_and_test(&nf->nf_ref)) > nfsd_file_free(nf); Reviewed-by: Jeff Layton <jlayton@kernel.org>
diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c index e342b2e76882..f5a92ac3f16f 100644 --- a/fs/nfsd/filecache.c +++ b/fs/nfsd/filecache.c @@ -371,20 +371,10 @@ nfsd_file_put(struct nfsd_file *nf) /* Try to add it to the LRU. If that fails, decrement. */ if (nfsd_file_lru_add(nf)) { - /* If it's still hashed, we're done */ - if (test_bit(NFSD_FILE_HASHED, &nf->nf_flags)) { - nfsd_file_schedule_laundrette(); - return; - } - - /* - * We're racing with unhashing, so try to remove it from - * the LRU. If removal fails, then someone else already - * has our reference. - */ - if (!nfsd_file_lru_remove(nf)) - return; + nfsd_file_schedule_laundrette(); + return; } + } if (refcount_dec_and_test(&nf->nf_ref)) nfsd_file_free(nf);
The race that this code tries to protect against is not interesting. The code is problematic as we access the "nf" after we have given our reference to the lru system. While that take 2+ seconds to free things it is still poor form. The only interesting race I can find would be with nfsd_file_close_inode_sync(); This is the only place that really doesn't want the file to stay on the LRU when unhashed (which is the direct consequence of the race). However for the race to happen, some other thread must own a reference to a file and be putting in while nfsd_file_close_inode_sync() is trying to close all files for an inode. If this is possible, that other thread could simply call nfsd_file_put() a little bit later and the result would be the same: not all files are closed when nfsd_file_close_inode_sync() completes. If this was really a problem, we would need to wait in close_inode_sync for the other references to be dropped. We probably don't want to do that. So it is best to simply remove this code. Signed-off-by: NeilBrown <neilb@suse.de> --- fs/nfsd/filecache.c | 16 +++------------- 1 file changed, 3 insertions(+), 13 deletions(-)