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 ==="

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 ==="

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.  Note that with this and also with the follow on patches the
baselines will still crash in some tests, and this series does not fix
that.

Changes since v1:
 - fix the nfsv4.0 hang

Diffstat:
 fs/nfs/callback_proc.c    |   13 -
 fs/nfs/client.c           |    3 
 fs/nfs/delegation.c       |  544 +++++++++++++++++++++++-----------------------
 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, 342 insertions(+), 328 deletions(-)

[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 ec1ce593dea2..3b4758e887e4 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 3b4758e887e4..6be300e8551a 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 6be300e8551a..5fe5b503dd89 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       | 160 +++++++++++++++++++-------------------
 fs/nfs/delegation.h       |   2 +-
 fs/nfs/nfs4trace.h        |   1 -
 include/linux/nfs_fs_sb.h |   7 +-
 5 files changed, 85 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..f0540c9843b3 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,12 @@ 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))
+		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);
 }
 
@@ -350,7 +366,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 +400,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 +470,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 +513,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 +597,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 +662,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 +680,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 +877,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 f0540c9843b3..0b256f461fe4 100644
--- a/fs/nfs/delegation.c
+++ b/fs/nfs/delegation.c
@@ -655,6 +655,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;
@@ -820,6 +874,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;
 
@@ -827,9 +882,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) &&
@@ -843,6 +896,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 0b256f461fe4..6497fdbd9516 100644
--- a/fs/nfs/delegation.c
+++ b/fs/nfs/delegation.c
@@ -1124,15 +1124,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;
 }
 
 /**
@@ -1143,13 +1149,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: add a LRU for delegations

[Chuck Lever]

On Wed, Jan 7, 2026, at 2:26 AM, Christoph Hellwig wrote:
> 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.

I'm curious if you tried other methodologies like LFU to decide which
delegation to return?


> 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.

-- 
Chuck Lever

Re: add a LRU for delegations

[Christoph Hellwig]

On Wed, Jan 07, 2026 at 10:18:04AM -0500, Chuck Lever wrote:
> > 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.
> 
> I'm curious if you tried other methodologies like LFU to decide which
> delegation to return?

Not really.  LFU I guess means least frequently used.  That would
imply tracking usage frequency, and rotating a list of other lookup
structure a lot.  The first is something not really provided in the
delegation code, and the latter tends to be really horrible for in
terms of cache dirtying.

Re: add a LRU for delegations

[Sagi Grimberg]

On 07/01/2026 18:22, Christoph Hellwig wrote:
> On Wed, Jan 07, 2026 at 10:18:04AM -0500, Chuck Lever wrote:
>>> 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.
>> I'm curious if you tried other methodologies like LFU to decide which
>> delegation to return?
> Not really.  LFU I guess means least frequently used.  That would
> imply tracking usage frequency, and rotating a list of other lookup
> structure a lot.  The first is something not really provided in the
> delegation code, and the latter tends to be really horrible for in
> terms of cache dirtying.

Hey Chuck, Christoph,

I think that there is merit for tracking file usage frequency and 
accounting it for deleg return
policy, and I don't necessarily think that it would not be worth the 
overhead (compared to sending rpcs to the server)...
but I agree that its a much heavier lift (it can always be done 
incrementally to this patchset).

I do think that there are some potential heuristics that the client can 
use to utilize delegations better. For example, perhaps
read delegations can be held longer than write delegations. The 
benchmark that Christoph
ran shows a 97% reduction in rpc calls for python venv/conda like 
workloads (as well as bash imports etc) which
represent shared files that are likely to be updated very rarely, yet 
accessed frequently by lots of clients.
I think that as a general heuristics, read delegations are much more 
useful when held longer. Maybe even indefinitely?

Another idea for a heuristic could be to take into account the mtime 
when opening the file with a delegation, if the mtime is far in the past,
perhaps it can be an indication of a longer period can the client hold 
on to the delegation with "low risk" of a conflict.

And, in genenral, I think that the server is in a much better position 
to solicit preemptive delegation recalls as opposed
to the client voluntarily return delegations when crossing a somewhat 
arbitrary delegation_watermark.

Thoughts?

Re: add a LRU for delegations

[Christoph Hellwig]

On Wed, Jan 07, 2026 at 09:22:49PM +0200, Sagi Grimberg wrote:
> I think that there is merit for tracking file usage frequency and 
> accounting it for deleg return
> policy, and I don't necessarily think that it would not be worth the 
> overhead (compared to sending rpcs to the server)...
> but I agree that its a much heavier lift (it can always be done 
> incrementally to this patchset).

There's a reason the rest of the kernel uses fairly simple LRU (or
rather modified LRU / clock like) eviction policies like this, in
that the sampling is quite overboard.   I'm not going to keep you
from looking into this, but I don't think it's worthwhile.

> And, in genenral, I think that the server is in a much better position to 
> solicit preemptive delegation recalls as opposed
> to the client voluntarily return delegations when crossing a somewhat 
> arbitrary delegation_watermark.

Yes.  For that we'd need working RECALL_ANY support, though.  And
the way that works in the spec where the value is a number to keep
instead of a number to returns makes it pretty weird unfortunately.

Re: add a LRU for delegations

[Chuck Lever]

On Wed, Jan 7, 2026, at 2:22 PM, Sagi Grimberg wrote:
> And, in general, I think that the server is in a much better position 
> to solicit preemptive delegation recalls as opposed
> to the client voluntarily return delegations when crossing a somewhat 
> arbitrary delegation_watermark.

The server and client have orthogonal interests here, IMO.

The server is concerned with resource utilization -- memory consumed,
slots in tables, and so on -- that other active clients might benefit
from having freed. The server doesn't really care which delegations
are returned.

A client wants to keep delegation state that applications are using,
and it knows best which ones those are. It can identify specific
delegations that are not being actively used and return those.


-- 
Chuck Lever

Re: add a LRU for delegations

[Christoph Hellwig]

On Thu, Jan 08, 2026 at 09:34:28AM -0500, Chuck Lever wrote:
> The server and client have orthogonal interests here, IMO.
> 
> The server is concerned with resource utilization -- memory consumed,
> slots in tables, and so on -- that other active clients might benefit
> from having freed. The server doesn't really care which delegations
> are returned.
> 
> A client wants to keep delegation state that applications are using,
> and it knows best which ones those are. It can identify specific
> delegations that are not being actively used and return those.

Yes.  A good way to deal with that is implementing RECALLY_ANY as I
mentioned, which with these changes could reclaim off the end of the LRU.
But as this series is a big change already, and doing fine grained
recalls is another huge one I'd rather wait a bit before going there.

Re: add a LRU for delegations

[Sagi Grimberg]

On 08/01/2026 15:46, Christoph Hellwig wrote:
> On Wed, Jan 07, 2026 at 09:22:49PM +0200, Sagi Grimberg wrote:
>> I think that there is merit for tracking file usage frequency and
>> accounting it for deleg return
>> policy, and I don't necessarily think that it would not be worth the
>> overhead (compared to sending rpcs to the server)...
>> but I agree that its a much heavier lift (it can always be done
>> incrementally to this patchset).
> There's a reason the rest of the kernel uses fairly simple LRU (or
> rather modified LRU / clock like) eviction policies like this, in
> that the sampling is quite overboard.   I'm not going to keep you
> from looking into this, but I don't think it's worthwhile.

I understand. Regardless, this patchset is an obvious win IMO.

>
>> And, in genenral, I think that the server is in a much better position to
>> solicit preemptive delegation recalls as opposed
>> to the client voluntarily return delegations when crossing a somewhat
>> arbitrary delegation_watermark.
> Yes.  For that we'd need working RECALL_ANY support, though.  And
> the way that works in the spec where the value is a number to keep
> instead of a number to returns makes it pretty weird unfortunately.

And I see that the client does not pay attention to that keep arg, it 
nukes all the delegs afaict...

Re: add a LRU for delegations

[Sagi Grimberg]

On 08/01/2026 16:34, Chuck Lever wrote:
>
> On Wed, Jan 7, 2026, at 2:22 PM, Sagi Grimberg wrote:
>> And, in general, I think that the server is in a much better position
>> to solicit preemptive delegation recalls as opposed
>> to the client voluntarily return delegations when crossing a somewhat
>> arbitrary delegation_watermark.
> The server and client have orthogonal interests here, IMO.

They both share the interest of reducing rpc calls...

>
> The server is concerned with resource utilization -- memory consumed,
> slots in tables, and so on -- that other active clients might benefit
> from having freed. The server doesn't really care which delegations
> are returned.
>
> A client wants to keep delegation state that applications are using,
> and it knows best which ones those are. It can identify specific
> delegations that are not being actively used and return those.

Yes, I agree to some extent. However arguably the client may want keep 
delegation
states around for as long as it can "just in case" the application opens 
the file again (the client
doesn't know the nature of the workload) assuming they fit in client 
memory. It doesn't know
anything about other clients nor the server resources.

The server on the other hand, has the knowledge of what delegations 
conflict (or may conflict)
and act accordingly (not grant or recall).

Re: add a LRU for delegations

[Trond Myklebust]

On Fri, 2026-01-09 at 02:30 +0200, Sagi Grimberg wrote:
> 
> 
> On 08/01/2026 16:34, Chuck Lever wrote:
> > 
> > On Wed, Jan 7, 2026, at 2:22 PM, Sagi Grimberg wrote:
> > > And, in general, I think that the server is in a much better
> > > position
> > > to solicit preemptive delegation recalls as opposed
> > > to the client voluntarily return delegations when crossing a
> > > somewhat
> > > arbitrary delegation_watermark.
> > The server and client have orthogonal interests here, IMO.
> 
> They both share the interest of reducing rpc calls...
> 
> > 
> > The server is concerned with resource utilization -- memory
> > consumed,
> > slots in tables, and so on -- that other active clients might
> > benefit
> > from having freed. The server doesn't really care which delegations
> > are returned.
> > 
> > A client wants to keep delegation state that applications are
> > using,
> > and it knows best which ones those are. It can identify specific
> > delegations that are not being actively used and return those.
> 
> Yes, I agree to some extent. However arguably the client may want
> keep 
> delegation
> states around for as long as it can "just in case" the application
> opens 
> the file again (the client
> doesn't know the nature of the workload) assuming they fit in client 
> memory. It doesn't know
> anything about other clients nor the server resources.
> 
> The server on the other hand, has the knowledge of what delegations 
> conflict (or may conflict)
> and act accordingly (not grant or recall).

You can't just assume this: it really depends heavily on the workload.
Imagine an 'rm -rf' operation if you have to recall delegations from
various clients on all the files in the tree because they are caching
read delegations 'just in case'.

The current behaviour is based on the assumption that if your
application has not locally reopened the file for several minutes, then
that RPC call to reopen it on the server is by comparison not
particularly expensive.
Yes, there are some exceptions to that rule too, but by and large it is
a reasonable expectation, particularly so since the clients can all
still cache the file data for as long as the change attribute remains
the same. Any attempts to change the file would in any case trigger
delegation recalls, with delays to the writer while the server waits
for those recalls to complete.

IOW: Before agreeing to change the delegation expiration times, I'd
like to see actual data that justifies caching for longer as a general
rule, rather than just relying on peoples' assumptions that this is the
case.

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trondmy@kernel.org, trond.myklebust@hammerspace.com

Re: add a LRU for delegations

[Sagi Grimberg]

>> Yes, I agree to some extent. However arguably the client may want
>> keep
>> delegation
>> states around for as long as it can "just in case" the application
>> opens
>> the file again (the client
>> doesn't know the nature of the workload) assuming they fit in client
>> memory. It doesn't know
>> anything about other clients nor the server resources.
>>
>> The server on the other hand, has the knowledge of what delegations
>> conflict (or may conflict)
>> and act accordingly (not grant or recall).

Hey Trond,

> You can't just assume this: it really depends heavily on the workload.
> Imagine an 'rm -rf' operation if you have to recall delegations from
> various clients on all the files in the tree because they are caching
> read delegations 'just in case'.

I don't disagree that this can happen, but the scenario you are 
describing can happen regardless no?
Isn't that a concern of the server though? IMO the server should be the 
one that
can both track and account for other clients. Seems to me that giving up the
delegation just because some other client may modify the file is an 
assumption that the nfs client
takes without basing it on real data.

I'd think that the client should rely on the server to account cross 
clients state since
it has the awareness of what other clients are doing.

>
> The current behaviour is based on the assumption that if your
> application has not locally reopened the file for several minutes, then
> that RPC call to reopen it on the server is by comparison not
> particularly expensive.

That is reasonable, I agree. The use-case where I think this assumption does
not hold is when the client does not open the file, but rather stat it. 
Perhaps that case can
be mitigated if the client can add an opportunistic WANT_DELEG in the 
GETATTR compound
to ask for a read delegation. The server can always choose not to grant 
one...

> Yes, there are some exceptions to that rule too, but by and large it is
> a reasonable expectation, particularly so since the clients can all
> still cache the file data for as long as the change attribute remains
> the same. Any attempts to change the file would in any case trigger
> delegation recalls, with delays to the writer while the server waits
> for those recalls to complete.

Yes.

>
> IOW: Before agreeing to change the delegation expiration times, I'd
> like to see actual data that justifies caching for longer as a general
> rule, rather than just relying on peoples' assumptions that this is the
> case.

That makes perfect sense to me Trond.

Re: add a LRU for delegations

[Christoph Hellwig]

Any comments on this series except how to make it more complicated? :)

On Wed, Jan 07, 2026 at 08:26:51AM +0100, Christoph Hellwig wrote:
> 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.  Note that with this and also with the follow on patches the
> baselines will still crash in some tests, and this series does not fix
> that.
> 
> Changes since v1:
>  - fix the nfsv4.0 hang
> 
> Diffstat:
>  fs/nfs/callback_proc.c    |   13 -
>  fs/nfs/client.c           |    3 
>  fs/nfs/delegation.c       |  544 +++++++++++++++++++++++-----------------------
>  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, 342 insertions(+), 328 deletions(-)
---end quoted text---

Re: add a LRU for delegations

[Anna Schumaker]

Hi Christoph,

On 1/15/26 11:24 AM, Christoph Hellwig wrote:
> Any comments on this series except how to make it more complicated? :)

The patches look good to me. Thanks for doing this cleanup! I just pushed
them out to my linux-next branch for the next merge.

Anna

> 
> On Wed, Jan 07, 2026 at 08:26:51AM +0100, Christoph Hellwig wrote:
>> 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.  Note that with this and also with the follow on patches the
>> baselines will still crash in some tests, and this series does not fix
>> that.
>>
>> Changes since v1:
>>  - fix the nfsv4.0 hang
>>
>> Diffstat:
>>  fs/nfs/callback_proc.c    |   13 -
>>  fs/nfs/client.c           |    3 
>>  fs/nfs/delegation.c       |  544 +++++++++++++++++++++++-----------------------
>>  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, 342 insertions(+), 328 deletions(-)
> ---end quoted text---

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

[Chris Mason]

Christoph Hellwig <hch@lst.de> wrote:
> 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.

Hi everyone,

I ran some linux-next commits through AI review prmopts, and this one
was flagged.  The spinlock concern looks valid to me, but I didn't try to
confirm the RETURN_DELAYED comments.

> diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c
> --- a/fs/nfs/delegation.c
> +++ b/fs/nfs/delegation.c

[ ... ]

> @@ -674,15 +667,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;
>  }

Is there a missing spin_unlock() here? The spin_lock() is taken but
the function returns without releasing it. This appears to cause a
deadlock when delegations_lock is next acquired.

[ ... ]

> +static int nfs_return_one_delegation(struct nfs_server *server)
> +{
> +	struct nfs_delegation *delegation;
> +	struct inode *inode;
> +	int err = 0;
> +
> +	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);
> +
> +	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;
> +	}

When a delegation has RETURN_DELAYED set, the code removes it from
delegations_return (via list_del_init above) and then skips to
out_put_inode without re-adding it to the list. Since
nfs_server_clear_delayed_delegations() only iterates delegations_return,
will this delegation ever have its RETURN_DELAYED flag cleared so it
can be processed again?

The scenario is: nfs_end_delegation_return() returns -EAGAIN, which
sets RETURN_DELAYED via nfs_abort_delegation_return(), then
nfs_mark_return_delegation() re-adds the delegation to
delegations_return. On the next iteration of the loop, this check
triggers and the delegation is dropped from the list without being
re-queued.

> +	clear_bit(NFS_DELEGATION_RETURN_DELAYED, &delegation->flags);
> +	spin_unlock(&delegation->lock);

[ ... ]

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

[Christoph Hellwig]

On Sun, Jan 25, 2026 at 05:30:07AM -0800, Chris Mason wrote:
> > +	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;
> >  }
> 
> Is there a missing spin_unlock() here? The spin_lock() is taken but
> the function returns without releasing it. This appears to cause a
> deadlock when delegations_lock is next acquired.

Yes.  I'm kinda surprised the sparse lock context tracking didn't
catch it, but maybe it gives up after multiple 'bad constant expression'
from the module parameters.

> > +	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;
> > +	}
> 
> When a delegation has RETURN_DELAYED set, the code removes it from
> delegations_return (via list_del_init above) and then skips to
> out_put_inode without re-adding it to the list. Since
> nfs_server_clear_delayed_delegations() only iterates delegations_return,
> will this delegation ever have its RETURN_DELAYED flag cleared so it
> can be processed again?
> 
> The scenario is: nfs_end_delegation_return() returns -EAGAIN, which
> sets RETURN_DELAYED via nfs_abort_delegation_return(), then
> nfs_mark_return_delegation() re-adds the delegation to
> delegations_return. On the next iteration of the loop, this check
> triggers and the delegation is dropped from the list without being
> re-queued.

This looks like a mess before and after.  I think the best
idea is a separate list for the delayed returns, but I really
need to find a way to actually generate the delayed returns first.
I'll look into that, thanks!