From bcb2579c9d9ecdfebcc295fef7e780bc0d72eaf1 Mon Sep 17 00:00:00 2001 From: Benjamin Coddington Date: Tue, 15 Sep 2026 08:22:25 -0400 Subject: [PATCH] NFSv4/pnfs: Key the data-server cache by its address set and version Populating the data-server cache was quadratic: every GETDEVICEINFO decode scanned the whole per-net cache under one spinlock. At the anticipated scale of 1024 data servers a striping mount pays that in a burst at first access, and again on notification-driven re-resolution. Hash each DS to a bucket keyed on the whole cache key -- per-address jhash over exactly the fields same_sockaddr() compares, combined by addition so multipath ordering cannot change the bucket, then the NFS version folded in -- with the existing comparator as the in-bucket tiebreaker. Lookup and insert now touch one bucket; teardown is unchanged (hlist_del_init needs no bucket). Keying on the whole set requires the comparator to test set equality, so it is tightened from the subset test it did before -- a subset match would hash to a different bucket and simply never be found. That test was also wrong in a way worth naming. It answered "is dsaddrs1 a subset of dsaddrs2", and the caller passes the cached list first, so a cached data server whose address set was contained in an incoming one was returned for that incoming set. The aliasing was therefore one-directional: cache {A,B} first and an incoming {A} did not match, but cache {A} first and an incoming {A,B} did. The consequence was mild, which is why it went unnoticed: every address on one device's multipath list names the same data server, so a merged entry's addresses are all paths that server also advertised. The effect is lost path diversity and a truncated ds_remotestr (and the netaddr flexfiles reports in layoutstats), not I/O sent to the wrong server. Assisted-by: Claude:claude-fable-5 Signed-off-by: Benjamin Coddington Signed-off-by: Anna Schumaker --- fs/nfs/pnfs_nfs.c | 68 ++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 58 insertions(+), 10 deletions(-) diff --git a/fs/nfs/pnfs_nfs.c b/fs/nfs/pnfs_nfs.c index f88a9784988a..c9bbcb765f4a 100644 --- a/fs/nfs/pnfs_nfs.c +++ b/fs/nfs/pnfs_nfs.c @@ -15,6 +15,8 @@ #include "nfs4session.h" #include "internal.h" +#include +#include #include "pnfs.h" #include "netns.h" #include "nfs4trace.h" @@ -577,8 +579,8 @@ same_sockaddr(struct sockaddr *addr1, struct sockaddr *addr2) } /* - * Checks if 'dsaddrs1' contains a subset of 'dsaddrs2'. If it does, - * declare a match. + * Checks if 'dsaddrs1' and 'dsaddrs2' hold the same set of addresses. + * If they do, declare a match. */ static bool _same_data_server_addrs_locked(const struct list_head *dsaddrs1, @@ -588,6 +590,10 @@ _same_data_server_addrs_locked(const struct list_head *dsaddrs1, struct sockaddr *sa1, *sa2; bool match = false; + if (list_count_nodes((struct list_head *)dsaddrs1) != + list_count_nodes((struct list_head *)dsaddrs2)) + return false; + list_for_each_entry(da1, dsaddrs1, da_node) { sa1 = (struct sockaddr *)&da1->da_addr; match = false; @@ -603,6 +609,47 @@ _same_data_server_addrs_locked(const struct list_head *dsaddrs1, return match; } +/* Hash family, address bytes, and port - as same_sockaddr() */ +static u32 +nfs4_ds_addr_hash(const struct sockaddr *sa) +{ + u32 h = sa->sa_family; + + switch (sa->sa_family) { + case AF_INET: { + const struct sockaddr_in *a = (const struct sockaddr_in *)sa; + + h = jhash(&a->sin_addr.s_addr, sizeof(a->sin_addr.s_addr), h); + h = jhash(&a->sin_port, sizeof(a->sin_port), h); + break; + } + case AF_INET6: { + const struct sockaddr_in6 *a = (const struct sockaddr_in6 *)sa; + + h = jhash(&a->sin6_addr, sizeof(a->sin6_addr), h); + h = jhash(&a->sin6_port, sizeof(a->sin6_port), h); + break; + } + } + return h; +} + +/* + * Bucket index for a DS cache key. Per-address hashes combine by + * addition so the multipath list order cannot change the bucket, + * matching the order-independent set comparison above. + */ +static u32 +nfs4_ds_cache_hash(const struct list_head *dsaddrs, u32 version) +{ + const struct nfs4_pnfs_ds_addr *da; + u32 h = 0; + + list_for_each_entry(da, dsaddrs, da_node) + h += nfs4_ds_addr_hash((const struct sockaddr *)&da->da_addr); + return hash_32(jhash_1word(version, h), NFS4_DS_CACHE_HASH_BITS); +} + /* * Lookup DS by addresses and NFS version. nfs4_data_server_lock is held */ @@ -611,14 +658,12 @@ _data_server_lookup_locked(const struct nfs_net *nn, const struct list_head *dsaddrs, u32 version) { struct nfs4_pnfs_ds *ds; + u32 bucket = nfs4_ds_cache_hash(dsaddrs, version); - for (int i = 0; i < NFS4_DS_CACHE_HASH_SIZE; i++) - hlist_for_each_entry(ds, &nn->nfs4_data_server_cache[i], - ds_node) - if (ds->ds_version == version && - _same_data_server_addrs_locked(&ds->ds_addrs, - dsaddrs)) - return ds; + hlist_for_each_entry(ds, &nn->nfs4_data_server_cache[bucket], ds_node) + if (ds->ds_version == version && + _same_data_server_addrs_locked(&ds->ds_addrs, dsaddrs)) + return ds; return NULL; } @@ -735,6 +780,7 @@ nfs4_pnfs_ds_add(const struct net *net, struct list_head *dsaddrs, u32 version, { struct nfs_net *nn = net_generic(net, nfs_net_id); struct nfs4_pnfs_ds *tmp_ds, *ds = NULL; + struct hlist_head *bucket; char *remotestr; if (list_empty(dsaddrs)) { @@ -748,6 +794,8 @@ nfs4_pnfs_ds_add(const struct net *net, struct list_head *dsaddrs, u32 version, /* this is only used for debugging, so it's ok if its NULL */ remotestr = nfs4_pnfs_remotestr(dsaddrs, gfp_flags); + /* @dsaddrs is empty after the splice below. */ + bucket = &nn->nfs4_data_server_cache[nfs4_ds_cache_hash(dsaddrs, version)]; spin_lock(&nn->nfs4_data_server_lock); tmp_ds = _data_server_lookup_locked(nn, dsaddrs, version); @@ -760,7 +808,7 @@ nfs4_pnfs_ds_add(const struct net *net, struct list_head *dsaddrs, u32 version, ds->ds_net = net; ds->ds_clp = NULL; ds->ds_version = version; - hlist_add_head(&ds->ds_node, &nn->nfs4_data_server_cache[0]); + hlist_add_head(&ds->ds_node, bucket); dprintk("%s add new data server %s\n", __func__, ds->ds_remotestr); } else {