Re: [PATCH v7 06/20] nfs/nfsd: add "localio" support

[Chuck Lever <chuck.lever@oracle.com>]

On Tue, Jun 25, 2024 at 12:57:54AM -0400, Mike Snitzer wrote:
> On Mon, Jun 24, 2024 at 02:26:42PM -0400, Chuck Lever wrote:
> > On Mon, Jun 24, 2024 at 12:27:27PM -0400, Mike Snitzer wrote:
> > > From: Weston Andros Adamson <dros@primarydata.com>
> > > diff --git a/fs/nfsd/localio.c b/fs/nfsd/localio.c
> > > new file mode 100644
> > > index 000000000000..e9aa0997f898
> > > --- /dev/null
> > > +++ b/fs/nfsd/localio.c
> > > @@ -0,0 +1,244 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only
> > > +/*
> > > + * NFS server support for local clients to bypass network stack
> > > + *
> > > + * Copyright (C) 2014 Weston Andros Adamson <dros@primarydata.com>
> > > + * Copyright (C) 2019 Trond Myklebust <trond.myklebust@hammerspace.com>
> > > + * Copyright (C) 2024 Mike Snitzer <snitzer@hammerspace.com>
> > > + */
> > > +
> > > +#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
> > 
> > I think I'd rather prefer to see trace points in here rather than
> > new dprintk call sites. In any event, perhaps NFSDDBG_FH is not
> > especially appropriate?
> 
> I think NFSDDBG_FH is most appropriate given this localio is most
> focused on opening a file handle.  (The getuuid not so much)

If more than one debugging facility is appropriate, consider using
dfprintk() instead so each call site can specify its preferred
facility.


> I'm not loving how overdone the trace points interface is. in my
> experience, trace points are also a serious source of conflicts when
> backporting changes to older kernels.
> 
> But if you were inclined to switch this code over to trace points once
> it is merged, who am I to stop you! ;)

Have a heartfelt conversation with yourself about whether all of
these dprintk's need to stay, then.

Anything you've been using strictly as a development tool can be
considered disposable. Anything an admin or support engineer might
want to enable to diagnose a local configuration or deployment issue
is a candidate for staying in the code. And so on and so forth.


-- 
Chuck Lever