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.
As I said, I don't think this is a net win, but reasonable people might
disagree with me.
NeilBrown
