On Thu, 2025-04-10 at 15:39 -0400, Anna Schumaker wrote:
> Hi Jeff,
>
> On 4/10/25 2:12 PM, Jeff Layton wrote:
> > Since struct nfs4_pnfs_ds should not be shared between net namespaces,
> > move from a global list of objects to a per-netns list and spinlock.
> >
> > Tested-by: Sargun Dillon <sargun@xxxxxxxxx>
> > Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
> > ---
> > fs/nfs/client.c | 4 ++++
> > fs/nfs/inode.c | 3 +++
> > fs/nfs/netns.h | 6 +++++-
> > fs/nfs/pnfs_nfs.c | 31 +++++++++++++++++--------------
> > 4 files changed, 29 insertions(+), 15 deletions(-)
> >
> > diff --git a/fs/nfs/client.c b/fs/nfs/client.c
> > index 9500b46005b0148a5a9a7d464095ca944de06bb5..81c0f780ff82c8a020fafdb3df72552c8e6e535f 100644
> > --- a/fs/nfs/client.c
> > +++ b/fs/nfs/client.c
> > @@ -1199,6 +1199,10 @@ void nfs_clients_init(struct net *net)
> > INIT_LIST_HEAD(&nn->nfs_volume_list);
> > #if IS_ENABLED(CONFIG_NFS_V4)
> > idr_init(&nn->cb_ident_idr);
> > +#if IS_ENABLED(CONFIG_NFS_V4_1)
> > + INIT_LIST_HEAD(&nn->nfs4_data_server_cache);
> > + spin_lock_init(&nn->nfs4_data_server_lock);
> > +#endif
> > #endif
> > spin_lock_init(&nn->nfs_client_lock);
> > nn->boot_time = ktime_get_real();
> > diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
> > index 119e447758b994b34e55e7b28fd4f34fa089e2e1..ee796a726a1e4b0dfbd199cc62176c6802692671 100644
> > --- a/fs/nfs/inode.c
> > +++ b/fs/nfs/inode.c
> > @@ -2559,6 +2559,9 @@ static int nfs_net_init(struct net *net)
> >
> > static void nfs_net_exit(struct net *net)
> > {
> > + struct nfs_net *nn = net_generic(net, nfs_net_id);
> > +
> > + WARN_ON_ONCE(!list_empty(&nn->nfs4_data_server_cache));
>
> nfs4_data_server_cache is defined under an #if IS_ENABLED(CONFIG_NFS_V4_1) block,
> so the compiler complains if this isn't enabled:
>
> fs/nfs/inode.c:2564:32: error: no member named 'nfs4_data_server_cache' in 'struct nfs_net'
> 2564 | WARN_ON_ONCE(!list_empty(&nn->nfs4_data_server_cache));
>
> Anna
>
Good catch. I'll send v2 with that fixed.
Thanks!
> > rpc_proc_unregister(net, "nfs");
> > nfs_fs_proc_net_exit(net);
> > nfs_clients_exit(net);
> > diff --git a/fs/nfs/netns.h b/fs/nfs/netns.h
> > index a68b21603ea9a867ba513e2a667b08fbc6d80dd8..557cf822002663b7957194610d103210b159e5c4 100644
> > --- a/fs/nfs/netns.h
> > +++ b/fs/nfs/netns.h
> > @@ -31,7 +31,11 @@ struct nfs_net {
> > unsigned short nfs_callback_tcpport;
> > unsigned short nfs_callback_tcpport6;
> > int cb_users[NFS4_MAX_MINOR_VERSION + 1];
> > -#endif
> > +#if IS_ENABLED(CONFIG_NFS_V4_1)
> > + struct list_head nfs4_data_server_cache;
> > + spinlock_t nfs4_data_server_lock;
> > +#endif /* CONFIG_NFS_V4_1 */
> > +#endif /* CONFIG_NFS_V4 */
> > struct nfs_netns_client *nfs_client;
> > spinlock_t nfs_client_lock;
> > ktime_t boot_time;
> > diff --git a/fs/nfs/pnfs_nfs.c b/fs/nfs/pnfs_nfs.c
> > index 2ee20a0f0b36d3b38e35c4cad966b9d58fa822f4..91ef486f40b943a1dc55164e610378ef73781e55 100644
> > --- a/fs/nfs/pnfs_nfs.c
> > +++ b/fs/nfs/pnfs_nfs.c
> > @@ -16,6 +16,7 @@
> > #include "nfs4session.h"
> > #include "internal.h"
> > #include "pnfs.h"
> > +#include "netns.h"
> >
> > #define NFSDBG_FACILITY NFSDBG_PNFS
> >
> > @@ -504,14 +505,14 @@ EXPORT_SYMBOL_GPL(pnfs_generic_commit_pagelist);
> > /*
> > * Data server cache
> > *
> > - * Data servers can be mapped to different device ids.
> > - * nfs4_pnfs_ds reference counting
> > + * Data servers can be mapped to different device ids, but should
> > + * never be shared between net namespaces.
> > + *
> > + * nfs4_pnfs_ds reference counting:
> > * - set to 1 on allocation
> > * - incremented when a device id maps a data server already in the cache.
> > * - decremented when deviceid is removed from the cache.
> > */
> > -static DEFINE_SPINLOCK(nfs4_ds_cache_lock);
> > -static LIST_HEAD(nfs4_data_server_cache);
> >
> > /* Debug routines */
> > static void
> > @@ -604,12 +605,12 @@ _same_data_server_addrs_locked(const struct list_head *dsaddrs1,
> > * Lookup DS by addresses. nfs4_ds_cache_lock is held
> > */
> > static struct nfs4_pnfs_ds *
> > -_data_server_lookup_locked(const struct net *net, const struct list_head *dsaddrs)
> > +_data_server_lookup_locked(const struct nfs_net *nn, const struct list_head *dsaddrs)
> > {
> > struct nfs4_pnfs_ds *ds;
> >
> > - list_for_each_entry(ds, &nfs4_data_server_cache, ds_node)
> > - if (ds->ds_net == net && _same_data_server_addrs_locked(&ds->ds_addrs, dsaddrs))
> > + list_for_each_entry(ds, &nn->nfs4_data_server_cache, ds_node)
> > + if (_same_data_server_addrs_locked(&ds->ds_addrs, dsaddrs))
> > return ds;
> > return NULL;
> > }
> > @@ -653,10 +654,11 @@ static void destroy_ds(struct nfs4_pnfs_ds *ds)
> >
> > void nfs4_pnfs_ds_put(struct nfs4_pnfs_ds *ds)
> > {
> > - if (refcount_dec_and_lock(&ds->ds_count,
> > - &nfs4_ds_cache_lock)) {
> > + struct nfs_net *nn = net_generic(ds->ds_net, nfs_net_id);
> > +
> > + if (refcount_dec_and_lock(&ds->ds_count, &nn->nfs4_data_server_lock)) {
> > list_del_init(&ds->ds_node);
> > - spin_unlock(&nfs4_ds_cache_lock);
> > + spin_unlock(&nn->nfs4_data_server_lock);
> > destroy_ds(ds);
> > }
> > }
> > @@ -718,6 +720,7 @@ nfs4_pnfs_remotestr(struct list_head *dsaddrs, gfp_t gfp_flags)
> > struct nfs4_pnfs_ds *
> > nfs4_pnfs_ds_add(const struct net *net, struct list_head *dsaddrs, gfp_t gfp_flags)
> > {
> > + struct nfs_net *nn = net_generic(net, nfs_net_id);
> > struct nfs4_pnfs_ds *tmp_ds, *ds = NULL;
> > char *remotestr;
> >
> > @@ -733,8 +736,8 @@ nfs4_pnfs_ds_add(const struct net *net, struct list_head *dsaddrs, gfp_t gfp_fla
> > /* this is only used for debugging, so it's ok if its NULL */
> > remotestr = nfs4_pnfs_remotestr(dsaddrs, gfp_flags);
> >
> > - spin_lock(&nfs4_ds_cache_lock);
> > - tmp_ds = _data_server_lookup_locked(net, dsaddrs);
> > + spin_lock(&nn->nfs4_data_server_lock);
> > + tmp_ds = _data_server_lookup_locked(nn, dsaddrs);
> > if (tmp_ds == NULL) {
> > INIT_LIST_HEAD(&ds->ds_addrs);
> > list_splice_init(dsaddrs, &ds->ds_addrs);
> > @@ -743,7 +746,7 @@ nfs4_pnfs_ds_add(const struct net *net, struct list_head *dsaddrs, gfp_t gfp_fla
> > INIT_LIST_HEAD(&ds->ds_node);
> > ds->ds_net = net;
> > ds->ds_clp = NULL;
> > - list_add(&ds->ds_node, &nfs4_data_server_cache);
> > + list_add(&ds->ds_node, &nn->nfs4_data_server_cache);
> > dprintk("%s add new data server %s\n", __func__,
> > ds->ds_remotestr);
> > } else {
> > @@ -755,7 +758,7 @@ nfs4_pnfs_ds_add(const struct net *net, struct list_head *dsaddrs, gfp_t gfp_fla
> > refcount_read(&tmp_ds->ds_count));
> > ds = tmp_ds;
> > }
> > - spin_unlock(&nfs4_ds_cache_lock);
> > + spin_unlock(&nn->nfs4_data_server_lock);
> > out:
> > return ds;
> > }
> >
>
--
Jeff Layton <jlayton@xxxxxxxxxx>
