add a LRU for delegations

add a LRU for delegations

[Christoph Hellwig]

Hi all,

currently the NFS client is rather inefficient at managing delegations
not associated with an open file.  If the number of delegations is above
the watermark, the delegation for a free file is immediately returned,
even if delegations that were unused for much longer would be available.
Also the periodic freeing marks delegations as not referenced for return,
even if the file was open and thus force the return on close.

This series reworks the code to introduce an LRU and return the least
used delegations instead.

For a workload simulating repeated runs of a python program importing a
lot of modules, this leads to a 97% reduction of on-the-wire operations,
and ~40% speedup even for a fast local NFS server.  A reproducer script
is attached.

You'll want to make sure the dentry caching fix posted by Anna in reply
to the 6.19 NFS pull is included for testing, even if the patches apply
without it.


Diffstat:
 fs/nfs/callback_proc.c    |   13 -
 fs/nfs/client.c           |    3 
 fs/nfs/delegation.c       |  546 +++++++++++++++++++++++-----------------------
 fs/nfs/delegation.h       |    4 
 fs/nfs/nfs4proc.c         |   82 +++---
 fs/nfs/nfs4trace.h        |    2 
 fs/nfs/super.c            |   14 -
 include/linux/nfs_fs_sb.h |    8 
 8 files changed, 344 insertions(+), 328 deletions(-)

Reproducer:
#!/usr/bin/env bash
set -euo pipefail

if [ $# -ne 1 ]; then
    echo "Usage: $0 <NFS_MOUNT_PATH>"
    exit 1
fi

NFS_MOUNT="$1"
WARMUP_FILE_COUNT="8000"
RUNS=200
MODULE_COUNT=200
SAVEFILE="/tmp/nfsstat.bak"

echo "=== NFS delegation benchmark ==="
echo "NFS mount:          $NFS_MOUNT"
echo "Warmup file count:  $WARMUP_FILE_COUNT"
echo "Number of runs:     $RUNS"
echo "Module count:       $MODULE_COUNT"
echo


################################################################################
# Step 1: Create temporary directory on NFS
################################################################################
TEST_DIR=$(mktemp -d "$NFS_MOUNT/test_deleg_bench.XXXXXX")
MODULE_DIR="$TEST_DIR/pymods"
mkdir -p "$MODULE_DIR/delegtest"
MODDIR_INIT="$MODULE_DIR/delegtest/__init__.py"

cat > "$MODDIR_INIT" <<EOF
import os
import glob

file_paths = glob.glob(os.path.join(os.path.dirname(__file__), "*.py"))

__all__ = [
    os.path.basename(f)[:-3] 
    for f in file_paths 
    if os.path.isfile(f) and not f.endswith("__init__.py")
]
EOF

echo "[1] Creating $WARMUP_FILE_COUNT tiny files to accumulate delegations..."
mkdir -p "$TEST_DIR/fill"
for i in $(seq 1 "$WARMUP_FILE_COUNT"); do
    echo "f$i" > "$TEST_DIR/fill/file_$i"
done

echo "[1] Warmup delegation files created."

################################################################################
# Step 2: Create many tiny Python modules to exercise import workload
################################################################################
echo "[2] Creating $MODULE_COUNT dummy python modules..."
for i in $(seq 1 "$MODULE_COUNT"); do
    echo "x = $i" > "$MODULE_DIR/delegtest/mod$i.py"
done
#umount -o remount $NFS_MOUNT

# Python snippet:
# repeatedly import all modN modules; measure iterations completed
BENCH_SCRIPT="$TEST_DIR/bench.py"

cat > "$BENCH_SCRIPT" <<EOF
import sys

sys.path.insert(0, "$MODULE_DIR")

from delegtest import *
EOF

################################################################################
# Step 3: Pre-benchmark NFS client counters
################################################################################
echo "[3] Capturing baseline NFS client stats..."
sync $NFS_MOUNT
#mount -o remount $NFS_MOUNT
cp /proc/net/rpc/nfs $SAVEFILE

################################################################################
# Step 4: Run Python benchmark
################################################################################
echo "[4] Running Python import benchmark..."

time {
for i in $(seq 1 "$RUNS"); do
    python3 "$BENCH_SCRIPT"
done
}

################################################################################
# Step 5: Produce NFS client delta report
################################################################################
echo
echo "=== NFS client DELTA REPORT ==="
echo

nfsstat -c -S $SAVEFILE  -l
rm -f $SAVEFILE

echo
echo "Test directory:       $TEST_DIR"
echo
echo "=== Done ==="

[PATCH 01/24] NFS: remove __nfs_client_for_each_server

[Christoph Hellwig]

__nfs_client_for_each_server is only called by
nfs_client_for_each_server, so merge the two.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/nfs/super.c | 14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

diff --git a/fs/nfs/super.c b/fs/nfs/super.c
index 57d372db03b9..e74164d9c081 100644
--- a/fs/nfs/super.c
+++ b/fs/nfs/super.c
@@ -212,15 +212,14 @@ void nfs_sb_deactive(struct super_block *sb)
 }
 EXPORT_SYMBOL_GPL(nfs_sb_deactive);
 
-static int __nfs_list_for_each_server(struct list_head *head,
-		int (*fn)(struct nfs_server *, void *),
-		void *data)
+int nfs_client_for_each_server(struct nfs_client *clp,
+		int (*fn)(struct nfs_server *server, void *data), void *data)
 {
 	struct nfs_server *server, *last = NULL;
 	int ret = 0;
 
 	rcu_read_lock();
-	list_for_each_entry_rcu(server, head, client_link) {
+	list_for_each_entry_rcu(server, &clp->cl_superblocks, client_link) {
 		if (!(server->super && nfs_sb_active(server->super)))
 			continue;
 		rcu_read_unlock();
@@ -239,13 +238,6 @@ static int __nfs_list_for_each_server(struct list_head *head,
 		nfs_sb_deactive(last->super);
 	return ret;
 }
-
-int nfs_client_for_each_server(struct nfs_client *clp,
-		int (*fn)(struct nfs_server *, void *),
-		void *data)
-{
-	return __nfs_list_for_each_server(&clp->cl_superblocks, fn, data);
-}
 EXPORT_SYMBOL_GPL(nfs_client_for_each_server);
 
 /*
-- 
2.47.3

[PATCH 02/24] NFS: remove nfs_client_mark_return_unused_delegation_types

[Christoph Hellwig]

nfs_client_mark_return_unused_delegation_types is only called by
nfs_expire_unused_delegation_types, so merge the two.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/nfs/delegation.c | 17 +++--------------
 1 file changed, 3 insertions(+), 14 deletions(-)

diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c
index 2248e3ad089a..5648f1f0943b 100644
--- a/fs/nfs/delegation.c
+++ b/fs/nfs/delegation.c
@@ -1001,8 +1001,7 @@ static void nfs_mark_return_unused_delegation_types(struct nfs_server *server,
 	}
 }
 
-static void nfs_client_mark_return_unused_delegation_types(struct nfs_client *clp,
-							fmode_t flags)
+void nfs_expire_unused_delegation_types(struct nfs_client *clp, fmode_t flags)
 {
 	struct nfs_server *server;
 
@@ -1010,6 +1009,8 @@ static void nfs_client_mark_return_unused_delegation_types(struct nfs_client *cl
 	list_for_each_entry_rcu(server, &clp->cl_superblocks, client_link)
 		nfs_mark_return_unused_delegation_types(server, flags);
 	rcu_read_unlock();
+
+	nfs_delegation_run_state_manager(clp);
 }
 
 static void nfs_revoke_delegation(struct inode *inode,
@@ -1106,18 +1107,6 @@ void nfs_remove_bad_delegation(struct inode *inode,
 }
 EXPORT_SYMBOL_GPL(nfs_remove_bad_delegation);
 
-/**
- * nfs_expire_unused_delegation_types
- * @clp: client to process
- * @flags: delegation types to expire
- *
- */
-void nfs_expire_unused_delegation_types(struct nfs_client *clp, fmode_t flags)
-{
-	nfs_client_mark_return_unused_delegation_types(clp, flags);
-	nfs_delegation_run_state_manager(clp);
-}
-
 static void nfs_mark_return_unreferenced_delegations(struct nfs_server *server)
 {
 	struct nfs_delegation *delegation;
-- 
2.47.3

[PATCH 03/24] NFS: remove nfs_client_mark_return_all_delegations

[Christoph Hellwig]

Fold nfs_client_mark_return_all_delegations into
nfs_expire_all_delegations, which is the only caller.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/nfs/delegation.c | 18 +++++++-----------
 1 file changed, 7 insertions(+), 11 deletions(-)

diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c
index 5648f1f0943b..2ef8fe01ef4a 100644
--- a/fs/nfs/delegation.c
+++ b/fs/nfs/delegation.c
@@ -938,16 +938,6 @@ static bool nfs_server_mark_return_all_delegations(struct nfs_server *server)
 	return ret;
 }
 
-static void nfs_client_mark_return_all_delegations(struct nfs_client *clp)
-{
-	struct nfs_server *server;
-
-	rcu_read_lock();
-	list_for_each_entry_rcu(server, &clp->cl_superblocks, client_link)
-		nfs_server_mark_return_all_delegations(server);
-	rcu_read_unlock();
-}
-
 static void nfs_delegation_run_state_manager(struct nfs_client *clp)
 {
 	if (test_bit(NFS4CLNT_DELEGRETURN, &clp->cl_state))
@@ -961,7 +951,13 @@ static void nfs_delegation_run_state_manager(struct nfs_client *clp)
  */
 void nfs_expire_all_delegations(struct nfs_client *clp)
 {
-	nfs_client_mark_return_all_delegations(clp);
+	struct nfs_server *server;
+
+	rcu_read_lock();
+	list_for_each_entry_rcu(server, &clp->cl_superblocks, client_link)
+		nfs_server_mark_return_all_delegations(server);
+	rcu_read_unlock();
+
 	nfs_delegation_run_state_manager(clp);
 }
 
-- 
2.47.3

[PATCH 04/24] NFS: remove the NULL inode check in nfs4_inode_return_delegation_on_close

[Christoph Hellwig]

The only caller dereferences a field in the inode just before calling
nfs4_inode_return_delegation_on_close.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/nfs/delegation.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c
index 2ef8fe01ef4a..b44e39a50499 100644
--- a/fs/nfs/delegation.c
+++ b/fs/nfs/delegation.c
@@ -855,8 +855,6 @@ void nfs4_inode_return_delegation_on_close(struct inode *inode)
 	struct nfs_delegation *delegation;
 	struct nfs_delegation *ret = NULL;
 
-	if (!inode)
-		return;
 	rcu_read_lock();
 	delegation = nfs4_get_valid_delegation(inode);
 	if (!delegation)
-- 
2.47.3

[PATCH 05/24] NFS: remove nfs_inode_detach_delegation

[Christoph Hellwig]

Fold it into the only caller.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/nfs/delegation.c | 37 +++++++++++++++----------------------
 1 file changed, 15 insertions(+), 22 deletions(-)

diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c
index b44e39a50499..cf01ebd3b3c5 100644
--- a/fs/nfs/delegation.c
+++ b/fs/nfs/delegation.c
@@ -402,21 +402,6 @@ static struct nfs_delegation *nfs_detach_delegation(struct nfs_inode *nfsi,
 	return delegation;
 }
 
-static struct nfs_delegation *
-nfs_inode_detach_delegation(struct inode *inode)
-{
-	struct nfs_inode *nfsi = NFS_I(inode);
-	struct nfs_server *server = NFS_SERVER(inode);
-	struct nfs_delegation *delegation;
-
-	rcu_read_lock();
-	delegation = rcu_dereference(nfsi->delegation);
-	if (delegation != NULL)
-		delegation = nfs_detach_delegation(nfsi, delegation, server);
-	rcu_read_unlock();
-	return delegation;
-}
-
 static void
 nfs_update_delegation_cred(struct nfs_delegation *delegation,
 		const struct cred *cred)
@@ -769,15 +754,23 @@ int nfs_client_return_marked_delegations(struct nfs_client *clp)
  */
 void nfs_inode_evict_delegation(struct inode *inode)
 {
+	struct nfs_inode *nfsi = NFS_I(inode);
+	struct nfs_server *server = NFS_SERVER(inode);
 	struct nfs_delegation *delegation;
 
-	delegation = nfs_inode_detach_delegation(inode);
-	if (delegation != NULL) {
-		set_bit(NFS_DELEGATION_RETURNING, &delegation->flags);
-		set_bit(NFS_DELEGATION_INODE_FREEING, &delegation->flags);
-		nfs_do_return_delegation(inode, delegation, 1);
-		nfs_free_delegation(NFS_SERVER(inode), delegation);
-	}
+	rcu_read_lock();
+	delegation = rcu_dereference(nfsi->delegation);
+	if (delegation)
+		delegation = nfs_detach_delegation(nfsi, delegation, server);
+	rcu_read_unlock();
+
+	if (!delegation)
+		return;
+
+	set_bit(NFS_DELEGATION_RETURNING, &delegation->flags);
+	set_bit(NFS_DELEGATION_INODE_FREEING, &delegation->flags);
+	nfs_do_return_delegation(inode, delegation, 1);
+	nfs_free_delegation(server, delegation);
 }
 
 /**
-- 
2.47.3

[PATCH 06/24] NFS: remove nfs_start_delegation_return

[Christoph Hellwig]

There is only one caller, so fold it into that.  With that,
nfs_start_delegation_return

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/nfs/delegation.c | 32 ++++++++++++--------------------
 1 file changed, 12 insertions(+), 20 deletions(-)

diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c
index cf01ebd3b3c5..ec87463f196d 100644
--- a/fs/nfs/delegation.c
+++ b/fs/nfs/delegation.c
@@ -336,17 +336,6 @@ nfs_start_delegation_return_locked(struct nfs_inode *nfsi)
 	return ret;
 }
 
-static struct nfs_delegation *
-nfs_start_delegation_return(struct nfs_inode *nfsi)
-{
-	struct nfs_delegation *delegation;
-
-	rcu_read_lock();
-	delegation = nfs_start_delegation_return_locked(nfsi);
-	rcu_read_unlock();
-	return delegation;
-}
-
 static void nfs_abort_delegation_return(struct nfs_delegation *delegation,
 					struct nfs_server *server, int err)
 {
@@ -788,15 +777,18 @@ int nfs4_inode_return_delegation(struct inode *inode)
 	struct nfs_inode *nfsi = NFS_I(inode);
 	struct nfs_delegation *delegation;
 
-	delegation = nfs_start_delegation_return(nfsi);
-	if (delegation != NULL) {
-		/* Synchronous recall of any application leases */
-		break_lease(inode, O_WRONLY | O_RDWR);
-		if (S_ISREG(inode->i_mode))
-			nfs_wb_all(inode);
-		return nfs_end_delegation_return(inode, delegation, 1);
-	}
-	return 0;
+	rcu_read_lock();
+	delegation = nfs_start_delegation_return_locked(nfsi);
+	rcu_read_unlock();
+
+	if (!delegation)
+		return 0;
+
+	/* Synchronous recall of any application leases */
+	break_lease(inode, O_WRONLY | O_RDWR);
+	if (S_ISREG(inode->i_mode))
+		nfs_wb_all(inode);
+	return nfs_end_delegation_return(inode, delegation, 1);
 }
 
 /**
-- 
2.47.3

[PATCH 07/24] NFS: assert rcu_read_lock is held in nfs_start_delegation_return_locked

[Christoph Hellwig]

And clean up the dereference of the delegation a bit.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/nfs/delegation.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c
index ec87463f196d..a5a3ecac8a43 100644
--- a/fs/nfs/delegation.c
+++ b/fs/nfs/delegation.c
@@ -318,10 +318,14 @@ static struct nfs_delegation *
 nfs_start_delegation_return_locked(struct nfs_inode *nfsi)
 {
 	struct nfs_delegation *ret = NULL;
-	struct nfs_delegation *delegation = rcu_dereference(nfsi->delegation);
+	struct nfs_delegation *delegation;
+
+	lockdep_assert_in_rcu_read_lock();
+
+	delegation = rcu_dereference(nfsi->delegation);
+	if (!delegation)
+		return NULL;
 
-	if (delegation == NULL)
-		goto out;
 	spin_lock(&delegation->lock);
 	if (delegation->inode &&
 	    !test_and_set_bit(NFS_DELEGATION_RETURNING, &delegation->flags)) {
@@ -332,7 +336,6 @@ nfs_start_delegation_return_locked(struct nfs_inode *nfsi)
 	spin_unlock(&delegation->lock);
 	if (ret)
 		nfs_clear_verifier_delegated(&nfsi->vfs_inode);
-out:
 	return ret;
 }
 
-- 
2.47.3

[PATCH 08/24] NFS: drop the _locked postfix from nfs_start_delegation_return

[Christoph Hellwig]

Now that nfs_start_delegation_return_locked is gone, and we have
RCU locking asserts, drop the extra postfix.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/nfs/delegation.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c
index a5a3ecac8a43..5049c65cc8e5 100644
--- a/fs/nfs/delegation.c
+++ b/fs/nfs/delegation.c
@@ -315,7 +315,7 @@ static struct inode *nfs_delegation_grab_inode(struct nfs_delegation *delegation
 }
 
 static struct nfs_delegation *
-nfs_start_delegation_return_locked(struct nfs_inode *nfsi)
+nfs_start_delegation_return(struct nfs_inode *nfsi)
 {
 	struct nfs_delegation *ret = NULL;
 	struct nfs_delegation *delegation;
@@ -583,7 +583,7 @@ static int nfs_end_delegation_return(struct inode *inode, struct nfs_delegation
 
 	err = nfs_do_return_delegation(inode, delegation, issync);
 out:
-	/* Refcount matched in nfs_start_delegation_return_locked() */
+	/* Refcount matched in nfs_start_delegation_return() */
 	nfs_put_delegation(delegation);
 	return err;
 }
@@ -658,7 +658,7 @@ static int nfs_server_return_marked_delegations(struct nfs_server *server,
 			}
 		}
 
