Re: [PATCH 0/6 v2] nfs_localio: fixes for races and errors from older compilers

NeilBrown
2025-05-10T02:44:56Z

On Sat, 10 May 2025, paulmck@kernel.org wrote:
> On Fri, May 09, 2025 at 05:02:29PM -0400, Mike Snitzer wrote:
> > On Fri, May 09, 2025 at 12:01:19PM -0400, Chuck Lever wrote:
> > > [ adding Paul McK ]
> > >
> > > On 5/8/25 8:46 PM, NeilBrown wrote:
> > > > This is a revised version a the earlier series. I've actually tested
> > > > this time and fixed a few issues including the one that Mike found.
> > >
> > > As Mike mentioned in a previous thread, at this point, any fix for this
> > > issue will need to be applied to recent stable kernels as well. This
> > > series looks a bit too complicated for that.
> > >
> > > I expect that other subsystems will encounter this issue eventually,
> > > so it would be beneficial to address the root cause. For that purpose, I
> > > think I like Vincent's proposal the best:
> > >
> > > https://lore.kernel.org/linux-nfs/8c67a295-8caa-4e53-a764-f691657bbe62@wanadoo.fr/raw
> > >
> > > None of this is to say that Neil's patches shouldn't be applied. But
> > > perhaps these are not a broad solution to the RCU compilation issue.
> >
> > I agree with your suggested approach. Hopefully Paul agrees and
> > Vincent can polish a patch for near-term inclusion.
>
> The main issue that I see with Vincent's patch is that we need the
> asterisks ("*") to remain in order to mesh with the definition of __rcu
> and to work correctly with the "sparse" tool. The __rcu address space
> applies to the pointed-to data, not the pointer. So removing the
> asterisks from __rcu_access_pointer() and friends would require the
> various uses of __rcu to move to the other side of the asterisk,
> right? For example, this:
>
> struct pci_dev __rcu *pdev;
>
> would need to become this:
>
> struct pci_dev *__rcu pdev
>
> And the same for the more than 1,000 other uses of __rcu.
>
> Or am I missing something here? In particular, did it turn out that
> sparse and the other tools were all happy with this change?

Sparse isn't happy with the change. I think it appears to sparse as:

__force __kernel struct foo __rcu *

rather than

struct foo __rcu __force __kernel *

and the last one wins - so sparse sees __kernel last in the upstream
code but sees __rcu last with the patch applied.
Certainly I see messages like:
../include/net/netns/generic.h:46:12: warning: incorrect type in assignment (different address spaces)
../include/net/netns/generic.h:46:12: expected struct net_generic *ng
../include/net/netns/generic.h:46:12: got struct net_generic [noderef] __rcu *

so the __rcu it still seen.

NeilBrown