> On Jun 27, 2024, at 1:27 PM, Mike Snitzer <snitzer@xxxxxxxxxx> wrote:
>
> On Thu, Jun 27, 2024 at 12:07:03PM -0400, Chuck Lever wrote:
>> On Thu, Jun 27, 2024 at 11:48:09AM -0400, Jeff Layton wrote:
>>> On Wed, 2024-06-26 at 14:24 -0400, Mike Snitzer wrote:
>>>> Pass the stored cl_nfssvc_net from the client to the server as
>>>> first argument to nfsd_open_local_fh() to ensure the proper network
>>>> namespace is used for localio.
>>>>
>>>> Signed-off-by: Weston Andros Adamson <dros@xxxxxxxxxxxxxxx>
>>>> Signed-off-by: Peng Tao <tao.peng@xxxxxxxxxxxxxxx>
>>>> Signed-off-by: Lance Shelton <lance.shelton@xxxxxxxxxxxxxxx>
>>>> Signed-off-by: Trond Myklebust <trond.myklebust@xxxxxxxxxxxxxxx>
>>>> Signed-off-by: Mike Snitzer <snitzer@xxxxxxxxxx>
>>>> ---
>>>> fs/nfsd/Makefile | 1 +
>>>> fs/nfsd/filecache.c | 2 +-
>>>> fs/nfsd/localio.c | 246 ++++++++++++++++++++++++++++++++++++++++++++
>>>> fs/nfsd/nfssvc.c | 1 +
>>>> fs/nfsd/trace.h | 3 +-
>>>> fs/nfsd/vfs.h | 9 ++
>>>> 6 files changed, 260 insertions(+), 2 deletions(-)
>>>> create mode 100644 fs/nfsd/localio.c
>>>>
>>>> diff --git a/fs/nfsd/Makefile b/fs/nfsd/Makefile
>>>> index b8736a82e57c..78b421778a79 100644
>>>> --- a/fs/nfsd/Makefile
>>>> +++ b/fs/nfsd/Makefile
>>>> @@ -23,3 +23,4 @@ nfsd-$(CONFIG_NFSD_PNFS) += nfs4layouts.o
>>>> nfsd-$(CONFIG_NFSD_BLOCKLAYOUT) += blocklayout.o blocklayoutxdr.o
>>>> nfsd-$(CONFIG_NFSD_SCSILAYOUT) += blocklayout.o blocklayoutxdr.o
>>>> nfsd-$(CONFIG_NFSD_FLEXFILELAYOUT) += flexfilelayout.o flexfilelayoutxdr.o
>>>> +nfsd-$(CONFIG_NFSD_LOCALIO) += localio.o
>>>> diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
>>>> index ad9083ca144b..99631fa56662 100644
>>>> --- a/fs/nfsd/filecache.c
>>>> +++ b/fs/nfsd/filecache.c
>>>> @@ -52,7 +52,7 @@
>>>> #define NFSD_FILE_CACHE_UP (0)
>>>>
>>>> /* We only care about NFSD_MAY_READ/WRITE for this cache */
>>>> -#define NFSD_FILE_MAY_MASK (NFSD_MAY_READ|NFSD_MAY_WRITE)
>>>> +#define NFSD_FILE_MAY_MASK (NFSD_MAY_READ|NFSD_MAY_WRITE|NFSD_MAY_LOCALIO)
>>>>
>>>> static DEFINE_PER_CPU(unsigned long, nfsd_file_cache_hits);
>>>> static DEFINE_PER_CPU(unsigned long, nfsd_file_acquisitions);
>>>> diff --git a/fs/nfsd/localio.c b/fs/nfsd/localio.c
>>>> new file mode 100644
>>>> index 000000000000..ba9187735947
>>>> --- /dev/null
>>>> +++ b/fs/nfsd/localio.c
>>>> @@ -0,0 +1,246 @@
>>>> +// SPDX-License-Identifier: GPL-2.0-only
>>>> +/*
>>>> + * NFS server support for local clients to bypass network stack
>>>> + *
>>>> + * Copyright (C) 2014 Weston Andros Adamson <dros@xxxxxxxxxxxxxxx>
>>>> + * Copyright (C) 2019 Trond Myklebust <trond.myklebust@xxxxxxxxxxxxxxx>
>>>> + * Copyright (C) 2024 Mike Snitzer <snitzer@xxxxxxxxxxxxxxx>
>>>> + */
>>>> +
>>>> +#include <linux/exportfs.h>
>>>> +#include <linux/sunrpc/svcauth_gss.h>
>>>> +#include <linux/sunrpc/clnt.h>
>>>> +#include <linux/nfs.h>
>>>> +#include <linux/string.h>
>>>> +
>>>> +#include "nfsd.h"
>>>> +#include "vfs.h"
>>>> +#include "netns.h"
>>>> +#include "filecache.h"
>>>> +
>>>> +#define NFSDDBG_FACILITY NFSDDBG_FH
>>>> +
>>>> +/*
>>>> + * We need to translate between nfs status return values and
>>>> + * the local errno values which may not be the same.
>>>> + * - duplicated from fs/nfs/nfs2xdr.c to avoid needless bloat of
>>>> + * all compiled nfs objects if it were in include/linux/nfs.h
>>>> + */
>>>> +static const struct {
>>>> + int stat;
>>>> + int errno;
>>>> +} nfs_common_errtbl[] = {
>>>> + { NFS_OK, 0 },
>>>> + { NFSERR_PERM, -EPERM },
>>>> + { NFSERR_NOENT, -ENOENT },
>>>> + { NFSERR_IO, -EIO },
>>>> + { NFSERR_NXIO, -ENXIO },
>>>> +/* { NFSERR_EAGAIN, -EAGAIN }, */
>>>> + { NFSERR_ACCES, -EACCES },
>>>> + { NFSERR_EXIST, -EEXIST },
>>>> + { NFSERR_XDEV, -EXDEV },
>>>> + { NFSERR_NODEV, -ENODEV },
>>>> + { NFSERR_NOTDIR, -ENOTDIR },
>>>> + { NFSERR_ISDIR, -EISDIR },
>>>> + { NFSERR_INVAL, -EINVAL },
>>>> + { NFSERR_FBIG, -EFBIG },
>>>> + { NFSERR_NOSPC, -ENOSPC },
>>>> + { NFSERR_ROFS, -EROFS },
>>>> + { NFSERR_MLINK, -EMLINK },
>>>> + { NFSERR_NAMETOOLONG, -ENAMETOOLONG },
>>>> + { NFSERR_NOTEMPTY, -ENOTEMPTY },
>>>> + { NFSERR_DQUOT, -EDQUOT },
>>>> + { NFSERR_STALE, -ESTALE },
>>>> + { NFSERR_REMOTE, -EREMOTE },
>>>> +#ifdef EWFLUSH
>>>> + { NFSERR_WFLUSH, -EWFLUSH },
>>>> +#endif
>>>> + { NFSERR_BADHANDLE, -EBADHANDLE },
>>>> + { NFSERR_NOT_SYNC, -ENOTSYNC },
>>>> + { NFSERR_BAD_COOKIE, -EBADCOOKIE },
>>>> + { NFSERR_NOTSUPP, -ENOTSUPP },
>>>> + { NFSERR_TOOSMALL, -ETOOSMALL },
>>>> + { NFSERR_SERVERFAULT, -EREMOTEIO },
>>>> + { NFSERR_BADTYPE, -EBADTYPE },
>>>> + { NFSERR_JUKEBOX, -EJUKEBOX },
>>>> + { -1, -EIO }
>>>> +};
>>>> +
>>>> +/**
>>>> + * nfs_stat_to_errno - convert an NFS status code to a local errno
>>>> + * @status: NFS status code to convert
>>>> + *
>>>> + * Returns a local errno value, or -EIO if the NFS status code is
>>>> + * not recognized. nfsd_file_acquire() returns an nfsstat that
>>>> + * needs to be translated to an errno before being returned to a
>>>> + * local client application.
>>>> + */
>>>> +static int nfs_stat_to_errno(enum nfs_stat status)
>>>> +{
>>>> + int i;
>>>> +
>>>> + for (i = 0; nfs_common_errtbl[i].stat != -1; i++) {
>>>> + if (nfs_common_errtbl[i].stat == (int)status)
>>>> + return nfs_common_errtbl[i].errno;
>>>> + }
>>>> + return nfs_common_errtbl[i].errno;
>>>> +}
>>>> +
>>>> +static void
>>>> +nfsd_local_fakerqst_destroy(struct svc_rqst *rqstp)
>>>> +{
>>>> + if (rqstp->rq_client)
>>>> + auth_domain_put(rqstp->rq_client);
>>>> + if (rqstp->rq_cred.cr_group_info)
>>>> + put_group_info(rqstp->rq_cred.cr_group_info);
>>>> + /* rpcauth_map_to_svc_cred_local() clears cr_principal */
>>>> + WARN_ON_ONCE(rqstp->rq_cred.cr_principal != NULL);
>>>> + kfree(rqstp->rq_xprt);
>>>> + kfree(rqstp);
>>>> +}
>>>> +
>>>> +static struct svc_rqst *
>>>> +nfsd_local_fakerqst_create(struct net *net, struct rpc_clnt *rpc_clnt,
>>>> + const struct cred *cred)
>>>> +{
>>>> + struct svc_rqst *rqstp;
>>>> + struct nfsd_net *nn = net_generic(net, nfsd_net_id);
>>>> + int status;
>>>> +
>>>> + /* FIXME: not running in nfsd context, must get reference on nfsd_serv */
>>>> + if (unlikely(!READ_ONCE(nn->nfsd_serv))) {
>>>> + dprintk("%s: localio denied. Server not running\n", __func__);
>>>
>>> Chuck mentioned this earlier, but I don't think we ought to merge the
>>> dprintks. If they're useful for debugging then they should be turned
>>> into tracepoints. This one, I'd probably just drop.
>>
>> Right; the problem with dprintk() is they can create so much chatter
>> that the systemd journal will automatically toss those messages and
>> they are lost. No diagnostic value in that.
>>
>> Also you probably won't find it useful if lots of debugging info
>> goes into the trace log but a handful of the stuff you are
>> looking for is dumped into the system journal; the two use different
>> timestamps and so are really hard to line up after the fact.
>>
>> We're trying to transition away from dprintk() in NFSD for these
>> reasons.
>
> OK, I understand wanting to not allow new dprintk() to be added.
>
> Meanwhile:
> $ grep -ri dprintk fs/nfsd/*.[ch] | wc -l
> 181
>
> So I'm struggling to get motivated to convert to tracepoints. Feels
> like a needless make-work hurdle, these could be converted by others
> more proficient with tracepoints if/when needed.
>
> Making everyone have to be proficient at developing debugging via
> tracepoints seems misplaced (but I also understand that forcing others
> to fish enables "others" to not be doing the conversion work).
Trace points are part of the cost of contributing to NFSD,
just like XDR, RCU, lockdep_assert, and dozens of other
kernel facilities. Not a hurdle, and I don't ask for busy
work changes.
> This is the end of my mild protest.. I'll shutup and either convert
> the dprintk()s or kill them all. ;)
IMO, if a dprintk is killable (or you don't know its needed
yet) then it shouldn't be included in the patch series.
--
Chuck Lever
