public inbox for linux-nfs@vger.kernel.org
 help / color / mirror / Atom feed
From: NeilBrown <neilb@suse.de>
To: Chuck Lever <chuck.lever@oracle.com>, Jeff Layton <jlayton@kernel.org>
Cc: linux-nfs@vger.kernel.org,
	Olga Kornievskaia <okorniev@redhat.com>,
	Dai Ngo <Dai.Ngo@oracle.com>, Tom Talpey <tom@talpey.com>,
	Dave Chinner <david@fromorbit.com>
Subject: [PATCH 1/7] nfsd: filecache: remove race handling.
Date: Mon, 27 Jan 2025 12:20:32 +1100	[thread overview]
Message-ID: <20250127012257.1803314-2-neilb@suse.de> (raw)
In-Reply-To: <20250127012257.1803314-1-neilb@suse.de>

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);
-- 
2.47.1


  reply	other threads:[~2025-01-27  1:23 UTC|newest]

Thread overview: 23+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-01-27  1:20 [PATCH 0/7] nfsd: filecache: change garbage collection lists NeilBrown
2025-01-27  1:20 ` NeilBrown [this message]
2025-01-27 13:42   ` [PATCH 1/7] nfsd: filecache: remove race handling Jeff Layton
2025-01-27  1:20 ` [PATCH 2/7] nfsd: filecache: use nfsd_file_dispose_list() in nfsd_file_close_inode_sync() NeilBrown
2025-01-27  1:20 ` [PATCH 3/7] nfsd: filecache: move globals nfsd_file_lru and nfsd_file_shrinker to be per-net NeilBrown
2025-01-27  1:20 ` [PATCH 4/7] nfsd: filecache: change garbage collection list management NeilBrown
2025-01-27 14:15   ` Jeff Layton
2025-01-27  1:20 ` [PATCH 5/7] nfsd: filecache: document the arbitrary limit on file-disposes-per-loop NeilBrown
2025-01-27 14:40   ` Jeff Layton
2025-01-27  1:20 ` [PATCH 6/7] nfsd: filecache: change garbage collection to a timer NeilBrown
2025-01-27 14:39   ` Jeff Layton
2025-01-27  1:20 ` [PATCH 7/7] nfsd: filecache: give disposal lock a unique class name NeilBrown
2025-01-27 14:29   ` Chuck Lever
2025-01-27 14:40   ` Jeff Layton
2025-01-28  6:37 ` [PATCH 0/7] nfsd: filecache: change garbage collection lists Dave Chinner
2025-01-28 14:27   ` Chuck Lever
2025-01-28 16:05     ` Chuck Lever
2025-01-29 21:34   ` NeilBrown
2025-02-06  2:21     ` Dave Chinner
2025-02-06  3:04       ` NeilBrown
2025-02-06 14:35         ` Chuck Lever
2025-02-05 23:04 ` NeilBrown
2025-02-06  3:02   ` Chuck Lever

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20250127012257.1803314-2-neilb@suse.de \
    --to=neilb@suse.de \
    --cc=Dai.Ngo@oracle.com \
    --cc=chuck.lever@oracle.com \
    --cc=david@fromorbit.com \
    --cc=jlayton@kernel.org \
    --cc=linux-nfs@vger.kernel.org \
    --cc=okorniev@redhat.com \
    --cc=tom@talpey.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox