Re: [PATCH v2] nfs: add dummy definition for nfsd_file

NeilBrown
2025-05-07T02:29:56Z

On Sun, 04 May 2025, Pali Rohár wrote:
> On Wednesday 23 April 2025 00:02:00 Pali Rohár wrote:
> > On Wednesday 23 April 2025 07:54:40 NeilBrown wrote:
> > > On Wed, 23 Apr 2025, Pali Rohár wrote:
> > > > On Sunday 20 April 2025 12:12:22 Mike Snitzer wrote:
> > > > > On Sat, Apr 19, 2025 at 01:52:31PM -0400, Chuck Lever wrote:
> > > > > > On 4/18/25 5:34 PM, Mike Snitzer wrote:
> > > > > > > On Wed, Apr 16, 2025 at 09:31:55AM -0400, Chuck Lever wrote:
> > > > > > >> On 4/15/25 10:41 PM, Vincent Mailhol wrote:
> > > > > > >>> +CC: Neil Brown
> > > > > > >>> +CC: Olga Kornievskaia
> > > > > > >>> +CC: Dai Ngo
> > > > > > >>> +CC: Tom Talpey
> > > > > > >>> +CC: Trond Myklebust
> > > > > > >>> +CC: Anna Schumaker
> > > > > > >>>
> > > > > > >>> (just to make sure that anyone listed in
> > > > > > >>>
> > > > > > >>> ./scripts/get_maintainer.pl fs/nfs_common/nfslocalio.c
> > > > > > >>>
> > > > > > >>> get copied).
> > > > > > >>>
> > > > > > >>> Here is the link to the full thread:
> > > > > > >>>
> > > > > > >>> https://lore.kernel.org/all/Z_coQbSdvMWD92IA@kernel.org/
> > > > > > >>>
> > > > > > >>>
> > > > > > >>> On 10/04/2025 at 11:09, Mike Snitzer:
> > > > > > >>>> Add dummy definition for nfsd_file in both nfslocalio.c and localio.c
> > > > > > >>>> so various compilers (e.g. gcc 8.5.0 and 9.5.0) can be used. Otherwise
> > > > > > >>>> RCU code (rcu_dereference and rcu_access_pointer) will dereference
> > > > > > >>>> what should just be an opaque pointer (by using typeof(*ptr)).
> > > > > > >>>>
> > > > > > >>>> Fixes: 86e00412254a ("nfs: cache all open LOCALIO nfsd_file(s) in client")
> > > > > > >>>> Cc: stable@vger.kernel.org
> > > > > > >>>> Tested-by: Jeff Johnson <jeff.johnson@oss.qualcomm.com>
> > > > > > >>>> Tested-by: Pali Rohár <pali@kernel.org>
> > > > > > >>>> Tested-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> > > > > > >>>> Signed-off-by: Mike Snitzer <snitzer@kernel.org>
> > > > > > >>>
> > > > > > >>> Hi everyone,
> > > > > > >>>
> > > > > > >>> The build has been broken for several weeks already. Does anyone have
> > > > > > >>> intention to pick-up this patch?
> > > > > > >>>
> > > > > > >>> (please ignore if someone already picked it up and if it is already on
> > > > > > >>> its way to Linus's tree).
> > > > > > >>
> > > > > > >> I assumed that, like all LOCALIO-related changes, this fix would go
> > > > > > >> in through the NFS client tree. Let me know if it needs to go via NFSD.
> > > > > > >
> > > > > > > Since we haven't heard from Trond or Anna about it, I think you'd be
> > > > > > > perfectly fine to pick it up. It is a compiler fixup associated with
> > > > > > > nfd_file being kept opaque to the client -- but given it is "struct
> > > > > > > nfsd_file" that gives you full license to grab it (IMO).
> > > > > > >
> > > > > > > I'm also unaware of any conflicting changes in the NFS client tree.
> > > > > >
> > > > > > Hi Mike -
> > > > > >
> > > > > > I just looked at this one again. The patch's diffstat is:
> > > > > >
> > > > > > fs/nfs/localio.c | 8 ++++++++
> > > > > > fs/nfs_common/nfslocalio.c | 8 ++++++++
> > > > > >
> > > > > > Although fs/nfs_common/ is part of both trees, fs/nfs/localio.c is
> > > > > > definitely a client file. I'm still happy to pick it up, but technically
> > > > > > I would need an Acked-by: from one of the NFS client maintainers.
> > > > > >
> > > > > > My impression is that Trond is managing the NFS client pulls for v6.15.
> > > > >
> > > > > Sure, that's my understanding too. Feel free to offer your Acked-by
> > > > > (for fs/nfs_common/) and hopefully it'll get picked up. I can
> > > > > followup with Trond later this coming week if/as needed.
> > > > >
> > > > > Thanks,
> > > > > Mike
> > > >
> > > > Can we move forward here? The compilation of kernel is broken for at
> > > > least 2 months. Also I have tried to contact Trond for more months but
> > > > has not responded to my emails.
> > > >
> > > > Could be this change picked with a slightly higher priority than just
> > > > waiting another two months? Note that nobody objected this particular
> > > > fix and there are 3+ Tested-by lines. And it is not a good image if some
> > > > kernel component does not compile...
> > > >
> > >
> > > Actually I do object to this fix (though I've been busy and hadn't had
> > > much change to look at it properly).
> > > The fix is ugly. At the very least it should be wrapping in an
> > > #if GCC_VERSION < whatever
> >
> > The problem is that this compile issue happens also with some builds of
> > gcc 13.3.0 as was discussed in the email thread.
> >
> > So is not clear what is that "whatever". For me it looks like that it
> > would be more than the version, probably also some build characteristics
> > of gcc. But I have not been investigating it deeper.
>
> Neil, thank you for taking care about this. I will retest your next
> patches later.
>
> > > to make the purpose more clear. But I'd rather a deeper fix.
> > >
> > > GCC is complaining that rcu_dereference is being called on a point to a
> > > structure that it doesn't know the content of.
> > > So the code is says "I'm going to dereference this pointer even that
> > > that is actually impossible as I don't know what any of the fields are".
> > >
> > > I'd rather it didn't do that. I've been playing with the code and I
> > > think it can be made quite a bit cleaner by moving the rcu_dereference()
> > > call into the nfsd side of the code. Hopefully I'll have a patch in a
> > > day or so to demonstrate what I mean.
> > >
> > > I understand your desire for some action - but the reality is that you
> > > have the full source code of the kernel and you can do whatever you want
> > > to the kernel that you are working on. You don't need us.
> > > Getting code upstream is certainly good and we should continue to work
> > > on doing that, but if you *need* something to be upstream then you might
> > > want to consider whether your processes are really serving you.
>
> I need to comment this. I read more times that kernel developers are
> complaining about vendors who do not provide accurate kernel support for
> their hardware, like not sending changes to mainline kernel and instead
> just publishing their own SDK (=copy of kernel with own changes) or not
> providing nothing at all, and that this is being repeated.
> And here you are saying that it OK and basically preferred, you have
> sources do that and do not care about mainline kernel.
> Then we have there developers who are saying that usage of non-mainline
> or patched kernel is not supported and would not take any bug report and
> instead redirect them to vendor portal. So at the end the result for
> some specific hardware is that you cannot use mainline kernel because of
> broken A, you have to use vendor kernel, but there is a broken thing B
> about which vendor do not care (and taking it from mainline) and you
> cannot report it to kernel developers because you are not using mainline.
> Which makes the situation really bad for (end) users...

Hi,
thank you for caring about the status of the upstream kernel and for
doing what you can to improve it. That is appreciated.

My point was not that you shouldn't do what you can. It was more that
your ability to influence upstream does have limits.

I'm specifically responding to this:

> Can we move forward here? The compilation of kernel is broken for at
> least 2 months. Also I have tried to contact Trond for more months but
> has not responded to my emails.
>
> Could be this change picked with a slightly higher priority than just
> waiting another two months?

This sounds to me like nagging, complaining, and maybe a little bit of
guilt-tripping. You probably didn't mean it that way, but that it how
it sounds to me.

It is really good that you reported the problem - thanks.
It is really good that you tested the proposed fix - thanks.
It is really good that you sent occasional reminders - thanks.

but ultimately you don't have control. You cannot force the maintainer
to accept the patch, and the above is unlikely to be more effective than
occasional simple reminders.

Anything that you do to increase the chance of a fix must be helpful.
Complaining is rarely helpful.

>
> > > Trond does often seem slow to take patches, and often they simply appear
> > > in git://git.linux-nfs.org/projects/trondmy/nfs-2.6.git without any
> > > reply to the emails, but he or Anna does usually get to stuff
> > > eventually.
> > >
> > > NeilBrown
>
> So how to contact those people if they do not react for year? I tried to
> remind changes more times, but no response. I was already told by more
> people that this is a private group which is not taking changes or
> (security) bug reports from outside, and my experience just proves it.
>

If you genuinely think any maintainer isn't doing their job properly, the
question to ask is "what can I do to help?". Maybe there is nothing you
can do. Maybe you can offer to be a co-maintainer (if you can
demonstrate ability). Maybe you can fund someone else to be the
maintainer. If you believe a maintainer is acting in a way that is not
just unhelpful but is actively harmful, you can always try to bring the
problem directly to Linus' attention.
(I don't think any of this applies to NFS - but you might see it differently).

In the case of a regression, as this is, you can always try involving
the "KERNEL REGRESSIONS" maintainer - see the maintainers file. That
might result in the regression becoming visible to Linus and he might
choose to act in some way.

I agree that it is frustrating when you can see an opportunity to
improve the kernel (or any OSS project) but cannot get your voice heard.
Sometimes you just have to do your best, and move on.

Thanks,
NeilBrown