> On Nov 30, 2021, at 6:36 PM, NeilBrown <neilb@xxxxxxx> wrote: > > On Wed, 01 Dec 2021, paulmck@xxxxxxxxxx wrote: >> On Mon, Nov 29, 2021 at 01:46:07PM -0500, Chuck Lever wrote: >>> To address this error: >>> >>> CC [M] fs/nfsd/filecache.o >>> CHECK /home/cel/src/linux/linux/fs/nfsd/filecache.c >>> /home/cel/src/linux/linux/fs/nfsd/filecache.c:772:9: error: incompatible types in comparison expression (different address spaces): >>> /home/cel/src/linux/linux/fs/nfsd/filecache.c:772:9: struct net [noderef] __rcu * >>> /home/cel/src/linux/linux/fs/nfsd/filecache.c:772:9: struct net * >>> >>> The "net" field in struct nfsd_fcache_disposal is not annotated as >>> requiring an RCU assignment, so replace the macro that includes an >>> invocation of rcu_check_sparse() with an equivalent that does not. >>> >>> Signed-off-by: Chuck Lever <chuck.lever@xxxxxxxxxx> >> >> From an RCU perspective: >> >> Acked-by: Paul E. McKenney <paulmck@xxxxxxxxxx> >> >> But it would be good to get someone more familiar with the code to >> look at this. > > There is a global list of 'struct nfsd_fcache_disposal', potentially one > for each network namespace. Each contains a '*net' which is effectively > an opaque lookup key. It is never dereferenced - only used to find the > nfsd_fcache_disposal which matches a given 'struct net *'. > > The lookup happens under rcu_read_lock(). A 'struct net' is freed after > a grace period (see cleanup_net() in net_namespace.c) so to ensure the > lookup doesn't find a false positive - caused by a 'struct net' being > deallocated and reallocated, it is sufficient to clear the ->net pointer > while the the net is known to still be active. At most, a write-barrier > might be justified. i.e. smp_store_release(l->net, NULL); By way of further explanation: The Documentation/ for RCU (ie, "RCU for Dummies") suggests that rcu_assign_pointer() is adequate for preventing undesirable compiler optimizations or CPU load/store reordering. rcu_assign_pointer() uses WRITE_ONCE for constants like NULL and smp_store_release() when assigning variable values. I copied the use of WRITE_ONCE() from rcu_assign_pointer(). So I expect exactly zero change in behavior or compiled code... (but I should have checked the generated object to verify that is the case). > However ... the list of nfsd_fcache_disposal seems unnecessary. > nfsd has a 'struct nfsd_net' which parallels any interesting 'struct > net' using the net_generic() framework. > If the nfs_fcache_disposal were linked from the nfsd_net, there would be > no need for the list, no need to record the ->net, and not need for the > code in question here. > The nfsd_fcache_disposal is destroyed (nfsd_file_cache_shutdown_new()) > before the nfsd_net is destroyed, so there are no lifetime issues with > keeping the separate list. Sure. If it makes sense to use nfsd_net instead of a separate data structure, then that can be made to happen. I would like to hear from Trond regarding why he felt a separate data structure was necessary for fcache_disposal. -- Chuck Lever
