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@xxxxxxx>
> ---
> 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@xxxxxxxxxx>
