use a hash for looking up delegation

use a hash for looking up delegation

[Christoph Hellwig]

Hi all,

currently recalling delegations has to walk the server->delegations
list, and then take the lock for each delegation.  This can take a lot of
time and has adverse effects to the rest of the system due to the number
atomic operations, cache lines touched and a long RCU critical section.

This series first converts the delegation watermark to be per-server, as
all the state guarded by it is per-server and the commit message adding
it talks about server side overhead as well, and then adds a very simple
hash for finding the delegation for a given file handle in
nfs_delegation_find_inode_server.

With this hash sample microbenchmarks that cause delegation recalls in
reverse list order are sped up ~5 percent, although the time is still
very variable due to other factors.

Diffstat:
 fs/nfs/client.c           |   24 +++++++++--
 fs/nfs/delegation.c       |   99 ++++++++++++++++++++++++++--------------------
 fs/nfs/delegation.h       |    3 +
 include/linux/nfs_fs_sb.h |    3 +
 4 files changed, 82 insertions(+), 47 deletions(-)

[PATCH 1/4] NFS: cleanup nfs_inode_reclaim_delegation

[Christoph Hellwig]

Reduce a level of indentation for most of the code in this function.

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

diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c
index 519bdc91112c..56bb2a7e1793 100644
--- a/fs/nfs/delegation.c
+++ b/fs/nfs/delegation.c
@@ -237,34 +237,34 @@ void nfs_inode_reclaim_delegation(struct inode *inode, const struct cred *cred,
 
 	rcu_read_lock();
 	delegation = rcu_dereference(NFS_I(inode)->delegation);
-	if (delegation != NULL) {
-		spin_lock(&delegation->lock);
-		nfs4_stateid_copy(&delegation->stateid, stateid);
-		delegation->type = type;
-		delegation->pagemod_limit = pagemod_limit;
-		oldcred = delegation->cred;
-		delegation->cred = get_cred(cred);
-		switch (deleg_type) {
-		case NFS4_OPEN_DELEGATE_READ_ATTRS_DELEG:
-		case NFS4_OPEN_DELEGATE_WRITE_ATTRS_DELEG:
-			set_bit(NFS_DELEGATION_DELEGTIME, &delegation->flags);
-			break;
-		default:
-			clear_bit(NFS_DELEGATION_DELEGTIME, &delegation->flags);
-		}
-		clear_bit(NFS_DELEGATION_NEED_RECLAIM, &delegation->flags);
-		if (test_and_clear_bit(NFS_DELEGATION_REVOKED,
-				       &delegation->flags))
-			atomic_long_inc(&nfs_active_delegations);
-		spin_unlock(&delegation->lock);
-		rcu_read_unlock();
-		put_cred(oldcred);
-		trace_nfs4_reclaim_delegation(inode, type);
-	} else {
+	if (!delegation) {
 		rcu_read_unlock();
 		nfs_inode_set_delegation(inode, cred, type, stateid,
 					 pagemod_limit, deleg_type);
+		return;
+	}
+
+	spin_lock(&delegation->lock);
+	nfs4_stateid_copy(&delegation->stateid, stateid);
+	delegation->type = type;
+	delegation->pagemod_limit = pagemod_limit;
+	oldcred = delegation->cred;
+	delegation->cred = get_cred(cred);
+	switch (deleg_type) {
+	case NFS4_OPEN_DELEGATE_READ_ATTRS_DELEG:
+	case NFS4_OPEN_DELEGATE_WRITE_ATTRS_DELEG:
+		set_bit(NFS_DELEGATION_DELEGTIME, &delegation->flags);
+		break;
+	default:
+		clear_bit(NFS_DELEGATION_DELEGTIME, &delegation->flags);
 	}
+	clear_bit(NFS_DELEGATION_NEED_RECLAIM, &delegation->flags);
+	if (test_and_clear_bit(NFS_DELEGATION_REVOKED, &delegation->flags))
+		atomic_long_inc(&nfs_active_delegations);
+	spin_unlock(&delegation->lock);
+	rcu_read_unlock();
+	put_cred(oldcred);
+	trace_nfs4_reclaim_delegation(inode, type);
 }
 
 static int nfs_do_return_delegation(struct inode *inode,
-- 
2.47.2

[PATCH 2/4] NFS: move the delegation_watermark module parameter

[Christoph Hellwig]

Keep the module_param_named next to the variable declaration instead of
somewhere unrelated, following the best practice in the rest of the
kernel.

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

diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c
index 56bb2a7e1793..d036796dbe69 100644
--- a/fs/nfs/delegation.c
+++ b/fs/nfs/delegation.c
@@ -29,6 +29,7 @@
 
 static atomic_long_t nfs_active_delegations;
 static unsigned nfs_delegation_watermark = NFS_DEFAULT_DELEGATION_WATERMARK;
+module_param_named(delegation_watermark, nfs_delegation_watermark, uint, 0644);
 
 static void __nfs_free_delegation(struct nfs_delegation *delegation)
 {
@@ -1575,5 +1576,3 @@ bool nfs4_delegation_flush_on_close(const struct inode *inode)
 	rcu_read_unlock();
 	return ret;
 }
-
-module_param_named(delegation_watermark, nfs_delegation_watermark, uint, 0644);
-- 
2.47.2

[PATCH 3/4] NFS: track active delegations per-server

[Christoph Hellwig]

The active delegation watermark was added to avoid overloading servers.
Track the active delegation per-server instead of globally so that clients
talking to multiple servers aren't limited by the global limit.

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

diff --git a/fs/nfs/client.c b/fs/nfs/client.c
index 1a55debab6e5..f55188928f67 100644
--- a/fs/nfs/client.c
+++ b/fs/nfs/client.c
@@ -1017,6 +1017,7 @@ struct nfs_server *nfs_alloc_server(void)
 	INIT_LIST_HEAD(&server->ss_src_copies);
 
 	atomic_set(&server->active, 0);
+	atomic_long_set(&server->nr_active_delegations, 0);
 
 	server->io_stats = nfs_alloc_iostats();
 	if (!server->io_stats) {
diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c
index d036796dbe69..621b635d1c56 100644
--- a/fs/nfs/delegation.c
+++ b/fs/nfs/delegation.c
@@ -27,7 +27,6 @@
 
 #define NFS_DEFAULT_DELEGATION_WATERMARK (15000U)
 
-static atomic_long_t nfs_active_delegations;
 static unsigned nfs_delegation_watermark = NFS_DEFAULT_DELEGATION_WATERMARK;
 module_param_named(delegation_watermark, nfs_delegation_watermark, uint, 0644);
 
@@ -38,11 +37,12 @@ static void __nfs_free_delegation(struct nfs_delegation *delegation)
 	kfree_rcu(delegation, rcu);
 }
 
-static void nfs_mark_delegation_revoked(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(&nfs_active_delegations);
+		atomic_long_dec(&server->nr_active_delegations);
 		if (!test_bit(NFS_DELEGATION_RETURNING, &delegation->flags))
 			nfs_clear_verifier_delegated(delegation->inode);
 	}
@@ -60,9 +60,10 @@ static void nfs_put_delegation(struct nfs_delegation *delegation)
 		__nfs_free_delegation(delegation);
 }
 
-static void nfs_free_delegation(struct nfs_delegation *delegation)
+static void nfs_free_delegation(struct nfs_server *server,
+		struct nfs_delegation *delegation)
 {
-	nfs_mark_delegation_revoked(delegation);
+	nfs_mark_delegation_revoked(server, delegation);
 	nfs_put_delegation(delegation);
 }
 
@@ -261,7 +262,7 @@ void nfs_inode_reclaim_delegation(struct inode *inode, const struct cred *cred,
 	}
 	clear_bit(NFS_DELEGATION_NEED_RECLAIM, &delegation->flags);
 	if (test_and_clear_bit(NFS_DELEGATION_REVOKED, &delegation->flags))
-		atomic_long_inc(&nfs_active_delegations);
+		atomic_long_inc(&NFS_SERVER(inode)->nr_active_delegations);
 	spin_unlock(&delegation->lock);
 	rcu_read_unlock();
 	put_cred(oldcred);
@@ -413,7 +414,8 @@ nfs_update_delegation_cred(struct nfs_delegation *delegation,
 }
 
 static void
-nfs_update_inplace_delegation(struct nfs_delegation *delegation,
+nfs_update_inplace_delegation(struct nfs_server *server,
+		struct nfs_delegation *delegation,
 		const struct nfs_delegation *update)
 {
 	if (nfs4_stateid_is_newer(&update->stateid, &delegation->stateid)) {
@@ -426,7 +428,7 @@ nfs_update_inplace_delegation(struct nfs_delegation *delegation,
 			nfs_update_delegation_cred(delegation, update->cred);
 			/* smp_mb__before_atomic() is implicit due to xchg() */
 			clear_bit(NFS_DELEGATION_REVOKED, &delegation->flags);
-			atomic_long_inc(&nfs_active_delegations);
+			atomic_long_inc(&server->nr_active_delegations);
 		}
 	}
 }
@@ -481,7 +483,7 @@ int nfs_inode_set_delegation(struct inode *inode, const struct cred *cred,
 	if (nfs4_stateid_match_other(&old_delegation->stateid,
 				&delegation->stateid)) {
 		spin_lock(&old_delegation->lock);
-		nfs_update_inplace_delegation(old_delegation,
+		nfs_update_inplace_delegation(server, old_delegation,
 				delegation);
 		spin_unlock(&old_delegation->lock);
 		goto out;
@@ -530,7 +532,7 @@ int nfs_inode_set_delegation(struct inode *inode, const struct cred *cred,
 	rcu_assign_pointer(nfsi->delegation, delegation);
 	delegation = NULL;
 
-	atomic_long_inc(&nfs_active_delegations);
+	atomic_long_inc(&server->nr_active_delegations);
 
 	trace_nfs4_set_delegation(inode, type);
 
@@ -544,7 +546,7 @@ 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(freeme);
+		nfs_free_delegation(server, freeme);
 	}
 	return status;
 }
@@ -756,7 +758,7 @@ void nfs_inode_evict_delegation(struct inode *inode)
 		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(delegation);
+		nfs_free_delegation(NFS_SERVER(inode), delegation);
 	}
 }
 
@@ -842,7 +844,8 @@ void nfs4_inode_return_delegation_on_close(struct inode *inode)
 	if (!delegation)
 		goto out;
 	if (test_bit(NFS_DELEGATION_RETURN_IF_CLOSED, &delegation->flags) ||
-	    atomic_long_read(&nfs_active_delegations) >= nfs_delegation_watermark) {
+	    atomic_long_read(&NFS_SERVER(inode)->nr_active_delegations) >=
+	    nfs_delegation_watermark) {
 		spin_lock(&delegation->lock);
 		if (delegation->inode &&
 		    list_empty(&NFS_I(inode)->open_files) &&
@@ -1018,7 +1021,7 @@ static void nfs_revoke_delegation(struct inode *inode,
 		}
 		spin_unlock(&delegation->lock);
 	}
-	nfs_mark_delegation_revoked(delegation);
+	nfs_mark_delegation_revoked(NFS_SERVER(inode), delegation);
 	ret = true;
 out:
 	rcu_read_unlock();
@@ -1050,7 +1053,7 @@ void nfs_delegation_mark_returned(struct inode *inode,
 			delegation->stateid.seqid = stateid->seqid;
 	}
 
-	nfs_mark_delegation_revoked(delegation);
+	nfs_mark_delegation_revoked(NFS_SERVER(inode), delegation);
 	clear_bit(NFS_DELEGATION_RETURNING, &delegation->flags);
 	spin_unlock(&delegation->lock);
 	if (nfs_detach_delegation(NFS_I(inode), delegation, NFS_SERVER(inode)))
@@ -1272,7 +1275,7 @@ static int nfs_server_reap_unclaimed_delegations(struct nfs_server *server,
 		if (delegation != NULL) {
 			if (nfs_detach_delegation(NFS_I(inode), delegation,
 						server) != NULL)
-				nfs_free_delegation(delegation);
+				nfs_free_delegation(server, delegation);
 			/* Match nfs_start_delegation_return_locked */
 			nfs_put_delegation(delegation);
 		}
diff --git a/include/linux/nfs_fs_sb.h b/include/linux/nfs_fs_sb.h
index 73bed04529a7..fe930d685780 100644
--- a/include/linux/nfs_fs_sb.h
+++ b/include/linux/nfs_fs_sb.h
@@ -255,6 +255,7 @@ struct nfs_server {
 	struct list_head	state_owners_lru;
 	struct list_head	layouts;
 	struct list_head	delegations;
+	atomic_long_t		nr_active_delegations;
 	struct list_head	ss_copies;
 	struct list_head	ss_src_copies;
 
-- 
2.47.2

[PATCH 4/4] NFS: use a hash table for delegation lookup

[Christoph Hellwig]

nfs_delegation_find_inode currently has to walk the entire list of
delegations per inode, which can become pretty large, and can become even
larger when increasing the delegation watermark.

Add a hash table to speed up the delegation lookup, sized as a fraction
of the delegation watermark.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/nfs/client.c           | 23 +++++++++++++++++++----
 fs/nfs/delegation.c       | 15 +++++++++++++--
 fs/nfs/delegation.h       |  3 +++
 include/linux/nfs_fs_sb.h |  2 ++
 4 files changed, 37 insertions(+), 6 deletions(-)

diff --git a/fs/nfs/client.c b/fs/nfs/client.c
index f55188928f67..94684a476dd8 100644
--- a/fs/nfs/client.c
+++ b/fs/nfs/client.c
@@ -994,6 +994,7 @@ static DEFINE_IDA(s_sysfs_ids);
 struct nfs_server *nfs_alloc_server(void)
 {
 	struct nfs_server *server;
+	int delegation_buckets, i;
 
 	server = kzalloc(sizeof(struct nfs_server), GFP_KERNEL);
 	if (!server)
@@ -1019,11 +1020,18 @@ struct nfs_server *nfs_alloc_server(void)
 	atomic_set(&server->active, 0);
 	atomic_long_set(&server->nr_active_delegations, 0);
 
+	delegation_buckets = roundup_pow_of_two(nfs_delegation_watermark / 16);
+	server->delegation_hash_mask = delegation_buckets - 1;
+	server->delegation_hash_table = kmalloc_array(delegation_buckets,
+			sizeof(*server->delegation_hash_table), GFP_KERNEL);
+	if (!server->delegation_hash_table)
+		goto out_free_server;
+	for (i = 0; i < delegation_buckets; i++)
+		INIT_HLIST_HEAD(&server->delegation_hash_table[i]);
+
 	server->io_stats = nfs_alloc_iostats();
-	if (!server->io_stats) {
-		kfree(server);
-		return NULL;
-	}
+	if (!server->io_stats)
+		goto out_free_delegation_hash;
 
 	server->change_attr_type = NFS4_CHANGE_TYPE_IS_UNDEFINED;
 
@@ -1036,6 +1044,12 @@ struct nfs_server *nfs_alloc_server(void)
 	rpc_init_wait_queue(&server->uoc_rpcwaitq, "NFS UOC");
 
 	return server;
+
+out_free_delegation_hash:
+	kfree(server->delegation_hash_table);
+out_free_server:
+	kfree(server);
+	return NULL;
 }
 EXPORT_SYMBOL_GPL(nfs_alloc_server);
 
@@ -1044,6 +1058,7 @@ static void delayed_free(struct rcu_head *p)
 	struct nfs_server *server = container_of(p, struct nfs_server, rcu);
 
 	nfs_free_iostats(server->io_stats);
+	kfree(server->delegation_hash_table);
 	kfree(server);
 }
 
diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c
index 621b635d1c56..ca830ceb466e 100644
--- a/fs/nfs/delegation.c
+++ b/fs/nfs/delegation.c
@@ -27,9 +27,16 @@
 
 #define NFS_DEFAULT_DELEGATION_WATERMARK (15000U)
 
-static unsigned nfs_delegation_watermark = NFS_DEFAULT_DELEGATION_WATERMARK;
+unsigned nfs_delegation_watermark = NFS_DEFAULT_DELEGATION_WATERMARK;
 module_param_named(delegation_watermark, nfs_delegation_watermark, uint, 0644);
 
+static struct hlist_head *nfs_delegation_hash(struct nfs_server *server,
+		const struct nfs_fh *fhandle)
+{
+	return server->delegation_hash_table +
+		(nfs_fhandle_hash(fhandle) & server->delegation_hash_mask);
+}
+
 static void __nfs_free_delegation(struct nfs_delegation *delegation)
 {
 	put_cred(delegation->cred);
@@ -367,6 +374,7 @@ nfs_detach_delegation_locked(struct nfs_inode *nfsi,
 		spin_unlock(&delegation->lock);
 		return NULL;
 	}
+	hlist_del_init_rcu(&delegation->hash);
 	list_del_rcu(&delegation->super_list);
 	delegation->inode = NULL;
 	rcu_assign_pointer(nfsi->delegation, NULL);
@@ -529,6 +537,8 @@ int nfs_inode_set_delegation(struct inode *inode, const struct cred *cred,
 	spin_unlock(&inode->i_lock);
 
 	list_add_tail_rcu(&delegation->super_list, &server->delegations);
+	hlist_add_head_rcu(&delegation->hash,
+			nfs_delegation_hash(server, &NFS_I(inode)->fh));
 	rcu_assign_pointer(nfsi->delegation, delegation);
 	delegation = NULL;
 
@@ -1166,11 +1176,12 @@ static struct inode *
 nfs_delegation_find_inode_server(struct nfs_server *server,
 				 const struct nfs_fh *fhandle)
 {
+	struct hlist_head *head = nfs_delegation_hash(server, fhandle);
 	struct nfs_delegation *delegation;
 	struct super_block *freeme = NULL;
 	struct inode *res = NULL;
 
-	list_for_each_entry_rcu(delegation, &server->delegations, super_list) {
+	hlist_for_each_entry_rcu(delegation, head, hash) {
 		spin_lock(&delegation->lock);
 		if (delegation->inode != NULL &&
 		    !test_bit(NFS_DELEGATION_REVOKED, &delegation->flags) &&
diff --git a/fs/nfs/delegation.h b/fs/nfs/delegation.h
index 8ff5ab9c5c25..9f1fb9b39c43 100644
--- a/fs/nfs/delegation.h
+++ b/fs/nfs/delegation.h
@@ -14,6 +14,7 @@
  * NFSv4 delegation
  */
 struct nfs_delegation {
+	struct hlist_node hash;
 	struct list_head super_list;
 	const struct cred *cred;
 	struct inode *inode;
@@ -123,4 +124,6 @@ static inline int nfs_have_delegated_mtime(struct inode *inode)
 						 NFS_DELEGATION_FLAG_TIME);
 }
 
+extern unsigned nfs_delegation_watermark;
+
 #endif
diff --git a/include/linux/nfs_fs_sb.h b/include/linux/nfs_fs_sb.h
index fe930d685780..88212306fd87 100644
--- a/include/linux/nfs_fs_sb.h
+++ b/include/linux/nfs_fs_sb.h
@@ -256,6 +256,8 @@ struct nfs_server {
 	struct list_head	layouts;
 	struct list_head	delegations;
 	atomic_long_t		nr_active_delegations;
+	unsigned int		delegation_hash_mask;
+	struct hlist_head	*delegation_hash_table;
 	struct list_head	ss_copies;
 	struct list_head	ss_src_copies;
 
-- 
2.47.2

Re: [PATCH 1/4] NFS: cleanup nfs_inode_reclaim_delegation

[Jeff Layton]

On Mon, 2025-07-14 at 13:16 +0200, Christoph Hellwig wrote:
> Reduce a level of indentation for most of the code in this function.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/nfs/delegation.c | 48 ++++++++++++++++++++++-----------------------
>  1 file changed, 24 insertions(+), 24 deletions(-)
> 
> diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c
> index 519bdc91112c..56bb2a7e1793 100644
> --- a/fs/nfs/delegation.c
> +++ b/fs/nfs/delegation.c
> @@ -237,34 +237,34 @@ void nfs_inode_reclaim_delegation(struct inode *inode, const struct cred *cred,
>  
>  	rcu_read_lock();
>  	delegation = rcu_dereference(NFS_I(inode)->delegation);
> -	if (delegation != NULL) {
> -		spin_lock(&delegation->lock);
> -		nfs4_stateid_copy(&delegation->stateid, stateid);
> -		delegation->type = type;
> -		delegation->pagemod_limit = pagemod_limit;
> -		oldcred = delegation->cred;
> -		delegation->cred = get_cred(cred);
> -		switch (deleg_type) {
> -		case NFS4_OPEN_DELEGATE_READ_ATTRS_DELEG:
> -		case NFS4_OPEN_DELEGATE_WRITE_ATTRS_DELEG:
> -			set_bit(NFS_DELEGATION_DELEGTIME, &delegation->flags);
> -			break;
> -		default:
> -			clear_bit(NFS_DELEGATION_DELEGTIME, &delegation->flags);
> -		}
> -		clear_bit(NFS_DELEGATION_NEED_RECLAIM, &delegation->flags);
> -		if (test_and_clear_bit(NFS_DELEGATION_REVOKED,
> -				       &delegation->flags))
> -			atomic_long_inc(&nfs_active_delegations);
> -		spin_unlock(&delegation->lock);
> -		rcu_read_unlock();
> -		put_cred(oldcred);
> -		trace_nfs4_reclaim_delegation(inode, type);
> -	} else {
> +	if (!delegation) {
>  		rcu_read_unlock();
>  		nfs_inode_set_delegation(inode, cred, type, stateid,
>  					 pagemod_limit, deleg_type);
> +		return;
> +	}
> +
> +	spin_lock(&delegation->lock);
> +	nfs4_stateid_copy(&delegation->stateid, stateid);
> +	delegation->type = type;
> +	delegation->pagemod_limit = pagemod_limit;
> +	oldcred = delegation->cred;
> +	delegation->cred = get_cred(cred);
> +	switch (deleg_type) {
> +	case NFS4_OPEN_DELEGATE_READ_ATTRS_DELEG:
> +	case NFS4_OPEN_DELEGATE_WRITE_ATTRS_DELEG:
> +		set_bit(NFS_DELEGATION_DELEGTIME, &delegation->flags);
> +		break;
> +	default:
> +		clear_bit(NFS_DELEGATION_DELEGTIME, &delegation->flags);
>  	}
> +	clear_bit(NFS_DELEGATION_NEED_RECLAIM, &delegation->flags);
> +	if (test_and_clear_bit(NFS_DELEGATION_REVOKED, &delegation->flags))
> +		atomic_long_inc(&nfs_active_delegations);
> +	spin_unlock(&delegation->lock);
> +	rcu_read_unlock();
> +	put_cred(oldcred);
> +	trace_nfs4_reclaim_delegation(inode, type);
>  }
>  
>  static int nfs_do_return_delegation(struct inode *inode,

Reviewed-by: Jeff Layton <jlayton@kernel.org>

Re: [PATCH 2/4] NFS: move the delegation_watermark module parameter

[Jeff Layton]

On Mon, 2025-07-14 at 13:16 +0200, Christoph Hellwig wrote:
> Keep the module_param_named next to the variable declaration instead of
> somewhere unrelated, following the best practice in the rest of the
> kernel.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/nfs/delegation.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c
> index 56bb2a7e1793..d036796dbe69 100644
> --- a/fs/nfs/delegation.c
> +++ b/fs/nfs/delegation.c
> @@ -29,6 +29,7 @@
>  
>  static atomic_long_t nfs_active_delegations;
>  static unsigned nfs_delegation_watermark = NFS_DEFAULT_DELEGATION_WATERMARK;
> +module_param_named(delegation_watermark, nfs_delegation_watermark, uint, 0644);
>  
>  static void __nfs_free_delegation(struct nfs_delegation *delegation)
>  {
> @@ -1575,5 +1576,3 @@ bool nfs4_delegation_flush_on_close(const struct inode *inode)
>  	rcu_read_unlock();
>  	return ret;
>  }
> -
> -module_param_named(delegation_watermark, nfs_delegation_watermark, uint, 0644);

Sure, but I'd just squash this into patch #3:

Reviewed-by: Jeff Layton <jlayton@kernel.org>

Re: [PATCH 3/4] NFS: track active delegations per-server

[Jeff Layton]

On Mon, 2025-07-14 at 13:16 +0200, Christoph Hellwig wrote:
> The active delegation watermark was added to avoid overloading servers.
> Track the active delegation per-server instead of globally so that clients
> talking to multiple servers aren't limited by the global limit.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/nfs/client.c           |  1 +
>  fs/nfs/delegation.c       | 35 +++++++++++++++++++----------------
>  include/linux/nfs_fs_sb.h |  1 +
>  3 files changed, 21 insertions(+), 16 deletions(-)
> 
> diff --git a/fs/nfs/client.c b/fs/nfs/client.c
> index 1a55debab6e5..f55188928f67 100644
> --- a/fs/nfs/client.c
> +++ b/fs/nfs/client.c
> @@ -1017,6 +1017,7 @@ struct nfs_server *nfs_alloc_server(void)
>  	INIT_LIST_HEAD(&server->ss_src_copies);
>  
>  	atomic_set(&server->active, 0);
> +	atomic_long_set(&server->nr_active_delegations, 0);
>  
>  	server->io_stats = nfs_alloc_iostats();
>  	if (!server->io_stats) {
> diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c
> index d036796dbe69..621b635d1c56 100644
> --- a/fs/nfs/delegation.c
> +++ b/fs/nfs/delegation.c
> @@ -27,7 +27,6 @@
>  
>  #define NFS_DEFAULT_DELEGATION_WATERMARK (15000U)
>  
> -static atomic_long_t nfs_active_delegations;
>  static unsigned nfs_delegation_watermark = NFS_DEFAULT_DELEGATION_WATERMARK;
>  module_param_named(delegation_watermark, nfs_delegation_watermark, uint, 0644);
>  
> @@ -38,11 +37,12 @@ static void __nfs_free_delegation(struct nfs_delegation *delegation)
>  	kfree_rcu(delegation, rcu);
>  }
>  
> -static void nfs_mark_delegation_revoked(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(&nfs_active_delegations);
> +		atomic_long_dec(&server->nr_active_delegations);
>  		if (!test_bit(NFS_DELEGATION_RETURNING, &delegation->flags))
>  			nfs_clear_verifier_delegated(delegation->inode);
>  	}
> @@ -60,9 +60,10 @@ static void nfs_put_delegation(struct nfs_delegation *delegation)
>  		__nfs_free_delegation(delegation);
>  }
>  
> -static void nfs_free_delegation(struct nfs_delegation *delegation)
> +static void nfs_free_delegation(struct nfs_server *server,
> +		struct nfs_delegation *delegation)
>  {
> -	nfs_mark_delegation_revoked(delegation);
> +	nfs_mark_delegation_revoked(server, delegation);
>  	nfs_put_delegation(delegation);
>  }
>  
> @@ -261,7 +262,7 @@ void nfs_inode_reclaim_delegation(struct inode *inode, const struct cred *cred,
>  	}
>  	clear_bit(NFS_DELEGATION_NEED_RECLAIM, &delegation->flags);
>  	if (test_and_clear_bit(NFS_DELEGATION_REVOKED, &delegation->flags))
> -		atomic_long_inc(&nfs_active_delegations);
> +		atomic_long_inc(&NFS_SERVER(inode)->nr_active_delegations);
>  	spin_unlock(&delegation->lock);
>  	rcu_read_unlock();
>  	put_cred(oldcred);
> @@ -413,7 +414,8 @@ nfs_update_delegation_cred(struct nfs_delegation *delegation,
>  }
>  
>  static void
> -nfs_update_inplace_delegation(struct nfs_delegation *delegation,
> +nfs_update_inplace_delegation(struct nfs_server *server,
> +		struct nfs_delegation *delegation,
>  		const struct nfs_delegation *update)
>  {
>  	if (nfs4_stateid_is_newer(&update->stateid, &delegation->stateid)) {
> @@ -426,7 +428,7 @@ nfs_update_inplace_delegation(struct nfs_delegation *delegation,
>  			nfs_update_delegation_cred(delegation, update->cred);
>  			/* smp_mb__before_atomic() is implicit due to xchg() */
>  			clear_bit(NFS_DELEGATION_REVOKED, &delegation->flags);
> -			atomic_long_inc(&nfs_active_delegations);
> +			atomic_long_inc(&server->nr_active_delegations);
>  		}
>  	}
>  }
> @@ -481,7 +483,7 @@ int nfs_inode_set_delegation(struct inode *inode, const struct cred *cred,
>  	if (nfs4_stateid_match_other(&old_delegation->stateid,
>  				&delegation->stateid)) {
>  		spin_lock(&old_delegation->lock);
> -		nfs_update_inplace_delegation(old_delegation,
> +		nfs_update_inplace_delegation(server, old_delegation,
>  				delegation);
>  		spin_unlock(&old_delegation->lock);
>  		goto out;
> @@ -530,7 +532,7 @@ int nfs_inode_set_delegation(struct inode *inode, const struct cred *cred,
>  	rcu_assign_pointer(nfsi->delegation, delegation);
>  	delegation = NULL;
>  
> -	atomic_long_inc(&nfs_active_delegations);
> +	atomic_long_inc(&server->nr_active_delegations);
>  
>  	trace_nfs4_set_delegation(inode, type);
>  
> @@ -544,7 +546,7 @@ 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(freeme);
> +		nfs_free_delegation(server, freeme);
>  	}
>  	return status;
>  }
> @@ -756,7 +758,7 @@ void nfs_inode_evict_delegation(struct inode *inode)
>  		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(delegation);
> +		nfs_free_delegation(NFS_SERVER(inode), delegation);
>  	}
>  }
>  
> @@ -842,7 +844,8 @@ void nfs4_inode_return_delegation_on_close(struct inode *inode)
>  	if (!delegation)
>  		goto out;
>  	if (test_bit(NFS_DELEGATION_RETURN_IF_CLOSED, &delegation->flags) ||
> -	    atomic_long_read(&nfs_active_delegations) >= nfs_delegation_watermark) {
> +	    atomic_long_read(&NFS_SERVER(inode)->nr_active_delegations) >=
> +	    nfs_delegation_watermark) {
>  		spin_lock(&delegation->lock);
>  		if (delegation->inode &&
>  		    list_empty(&NFS_I(inode)->open_files) &&
> @@ -1018,7 +1021,7 @@ static void nfs_revoke_delegation(struct inode *inode,
>  		}
>  		spin_unlock(&delegation->lock);
>  	}
> -	nfs_mark_delegation_revoked(delegation);
> +	nfs_mark_delegation_revoked(NFS_SERVER(inode), delegation);
>  	ret = true;
>  out:
>  	rcu_read_unlock();
> @@ -1050,7 +1053,7 @@ void nfs_delegation_mark_returned(struct inode *inode,
>  			delegation->stateid.seqid = stateid->seqid;
>  	}
>  
> -	nfs_mark_delegation_revoked(delegation);
> +	nfs_mark_delegation_revoked(NFS_SERVER(inode), delegation);
>  	clear_bit(NFS_DELEGATION_RETURNING, &delegation->flags);
>  	spin_unlock(&delegation->lock);
>  	if (nfs_detach_delegation(NFS_I(inode), delegation, NFS_SERVER(inode)))
> @@ -1272,7 +1275,7 @@ static int nfs_server_reap_unclaimed_delegations(struct nfs_server *server,
>  		if (delegation != NULL) {
>  			if (nfs_detach_delegation(NFS_I(inode), delegation,
>  						server) != NULL)
> -				nfs_free_delegation(delegation);
> +				nfs_free_delegation(server, delegation);
>  			/* Match nfs_start_delegation_return_locked */
>  			nfs_put_delegation(delegation);
>  		}
> diff --git a/include/linux/nfs_fs_sb.h b/include/linux/nfs_fs_sb.h
> index 73bed04529a7..fe930d685780 100644
> --- a/include/linux/nfs_fs_sb.h
> +++ b/include/linux/nfs_fs_sb.h
> @@ -255,6 +255,7 @@ struct nfs_server {
>  	struct list_head	state_owners_lru;
>  	struct list_head	layouts;
>  	struct list_head	delegations;
> +	atomic_long_t		nr_active_delegations;
>  	struct list_head	ss_copies;
>  	struct list_head	ss_src_copies;
>  

Good idea. I like this.

Reviewed-by: Jeff Layton <jlayton@kernel.org>

Re: [PATCH 4/4] NFS: use a hash table for delegation lookup

[Jeff Layton]

On Mon, 2025-07-14 at 13:16 +0200, Christoph Hellwig wrote:
> nfs_delegation_find_inode currently has to walk the entire list of
> delegations per inode, which can become pretty large, and can become even
> larger when increasing the delegation watermark.
> 
> Add a hash table to speed up the delegation lookup, sized as a fraction
> of the delegation watermark.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/nfs/client.c           | 23 +++++++++++++++++++----
>  fs/nfs/delegation.c       | 15 +++++++++++++--
>  fs/nfs/delegation.h       |  3 +++
>  include/linux/nfs_fs_sb.h |  2 ++
>  4 files changed, 37 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/nfs/client.c b/fs/nfs/client.c
> index f55188928f67..94684a476dd8 100644
> --- a/fs/nfs/client.c
> +++ b/fs/nfs/client.c
> @@ -994,6 +994,7 @@ static DEFINE_IDA(s_sysfs_ids);
>  struct nfs_server *nfs_alloc_server(void)
>  {
>  	struct nfs_server *server;
> +	int delegation_buckets, i;
>  
>  	server = kzalloc(sizeof(struct nfs_server), GFP_KERNEL);
>  	if (!server)
> @@ -1019,11 +1020,18 @@ struct nfs_server *nfs_alloc_server(void)
>  	atomic_set(&server->active, 0);
>  	atomic_long_set(&server->nr_active_delegations, 0);
>  
> +	delegation_buckets = roundup_pow_of_two(nfs_delegation_watermark / 16);
> +	server->delegation_hash_mask = delegation_buckets - 1;
> +	server->delegation_hash_table = kmalloc_array(delegation_buckets,
> +			sizeof(*server->delegation_hash_table), GFP_KERNEL);
> +	if (!server->delegation_hash_table)
> +		goto out_free_server;
> +	for (i = 0; i < delegation_buckets; i++)
> +		INIT_HLIST_HEAD(&server->delegation_hash_table[i]);
> +

This is going to get created for any mount, even v3 ones. It might be
better to only bother with this for v4 mounts. Maybe do this in
nfs4_server_common_setup() instead?

Also, I wonder if you'd be better off using the rhashtable
infrastructure instead of adding a fixed-size one? The number of
delegations in flight is very workload-dependent, so a resizeable
hashtable may be a better option.

>  	server->io_stats = nfs_alloc_iostats();
> -	if (!server->io_stats) {
> -		kfree(server);
> -		return NULL;
> -	}
> +	if (!server->io_stats)
> +		goto out_free_delegation_hash;
>  
>  	server->change_attr_type = NFS4_CHANGE_TYPE_IS_UNDEFINED;
>  
> @@ -1036,6 +1044,12 @@ struct nfs_server *nfs_alloc_server(void)
>  	rpc_init_wait_queue(&server->uoc_rpcwaitq, "NFS UOC");
>  
>  	return server;
> +
> +out_free_delegation_hash:
> +	kfree(server->delegation_hash_table);
> +out_free_server:
> +	kfree(server);
> +	return NULL;
>  }
>  EXPORT_SYMBOL_GPL(nfs_alloc_server);
>  
> @@ -1044,6 +1058,7 @@ static void delayed_free(struct rcu_head *p)
>  	struct nfs_server *server = container_of(p, struct nfs_server, rcu);
>  
>  	nfs_free_iostats(server->io_stats);
> +	kfree(server->delegation_hash_table);
>  	kfree(server);
>  }
>  
> diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c
> index 621b635d1c56..ca830ceb466e 100644
> --- a/fs/nfs/delegation.c
> +++ b/fs/nfs/delegation.c
> @@ -27,9 +27,16 @@
>  
>  #define NFS_DEFAULT_DELEGATION_WATERMARK (15000U)
>  
> -static unsigned nfs_delegation_watermark = NFS_DEFAULT_DELEGATION_WATERMARK;
> +unsigned nfs_delegation_watermark = NFS_DEFAULT_DELEGATION_WATERMARK;
>  module_param_named(delegation_watermark, nfs_delegation_watermark, uint, 0644);
>  
> +static struct hlist_head *nfs_delegation_hash(struct nfs_server *server,
> +		const struct nfs_fh *fhandle)
> +{
> +	return server->delegation_hash_table +
> +		(nfs_fhandle_hash(fhandle) & server->delegation_hash_mask);
> +}
> +
>  static void __nfs_free_delegation(struct nfs_delegation *delegation)
>  {
>  	put_cred(delegation->cred);
> @@ -367,6 +374,7 @@ nfs_detach_delegation_locked(struct nfs_inode *nfsi,
>  		spin_unlock(&delegation->lock);
>  		return NULL;
>  	}
> +	hlist_del_init_rcu(&delegation->hash);
>  	list_del_rcu(&delegation->super_list);
>  	delegation->inode = NULL;
>  	rcu_assign_pointer(nfsi->delegation, NULL);
> @@ -529,6 +537,8 @@ int nfs_inode_set_delegation(struct inode *inode, const struct cred *cred,
>  	spin_unlock(&inode->i_lock);
>  
>  	list_add_tail_rcu(&delegation->super_list, &server->delegations);
> +	hlist_add_head_rcu(&delegation->hash,
> +			nfs_delegation_hash(server, &NFS_I(inode)->fh));
>  	rcu_assign_pointer(nfsi->delegation, delegation);
>  	delegation = NULL;
>  
> @@ -1166,11 +1176,12 @@ static struct inode *
>  nfs_delegation_find_inode_server(struct nfs_server *server,
>  				 const struct nfs_fh *fhandle)
>  {
> +	struct hlist_head *head = nfs_delegation_hash(server, fhandle);
>  	struct nfs_delegation *delegation;
>  	struct super_block *freeme = NULL;
>  	struct inode *res = NULL;
>  
> -	list_for_each_entry_rcu(delegation, &server->delegations, super_list) {
> +	hlist_for_each_entry_rcu(delegation, head, hash) {
>  		spin_lock(&delegation->lock);
>  		if (delegation->inode != NULL &&
>  		    !test_bit(NFS_DELEGATION_REVOKED, &delegation->flags) &&
> diff --git a/fs/nfs/delegation.h b/fs/nfs/delegation.h
> index 8ff5ab9c5c25..9f1fb9b39c43 100644
> --- a/fs/nfs/delegation.h
> +++ b/fs/nfs/delegation.h
> @@ -14,6 +14,7 @@
>   * NFSv4 delegation
>   */
>  struct nfs_delegation {
> +	struct hlist_node hash;
>  	struct list_head super_list;
>  	const struct cred *cred;
>  	struct inode *inode;
> @@ -123,4 +124,6 @@ static inline int nfs_have_delegated_mtime(struct inode *inode)
>  						 NFS_DELEGATION_FLAG_TIME);
>  }
>  
> +extern unsigned nfs_delegation_watermark;
> +
>  #endif
> diff --git a/include/linux/nfs_fs_sb.h b/include/linux/nfs_fs_sb.h
> index fe930d685780..88212306fd87 100644
> --- a/include/linux/nfs_fs_sb.h
> +++ b/include/linux/nfs_fs_sb.h
> @@ -256,6 +256,8 @@ struct nfs_server {
>  	struct list_head	layouts;
>  	struct list_head	delegations;
>  	atomic_long_t		nr_active_delegations;
> +	unsigned int		delegation_hash_mask;
> +	struct hlist_head	*delegation_hash_table;
>  	struct list_head	ss_copies;
>  	struct list_head	ss_src_copies;
>  

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH 4/4] NFS: use a hash table for delegation lookup

[Christoph Hellwig]

On Mon, Jul 14, 2025 at 09:14:27AM -0400, Jeff Layton wrote:
> > +	delegation_buckets = roundup_pow_of_two(nfs_delegation_watermark / 16);
> > +	server->delegation_hash_mask = delegation_buckets - 1;
> > +	server->delegation_hash_table = kmalloc_array(delegation_buckets,
> > +			sizeof(*server->delegation_hash_table), GFP_KERNEL);
> > +	if (!server->delegation_hash_table)
> > +		goto out_free_server;
> > +	for (i = 0; i < delegation_buckets; i++)
> > +		INIT_HLIST_HEAD(&server->delegation_hash_table[i]);
> > +
> 
> This is going to get created for any mount, even v3 ones. It might be
> better to only bother with this for v4 mounts. Maybe do this in
> nfs4_server_common_setup() instead?

Yeah, good idea.

> Also, I wonder if you'd be better off using the rhashtable
> infrastructure instead of adding a fixed-size one? The number of
> delegations in flight is very workload-dependent, so a resizeable
> hashtable may be a better option.

I did try that first.  But the rhashtable expects the hash key to
be embedded into the structure that has the rhash_head embedded into
it, while delegations use the file handle embeded into the inode as
the key.  So we'd have to bloat the deleations with a key copy for
that.

Re: [PATCH 2/4] NFS: move the delegation_watermark module parameter

[Christoph Hellwig]

On Mon, Jul 14, 2025 at 09:06:42AM -0400, Jeff Layton wrote:
> > -
> > -module_param_named(delegation_watermark, nfs_delegation_watermark, uint, 0644);
> 
> Sure, but I'd just squash this into patch #3:

Why?  It's completely unrelated to the changes there.

Re: [PATCH 4/4] NFS: use a hash table for delegation lookup

[Christoph Hellwig]

On Mon, Jul 14, 2025 at 09:14:27AM -0400, Jeff Layton wrote:
> > +	delegation_buckets = roundup_pow_of_two(nfs_delegation_watermark / 16);
> > +	server->delegation_hash_mask = delegation_buckets - 1;
> > +	server->delegation_hash_table = kmalloc_array(delegation_buckets,
> > +			sizeof(*server->delegation_hash_table), GFP_KERNEL);
> > +	if (!server->delegation_hash_table)
> > +		goto out_free_server;
> > +	for (i = 0; i < delegation_buckets; i++)
> > +		INIT_HLIST_HEAD(&server->delegation_hash_table[i]);
> > +
> 
> This is going to get created for any mount, even v3 ones. It might be
> better to only bother with this for v4 mounts. Maybe do this in
> nfs4_server_common_setup() instead?

I tried that, but it crashes because the usual mount process goes
through nfs_clone_server, which then doesn't set up the hash table.

I think the best idea is to pass the version to nfs_allocate_server
and just make this code conditional, but I'm open to other suggestions.

Re: [PATCH 2/4] NFS: move the delegation_watermark module parameter

[Jeff Layton]

On Tue, 2025-07-15 at 10:03 +0200, Christoph Hellwig wrote:
> On Mon, Jul 14, 2025 at 09:06:42AM -0400, Jeff Layton wrote:
> > > -
> > > -module_param_named(delegation_watermark, nfs_delegation_watermark, uint, 0644);
> > 
> > Sure, but I'd just squash this into patch #3:
> 
> Why?  It's completely unrelated to the changes there.

It's a trivial change and patch #3 makes changes in the same area. It's
not a big deal though.
-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH 4/4] NFS: use a hash table for delegation lookup

[Jeff Layton]

On Tue, 2025-07-15 at 10:58 +0200, Christoph Hellwig wrote:
> On Mon, Jul 14, 2025 at 09:14:27AM -0400, Jeff Layton wrote:
> > > +	delegation_buckets = roundup_pow_of_two(nfs_delegation_watermark / 16);
> > > +	server->delegation_hash_mask = delegation_buckets - 1;
> > > +	server->delegation_hash_table = kmalloc_array(delegation_buckets,
> > > +			sizeof(*server->delegation_hash_table), GFP_KERNEL);
> > > +	if (!server->delegation_hash_table)
> > > +		goto out_free_server;
> > > +	for (i = 0; i < delegation_buckets; i++)
> > > +		INIT_HLIST_HEAD(&server->delegation_hash_table[i]);
> > > +
> > 
> > This is going to get created for any mount, even v3 ones. It might be
> > better to only bother with this for v4 mounts. Maybe do this in
> > nfs4_server_common_setup() instead?
> 
> I tried that, but it crashes because the usual mount process goes
> through nfs_clone_server, which then doesn't set up the hash table.
> 
> I think the best idea is to pass the version to nfs_allocate_server
> and just make this code conditional, but I'm open to other suggestions.

The other thing you could do is move the allocation and setup into a
helper function, and have nfs_clone_server() call that if the source
server has a hashtable present.

-- 
Jeff Layton <jlayton@kernel.org>

[PATCH 4/4] NFS: use a hash table for delegation lookup

[Christoph Hellwig]

nfs_delegation_find_inode currently has to walk the entire list of
delegations per inode, which can become pretty large, and can become even
larger when increasing the delegation watermark.

Add a hash table to speed up the delegation lookup, sized as a fraction
of the delegation watermark.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/nfs/client.c           | 14 +++++++++++---
 fs/nfs/delegation.c       | 28 +++++++++++++++++++++++++++-
 fs/nfs/delegation.h       |  3 +++
 fs/nfs/nfs4client.c       | 10 +++++++++-
 include/linux/nfs_fs_sb.h |  2 ++
 5 files changed, 52 insertions(+), 5 deletions(-)

diff --git a/fs/nfs/client.c b/fs/nfs/client.c
index e13eb429b8b5..5e1a5263e326 100644
--- a/fs/nfs/client.c
+++ b/fs/nfs/client.c
@@ -1158,6 +1158,12 @@ struct nfs_server *nfs_clone_server(struct nfs_server *source,
 
 	server->fsid = fattr->fsid;
 
+	if (IS_ENABLED(CONFIG_NFS_V4) && server->delegation_hash_table) {
+		error = nfs4_delegation_hash_alloc(server);
+		if (error)
+			goto out_free_server;
+	}
+
 	nfs_sysfs_add_server(server);
 
 	nfs_sysfs_link_rpc_client(server,
@@ -1167,25 +1173,27 @@ struct nfs_server *nfs_clone_server(struct nfs_server *source,
 			source->client->cl_timeout,
 			flavor);
 	if (error < 0)
-		goto out_free_server;
+		goto out_free_delegation_hash;
 
 	/* probe the filesystem info for this server filesystem */
 	error = nfs_probe_server(server, fh);
 	if (error < 0)
-		goto out_free_server;
+		goto out_free_delegation_hash;
 
 	if (server->namelen == 0 || server->namelen > NFS4_MAXNAMLEN)
 		server->namelen = NFS4_MAXNAMLEN;
 
 	error = nfs_start_lockd(server);
 	if (error < 0)
-		goto out_free_server;
+		goto out_free_delegation_hash;
 
 	nfs_server_insert_lists(server);
 	server->mount_time = jiffies;
 
 	return server;
 
+out_free_delegation_hash:
+	kfree(server->delegation_hash_table);
 out_free_server:
 	nfs_free_server(server);
 	return ERR_PTR(error);
diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c
index ea96f77e38c2..9d3a5f29f17f 100644
--- a/fs/nfs/delegation.c
+++ b/fs/nfs/delegation.c
@@ -30,6 +30,13 @@
 static unsigned nfs_delegation_watermark = NFS_DEFAULT_DELEGATION_WATERMARK;
 module_param_named(delegation_watermark, nfs_delegation_watermark, uint, 0644);
 
+static struct hlist_head *nfs_delegation_hash(struct nfs_server *server,
+		const struct nfs_fh *fhandle)
+{
+	return server->delegation_hash_table +
+		(nfs_fhandle_hash(fhandle) & server->delegation_hash_mask);
+}
+
 static void __nfs_free_delegation(struct nfs_delegation *delegation)
 {
 	put_cred(delegation->cred);
@@ -367,6 +374,7 @@ nfs_detach_delegation_locked(struct nfs_inode *nfsi,
 		spin_unlock(&delegation->lock);
 		return NULL;
 	}
+	hlist_del_init_rcu(&delegation->hash);
 	list_del_rcu(&delegation->super_list);
 	delegation->inode = NULL;
 	rcu_assign_pointer(nfsi->delegation, NULL);
@@ -529,6 +537,8 @@ int nfs_inode_set_delegation(struct inode *inode, const struct cred *cred,
 	spin_unlock(&inode->i_lock);
 
 	list_add_tail_rcu(&delegation->super_list, &server->delegations);
+	hlist_add_head_rcu(&delegation->hash,
+			nfs_delegation_hash(server, &NFS_I(inode)->fh));
 	rcu_assign_pointer(nfsi->delegation, delegation);
 	delegation = NULL;
 
@@ -1166,11 +1176,12 @@ static struct inode *
 nfs_delegation_find_inode_server(struct nfs_server *server,
 				 const struct nfs_fh *fhandle)
 {
+	struct hlist_head *head = nfs_delegation_hash(server, fhandle);
 	struct nfs_delegation *delegation;
 	struct super_block *freeme = NULL;
 	struct inode *res = NULL;
 
-	list_for_each_entry_rcu(delegation, &server->delegations, super_list) {
+	hlist_for_each_entry_rcu(delegation, head, hash) {
 		spin_lock(&delegation->lock);
 		if (delegation->inode != NULL &&
 		    !test_bit(NFS_DELEGATION_REVOKED, &delegation->flags) &&
@@ -1577,3 +1588,18 @@ bool nfs4_delegation_flush_on_close(const struct inode *inode)
 	rcu_read_unlock();
 	return ret;
 }
+
+int nfs4_delegation_hash_alloc(struct nfs_server *server)
+{
+	int delegation_buckets, i;
+
+	delegation_buckets = roundup_pow_of_two(nfs_delegation_watermark / 16);
+	server->delegation_hash_mask = delegation_buckets - 1;
+	server->delegation_hash_table = kmalloc_array(delegation_buckets,
+			sizeof(*server->delegation_hash_table), GFP_KERNEL);
+	if (!server->delegation_hash_table)
+		return -ENOMEM;
+	for (i = 0; i < delegation_buckets; i++)
+		INIT_HLIST_HEAD(&server->delegation_hash_table[i]);
+	return 0;
+}
diff --git a/fs/nfs/delegation.h b/fs/nfs/delegation.h
index 8ff5ab9c5c25..08ec2e9c68a4 100644
--- a/fs/nfs/delegation.h
+++ b/fs/nfs/delegation.h
@@ -14,6 +14,7 @@
  * NFSv4 delegation
  */
 struct nfs_delegation {
+	struct hlist_node hash;
 	struct list_head super_list;
 	const struct cred *cred;
 	struct inode *inode;
@@ -123,4 +124,6 @@ static inline int nfs_have_delegated_mtime(struct inode *inode)
 						 NFS_DELEGATION_FLAG_TIME);
 }
 
+int nfs4_delegation_hash_alloc(struct nfs_server *server);
+
 #endif
diff --git a/fs/nfs/nfs4client.c b/fs/nfs/nfs4client.c
index 2e623da1a787..31dc0d40209a 100644
--- a/fs/nfs/nfs4client.c
+++ b/fs/nfs/nfs4client.c
@@ -802,6 +802,7 @@ static void nfs4_destroy_server(struct nfs_server *server)
 	unset_pnfs_layoutdriver(server);
 	nfs4_purge_state_owners(server, &freeme);
 	nfs4_free_state_owners(&freeme);
+	kfree(server->delegation_hash_table);
 }
 
 /*
@@ -1096,9 +1097,14 @@ static int nfs4_server_common_setup(struct nfs_server *server,
 {
 	int error;
 
+	error = nfs4_delegation_hash_alloc(server);
+	if (error)
+		return error;
+
 	/* data servers support only a subset of NFSv4.1 */
+	error = -EPROTONOSUPPORT;
 	if (is_ds_only_client(server->nfs_client))
-		return -EPROTONOSUPPORT;
+		goto out;
 
 	/* We must ensure the session is initialised first */
 	error = nfs4_init_session(server->nfs_client);
@@ -1130,7 +1136,9 @@ static int nfs4_server_common_setup(struct nfs_server *server,
 	nfs_server_insert_lists(server);
 	server->mount_time = jiffies;
 	server->destroy = nfs4_destroy_server;
+	return 0;
 out:
+	kfree(server->delegation_hash_table);
 	return error;
 }
 
diff --git a/include/linux/nfs_fs_sb.h b/include/linux/nfs_fs_sb.h
index a9b44f12623f..d30c0245031c 100644
--- a/include/linux/nfs_fs_sb.h
+++ b/include/linux/nfs_fs_sb.h
@@ -255,6 +255,8 @@ struct nfs_server {
 	struct list_head	layouts;
 	struct list_head	delegations;
 	atomic_long_t		nr_active_delegations;
+	unsigned int		delegation_hash_mask;
+	struct hlist_head	*delegation_hash_table;
 	struct list_head	ss_copies;
 	struct list_head	ss_src_copies;
 
-- 
2.47.2

Re: [PATCH 4/4] NFS: use a hash table for delegation lookup

[kernel test robot]

Hi Christoph,

kernel test robot noticed the following build errors:

[auto build test ERROR on trondmy-nfs/linux-next]
[also build test ERROR on linus/master v6.16-rc6 next-20250716]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Christoph-Hellwig/NFS-move-the-delegation_watermark-module-parameter/20250716-222708
base:   git://git.linux-nfs.org/projects/trondmy/linux-nfs.git linux-next
patch link:    https://lore.kernel.org/r/20250716132657.2167548-5-hch%40lst.de
patch subject: [PATCH 4/4] NFS: use a hash table for delegation lookup
config: i386-randconfig-013-20250717 (https://download.01.org/0day-ci/archive/20250717/202507171246.w67NRPWN-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14+deb12u1) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250717/202507171246.w67NRPWN-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202507171246.w67NRPWN-lkp@intel.com/

All errors (new ones prefixed by >>):

   ld: fs/nfs/client.o: in function `nfs_clone_server':
>> fs/nfs/client.c:1162: undefined reference to `nfs4_delegation_hash_alloc'


vim +1162 fs/nfs/client.c

  1135	
  1136	/*
  1137	 * Clone an NFS2, NFS3 or NFS4 server record
  1138	 */
  1139	struct nfs_server *nfs_clone_server(struct nfs_server *source,
  1140					    struct nfs_fh *fh,
  1141					    struct nfs_fattr *fattr,
  1142					    rpc_authflavor_t flavor)
  1143	{
  1144		struct nfs_server *server;
  1145		int error;
  1146	
  1147		server = nfs_alloc_server();
  1148		if (!server)
  1149			return ERR_PTR(-ENOMEM);
  1150	
  1151		server->cred = get_cred(source->cred);
  1152	
  1153		/* Copy data from the source */
  1154		server->nfs_client = source->nfs_client;
  1155		server->destroy = source->destroy;
  1156		refcount_inc(&server->nfs_client->cl_count);
  1157		nfs_server_copy_userdata(server, source);
  1158	
  1159		server->fsid = fattr->fsid;
  1160	
  1161		if (IS_ENABLED(CONFIG_NFS_V4) && server->delegation_hash_table) {
> 1162			error = nfs4_delegation_hash_alloc(server);
  1163			if (error)
  1164				goto out_free_server;
  1165		}
  1166	
  1167		nfs_sysfs_add_server(server);
  1168	
  1169		nfs_sysfs_link_rpc_client(server,
  1170			server->nfs_client->cl_rpcclient, "_state");
  1171	
  1172		error = nfs_init_server_rpcclient(server,
  1173				source->client->cl_timeout,
  1174				flavor);
  1175		if (error < 0)
  1176			goto out_free_delegation_hash;
  1177	
  1178		/* probe the filesystem info for this server filesystem */
  1179		error = nfs_probe_server(server, fh);
  1180		if (error < 0)
  1181			goto out_free_delegation_hash;
  1182	
  1183		if (server->namelen == 0 || server->namelen > NFS4_MAXNAMLEN)
  1184			server->namelen = NFS4_MAXNAMLEN;
  1185	
  1186		error = nfs_start_lockd(server);
  1187		if (error < 0)
  1188			goto out_free_delegation_hash;
  1189	
  1190		nfs_server_insert_lists(server);
  1191		server->mount_time = jiffies;
  1192	
  1193		return server;
  1194	
  1195	out_free_delegation_hash:
  1196		kfree(server->delegation_hash_table);
  1197	out_free_server:
  1198		nfs_free_server(server);
  1199		return ERR_PTR(error);
  1200	}
  1201	EXPORT_SYMBOL_GPL(nfs_clone_server);
  1202	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 4/4] NFS: use a hash table for delegation lookup

[Christoph Hellwig]

So nfs4 can be built modular, and delegation.o is built into that,
i.e. we can't call into deletation code from clone_server.

Back to passing a major number to nfs_alloc_server?

On Thu, Jul 17, 2025 at 12:35:07PM +0800, kernel test robot wrote:
> Hi Christoph,
> 
> kernel test robot noticed the following build errors:
> 
> [auto build test ERROR on trondmy-nfs/linux-next]
> [also build test ERROR on linus/master v6.16-rc6 next-20250716]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
> 
> url:    https://github.com/intel-lab-lkp/linux/commits/Christoph-Hellwig/NFS-move-the-delegation_watermark-module-parameter/20250716-222708
> base:   git://git.linux-nfs.org/projects/trondmy/linux-nfs.git linux-next
> patch link:    https://lore.kernel.org/r/20250716132657.2167548-5-hch%40lst.de
> patch subject: [PATCH 4/4] NFS: use a hash table for delegation lookup
> config: i386-randconfig-013-20250717 (https://download.01.org/0day-ci/archive/20250717/202507171246.w67NRPWN-lkp@intel.com/config)
> compiler: gcc-12 (Debian 12.2.0-14+deb12u1) 12.2.0
> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250717/202507171246.w67NRPWN-lkp@intel.com/reproduce)
> 
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202507171246.w67NRPWN-lkp@intel.com/
> 
> All errors (new ones prefixed by >>):
> 
>    ld: fs/nfs/client.o: in function `nfs_clone_server':
> >> fs/nfs/client.c:1162: undefined reference to `nfs4_delegation_hash_alloc'
> 
> 
> vim +1162 fs/nfs/client.c
> 
>   1135	
>   1136	/*
>   1137	 * Clone an NFS2, NFS3 or NFS4 server record
>   1138	 */
>   1139	struct nfs_server *nfs_clone_server(struct nfs_server *source,
>   1140					    struct nfs_fh *fh,
>   1141					    struct nfs_fattr *fattr,
>   1142					    rpc_authflavor_t flavor)
>   1143	{
>   1144		struct nfs_server *server;
>   1145		int error;
>   1146	
>   1147		server = nfs_alloc_server();
>   1148		if (!server)
>   1149			return ERR_PTR(-ENOMEM);
>   1150	
>   1151		server->cred = get_cred(source->cred);
>   1152	
>   1153		/* Copy data from the source */
>   1154		server->nfs_client = source->nfs_client;
>   1155		server->destroy = source->destroy;
>   1156		refcount_inc(&server->nfs_client->cl_count);
>   1157		nfs_server_copy_userdata(server, source);
>   1158	
>   1159		server->fsid = fattr->fsid;
>   1160	
>   1161		if (IS_ENABLED(CONFIG_NFS_V4) && server->delegation_hash_table) {
> > 1162			error = nfs4_delegation_hash_alloc(server);
>   1163			if (error)
>   1164				goto out_free_server;
>   1165		}
>   1166	
>   1167		nfs_sysfs_add_server(server);
>   1168	
>   1169		nfs_sysfs_link_rpc_client(server,
>   1170			server->nfs_client->cl_rpcclient, "_state");
>   1171	
>   1172		error = nfs_init_server_rpcclient(server,
>   1173				source->client->cl_timeout,
>   1174				flavor);
>   1175		if (error < 0)
>   1176			goto out_free_delegation_hash;
>   1177	
>   1178		/* probe the filesystem info for this server filesystem */
>   1179		error = nfs_probe_server(server, fh);
>   1180		if (error < 0)
>   1181			goto out_free_delegation_hash;
>   1182	
>   1183		if (server->namelen == 0 || server->namelen > NFS4_MAXNAMLEN)
>   1184			server->namelen = NFS4_MAXNAMLEN;
>   1185	
>   1186		error = nfs_start_lockd(server);
>   1187		if (error < 0)
>   1188			goto out_free_delegation_hash;
>   1189	
>   1190		nfs_server_insert_lists(server);
>   1191		server->mount_time = jiffies;
>   1192	
>   1193		return server;
>   1194	
>   1195	out_free_delegation_hash:
>   1196		kfree(server->delegation_hash_table);
>   1197	out_free_server:
>   1198		nfs_free_server(server);
>   1199		return ERR_PTR(error);
>   1200	}
>   1201	EXPORT_SYMBOL_GPL(nfs_clone_server);
>   1202	
> 
> -- 
> 0-DAY CI Kernel Test Service
> https://github.com/intel/lkp-tests/wiki
---end quoted text---

Re: [PATCH 4/4] NFS: use a hash table for delegation lookup

[Trond Myklebust]

On Thu, 2025-07-17 at 07:13 +0200, Christoph Hellwig wrote:
> So nfs4 can be built modular, and delegation.o is built into that,
> i.e. we can't call into deletation code from clone_server.
> 
> Back to passing a major number to nfs_alloc_server?

Why not just build a small wrapper in fs/nfs/nfs4proc.c that does the
allocation after calling the regular nfs_clone_server(), and substitute
that into the nfs_v4_clientops.clone_server callback?

> 
> On Thu, Jul 17, 2025 at 12:35:07PM +0800, kernel test robot wrote:
> > Hi Christoph,
> > 
> > kernel test robot noticed the following build errors:
> > 
> > [auto build test ERROR on trondmy-nfs/linux-next]
> > [also build test ERROR on linus/master v6.16-rc6 next-20250716]
> > [If your patch is applied to the wrong git tree, kindly drop us a
> > note.
> > And when submitting patch, we suggest to use '--base' as documented
> > in

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