On Sat, 24 Aug 2024, Mike Snitzer wrote:
> +/*
> + * nfs_local_disable - disable local i/o for an nfs_client
> + */
> +void nfs_local_disable(struct nfs_client *clp)
> +{
> + if (test_and_clear_bit(NFS_CS_LOCAL_IO, &clp->cl_flags)) {
This use of test_and_clear_bit() led me to think about the atomicity
requirements here.
If this were called (on a different CPU) between the
nfs_server_is_local() test in nfs_local_file_open() and the
nfs_local_open_fh() call which follows, then nfs_to.nfsd_open_local_fh()
could be passed a NULL client which wouldn't end well.
Maybe we need an rcu-protected auth_domain_tryget() in
nfsd_local_open_fh(), and the auth_domain_put() below need to be behind
call_rcu().
Or something...
> + trace_nfs_local_disable(clp);
> + clp->cl_nfssvc_net = NULL;
> + if (clp->cl_nfssvc_dom) {
> + auth_domain_put(clp->cl_nfssvc_dom);
> + clp->cl_nfssvc_dom = NULL;
> + }
> + }
> +}
> +
> +struct nfsd_file *
> +nfs_local_file_open(struct nfs_client *clp, const struct cred *cred,
> + struct nfs_fh *fh, struct nfs_open_context *ctx)
> +{
> + struct nfsd_file *nf;
> +
> + if (!nfs_server_is_local(clp))
> + return NULL;
> +
> + nf = nfs_local_open_fh(clp, cred, fh, ctx->mode);
> + if (IS_ERR(nf))
> + return NULL;
> +
> + return nf;
> +}
It isn't clear to be what nfs_local_file_open() adds any value over
nfs_local_open_fh. The nfs_server_is_local() test is cheap and can
safely be in nfs_local_open_fh(), and the current callers of
nfs_local_file_open() can simple pass ctx->mode to _fh instead of
passing ctx to _open.
Thanks,
NeilBrown
