On Fri, 06 Sep 2024, Mike Snitzer wrote:
> On Wed, Sep 04, 2024 at 09:47:07AM -0400, Chuck Lever wrote:
> > 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?
>
> I dislike expanding from 1 indirect function call to 2 in rapid
> succession (3 for the error path, not a problem, just being precise.
> But I otherwise like it.. maybe.. heh.
>
> FYI, I did run with the suggestion to make nfs_to a pointer that just
> needs a simple assignment rather than memcpy to initialize. So Neil's
> above code becames:
>
> rcu_read_lock();
> net = rcu_dereference(uuid->net);
> if (!net || !nfs_to->nfsd_serv_try_get(net)) {
> rcu_read_unlock();
> return ERR_PTR(-ENXIO);
> }
> rcu_read_unlock();
> /* We have an implied reference to net thanks to nfsd_serv_try_get */
> localio = nfs_to->nfsd_open_local_fh(net, uuid->dom, rpc_clnt,
> cred, nfs_fh, fmode);
> if (IS_ERR(localio))
> nfs_to->nfsd_serv_put(net);
> return localio;
>
> I do think it cleans the code up... full patch is here:
> https://git.kernel.org/pub/scm/linux/kernel/git/snitzer/linux.git/commit/?h=nfs-localio-for-next.v15-with-fixups&id=e85306941878a87070176702de687f2779436061
>
> But I'm still on the fence.. someone help push me over!
I think the new code is unquestionable clearer, and not taking this
approach would be a micro-optimisation which would need to be
numerically justified. So I'm pushing for the three-interface version
(despite what I said before).
Unfortunately the new code is not bug-free - not quite.
As soon as nfs_to->nfsd_serv_put() calls percpu_ref_put() the nfsd
module can be unloaded, and the "return" instruction might not be
present. For this to go wrong would require a lot of bad luck, but if
the CPU took an interrupt at the wrong time were would be room.
[Ever since module_put_and_exit() was added (now ..and_kthread_exit)
I've been sensitive to dropping the ref to a module in code running in
the module]
So I think nfsd_serv_put (and nfsd_serv_try_get() __must_hold(RCU) and
nfs_open_local_fh() needs rcu_read_lock() before calling
nfs_to->nfsd_serv_put(net).
>
> Tangent, but in the related business of "what are next steps?":
>
> I updated headers with various provided Reviewed-by:s and Acked-by:s,
> fixed at least 1 commit header, fixed some sparse issues, various
> fixes to nfs_to patch (removed EXPORT_SYMBOL_GPL, switched to using
> pointer, updated nfs_to callers). Etc...
>
> But if I fold those changes in I compromise the provided Reviewed-by
> and Acked-by.. so I'm leaning toward posting a v16 that has
> these incremental fixes/improvements, see the 3 topmost commits here:
> https://git.kernel.org/pub/scm/linux/kernel/git/snitzer/linux.git/log/?h=nfs-localio-for-next.v15-with-fixups
>
> Or if you can review the incremental patches I can fold them in and
> preserve the various Reviewed-by and Acked-by...
I have reviewed the incremental patches and I'm happy for all my tags to
apply to the new versions of the patches.
NeilBrown
