On Wed, Sep 04, 2024 at 03:01:46PM +1000, NeilBrown wrote:
> On Wed, 04 Sep 2024, NeilBrown wrote:
> >
> > I agree that dropping and reclaiming a lock is an anti-pattern and in
> > best avoided in general. I cannot see a better alternative in this
> > case.
>
> It occurred to me what I should spell out the alternate that I DO see so
> you have the option of disagreeing with my assessment that it isn't
> "better".
>
> We need RCU to call into nfsd, we need a per-cpu ref on the net (which
> we can only get inside nfsd) and NOT RCU to call
> nfsd_file_acquire_local().
>
> The current code combines these (because they are only used together)
> and so the need to drop rcu.
>
> I thought briefly that it could simply drop rcu and leave it dropped
> (__releases(rcu)) but not only do I generally like that LESS than
> dropping and reclaiming, I think it would be buggy. While in the nfsd
> module code we need to be holding either rcu or a ref on the server else
> the code could disappear out from under the CPU. So if we exit without
> a ref on the server - which we do if nfsd_file_acquire_local() fails -
> then we need to reclaim RCU *before* dropping the ref. So the current
> code is slightly buggy.
>
> We could instead split the combined call into multiple nfs_to
> interfaces.
>
> So nfs_open_local_fh() in nfs_common/nfslocalio.c would be something
> like:
>
> rcu_read_lock();
> net = READ_ONCE(uuid->net);
> if (!net || !nfs_to.get_net(net)) {
> rcu_read_unlock();
> return ERR_PTR(-ENXIO);
> }
> rcu_read_unlock();
> localio = nfs_to.nfsd_open_local_fh(....);
> if (IS_ERR(localio))
> nfs_to.put_net(net);
> return localio;
>
> So we have 3 interfaces instead of 1, but no hidden unlock/lock.
Splitting up the function call occurred to me as well, but I didn't
come up with a specific bit of surgery. Thanks for the suggestion.
At this point, my concern is that we will lose your cogent
explanation of why the release/lock is done. Having it in email is
great, but email is more ephemeral than actually putting it in the
code.
> As I said, I don't think this is a net win, but reasonable people might
> disagree with me.
The "win" here is that it makes this code self-documenting and
somewhat less likely to be broken down the road by changes in and
around this area. Since I'm more forgetful these days I lean towards
the more obvious kinds of coding solutions. ;-)
Mike, how do you feel about the 3-interface suggestion?
--
Chuck Lever
