Re: [PATCH v2 31/31] nfsd: convert dprintks in check_export() to tracepoints

[Jeff Layton <jlayton@kernel.org>]

On Mon, 2026-01-19 at 11:47 -0500, Chuck Lever wrote:
> On 1/19/26 11:26 AM, Jeff Layton wrote:
> > Get rid of the dprintk messages in check_export(). Instead add new
> > tracepoints that show the terminal inode and the flags.
> > 
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> >  fs/nfsd/export.c | 11 ++++++-----
> >  fs/nfsd/trace.h  | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 58 insertions(+), 5 deletions(-)
> > 
> > diff --git a/fs/nfsd/export.c b/fs/nfsd/export.c
> > index bc703cf58bfa210c7c57d49f22f15bc10d7cfc91..3cc336b953b38573966c43000f31cd341380837b 100644
> > --- a/fs/nfsd/export.c
> > +++ b/fs/nfsd/export.c
> > @@ -435,31 +435,32 @@ static int check_export(const struct path *path, int *flags, unsigned char *uuid
> >  	if (!(inode->i_sb->s_type->fs_flags & FS_REQUIRES_DEV) &&
> >  	    !(*flags & NFSEXP_FSID) &&
> >  	    uuid == NULL) {
> > -		dprintk("exp_export: export of non-dev fs without fsid\n");
> > +		trace_nfsd_check_export_need_fsid(inode, *flags);
> >  		return -EINVAL;
> >  	}
> >  
> >  	if (!exportfs_can_decode_fh(inode->i_sb->s_export_op)) {
> > -		dprintk("exp_export: export of invalid fs type.\n");
> > +		trace_nfsd_check_export_invalid_fstype(inode, *flags);
> >  		return -EINVAL;
> >  	}
> >  
> >  	if (!(inode->i_sb->s_export_op->flags & EXPORT_OP_STABLE_HANDLES)) {
> > -		dprintk("%s: fs does not provide stable filehandles!\n", __func__);
> > +		trace_nfsd_check_export_no_stable_fh(inode, *flags);
> >  		return -EINVAL;
> >  	}
> >  
> >  	if (is_idmapped_mnt(path->mnt)) {
> >  		dprintk("exp_export: export of idmapped mounts not yet supported.\n");

Doh! I left the above dprintk() in -- fixed in tree.

> > +		trace_nfsd_check_export_idmapped(inode, *flags);
> >  		return -EINVAL;
> >  	}
> >  
> >  	if (inode->i_sb->s_export_op->flags & EXPORT_OP_NOSUBTREECHK &&
> >  	    !(*flags & NFSEXP_NOSUBTREECHECK)) {
> > -		dprintk("%s: %s does not support subtree checking!\n",
> > -			__func__, inode->i_sb->s_type->name);
> > +		trace_nfsd_check_export_subtree(inode, *flags);
> >  		return -EINVAL;
> >  	}
> > +	trace_nfsd_check_export_success(inode, *flags);
> >  	return 0;
> >  }
> >  
> > diff --git a/fs/nfsd/trace.h b/fs/nfsd/trace.h
> > index 5ae2a611e57f4b4e51a4d9eb6e0fccb66ad8d288..e3f5fe1181b605b34cb70d53f32739c3ef9b82f6 100644
> > --- a/fs/nfsd/trace.h
> > +++ b/fs/nfsd/trace.h
> > @@ -339,6 +339,58 @@ DEFINE_EVENT_CONDITION(nfsd_fh_err_class, nfsd_##name,	\
> >  DEFINE_NFSD_FH_ERR_EVENT(set_fh_dentry_badexport);
> >  DEFINE_NFSD_FH_ERR_EVENT(set_fh_dentry_badhandle);
> >  
> > +#define show_export_flags(val)						\
> 
> Whacky. I thought we had one of these already, but I can't find one.
> 
> 
> > +	__print_flags(val, "|",						\
> > +		{ NFSEXP_READONLY,		"READONLY" },		\
> > +		{ NFSEXP_INSECURE_PORT,		"INSECURE" },		\
> > +		{ NFSEXP_ROOTSQUASH,		"ROOTSQUASH" },		\
> > +		{ NFSEXP_ALLSQUASH,		"ALLSQUASH" },		\
> > +		{ NFSEXP_ASYNC,			"ASYNC" },		\
> > +		{ NFSEXP_GATHERED_WRITES,	"GATHERED_WRITES" },	\
> > +		{ NFSEXP_NOREADDIRPLUS,		"NOREADDIRPLUS" },	\
> > +		{ NFSEXP_SECURITY_LABEL,	"SECURITY_LABEL" },	\
> > +		{ NFSEXP_NOHIDE,		"NOHIDE" },		\
> > +		{ NFSEXP_NOSUBTREECHECK,	"NOSUBTREECHECK" },	\
> > +		{ NFSEXP_NOAUTHNLM,		"NOAUTHNLM" },		\
> > +		{ NFSEXP_MSNFS,			"MSNFS" },		\
> > +		{ NFSEXP_FSID,			"FSID" },		\
> > +		{ NFSEXP_CROSSMOUNT,		"CROSSMOUNT" },		\
> > +		{ NFSEXP_NOACL,			"NOACL" },		\
> > +		{ NFSEXP_V4ROOT,		"V4ROOT" },		\
> > +		{ NFSEXP_PNFS,			"PNFS" })
> > +
> > +DECLARE_EVENT_CLASS(nfsd_check_export_class,
> > +	TP_PROTO(const struct inode *inode,
> > +		 int flags),
> > +	TP_ARGS(inode, flags),
> > +	TP_STRUCT__entry(
> > +		__field(dev_t, dev)
> > +		__field(ino_t, ino)
> > +		__field(int, flags)
> > +	),
> > +	TP_fast_assign(
> > +		__entry->dev = inode->i_sb->s_dev;
> > +		__entry->ino = inode->i_ino;
> > +		__entry->flags = flags;
> > +	),
> > +	TP_printk("dev=%u:%u:%lu flags=%s",
> > +		  MAJOR(__entry->dev), MINOR(__entry->dev),
> > +		  __entry->ino, show_export_flags(__entry->flags))
> > +)
> > +
> > +#define DEFINE_NFSD_CHECK_EXPORT_EVENT(name)			\
> > +DEFINE_EVENT(nfsd_check_export_class, nfsd_check_export_##name,	\
> > +	TP_PROTO(const struct inode *inode,			\
> > +		 int flags),					\
> > +	TP_ARGS(inode, flags))
> > +
> > +DEFINE_NFSD_CHECK_EXPORT_EVENT(need_fsid);
> > +DEFINE_NFSD_CHECK_EXPORT_EVENT(invalid_fstype);
> > +DEFINE_NFSD_CHECK_EXPORT_EVENT(no_stable_fh);
> > +DEFINE_NFSD_CHECK_EXPORT_EVENT(idmapped);
> > +DEFINE_NFSD_CHECK_EXPORT_EVENT(subtree);
> > +DEFINE_NFSD_CHECK_EXPORT_EVENT(success);
> > +
> >  TRACE_EVENT(nfsd_exp_find_key,
> >  	TP_PROTO(const struct svc_expkey *key,
> >  		 int status),
> > 
> 
> 'Twould be nice to report the namespace or client address that
> was making the failing request, but maybe that information is
> not available in check_export.
>
> Reviewed-by: Chuck Lever <chuck.lever@oracle.com>
> 

That might be possible, but it means refactoring check_export().

Given that we're still haggling over the flag name and semantics, lets
drop this patch from the series for now.

Once the other bits are settled, I'll respin the tracepoint patches on
top. That part can be better sorted out on the linux-nfs ml anyway.

Thanks!
-- 
Jeff Layton <jlayton@kernel.org>