-		delegation = nfs_start_delegation_return_locked(NFS_I(inode));
+		delegation = nfs_start_delegation_return(NFS_I(inode));
 		rcu_read_unlock();
 
 		iput(to_put);
@@ -781,7 +781,7 @@ int nfs4_inode_return_delegation(struct inode *inode)
 	struct nfs_delegation *delegation;
 
 	rcu_read_lock();
-	delegation = nfs_start_delegation_return_locked(nfsi);
+	delegation = nfs_start_delegation_return(nfsi);
 	rcu_read_unlock();
 
 	if (!delegation)
@@ -1258,13 +1258,13 @@ static int nfs_server_reap_unclaimed_delegations(struct nfs_server *server,
 		inode = nfs_delegation_grab_inode(delegation);
 		if (inode == NULL)
 			continue;
-		delegation = nfs_start_delegation_return_locked(NFS_I(inode));
+		delegation = nfs_start_delegation_return(NFS_I(inode));
 		rcu_read_unlock();
 		if (delegation != NULL) {
 			if (nfs_detach_delegation(NFS_I(inode), delegation,
 						server) != NULL)
 				nfs_free_delegation(server, delegation);
-			/* Match nfs_start_delegation_return_locked */
+			/* Match nfs_start_delegation_return */
 			nfs_put_delegation(delegation);
 		}
 		iput(inode);
-- 
2.47.3

[PATCH 09/24] NFS: remove NFS_DELEGATION_INODE_FREEING

[Christoph Hellwig]

This essentially reverts commit 6f9449be53f3 ("NFS: Fix a soft lockup in
the delegation recovery code") because the code walking the per-server
delegation list has been fixed to just skip inodes for which
nfs_delegation_grab_inode fails, instead of having to restart the entire
series in commit f92214e4c312 ("NFS: Avoid unnecessary rescanning of the
per-server delegation list").

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/nfs/delegation.c | 13 ++-----------
 fs/nfs/delegation.h |  1 -
 fs/nfs/nfs4trace.h  |  1 -
 3 files changed, 2 insertions(+), 13 deletions(-)

diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c
index 5049c65cc8e5..1ff496147b1e 100644
--- a/fs/nfs/delegation.c
+++ b/fs/nfs/delegation.c
@@ -308,8 +308,6 @@ static struct inode *nfs_delegation_grab_inode(struct nfs_delegation *delegation
 	spin_lock(&delegation->lock);
 	if (delegation->inode != NULL)
 		inode = igrab(delegation->inode);
-	if (!inode)
-		set_bit(NFS_DELEGATION_INODE_FREEING, &delegation->flags);
 	spin_unlock(&delegation->lock);
 	return inode;
 }
@@ -638,8 +636,6 @@ static int nfs_server_return_marked_delegations(struct nfs_server *server,
 	list_for_each_entry_from_rcu(delegation, &server->delegations, super_list) {
 		struct inode *to_put = NULL;
 
-		if (test_bit(NFS_DELEGATION_INODE_FREEING, &delegation->flags))
-			continue;
 		if (!nfs_delegation_need_return(delegation)) {
 			if (nfs4_is_valid_delegation(delegation, 0))
 				prev = delegation;
@@ -760,7 +756,6 @@ void nfs_inode_evict_delegation(struct inode *inode)
 		return;
 
 	set_bit(NFS_DELEGATION_RETURNING, &delegation->flags);
-	set_bit(NFS_DELEGATION_INODE_FREEING, &delegation->flags);
 	nfs_do_return_delegation(inode, delegation, 1);
 	nfs_free_delegation(server, delegation);
 }
@@ -1248,9 +1243,7 @@ static int nfs_server_reap_unclaimed_delegations(struct nfs_server *server,
 restart:
 	rcu_read_lock();
 	list_for_each_entry_rcu(delegation, &server->delegations, super_list) {
-		if (test_bit(NFS_DELEGATION_INODE_FREEING,
-					&delegation->flags) ||
-		    test_bit(NFS_DELEGATION_RETURNING,
+		if (test_bit(NFS_DELEGATION_RETURNING,
 					&delegation->flags) ||
 		    test_bit(NFS_DELEGATION_NEED_RECLAIM,
 					&delegation->flags) == 0)
@@ -1385,9 +1378,7 @@ static int nfs_server_reap_expired_delegations(struct nfs_server *server,
 restart:
 	rcu_read_lock();
 	list_for_each_entry_rcu(delegation, &server->delegations, super_list) {
-		if (test_bit(NFS_DELEGATION_INODE_FREEING,
-					&delegation->flags) ||
-		    test_bit(NFS_DELEGATION_RETURNING,
+		if (test_bit(NFS_DELEGATION_RETURNING,
 					&delegation->flags) ||
 		    test_bit(NFS_DELEGATION_TEST_EXPIRED,
 					&delegation->flags) == 0 ||
diff --git a/fs/nfs/delegation.h b/fs/nfs/delegation.h
index 46d866adb5c2..fef1f4126e8f 100644
--- a/fs/nfs/delegation.h
+++ b/fs/nfs/delegation.h
@@ -37,7 +37,6 @@ enum {
 	NFS_DELEGATION_RETURNING,
 	NFS_DELEGATION_REVOKED,
 	NFS_DELEGATION_TEST_EXPIRED,
-	NFS_DELEGATION_INODE_FREEING,
 	NFS_DELEGATION_RETURN_DELAYED,
 	NFS_DELEGATION_DELEGTIME,
 };
diff --git a/fs/nfs/nfs4trace.h b/fs/nfs/nfs4trace.h
index 6285128e631a..18d02b4715bb 100644
--- a/fs/nfs/nfs4trace.h
+++ b/fs/nfs/nfs4trace.h
@@ -996,7 +996,6 @@ DEFINE_NFS4_SET_DELEGATION_EVENT(nfs4_detach_delegation);
 		{ BIT(NFS_DELEGATION_RETURNING), "RETURNING" }, \
 		{ BIT(NFS_DELEGATION_REVOKED), "REVOKED" }, \
 		{ BIT(NFS_DELEGATION_TEST_EXPIRED), "TEST_EXPIRED" }, \
-		{ BIT(NFS_DELEGATION_INODE_FREEING), "INODE_FREEING" }, \
 		{ BIT(NFS_DELEGATION_RETURN_DELAYED), "RETURN_DELAYED" })
 
 DECLARE_EVENT_CLASS(nfs4_delegation_event,
-- 
2.47.3

[PATCH 10/24] NFS: open code nfs_delegation_need_return

[Christoph Hellwig]

There is only a single caller, and the function can be condensed into a
single if statement, making it more clear what is being tested there.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/nfs/delegation.c | 24 +++++++-----------------
 1 file changed, 7 insertions(+), 17 deletions(-)

diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c
index 1ff496147b1e..3c50a1467022 100644
--- a/fs/nfs/delegation.c
+++ b/fs/nfs/delegation.c
@@ -586,22 +586,6 @@ static int nfs_end_delegation_return(struct inode *inode, struct nfs_delegation
 	return err;
 }
 
-static bool nfs_delegation_need_return(struct nfs_delegation *delegation)
-{
-	bool ret = false;
-
-	trace_nfs_delegation_need_return(delegation);
-
-	if (test_and_clear_bit(NFS_DELEGATION_RETURN, &delegation->flags))
-		ret = true;
-	if (test_bit(NFS_DELEGATION_RETURNING, &delegation->flags) ||
-	    test_bit(NFS_DELEGATION_RETURN_DELAYED, &delegation->flags) ||
-	    test_bit(NFS_DELEGATION_REVOKED, &delegation->flags))
-		ret = false;
-
-	return ret;
-}
-
 static int nfs_server_return_marked_delegations(struct nfs_server *server,
 		void __always_unused *data)
 {
@@ -636,11 +620,17 @@ static int nfs_server_return_marked_delegations(struct nfs_server *server,
 	list_for_each_entry_from_rcu(delegation, &server->delegations, super_list) {
 		struct inode *to_put = NULL;
 
-		if (!nfs_delegation_need_return(delegation)) {
+		trace_nfs_delegation_need_return(delegation);
+
+		if (!test_and_clear_bit(NFS_DELEGATION_RETURN, &delegation->flags) ||
+		    test_bit(NFS_DELEGATION_RETURNING, &delegation->flags) ||
+		    test_bit(NFS_DELEGATION_RETURN_DELAYED, &delegation->flags) ||
+		    test_bit(NFS_DELEGATION_REVOKED, &delegation->flags)) {
 			if (nfs4_is_valid_delegation(delegation, 0))
 				prev = delegation;
 			continue;
 		}
+
 		inode = nfs_delegation_grab_inode(delegation);
 		if (inode == NULL)
 			continue;
-- 
2.47.3

[PATCH 11/24] NFS: remove nfs_free_delegation

[Christoph Hellwig]

Open code nfs_free_delegation in the callers, because having a "free"
function that wraps a revoke and put operation is a bit confusing,
especially when the __free version does the actual freeing triggered by
the last put.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/nfs/delegation.c | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c
index 3c50a1467022..96812d599400 100644
--- a/fs/nfs/delegation.c
+++ b/fs/nfs/delegation.c
@@ -72,13 +72,6 @@ static void nfs_put_delegation(struct nfs_delegation *delegation)
 		__nfs_free_delegation(delegation);
 }
 
-static void nfs_free_delegation(struct nfs_server *server,
-		struct nfs_delegation *delegation)
-{
-	nfs_mark_delegation_revoked(server, delegation);
-	nfs_put_delegation(delegation);
-}
-
 /**
  * nfs_mark_delegation_referenced - set delegation's REFERENCED flag
  * @delegation: delegation to process
@@ -539,7 +532,8 @@ int nfs_inode_set_delegation(struct inode *inode, const struct cred *cred,
 		__nfs_free_delegation(delegation);
 	if (freeme != NULL) {
 		nfs_do_return_delegation(inode, freeme, 0);
-		nfs_free_delegation(server, freeme);
+		nfs_mark_delegation_revoked(server, freeme);
+		nfs_put_delegation(freeme);
 	}
 	return status;
 }
@@ -747,7 +741,8 @@ void nfs_inode_evict_delegation(struct inode *inode)
 
 	set_bit(NFS_DELEGATION_RETURNING, &delegation->flags);
 	nfs_do_return_delegation(inode, delegation, 1);
-	nfs_free_delegation(server, delegation);
+	nfs_mark_delegation_revoked(server, delegation);
+	nfs_put_delegation(delegation);
 }
 
 /**
@@ -1245,8 +1240,10 @@ static int nfs_server_reap_unclaimed_delegations(struct nfs_server *server,
 		rcu_read_unlock();
 		if (delegation != NULL) {
 			if (nfs_detach_delegation(NFS_I(inode), delegation,
-						server) != NULL)
-				nfs_free_delegation(server, delegation);
+						server) != NULL) {
+				nfs_mark_delegation_revoked(server, delegation);
+				nfs_put_delegation(delegation);
+			}
 			/* Match nfs_start_delegation_return */
 			nfs_put_delegation(delegation);
 		}
-- 
2.47.3

[PATCH 12/24] NFS: rewrite nfs_delegations_present in terms of nr_active_delegations

[Christoph Hellwig]

Renewal only cares for active delegations and not revoked ones.  Replace
the list empty check with reading the active delegation counter to
implement this.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/nfs/delegation.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c
index 96812d599400..5141f6e5def1 100644
--- a/fs/nfs/delegation.c
+++ b/fs/nfs/delegation.c
@@ -1448,7 +1448,7 @@ int nfs_delegations_present(struct nfs_client *clp)
 
 	rcu_read_lock();
 	list_for_each_entry_rcu(server, &clp->cl_superblocks, client_link)
-		if (!list_empty(&server->delegations)) {
+		if (atomic_long_read(&server->nr_active_delegations) > 0) {
 			ret = 1;
 			break;
 		}
-- 
2.47.3

[PATCH 13/24] NFS: move delegation lookup into can_open_delegated

[Christoph Hellwig]

Keep the delegation handling in a single place, and just return the
stateid in an optional argument.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/nfs/nfs4proc.c | 65 ++++++++++++++++++++++++-----------------------
 1 file changed, 33 insertions(+), 32 deletions(-)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index d74cd8659999..d5948b8060f2 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -1609,26 +1609,37 @@ static int can_open_cached(struct nfs4_state *state, fmode_t mode,
 	return ret;
 }
 
-static int can_open_delegated(struct nfs_delegation *delegation, fmode_t fmode,
-		enum open_claim_type4 claim)
+static bool can_open_delegated(const struct inode *inode, fmode_t fmode,
+		enum open_claim_type4 claim, nfs4_stateid *stateid)
 {
-	if (delegation == NULL)
-		return 0;
-	if ((delegation->type & fmode) != fmode)
-		return 0;
+	struct nfs_delegation *delegation;
+	bool ret = false;
+
+	rcu_read_lock();
+	delegation = nfs4_get_valid_delegation(inode);
+	if (!delegation || (delegation->type & fmode) != fmode)
+		goto out_unlock;
+
 	switch (claim) {
-	case NFS4_OPEN_CLAIM_NULL:
-	case NFS4_OPEN_CLAIM_FH:
-		break;
 	case NFS4_OPEN_CLAIM_PREVIOUS:
-		if (!test_bit(NFS_DELEGATION_NEED_RECLAIM, &delegation->flags))
+		if (test_bit(NFS_DELEGATION_NEED_RECLAIM, &delegation->flags))
 			break;
 		fallthrough;
+	case NFS4_OPEN_CLAIM_NULL:
+	case NFS4_OPEN_CLAIM_FH:
+		nfs_mark_delegation_referenced(delegation);
+		/* Save the delegation stateid */
+		if (stateid)
+			nfs4_stateid_copy(stateid, &delegation->stateid);
+		ret = true;
+		break;
 	default:
-		return 0;
+		break;
 	}
-	nfs_mark_delegation_referenced(delegation);
-	return 1;
+
+out_unlock:
+	rcu_read_unlock();
+	return ret;
 }
 
 static void update_open_stateflags(struct nfs4_state *state, fmode_t fmode)
@@ -1981,7 +1992,6 @@ static void nfs4_return_incompatible_delegation(struct inode *inode, fmode_t fmo
 static struct nfs4_state *nfs4_try_open_cached(struct nfs4_opendata *opendata)
 {
 	struct nfs4_state *state = opendata->state;
-	struct nfs_delegation *delegation;
 	int open_mode = opendata->o_arg.open_flags;
 	fmode_t fmode = opendata->o_arg.fmode;
 	enum open_claim_type4 claim = opendata->o_arg.claim;
@@ -1996,15 +2006,10 @@ static struct nfs4_state *nfs4_try_open_cached(struct nfs4_opendata *opendata)
 			goto out_return_state;
 		}
 		spin_unlock(&state->owner->so_lock);
-		rcu_read_lock();
-		delegation = nfs4_get_valid_delegation(state->inode);
-		if (!can_open_delegated(delegation, fmode, claim)) {
-			rcu_read_unlock();
+
+		if (!can_open_delegated(state->inode, fmode, claim, &stateid))
 			break;
-		}
-		/* Save the delegation */
-		nfs4_stateid_copy(&stateid, &delegation->stateid);
-		rcu_read_unlock();
+
 		nfs_release_seqid(opendata->o_arg.seqid);
 		if (!opendata->is_recover) {
 			ret = nfs_may_open(state->inode, state->owner->so_cred, open_mode);
@@ -2556,16 +2561,14 @@ static void nfs4_open_prepare(struct rpc_task *task, void *calldata)
 	 * a delegation instead.
 	 */
 	if (data->state != NULL) {
-		struct nfs_delegation *delegation;
-
 		if (can_open_cached(data->state, data->o_arg.fmode,
 					data->o_arg.open_flags, claim))
 			goto out_no_action;
-		rcu_read_lock();
-		delegation = nfs4_get_valid_delegation(data->state->inode);
-		if (can_open_delegated(delegation, data->o_arg.fmode, claim))
-			goto unlock_no_action;
-		rcu_read_unlock();
+		if (can_open_delegated(data->state->inode, data->o_arg.fmode,
+				claim, NULL)) {
+			trace_nfs4_cached_open(data->state);
+			goto out_no_action;
+		}
 	}
 	/* Update client id. */
 	data->o_arg.clientid = clp->cl_clientid;
@@ -2601,9 +2604,7 @@ static void nfs4_open_prepare(struct rpc_task *task, void *calldata)
 			data->o_arg.createmode = NFS4_CREATE_GUARDED;
 	}
 	return;
-unlock_no_action:
-	trace_nfs4_cached_open(data->state);
-	rcu_read_unlock();
+
 out_no_action:
 	task->tk_action = NULL;
 out_wait:
-- 
2.47.3

[PATCH 14/24] NFS: return bool from nfs_detach_delegation{,_locked}

[Christoph Hellwig]

nfs_detach_delegation always returns either the passed in delegation
or NULL, simplify this to a bool return.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/nfs/delegation.c | 27 ++++++++++++++-------------
 1 file changed, 14 insertions(+), 13 deletions(-)

diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c
index 5141f6e5def1..6946b48584f8 100644
--- a/fs/nfs/delegation.c
+++ b/fs/nfs/delegation.c
@@ -345,7 +345,7 @@ static void nfs_abort_delegation_return(struct nfs_delegation *delegation,
 	spin_unlock(&delegation->lock);
 }
 
-static struct nfs_delegation *
+static bool
 nfs_detach_delegation_locked(struct nfs_inode *nfsi,
 		struct nfs_delegation *delegation,
 		struct nfs_client *clp)
@@ -356,13 +356,13 @@ nfs_detach_delegation_locked(struct nfs_inode *nfsi,
 
 	trace_nfs4_detach_delegation(&nfsi->vfs_inode, delegation->type);
 
-	if (deleg_cur == NULL || delegation != deleg_cur)
-		return NULL;
+	if (delegation != deleg_cur)
+		return false;
 
 	spin_lock(&delegation->lock);
 	if (!delegation->inode) {
 		spin_unlock(&delegation->lock);
-		return NULL;
+		return false;
 	}
 	hlist_del_init_rcu(&delegation->hash);
 	list_del_rcu(&delegation->super_list);
@@ -370,19 +370,20 @@ nfs_detach_delegation_locked(struct nfs_inode *nfsi,
 	rcu_assign_pointer(nfsi->delegation, NULL);
 	spin_unlock(&delegation->lock);
 	clear_bit(NFS_INO_REQ_DIR_DELEG, &nfsi->flags);
-	return delegation;
+	return true;
 }
 
-static struct nfs_delegation *nfs_detach_delegation(struct nfs_inode *nfsi,
+static bool nfs_detach_delegation(struct nfs_inode *nfsi,
 		struct nfs_delegation *delegation,
 		struct nfs_server *server)
 {
 	struct nfs_client *clp = server->nfs_client;
+	bool ret;
 
 	spin_lock(&clp->cl_lock);
-	delegation = nfs_detach_delegation_locked(nfsi, delegation, clp);
+	ret = nfs_detach_delegation_locked(nfsi, delegation, clp);
 	spin_unlock(&clp->cl_lock);
-	return delegation;
+	return ret;
 }
 
 static void
@@ -492,9 +493,9 @@ int nfs_inode_set_delegation(struct inode *inode, const struct cred *cred,
 					&old_delegation->flags))
 			goto out;
 	}
-	freeme = nfs_detach_delegation_locked(nfsi, old_delegation, clp);
-	if (freeme == NULL)
+	if (!nfs_detach_delegation_locked(nfsi, old_delegation, clp))
 		goto out;
+	freeme = old_delegation;
 add_new:
 	/*
 	 * If we didn't revalidate the change attribute before setting
@@ -732,8 +733,8 @@ void nfs_inode_evict_delegation(struct inode *inode)
 
 	rcu_read_lock();
 	delegation = rcu_dereference(nfsi->delegation);
-	if (delegation)
-		delegation = nfs_detach_delegation(nfsi, delegation, server);
+	if (delegation && !nfs_detach_delegation(nfsi, delegation, server))
+		delegation = NULL;
 	rcu_read_unlock();
 
 	if (!delegation)
@@ -1240,7 +1241,7 @@ static int nfs_server_reap_unclaimed_delegations(struct nfs_server *server,
 		rcu_read_unlock();
 		if (delegation != NULL) {
 			if (nfs_detach_delegation(NFS_I(inode), delegation,
-						server) != NULL) {
+					server)) {
 				nfs_mark_delegation_revoked(server, delegation);
 				nfs_put_delegation(delegation);
 			}
-- 
2.47.3

[PATCH 15/24] NFS: move the deleg_cur check out of nfs_detach_delegation_locked

[Christoph Hellwig]

nfs_inode_set_delegation as the only direct caller of
nfs_detach_delegation_locked already check this under cl_lock, so
don't repeat it.

Replace the lockdep coverage for the lock that was implicitly provided by
the rcu_dereference_protected call that is removed with an explicit
lockdep assert to keep the coverage.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/nfs/delegation.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c
index 6946b48584f8..f7d5622c625a 100644
--- a/fs/nfs/delegation.c
+++ b/fs/nfs/delegation.c
@@ -350,15 +350,10 @@ nfs_detach_delegation_locked(struct nfs_inode *nfsi,
 		struct nfs_delegation *delegation,
 		struct nfs_client *clp)
 {
-	struct nfs_delegation *deleg_cur =
-		rcu_dereference_protected(nfsi->delegation,
-				lockdep_is_held(&clp->cl_lock));
+	lockdep_assert_held(&clp->cl_lock);
 
 	trace_nfs4_detach_delegation(&nfsi->vfs_inode, delegation->type);
 
-	if (delegation != deleg_cur)
-		return false;
-
 	spin_lock(&delegation->lock);
 	if (!delegation->inode) {
 		spin_unlock(&delegation->lock);
@@ -378,10 +373,14 @@ static bool nfs_detach_delegation(struct nfs_inode *nfsi,
 		struct nfs_server *server)
 {
 	struct nfs_client *clp = server->nfs_client;
-	bool ret;
+	struct nfs_delegation *deleg_cur;
+	bool ret = false;
 
 	spin_lock(&clp->cl_lock);
-	ret = nfs_detach_delegation_locked(nfsi, delegation, clp);
+	deleg_cur = rcu_dereference_protected(nfsi->delegation,
+				lockdep_is_held(&clp->cl_lock));
+	if (delegation == deleg_cur)
+		ret = nfs_detach_delegation_locked(nfsi, delegation, clp);
 	spin_unlock(&clp->cl_lock);
 	return ret;
 }
-- 
2.47.3

[PATCH 16/24] NFS: simplify the detached delegation check in update_open_stateid

[Christoph Hellwig]

When nfs_detach_delegation_locked detaches a delegation from an inode,
it clears both nfsi->delegation and delegation->inode.  Use the later
in update_open_stateid to check for a detached inode, as that avoids
an extra local variable, and removes the need for a RCU derefernence
as we already hold the lock in the delegation.  This prepares for
removing the surrounding RCU critical section.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/nfs/nfs4proc.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index d5948b8060f2..03b1f98eb989 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -1907,7 +1907,6 @@ int update_open_stateid(struct nfs4_state *state,
 {
 	struct nfs_server *server = NFS_SERVER(state->inode);
 	struct nfs_client *clp = server->nfs_client;
-	struct nfs_inode *nfsi = NFS_I(state->inode);
 	struct nfs_delegation *deleg_cur;
 	nfs4_stateid freeme = { };
 	int ret = 0;
@@ -1926,7 +1925,7 @@ int update_open_stateid(struct nfs4_state *state,
 		goto no_delegation;
 
 	spin_lock(&deleg_cur->lock);
-	if (rcu_dereference(nfsi->delegation) != deleg_cur ||
+	if (!deleg_cur->inode ||
 	   test_bit(NFS_DELEGATION_RETURNING, &deleg_cur->flags) ||
 	    (deleg_cur->type & fmode) != fmode)
 		goto no_delegation_unlock;
-- 
2.47.3

[PATCH 17/24] NFS: take a delegation reference in nfs4_get_valid_delegation

[Christoph Hellwig]

Currently most work on struct nfs_delegation happens directly under RCU
protection.  This is generally fine, despite that long RCU sections are
not good for performance.  But for operations later taking a reference
to the delegation to perform blocking work, refcount_inc is used, which
can be racy against dropping the last reference and thus lead to use
after frees in extremely rare cases.

Fix this by taking a reference in nfs4_get_valid_delegation using
refcount_inc_not_zero so that the callers have a stabilized reference
they can work with and can be moved outside the RCU critical section.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/nfs/callback_proc.c | 13 ++++++---
 fs/nfs/delegation.c    | 62 ++++++++++++++++++++++--------------------
 fs/nfs/delegation.h    |  1 +
 fs/nfs/nfs4proc.c      | 26 +++++++++---------
 4 files changed, 56 insertions(+), 46 deletions(-)

diff --git a/fs/nfs/callback_proc.c b/fs/nfs/callback_proc.c
index 8397c43358bd..57550020c819 100644
--- a/fs/nfs/callback_proc.c
+++ b/fs/nfs/callback_proc.c
@@ -51,12 +51,18 @@ __be32 nfs4_callback_getattr(void *argp, void *resp,
 				-ntohl(res->status));
 		goto out;
 	}
-	rcu_read_lock();
+
 	delegation = nfs4_get_valid_delegation(inode);
-	if (delegation == NULL || (delegation->type & FMODE_WRITE) == 0)
+	if (!delegation)
 		goto out_iput;
-	res->size = i_size_read(inode);
+	if ((delegation->type & FMODE_WRITE) == 0) {
+		nfs_put_delegation(delegation);
+		goto out_iput;
+	}
 	res->change_attr = delegation->change_attr;
+	nfs_put_delegation(delegation);
+
+	res->size = i_size_read(inode);
 	if (nfs_have_writebacks(inode))
 		res->change_attr++;
 	res->atime = inode_get_atime(inode);
@@ -71,7 +77,6 @@ __be32 nfs4_callback_getattr(void *argp, void *resp,
 			  FATTR4_WORD2_TIME_DELEG_MODIFY) & args->bitmap[2];
 	res->status = 0;
 out_iput:
-	rcu_read_unlock();
 	trace_nfs4_cb_getattr(cps->clp, &args->fh, inode, -ntohl(res->status));
 	nfs_iput_and_deactive(inode);
 out:
diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c
index f7d5622c625a..811e84b559ee 100644
--- a/fs/nfs/delegation.c
+++ b/fs/nfs/delegation.c
@@ -66,7 +66,7 @@ static struct nfs_delegation *nfs_get_delegation(struct nfs_delegation *delegati
 	return delegation;
 }
 
-static void nfs_put_delegation(struct nfs_delegation *delegation)
+void nfs_put_delegation(struct nfs_delegation *delegation)
 {
 	if (refcount_dec_and_test(&delegation->refcount))
 		__nfs_free_delegation(delegation);
@@ -104,10 +104,14 @@ struct nfs_delegation *nfs4_get_valid_delegation(const struct inode *inode)
 {
 	struct nfs_delegation *delegation;
 
+	rcu_read_lock();
 	delegation = rcu_dereference(NFS_I(inode)->delegation);
-	if (nfs4_is_valid_delegation(delegation, 0))
-		return delegation;
-	return NULL;
+	if (!nfs4_is_valid_delegation(delegation, 0) ||
+	    !refcount_inc_not_zero(&delegation->refcount))
+		delegation = NULL;
+	rcu_read_unlock();
+
+	return delegation;
 }
 
 static int nfs4_do_check_delegation(struct inode *inode, fmode_t type,
@@ -789,10 +793,11 @@ void nfs4_inode_set_return_delegation_on_close(struct inode *inode)
 
 	if (!inode)
 		return;
-	rcu_read_lock();
+
 	delegation = nfs4_get_valid_delegation(inode);
 	if (!delegation)
-		goto out;
+		return;
+
 	spin_lock(&delegation->lock);
 	if (!delegation->inode)
 		goto out_unlock;
@@ -806,8 +811,7 @@ void nfs4_inode_set_return_delegation_on_close(struct inode *inode)
 	spin_unlock(&delegation->lock);
 	if (ret)
 		nfs_clear_verifier_delegated(inode);
-out:
-	rcu_read_unlock();
+	nfs_put_delegation(delegation);
 	nfs_end_delegation_return(inode, ret, 0);
 }
 
@@ -823,10 +827,10 @@ void nfs4_inode_return_delegation_on_close(struct inode *inode)
 	struct nfs_delegation *delegation;
 	struct nfs_delegation *ret = NULL;
 
-	rcu_read_lock();
 	delegation = nfs4_get_valid_delegation(inode);
 	if (!delegation)
-		goto out;
+		return;
+
 	if (test_bit(NFS_DELEGATION_RETURN_IF_CLOSED, &delegation->flags) ||
 	    atomic_long_read(&NFS_SERVER(inode)->nr_active_delegations) >=
 	    nfs_delegation_watermark) {
@@ -842,8 +846,8 @@ void nfs4_inode_return_delegation_on_close(struct inode *inode)
 		if (ret)
 			nfs_clear_verifier_delegated(inode);
 	}
-out:
-	rcu_read_unlock();
+
+	nfs_put_delegation(delegation);
 	nfs_end_delegation_return(inode, ret, 0);
 }
 
@@ -858,17 +862,17 @@ void nfs4_inode_return_delegation_on_close(struct inode *inode)
 int nfs4_inode_make_writeable(struct inode *inode)
 {
 	struct nfs_delegation *delegation;
+	int error = 0;
 
-	rcu_read_lock();
 	delegation = nfs4_get_valid_delegation(inode);
-	if (delegation == NULL ||
-	    (nfs4_has_session(NFS_SERVER(inode)->nfs_client) &&
-	     (delegation->type & FMODE_WRITE))) {
-		rcu_read_unlock();
+	if (!delegation)
 		return 0;
-	}
-	rcu_read_unlock();
-	return nfs4_inode_return_delegation(inode);
+
+	if (!nfs4_has_session(NFS_SERVER(inode)->nfs_client) ||
+	    !(delegation->type & FMODE_WRITE))
+		error = nfs4_inode_return_delegation(inode);
+	nfs_put_delegation(delegation);
+	return error;
 }
 
 static void
@@ -1111,24 +1115,24 @@ int nfs_async_inode_return_delegation(struct inode *inode,
 	struct nfs_client *clp = server->nfs_client;
 	struct nfs_delegation *delegation;
 
-	rcu_read_lock();
 	delegation = nfs4_get_valid_delegation(inode);
-	if (delegation == NULL)
-		goto out_enoent;
+	if (!delegation)
+		return -ENOENT;
+
 	if (stateid != NULL &&
-	    !clp->cl_mvops->match_stateid(&delegation->stateid, stateid))
-		goto out_enoent;
+	    !clp->cl_mvops->match_stateid(&delegation->stateid, stateid)) {
+		nfs_put_delegation(delegation);
+		return -ENOENT;
+	}
+
 	nfs_mark_return_delegation(server, delegation);
-	rcu_read_unlock();
+	nfs_put_delegation(delegation);
 
 	/* If there are any application leases or delegations, recall them */
 	break_lease(inode, O_WRONLY | O_RDWR | O_NONBLOCK);
 
 	nfs_delegation_run_state_manager(clp);
 	return 0;
-out_enoent:
-	rcu_read_unlock();
-	return -ENOENT;
 }
 
 static struct inode *
diff --git a/fs/nfs/delegation.h b/fs/nfs/delegation.h
index fef1f4126e8f..d1c5da3e66ea 100644
--- a/fs/nfs/delegation.h
+++ b/fs/nfs/delegation.h
@@ -80,6 +80,7 @@ bool nfs4_copy_delegation_stateid(struct inode *inode, fmode_t flags, nfs4_state
 bool nfs4_refresh_delegation_stateid(nfs4_stateid *dst, struct inode *inode);
 
 struct nfs_delegation *nfs4_get_valid_delegation(const struct inode *inode);
+void nfs_put_delegation(struct nfs_delegation *delegation);
 void nfs_mark_delegation_referenced(struct nfs_delegation *delegation);
 int nfs4_have_delegation(struct inode *inode, fmode_t type, int flags);
 int nfs4_check_delegation(struct inode *inode, fmode_t type);
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 03b1f98eb989..2b28f56d8544 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -1615,10 +1615,11 @@ static bool can_open_delegated(const struct inode *inode, fmode_t fmode,
 	struct nfs_delegation *delegation;
 	bool ret = false;
 
-	rcu_read_lock();
 	delegation = nfs4_get_valid_delegation(inode);
-	if (!delegation || (delegation->type & fmode) != fmode)
-		goto out_unlock;
+	if (!delegation)
+		return false;
+	if ((delegation->type & fmode) != fmode)
+		goto out_put_delegation;
 
 	switch (claim) {
 	case NFS4_OPEN_CLAIM_PREVIOUS:
@@ -1637,8 +1638,8 @@ static bool can_open_delegated(const struct inode *inode, fmode_t fmode,
 		break;
 	}
 
-out_unlock:
-	rcu_read_unlock();
+out_put_delegation:
+	nfs_put_delegation(delegation);
 	return ret;
 }
 
@@ -1913,10 +1914,11 @@ int update_open_stateid(struct nfs4_state *state,
 
 	fmode &= (FMODE_READ|FMODE_WRITE);
 
-	rcu_read_lock();
 	spin_lock(&state->owner->so_lock);
 	if (open_stateid != NULL) {
+		rcu_read_lock();
 		nfs_state_set_open_stateid(state, open_stateid, fmode, &freeme);
+		rcu_read_unlock();
 		ret = 1;
 	}
 
@@ -1940,11 +1942,11 @@ int update_open_stateid(struct nfs4_state *state,
 	ret = 1;
 no_delegation_unlock:
 	spin_unlock(&deleg_cur->lock);
+	nfs_put_delegation(deleg_cur);
 no_delegation:
 	if (ret)
 		update_open_stateflags(state, fmode);
 	spin_unlock(&state->owner->so_lock);
-	rcu_read_unlock();
 
 	if (test_bit(NFS_STATE_RECLAIM_NOGRACE, &state->flags))
 		nfs4_schedule_state_manager(clp);
@@ -1978,14 +1980,12 @@ static void nfs4_return_incompatible_delegation(struct inode *inode, fmode_t fmo
 	struct nfs_delegation *delegation;
 
 	fmode &= FMODE_READ|FMODE_WRITE;
-	rcu_read_lock();
 	delegation = nfs4_get_valid_delegation(inode);
-	if (delegation == NULL || (delegation->type & fmode) == fmode) {
-		rcu_read_unlock();
+	if (!delegation)
 		return;
-	}
-	rcu_read_unlock();
-	nfs4_inode_return_delegation(inode);
+	if ((delegation->type & fmode) != fmode)
+		nfs4_inode_return_delegation(inode);
+	nfs_put_delegation(delegation);
 }
 
 static struct nfs4_state *nfs4_try_open_cached(struct nfs4_opendata *opendata)
-- 
2.47.3

[PATCH 18/24] NFS: don't consume a delegation reference in nfs_end_delegation_return

[Christoph Hellwig]

All callers now hold references to the delegation as part of the lookup,
removing the need for an extra reference for those that are actually
returned which is then dropped in nfs_end_delegation_return.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/nfs/delegation.c | 46 +++++++++++++++++++++++----------------------
 1 file changed, 24 insertions(+), 22 deletions(-)

diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c
index 811e84b559ee..5fb48a140169 100644
--- a/fs/nfs/delegation.c
+++ b/fs/nfs/delegation.c
@@ -325,7 +325,6 @@ nfs_start_delegation_return(struct nfs_inode *nfsi)
 	if (delegation->inode &&
 	    !test_and_set_bit(NFS_DELEGATION_RETURNING, &delegation->flags)) {
 		clear_bit(NFS_DELEGATION_RETURN_DELAYED, &delegation->flags);
-		/* Refcount matched in nfs_end_delegation_return() */
 		ret = nfs_get_delegation(delegation);
 	}
 	spin_unlock(&delegation->lock);
@@ -574,14 +573,10 @@ static int nfs_end_delegation_return(struct inode *inode, struct nfs_delegation
 
 	if (err) {
 		nfs_abort_delegation_return(delegation, server, err);
-		goto out;
+		return err;
 	}
 
-	err = nfs_do_return_delegation(inode, delegation, issync);
-out:
-	/* Refcount matched in nfs_start_delegation_return() */
-	nfs_put_delegation(delegation);
-	return err;
+	return nfs_do_return_delegation(inode, delegation, issync);
 }
 
 static int nfs_server_return_marked_delegations(struct nfs_server *server,
@@ -647,7 +642,11 @@ static int nfs_server_return_marked_delegations(struct nfs_server *server,
 
 		iput(to_put);
 
-		err = nfs_end_delegation_return(inode, delegation, 0);
+		if (delegation) {
+			err = nfs_end_delegation_return(inode, delegation, 0);
+			nfs_put_delegation(delegation);
+		}
+
 		iput(inode);
 		cond_resched();
 		if (!err)
@@ -763,6 +762,7 @@ int nfs4_inode_return_delegation(struct inode *inode)
 {
 	struct nfs_inode *nfsi = NFS_I(inode);
 	struct nfs_delegation *delegation;
+	int err;
 
 	rcu_read_lock();
 	delegation = nfs_start_delegation_return(nfsi);
@@ -775,7 +775,9 @@ int nfs4_inode_return_delegation(struct inode *inode)
 	break_lease(inode, O_WRONLY | O_RDWR);
 	if (S_ISREG(inode->i_mode))
 		nfs_wb_all(inode);
-	return nfs_end_delegation_return(inode, delegation, 1);
+	err = nfs_end_delegation_return(inode, delegation, 1);
+	nfs_put_delegation(delegation);
+	return err;
 }
 
 /**
@@ -789,7 +791,7 @@ int nfs4_inode_return_delegation(struct inode *inode)
 void nfs4_inode_set_return_delegation_on_close(struct inode *inode)
 {
 	struct nfs_delegation *delegation;
-	struct nfs_delegation *ret = NULL;
+	bool return_now = false;
 
 	if (!inode)
 		return;
@@ -802,17 +804,17 @@ void nfs4_inode_set_return_delegation_on_close(struct inode *inode)
 	if (!delegation->inode)
 		goto out_unlock;
 	if (list_empty(&NFS_I(inode)->open_files) &&
-	    !test_and_set_bit(NFS_DELEGATION_RETURNING, &delegation->flags)) {
-		/* Refcount matched in nfs_end_delegation_return() */
-		ret = nfs_get_delegation(delegation);
-	} else
+	    !test_and_set_bit(NFS_DELEGATION_RETURNING, &delegation->flags))
+		return_now = true;
+	else
 		set_bit(NFS_DELEGATION_RETURN_IF_CLOSED, &delegation->flags);
 out_unlock:
 	spin_unlock(&delegation->lock);
-	if (ret)
+	if (return_now) {
 		nfs_clear_verifier_delegated(inode);
+		nfs_end_delegation_return(inode, delegation, 0);
+	}
 	nfs_put_delegation(delegation);
-	nfs_end_delegation_return(inode, ret, 0);
 }
 
 /**
@@ -825,7 +827,7 @@ void nfs4_inode_set_return_delegation_on_close(struct inode *inode)
 void nfs4_inode_return_delegation_on_close(struct inode *inode)
 {
 	struct nfs_delegation *delegation;
-	struct nfs_delegation *ret = NULL;
+	bool return_now = false;
 
 	delegation = nfs4_get_valid_delegation(inode);
 	if (!delegation)
@@ -839,16 +841,16 @@ void nfs4_inode_return_delegation_on_close(struct inode *inode)
 		    list_empty(&NFS_I(inode)->open_files) &&
 		    !test_and_set_bit(NFS_DELEGATION_RETURNING, &delegation->flags)) {
 			clear_bit(NFS_DELEGATION_RETURN_IF_CLOSED, &delegation->flags);
-			/* Refcount matched in nfs_end_delegation_return() */
-			ret = nfs_get_delegation(delegation);
+			return_now = true;
 		}
 		spin_unlock(&delegation->lock);
-		if (ret)
-			nfs_clear_verifier_delegated(inode);
 	}
 
+	if (return_now) {
+		nfs_clear_verifier_delegated(inode);
+		nfs_end_delegation_return(inode, delegation, 0);
+	}
 	nfs_put_delegation(delegation);
-	nfs_end_delegation_return(inode, ret, 0);
 }
 
 /**
-- 
2.47.3

[PATCH 19/24] NFS: use refcount_inc_not_zero nfs_start_delegation_return

[Christoph Hellwig]

Using the unconditional reference increment means we can take a
reference to a delegation already in the RCU grace period, which could
cause a use after free under very unlikely conditions.  Switch to use
refcount_inc_not_zero instead.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/nfs/delegation.c | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c
index 5fb48a140169..5d9dba7ab430 100644
--- a/fs/nfs/delegation.c
+++ b/fs/nfs/delegation.c
@@ -60,12 +60,6 @@ static void nfs_mark_delegation_revoked(struct nfs_server *server,
 	}
 }
 
-static struct nfs_delegation *nfs_get_delegation(struct nfs_delegation *delegation)
-{
-	refcount_inc(&delegation->refcount);
-	return delegation;
-}
-
 void nfs_put_delegation(struct nfs_delegation *delegation)
 {
 	if (refcount_dec_and_test(&delegation->refcount))
@@ -312,25 +306,29 @@ static struct inode *nfs_delegation_grab_inode(struct nfs_delegation *delegation
 static struct nfs_delegation *
 nfs_start_delegation_return(struct nfs_inode *nfsi)
 {
-	struct nfs_delegation *ret = NULL;
 	struct nfs_delegation *delegation;
+	bool return_now = false;
 
 	lockdep_assert_in_rcu_read_lock();
 
 	delegation = rcu_dereference(nfsi->delegation);
-	if (!delegation)
+	if (!delegation || !refcount_inc_not_zero(&delegation->refcount))
 		return NULL;
 
 	spin_lock(&delegation->lock);
 	if (delegation->inode &&
 	    !test_and_set_bit(NFS_DELEGATION_RETURNING, &delegation->flags)) {
 		clear_bit(NFS_DELEGATION_RETURN_DELAYED, &delegation->flags);
-		ret = nfs_get_delegation(delegation);
+		return_now = true;
 	}
 	spin_unlock(&delegation->lock);
-	if (ret)
-		nfs_clear_verifier_delegated(&nfsi->vfs_inode);
-	return ret;
+
+	if (!return_now) {
+		nfs_put_delegation(delegation);
+		return NULL;
+	}
+	nfs_clear_verifier_delegated(&nfsi->vfs_inode);
+	return delegation;
 }
 
 static void nfs_abort_delegation_return(struct nfs_delegation *delegation,
-- 
2.47.3

[PATCH 20/24] NFS: use a local RCU critical section in nfs_start_delegation_return

[Christoph Hellwig]

Nested RCU critical sections are fine and very cheap.  Have a local one
in nfs_start_delegation_return so that the function is self-contained
and to prepare for simplifying the callers.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/nfs/delegation.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c
index 5d9dba7ab430..2ac897f67b01 100644
--- a/fs/nfs/delegation.c
+++ b/fs/nfs/delegation.c
@@ -309,11 +309,13 @@ nfs_start_delegation_return(struct nfs_inode *nfsi)
 	struct nfs_delegation *delegation;
 	bool return_now = false;
 
-	lockdep_assert_in_rcu_read_lock();
-
+	rcu_read_lock();
 	delegation = rcu_dereference(nfsi->delegation);
-	if (!delegation || !refcount_inc_not_zero(&delegation->refcount))
+	if (!delegation || !refcount_inc_not_zero(&delegation->refcount)) {
+		rcu_read_unlock();
 		return NULL;
+	}
+	rcu_read_unlock();
 
 	spin_lock(&delegation->lock);
 	if (delegation->inode &&
@@ -762,10 +764,7 @@ int nfs4_inode_return_delegation(struct inode *inode)
 	struct nfs_delegation *delegation;
 	int err;
 
-	rcu_read_lock();
 	delegation = nfs_start_delegation_return(nfsi);
-	rcu_read_unlock();
-
 	if (!delegation)
 		return 0;
 
-- 
2.47.3

[PATCH 21/24] NFS: reformat nfs_mark_delegation_revoked

[Christoph Hellwig]

Remove a level of indentation for the main code path.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/nfs/delegation.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c
index 2ac897f67b01..f46799448a0a 100644
--- a/fs/nfs/delegation.c
+++ b/fs/nfs/delegation.c
@@ -52,12 +52,13 @@ static void __nfs_free_delegation(struct nfs_delegation *delegation)
 static void nfs_mark_delegation_revoked(struct nfs_server *server,
 		struct nfs_delegation *delegation)
 {
-	if (!test_and_set_bit(NFS_DELEGATION_REVOKED, &delegation->flags)) {
-		delegation->stateid.type = NFS4_INVALID_STATEID_TYPE;
-		atomic_long_dec(&server->nr_active_delegations);
-		if (!test_bit(NFS_DELEGATION_RETURNING, &delegation->flags))
-			nfs_clear_verifier_delegated(delegation->inode);
-	}
+	if (test_and_set_bit(NFS_DELEGATION_REVOKED, &delegation->flags))
+		return;
+
+	delegation->stateid.type = NFS4_INVALID_STATEID_TYPE;
+	atomic_long_dec(&server->nr_active_delegations);
+	if (!test_bit(NFS_DELEGATION_RETURNING, &delegation->flags))
+		nfs_clear_verifier_delegated(delegation->inode);
 }
 
 void nfs_put_delegation(struct nfs_delegation *delegation)
-- 
2.47.3

[PATCH 22/24] NFS: add a separate delegation return list

[Christoph Hellwig]

Searching for returnable delegations in the per-server delegations list
can be very expensive.  While commit e04bbf6b1bbe ("NFS: Avoid quadratic
search when freeing delegations.") reduced the overhead a bit, the
fact that all the non-returnable delegations have to be searched limits
the amount of optimizations that can be done.

Fix this by introducing a separate list that only contains delegations
scheduled for return.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/nfs/client.c           |   2 +
 fs/nfs/delegation.c       | 162 +++++++++++++++++++-------------------
 fs/nfs/delegation.h       |   2 +-
 fs/nfs/nfs4trace.h        |   1 -
 include/linux/nfs_fs_sb.h |   7 +-
 5 files changed, 87 insertions(+), 87 deletions(-)

diff --git a/fs/nfs/client.c b/fs/nfs/client.c
index 2aaea9c98c2c..65b3de91b441 100644
--- a/fs/nfs/client.c
+++ b/fs/nfs/client.c
@@ -1060,6 +1060,8 @@ struct nfs_server *nfs_alloc_server(void)
 	INIT_LIST_HEAD(&server->client_link);
 	INIT_LIST_HEAD(&server->master_link);
 	INIT_LIST_HEAD(&server->delegations);
+	spin_lock_init(&server->delegations_lock);
+	INIT_LIST_HEAD(&server->delegations_return);
 	INIT_LIST_HEAD(&server->layouts);
 	INIT_LIST_HEAD(&server->state_owners_lru);
 	INIT_LIST_HEAD(&server->ss_copies);
diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c
index f46799448a0a..25f4bb598fd8 100644
--- a/fs/nfs/delegation.c
+++ b/fs/nfs/delegation.c
@@ -52,6 +52,8 @@ static void __nfs_free_delegation(struct nfs_delegation *delegation)
 static void nfs_mark_delegation_revoked(struct nfs_server *server,
 		struct nfs_delegation *delegation)
 {
+	bool put_ref = false;
+
 	if (test_and_set_bit(NFS_DELEGATION_REVOKED, &delegation->flags))
 		return;
 
@@ -59,6 +61,16 @@ static void nfs_mark_delegation_revoked(struct nfs_server *server,
 	atomic_long_dec(&server->nr_active_delegations);
 	if (!test_bit(NFS_DELEGATION_RETURNING, &delegation->flags))
 		nfs_clear_verifier_delegated(delegation->inode);
+
+	spin_lock(&server->delegations_lock);
+	if (!list_empty(&delegation->entry)) {
+		list_del_init(&delegation->entry);
+		put_ref = true;
+	}
+	spin_unlock(&server->delegations_lock);
+
+	if (put_ref)
+		nfs_put_delegation(delegation);
 }
 
 void nfs_put_delegation(struct nfs_delegation *delegation)
@@ -80,8 +92,14 @@ void nfs_mark_delegation_referenced(struct nfs_delegation *delegation)
 static void nfs_mark_return_delegation(struct nfs_server *server,
 				       struct nfs_delegation *delegation)
 {
-	set_bit(NFS_DELEGATION_RETURN, &delegation->flags);
-	set_bit(NFS4SERV_DELEGRETURN, &server->delegation_flags);
+	spin_lock(&server->delegations_lock);
+	if (list_empty(&delegation->entry)) {
+		list_add_tail(&delegation->entry,
+				&server->delegations_return);
+		refcount_inc(&delegation->refcount);
+	}
+	spin_unlock(&server->delegations_lock);
+
 	set_bit(NFS4CLNT_DELEGRETURN, &server->nfs_client->cl_state);
 }
 
@@ -350,7 +368,7 @@ static void nfs_abort_delegation_return(struct nfs_delegation *delegation,
 }
 
 static bool
-nfs_detach_delegation_locked(struct nfs_inode *nfsi,
+nfs_detach_delegations_locked(struct nfs_inode *nfsi,
 		struct nfs_delegation *delegation,
 		struct nfs_client *clp)
 {
@@ -384,7 +402,7 @@ static bool nfs_detach_delegation(struct nfs_inode *nfsi,
 	deleg_cur = rcu_dereference_protected(nfsi->delegation,
 				lockdep_is_held(&clp->cl_lock));
 	if (delegation == deleg_cur)
-		ret = nfs_detach_delegation_locked(nfsi, delegation, clp);
+		ret = nfs_detach_delegations_locked(nfsi, delegation, clp);
 	spin_unlock(&clp->cl_lock);
 	return ret;
 }
@@ -454,6 +472,7 @@ int nfs_inode_set_delegation(struct inode *inode, const struct cred *cred,
 	delegation->cred = get_cred(cred);
 	delegation->inode = inode;
 	delegation->flags = 1<<NFS_DELEGATION_REFERENCED;
+	INIT_LIST_HEAD(&delegation->entry);
 	switch (deleg_type) {
 	case NFS4_OPEN_DELEGATE_READ_ATTRS_DELEG:
 	case NFS4_OPEN_DELEGATE_WRITE_ATTRS_DELEG:
@@ -496,7 +515,7 @@ int nfs_inode_set_delegation(struct inode *inode, const struct cred *cred,
 					&old_delegation->flags))
 			goto out;
 	}
-	if (!nfs_detach_delegation_locked(nfsi, old_delegation, clp))
+	if (!nfs_detach_delegations_locked(nfsi, old_delegation, clp))
 		goto out;
 	freeme = old_delegation;
 add_new:
@@ -580,85 +599,61 @@ static int nfs_end_delegation_return(struct inode *inode, struct nfs_delegation
 	return nfs_do_return_delegation(inode, delegation, issync);
 }
 
-static int nfs_server_return_marked_delegations(struct nfs_server *server,
-		void __always_unused *data)
+static int nfs_return_one_delegation(struct nfs_server *server)
 {
 	struct nfs_delegation *delegation;
-	struct nfs_delegation *prev;
 	struct inode *inode;
-	struct inode *place_holder = NULL;
-	struct nfs_delegation *place_holder_deleg = NULL;
 	int err = 0;
 
-	if (!test_and_clear_bit(NFS4SERV_DELEGRETURN,
-				&server->delegation_flags))
-		return 0;
-restart:
-	/*
-	 * To avoid quadratic looping we hold a reference
-	 * to an inode place_holder.  Each time we restart, we
-	 * list delegation in the server from the delegations
-	 * of that inode.
-	 * prev is an RCU-protected pointer to a delegation which
-	 * wasn't marked for return and might be a good choice for
-	 * the next place_holder.
-	 */
-	prev = NULL;
-	delegation = NULL;
-	rcu_read_lock();
-	if (place_holder)
-		delegation = rcu_dereference(NFS_I(place_holder)->delegation);
-	if (!delegation || delegation != place_holder_deleg)
-		delegation = list_entry_rcu(server->delegations.next,
-					    struct nfs_delegation, super_list);
-	list_for_each_entry_from_rcu(delegation, &server->delegations, super_list) {
-		struct inode *to_put = NULL;
-
-		trace_nfs_delegation_need_return(delegation);
-
-		if (!test_and_clear_bit(NFS_DELEGATION_RETURN, &delegation->flags) ||
-		    test_bit(NFS_DELEGATION_RETURNING, &delegation->flags) ||
-		    test_bit(NFS_DELEGATION_RETURN_DELAYED, &delegation->flags) ||
-		    test_bit(NFS_DELEGATION_REVOKED, &delegation->flags)) {
-			if (nfs4_is_valid_delegation(delegation, 0))
-				prev = delegation;
-			continue;
-		}
+	spin_lock(&server->delegations_lock);
+	delegation = list_first_entry_or_null(&server->delegations_return,
+			struct nfs_delegation, entry);
+	if (!delegation) {
+		spin_unlock(&server->delegations_lock);
+		return 0; /* no more delegations */
+	}
+	list_del_init(&delegation->entry);
+	spin_unlock(&server->delegations_lock);
 
-		inode = nfs_delegation_grab_inode(delegation);
-		if (inode == NULL)
-			continue;
+	spin_lock(&delegation->lock);
+	inode = delegation->inode;
+	if (!inode || !igrab(inode)) {
+		spin_unlock(&delegation->lock);
+		goto out_put_delegation;
+	}
+	if (test_bit(NFS_DELEGATION_RETURN_DELAYED, &delegation->flags) ||
+	    test_bit(NFS_DELEGATION_REVOKED, &delegation->flags) ||
+	    test_and_set_bit(NFS_DELEGATION_RETURNING, &delegation->flags)) {
+		spin_unlock(&delegation->lock);
+		goto out_put_inode;
+	}
+	clear_bit(NFS_DELEGATION_RETURN_DELAYED, &delegation->flags);
+	spin_unlock(&delegation->lock);
 
-		if (prev) {
-			struct inode *tmp = nfs_delegation_grab_inode(prev);
-			if (tmp) {
-				to_put = place_holder;
-				place_holder = tmp;
-				place_holder_deleg = prev;
-			}
-		}
+	nfs_clear_verifier_delegated(inode);
 
-		delegation = nfs_start_delegation_return(NFS_I(inode));
-		rcu_read_unlock();
+	err = nfs_end_delegation_return(inode, delegation, 0);
+	if (err) {
+		nfs_mark_return_delegation(server, delegation);
+		goto out_put_inode;
+	}
 
-		iput(to_put);
+out_put_inode:
+	iput(inode);
+out_put_delegation:
+	nfs_put_delegation(delegation);
+	if (err)
+		return err;
+	return 1; /* keep going */
+}
 
-		if (delegation) {
-			err = nfs_end_delegation_return(inode, delegation, 0);
-			nfs_put_delegation(delegation);
-		}
+static int nfs_server_return_marked_delegations(struct nfs_server *server,
+		void __always_unused *data)
+{
+	int err;
 
-		iput(inode);
+	while ((err = nfs_return_one_delegation(server)) > 0)
 		cond_resched();
-		if (!err)
-			goto restart;
-		set_bit(NFS4SERV_DELEGRETURN, &server->delegation_flags);
-		set_bit(NFS4CLNT_DELEGRETURN, &server->nfs_client->cl_state);
-		goto out;
-	}
-	rcu_read_unlock();
-out:
-	iput(place_holder);
 	return err;
 }
 
@@ -669,15 +664,15 @@ static bool nfs_server_clear_delayed_delegations(struct nfs_server *server)
 
 	if (!test_and_clear_bit(NFS4SERV_DELEGRETURN_DELAYED,
 				&server->delegation_flags))
-		goto out;
-	list_for_each_entry_rcu (d, &server->delegations, super_list) {
-		if (!test_bit(NFS_DELEGATION_RETURN_DELAYED, &d->flags))
-			continue;
-		nfs_mark_return_delegation(server, d);
-		clear_bit(NFS_DELEGATION_RETURN_DELAYED, &d->flags);
+		return false;
+
+	spin_lock(&server->delegations_lock);
+	list_for_each_entry_rcu(d, &server->delegations_return, entry) {
+		if (test_bit(NFS_DELEGATION_RETURN_DELAYED, &d->flags))
+			clear_bit(NFS_DELEGATION_RETURN_DELAYED, &d->flags);
 		ret = true;
 	}
-out:
+
 	return ret;
 }
 
@@ -687,14 +682,17 @@ static bool nfs_client_clear_delayed_delegations(struct nfs_client *clp)
 	bool ret = false;
 
 	if (!test_and_clear_bit(NFS4CLNT_DELEGRETURN_DELAYED, &clp->cl_state))
-		goto out;
+		return false;
+
 	rcu_read_lock();
 	list_for_each_entry_rcu (server, &clp->cl_superblocks, client_link) {
 		if (nfs_server_clear_delayed_delegations(server))
 			ret = true;
 	}
 	rcu_read_unlock();
-out:
+
+	if (ret)
+		set_bit(NFS4CLNT_DELEGRETURN, &clp->cl_state);
 	return ret;
 }
 
@@ -881,7 +879,7 @@ nfs_mark_return_if_closed_delegation(struct nfs_server *server,
 {
 	struct inode *inode;
 
-	if (test_bit(NFS_DELEGATION_RETURN, &delegation->flags) ||
+	if (!list_empty_careful(&server->delegations_return) ||
 	    test_bit(NFS_DELEGATION_RETURN_IF_CLOSED, &delegation->flags))
 		return;
 	spin_lock(&delegation->lock);
diff --git a/fs/nfs/delegation.h b/fs/nfs/delegation.h
index d1c5da3e66ea..a6733f034442 100644
--- a/fs/nfs/delegation.h
+++ b/fs/nfs/delegation.h
@@ -26,12 +26,12 @@ struct nfs_delegation {
 	unsigned long flags;
 	refcount_t refcount;
 	spinlock_t lock;
+	struct list_head entry;
 	struct rcu_head rcu;
 };
 
 enum {
 	NFS_DELEGATION_NEED_RECLAIM = 0,
-	NFS_DELEGATION_RETURN,
 	NFS_DELEGATION_RETURN_IF_CLOSED,
 	NFS_DELEGATION_REFERENCED,
 	NFS_DELEGATION_RETURNING,
diff --git a/fs/nfs/nfs4trace.h b/fs/nfs/nfs4trace.h
index 18d02b4715bb..8ff6396bc206 100644
--- a/fs/nfs/nfs4trace.h
+++ b/fs/nfs/nfs4trace.h
@@ -990,7 +990,6 @@ DEFINE_NFS4_SET_DELEGATION_EVENT(nfs4_detach_delegation);
 #define show_delegation_flags(flags) \
 	__print_flags(flags, "|", \
 		{ BIT(NFS_DELEGATION_NEED_RECLAIM), "NEED_RECLAIM" }, \
-		{ BIT(NFS_DELEGATION_RETURN), "RETURN" }, \
 		{ BIT(NFS_DELEGATION_RETURN_IF_CLOSED), "RETURN_IF_CLOSED" }, \
 		{ BIT(NFS_DELEGATION_REFERENCED), "REFERENCED" }, \
 		{ BIT(NFS_DELEGATION_RETURNING), "RETURNING" }, \
diff --git a/include/linux/nfs_fs_sb.h b/include/linux/nfs_fs_sb.h
index c58b870f31ee..e377b8c7086e 100644
--- a/include/linux/nfs_fs_sb.h
+++ b/include/linux/nfs_fs_sb.h
@@ -259,6 +259,8 @@ struct nfs_server {
 	struct list_head	state_owners_lru;
 	struct list_head	layouts;
 	struct list_head	delegations;
+	spinlock_t		delegations_lock;
+	struct list_head	delegations_return;
 	atomic_long_t		nr_active_delegations;
 	unsigned int		delegation_hash_mask;
 	struct hlist_head	*delegation_hash_table;
@@ -266,9 +268,8 @@ struct nfs_server {
 	struct list_head	ss_src_copies;
 
 	unsigned long		delegation_flags;
-#define NFS4SERV_DELEGRETURN		(1)
-#define NFS4SERV_DELEGATION_EXPIRED	(2)
-#define NFS4SERV_DELEGRETURN_DELAYED	(3)
+#define NFS4SERV_DELEGATION_EXPIRED	(1)
+#define NFS4SERV_DELEGRETURN_DELAYED	(2)
 	unsigned long		delegation_gen;
 	unsigned long		mig_gen;
 	unsigned long		mig_status;
-- 
2.47.3

[PATCH 23/24] NFS: return delegations from the end of a LRU when over the watermark

[Christoph Hellwig]

Directly returning delegations on close when over the watermark is
rather suboptimal as these delegations are much more likely to be reused
than those that have been unused for a long time.  Switch to returning
unused delegations from a new LRU list when we are above the threshold and
there are reclaimable delegations instead.

Pass over referenced delegations during the first pass to give delegations
that aren't in active used by frequently used for stat() or similar another
chance to not be instantly reclaimed.  This scheme works the same as the
referenced flags in the VFS inode and dentry caches.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/nfs/client.c           |  1 +
 fs/nfs/delegation.c       | 61 +++++++++++++++++++++++++++++++++++++--
 include/linux/nfs_fs_sb.h |  1 +
 3 files changed, 60 insertions(+), 3 deletions(-)

diff --git a/fs/nfs/client.c b/fs/nfs/client.c
index 65b3de91b441..62aece00f810 100644
--- a/fs/nfs/client.c
+++ b/fs/nfs/client.c
@@ -1062,6 +1062,7 @@ struct nfs_server *nfs_alloc_server(void)
 	INIT_LIST_HEAD(&server->delegations);
 	spin_lock_init(&server->delegations_lock);
 	INIT_LIST_HEAD(&server->delegations_return);
+	INIT_LIST_HEAD(&server->delegations_lru);
 	INIT_LIST_HEAD(&server->layouts);
 	INIT_LIST_HEAD(&server->state_owners_lru);
 	INIT_LIST_HEAD(&server->ss_copies);
diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c
index 25f4bb598fd8..712cdb3381ad 100644
--- a/fs/nfs/delegation.c
+++ b/fs/nfs/delegation.c
@@ -657,6 +657,60 @@ static int nfs_server_return_marked_delegations(struct nfs_server *server,
 	return err;
 }
 
+static inline bool nfs_delegations_over_limit(struct nfs_server *server)
+{
+	return !list_empty_careful(&server->delegations_lru) &&
+		atomic_long_read(&server->nr_active_delegations) >
+		nfs_delegation_watermark;
+}
+
+static void nfs_delegations_return_from_lru(struct nfs_server *server)
+{
+	struct nfs_delegation *d, *n;
+	unsigned int pass = 0;
+	bool moved = false;
+
+retry:
+	spin_lock(&server->delegations_lock);
+	list_for_each_entry_safe(d, n, &server->delegations_lru, entry) {
+		if (!nfs_delegations_over_limit(server))
+			break;
+		if (pass == 0 && test_bit(NFS_DELEGATION_REFERENCED, &d->flags))
+			continue;
+		list_move_tail(&d->entry, &server->delegations_return);
+		moved = true;
+	}
+	spin_unlock(&server->delegations_lock);
+
+	/*
+	 * If we are still over the limit, try to reclaim referenced delegations
+	 * as well.
+	 */
+	if (pass == 0 && nfs_delegations_over_limit(server)) {
+		pass++;
+		goto retry;
+	}
+
+	if (moved) {
+		set_bit(NFS4CLNT_DELEGRETURN, &server->nfs_client->cl_state);
+		nfs4_schedule_state_manager(server->nfs_client);
+	}
+}
+
+static void nfs_delegation_add_lru(struct nfs_server *server,
+		struct nfs_delegation *delegation)
+{
+	spin_lock(&server->delegations_lock);
+	if (list_empty(&delegation->entry)) {
+		list_add_tail(&delegation->entry, &server->delegations_lru);
+		refcount_inc(&delegation->refcount);
+	}
+	spin_unlock(&server->delegations_lock);
+
+	if (nfs_delegations_over_limit(server))
+		nfs_delegations_return_from_lru(server);
+}
+
 static bool nfs_server_clear_delayed_delegations(struct nfs_server *server)
 {
 	struct nfs_delegation *d;
@@ -822,6 +876,7 @@ void nfs4_inode_set_return_delegation_on_close(struct inode *inode)
  */
 void nfs4_inode_return_delegation_on_close(struct inode *inode)
 {
+	struct nfs_server *server = NFS_SERVER(inode);
 	struct nfs_delegation *delegation;
 	bool return_now = false;
 
@@ -829,9 +884,7 @@ void nfs4_inode_return_delegation_on_close(struct inode *inode)
 	if (!delegation)
 		return;
 
-	if (test_bit(NFS_DELEGATION_RETURN_IF_CLOSED, &delegation->flags) ||
-	    atomic_long_read(&NFS_SERVER(inode)->nr_active_delegations) >=
-	    nfs_delegation_watermark) {
+	if (test_bit(NFS_DELEGATION_RETURN_IF_CLOSED, &delegation->flags)) {
 		spin_lock(&delegation->lock);
 		if (delegation->inode &&
 		    list_empty(&NFS_I(inode)->open_files) &&
@@ -845,6 +898,8 @@ void nfs4_inode_return_delegation_on_close(struct inode *inode)
 	if (return_now) {
 		nfs_clear_verifier_delegated(inode);
 		nfs_end_delegation_return(inode, delegation, 0);
+	} else {
+		nfs_delegation_add_lru(server, delegation);
 	}
 	nfs_put_delegation(delegation);
 }
diff --git a/include/linux/nfs_fs_sb.h b/include/linux/nfs_fs_sb.h
index e377b8c7086e..bb13a294b69e 100644
--- a/include/linux/nfs_fs_sb.h
+++ b/include/linux/nfs_fs_sb.h
@@ -261,6 +261,7 @@ struct nfs_server {
 	struct list_head	delegations;
 	spinlock_t		delegations_lock;
 	struct list_head	delegations_return;
+	struct list_head	delegations_lru;
 	atomic_long_t		nr_active_delegations;
 	unsigned int		delegation_hash_mask;
 	struct hlist_head	*delegation_hash_table;
-- 
2.47.3

[PATCH 24/24] NFS: make nfs_mark_return_unreferenced_delegations less aggressive

[Christoph Hellwig]

Currently nfs_mark_return_unreferenced_delegations marks all open but
not referenced delegations (i.e., those were found by a previous pass)
as return on close, which means that we'll return them on close without
a way out.

Replace this with only iterating delegations that are on the LRU list,
and avoid delegations that are in use by an open files to avoid this.

Delegations that were never referenced while open still are be prime
candidates for return from the LRU if the number of delegations is over
the watermark, or otherwise will be returned by the next
nfs_mark_return_unreferenced_delegations pass after they are closed.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/nfs/delegation.c | 24 +++++++++++++++++-------
 1 file changed, 17 insertions(+), 7 deletions(-)

diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c
index 712cdb3381ad..776020bdf8f3 100644
--- a/fs/nfs/delegation.c
+++ b/fs/nfs/delegation.c
@@ -1126,15 +1126,21 @@ void nfs_remove_bad_delegation(struct inode *inode,
 }
 EXPORT_SYMBOL_GPL(nfs_remove_bad_delegation);
 
-static void nfs_mark_return_unreferenced_delegations(struct nfs_server *server)
+static bool nfs_mark_return_unreferenced_delegations(struct nfs_server *server)
 {
-	struct nfs_delegation *delegation;
+	struct nfs_delegation *d, *n;
+	bool marked = false;
 
-	list_for_each_entry_rcu(delegation, &server->delegations, super_list) {
-		if (test_and_clear_bit(NFS_DELEGATION_REFERENCED, &delegation->flags))
+	spin_lock(&server->delegations_lock);
+	list_for_each_entry_safe(d, n, &server->delegations_lru, entry) {
+		if (test_and_clear_bit(NFS_DELEGATION_REFERENCED, &d->flags))
 			continue;
-		nfs_mark_return_if_closed_delegation(server, delegation);
+		list_move_tail(&d->entry, &server->delegations_return);
+		marked = true;
 	}
+	spin_unlock(&server->delegations_lock);
+
+	return marked;
 }
 
 /**
@@ -1145,13 +1151,17 @@ static void nfs_mark_return_unreferenced_delegations(struct nfs_server *server)
 void nfs_expire_unreferenced_delegations(struct nfs_client *clp)
 {
 	struct nfs_server *server;
+	bool marked = false;
 
 	rcu_read_lock();
 	list_for_each_entry_rcu(server, &clp->cl_superblocks, client_link)
-		nfs_mark_return_unreferenced_delegations(server);
+		marked |= nfs_mark_return_unreferenced_delegations(server);
 	rcu_read_unlock();
 
-	nfs_delegation_run_state_manager(clp);
+	if (marked) {
+		set_bit(NFS4CLNT_DELEGRETURN, &clp->cl_state);
+		nfs4_schedule_state_manager(clp);
+	}
 }
 
 /**
-- 
2.47.3

Re: [PATCH 23/24] NFS: return delegations from the end of a LRU when over the watermark

[Anna Schumaker]

Hi Christoph,

On 12/18/25 12:56 AM, Christoph Hellwig wrote:
> Directly returning delegations on close when over the watermark is
> rather suboptimal as these delegations are much more likely to be reused
> than those that have been unused for a long time.  Switch to returning
> unused delegations from a new LRU list when we are above the threshold and
> there are reclaimable delegations instead.
> 
> Pass over referenced delegations during the first pass to give delegations
> that aren't in active used by frequently used for stat() or similar another
> chance to not be instantly reclaimed.  This scheme works the same as the
> referenced flags in the VFS inode and dentry caches.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

I'm seeing fstests hang on generic/013 after applying this patch and running
with NFS v4.0. Other versions of NFS still seem to work just fine. Have you
seen anything like this in your testing?

Thanks,
Anna

> ---
>  fs/nfs/client.c           |  1 +
>  fs/nfs/delegation.c       | 61 +++++++++++++++++++++++++++++++++++++--
>  include/linux/nfs_fs_sb.h |  1 +
>  3 files changed, 60 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/nfs/client.c b/fs/nfs/client.c
> index 65b3de91b441..62aece00f810 100644
> --- a/fs/nfs/client.c
> +++ b/fs/nfs/client.c
> @@ -1062,6 +1062,7 @@ struct nfs_server *nfs_alloc_server(void)
>  	INIT_LIST_HEAD(&server->delegations);
>  	spin_lock_init(&server->delegations_lock);
>  	INIT_LIST_HEAD(&server->delegations_return);
> +	INIT_LIST_HEAD(&server->delegations_lru);
>  	INIT_LIST_HEAD(&server->layouts);
>  	INIT_LIST_HEAD(&server->state_owners_lru);
>  	INIT_LIST_HEAD(&server->ss_copies);
> diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c
> index 25f4bb598fd8..712cdb3381ad 100644
> --- a/fs/nfs/delegation.c
> +++ b/fs/nfs/delegation.c
> @@ -657,6 +657,60 @@ static int nfs_server_return_marked_delegations(struct nfs_server *server,
>  	return err;
>  }
>  
> +static inline bool nfs_delegations_over_limit(struct nfs_server *server)
> +{
> +	return !list_empty_careful(&server->delegations_lru) &&
> +		atomic_long_read(&server->nr_active_delegations) >
> +		nfs_delegation_watermark;
> +}
> +
> +static void nfs_delegations_return_from_lru(struct nfs_server *server)
> +{
> +	struct nfs_delegation *d, *n;
> +	unsigned int pass = 0;
> +	bool moved = false;
> +
> +retry:
> +	spin_lock(&server->delegations_lock);
> +	list_for_each_entry_safe(d, n, &server->delegations_lru, entry) {
> +		if (!nfs_delegations_over_limit(server))
> +			break;
> +		if (pass == 0 && test_bit(NFS_DELEGATION_REFERENCED, &d->flags))
> +			continue;
> +		list_move_tail(&d->entry, &server->delegations_return);
> +		moved = true;
> +	}
> +	spin_unlock(&server->delegations_lock);
> +
> +	/*
> +	 * If we are still over the limit, try to reclaim referenced delegations
> +	 * as well.
> +	 */
> +	if (pass == 0 && nfs_delegations_over_limit(server)) {
> +		pass++;
> +		goto retry;
> +	}
> +
> +	if (moved) {
> +		set_bit(NFS4CLNT_DELEGRETURN, &server->nfs_client->cl_state);
> +		nfs4_schedule_state_manager(server->nfs_client);
> +	}
> +}
> +
> +static void nfs_delegation_add_lru(struct nfs_server *server,
> +		struct nfs_delegation *delegation)
> +{
> +	spin_lock(&server->delegations_lock);
> +	if (list_empty(&delegation->entry)) {
> +		list_add_tail(&delegation->entry, &server->delegations_lru);
> +		refcount_inc(&delegation->refcount);
> +	}
> +	spin_unlock(&server->delegations_lock);
> +
> +	if (nfs_delegations_over_limit(server))
> +		nfs_delegations_return_from_lru(server);
> +}
> +
>  static bool nfs_server_clear_delayed_delegations(struct nfs_server *server)
>  {
>  	struct nfs_delegation *d;
> @@ -822,6 +876,7 @@ void nfs4_inode_set_return_delegation_on_close(struct inode *inode)
>   */
>  void nfs4_inode_return_delegation_on_close(struct inode *inode)
>  {
> +	struct nfs_server *server = NFS_SERVER(inode);
>  	struct nfs_delegation *delegation;
>  	bool return_now = false;
>  
> @@ -829,9 +884,7 @@ void nfs4_inode_return_delegation_on_close(struct inode *inode)
>  	if (!delegation)
>  		return;
>  
> -	if (test_bit(NFS_DELEGATION_RETURN_IF_CLOSED, &delegation->flags) ||
> -	    atomic_long_read(&NFS_SERVER(inode)->nr_active_delegations) >=
> -	    nfs_delegation_watermark) {
> +	if (test_bit(NFS_DELEGATION_RETURN_IF_CLOSED, &delegation->flags)) {
>  		spin_lock(&delegation->lock);
>  		if (delegation->inode &&
>  		    list_empty(&NFS_I(inode)->open_files) &&
> @@ -845,6 +898,8 @@ void nfs4_inode_return_delegation_on_close(struct inode *inode)
>  	if (return_now) {
>  		nfs_clear_verifier_delegated(inode);
>  		nfs_end_delegation_return(inode, delegation, 0);
> +	} else {
> +		nfs_delegation_add_lru(server, delegation);
>  	}
>  	nfs_put_delegation(delegation);
>  }
> diff --git a/include/linux/nfs_fs_sb.h b/include/linux/nfs_fs_sb.h
> index e377b8c7086e..bb13a294b69e 100644
> --- a/include/linux/nfs_fs_sb.h
> +++ b/include/linux/nfs_fs_sb.h
> @@ -261,6 +261,7 @@ struct nfs_server {
>  	struct list_head	delegations;
>  	spinlock_t		delegations_lock;
>  	struct list_head	delegations_return;
> +	struct list_head	delegations_lru;
>  	atomic_long_t		nr_active_delegations;
>  	unsigned int		delegation_hash_mask;
>  	struct hlist_head	*delegation_hash_table;

Re: [PATCH 23/24] NFS: return delegations from the end of a LRU when over the watermark

[Christoph Hellwig]

On Thu, Dec 18, 2025 at 05:02:26PM -0500, Anna Schumaker wrote:
> > Pass over referenced delegations during the first pass to give delegations
> > that aren't in active used by frequently used for stat() or similar another
> > chance to not be instantly reclaimed.  This scheme works the same as the
> > referenced flags in the VFS inode and dentry caches.
> > 
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> I'm seeing fstests hang on generic/013 after applying this patch and running
> with NFS v4.0. Other versions of NFS still seem to work just fine. Have you
> seen anything like this in your testing?

I have to admit I completely ignored NFSv4.0 as usual.  I can reproduce
the hang, but oddly enough not when running the test standalone, but only
as part of at least a quick xfstests run.  Looking into it now.

Re: [PATCH 23/24] NFS: return delegations from the end of a LRU when over the watermark

[Christoph Hellwig]

This was caused by a delegation already on the LRU not getting moved
to the return list.  The patch below fixes it and passes the xfstests
quick group with 4.0.  I'll fold it in after a little more testing.

diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c
index 776020bdf8f3..6497fdbd9516 100644
--- a/fs/nfs/delegation.c
+++ b/fs/nfs/delegation.c
@@ -93,11 +93,9 @@ static void nfs_mark_return_delegation(struct nfs_server *server,
 				       struct nfs_delegation *delegation)
 {
 	spin_lock(&server->delegations_lock);
-	if (list_empty(&delegation->entry)) {
-		list_add_tail(&delegation->entry,
-				&server->delegations_return);
+	if (list_empty(&delegation->entry))
 		refcount_inc(&delegation->refcount);
-	}
+	list_move_tail(&delegation->entry, &server->delegations_return);
 	spin_unlock(&server->delegations_lock);
 
 	set_bit(NFS4CLNT_DELEGRETURN, &server->nfs_client->cl_state);

Re: [PATCH 23/24] NFS: return delegations from the end of a LRU when over the watermark

[Anna Schumaker]

Hi Christoph,

On 12/19/25 6:14 AM, Christoph Hellwig wrote:
> This was caused by a delegation already on the LRU not getting moved
> to the return list.  The patch below fixes it and passes the xfstests
> quick group with 4.0.  I'll fold it in after a little more testing.

The patch fixed the issue on my end. Thanks for looking into it!

Anna

> 
> diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c
> index 776020bdf8f3..6497fdbd9516 100644
> --- a/fs/nfs/delegation.c
> +++ b/fs/nfs/delegation.c
> @@ -93,11 +93,9 @@ static void nfs_mark_return_delegation(struct nfs_server *server,
>  				       struct nfs_delegation *delegation)
>  {
>  	spin_lock(&server->delegations_lock);
> -	if (list_empty(&delegation->entry)) {
> -		list_add_tail(&delegation->entry,
> -				&server->delegations_return);
> +	if (list_empty(&delegation->entry))
>  		refcount_inc(&delegation->refcount);
> -	}
> +	list_move_tail(&delegation->entry, &server->delegations_return);
>  	spin_unlock(&server->delegations_lock);
>  
>  	set_bit(NFS4CLNT_DELEGRETURN, &server->nfs_client->cl_state);

[PATCH 05/24] NFS: remove nfs_inode_detach_delegation

[Christoph Hellwig]

Fold it into the only caller.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/nfs/delegation.c | 37 +++++++++++++++----------------------
 1 file changed, 15 insertions(+), 22 deletions(-)

diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c
index b44e39a50499..cf01ebd3b3c5 100644
--- a/fs/nfs/delegation.c
+++ b/fs/nfs/delegation.c
@@ -402,21 +402,6 @@ static struct nfs_delegation *nfs_detach_delegation(struct nfs_inode *nfsi,
 	return delegation;
 }
 
-static struct nfs_delegation *
-nfs_inode_detach_delegation(struct inode *inode)
-{
-	struct nfs_inode *nfsi = NFS_I(inode);
-	struct nfs_server *server = NFS_SERVER(inode);
-	struct nfs_delegation *delegation;
-
-	rcu_read_lock();
-	delegation = rcu_dereference(nfsi->delegation);
-	if (delegation != NULL)
-		delegation = nfs_detach_delegation(nfsi, delegation, server);
-	rcu_read_unlock();
-	return delegation;
-}
-
 static void
 nfs_update_delegation_cred(struct nfs_delegation *delegation,
 		const struct cred *cred)
@@ -769,15 +754,23 @@ int nfs_client_return_marked_delegations(struct nfs_client *clp)
  */
 void nfs_inode_evict_delegation(struct inode *inode)
 {
+	struct nfs_inode *nfsi = NFS_I(inode);
+	struct nfs_server *server = NFS_SERVER(inode);
 	struct nfs_delegation *delegation;
 
-	delegation = nfs_inode_detach_delegation(inode);
-	if (delegation != NULL) {
-		set_bit(NFS_DELEGATION_RETURNING, &delegation->flags);
-		set_bit(NFS_DELEGATION_INODE_FREEING, &delegation->flags);
-		nfs_do_return_delegation(inode, delegation, 1);
-		nfs_free_delegation(NFS_SERVER(inode), delegation);
-	}
+	rcu_read_lock();
+	delegation = rcu_dereference(nfsi->delegation);
+	if (delegation)
+		delegation = nfs_detach_delegation(nfsi, delegation, server);
+	rcu_read_unlock();
+
+	if (!delegation)
+		return;
+
+	set_bit(NFS_DELEGATION_RETURNING, &delegation->flags);
+	set_bit(NFS_DELEGATION_INODE_FREEING, &delegation->flags);
+	nfs_do_return_delegation(inode, delegation, 1);
+	nfs_free_delegation(server, delegation);
 }
 
 /**
-- 
2.47.3