Re: [PATCH v2 04/12] nfsd: add a tracepoint for nfsd_setattr

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

On 4/9/25 10:32 AM, Jeff Layton wrote:
> Turn Sargun's internal kprobe based implementation of this into a normal
> static tracepoint. Also, remove the dprintk's that got added recently
> with the fix for zero-length ACLs.
> 
> Cc: Sargun Dillon <sargun@sargun.me>
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>  fs/nfsd/trace.h         | 35 +++++++++++++++++++++++++++++++++++
>  fs/nfsd/vfs.c           |  5 ++---
>  include/trace/misc/fs.h | 21 +++++++++++++++++++++
>  3 files changed, 58 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/nfsd/trace.h b/fs/nfsd/trace.h
> index 0d49fc064f7273f32c93732a993fd77bc0783f5d..c496fed58e2eed15458f35a158fbfef39a972c55 100644
> --- a/fs/nfsd/trace.h
> +++ b/fs/nfsd/trace.h
> @@ -11,6 +11,7 @@
>  #include <linux/tracepoint.h>
>  #include <linux/sunrpc/clnt.h>
>  #include <linux/sunrpc/xprt.h>
> +#include <trace/misc/fs.h>
>  #include <trace/misc/nfs.h>
>  #include <trace/misc/sunrpc.h>
>  
> @@ -2337,6 +2338,40 @@ DEFINE_EVENT(nfsd_copy_async_done_class,		\
>  DEFINE_COPY_ASYNC_DONE_EVENT(done);
>  DEFINE_COPY_ASYNC_DONE_EVENT(cancel);
>  
> +TRACE_EVENT(nfsd_setattr,
> +	TP_PROTO(const struct svc_rqst *rqstp, const struct svc_fh *fhp,
> +		 const struct iattr *iap, const struct timespec64 *guardtime),
> +	TP_ARGS(rqstp, fhp, iap, guardtime),
> +	TP_STRUCT__entry(
> +		SVC_RQST_ENDPOINT_FIELDS(rqstp)
> +		__field(u32, fh_hash)
> +		__field(s64, gtime_tv_sec)
> +		__field(u32, gtime_tv_nsec)
> +		__field(unsigned int, ia_valid)
> +		__field(loff_t, ia_size)
> +		__field(uid_t, ia_uid)
> +		__field(gid_t, ia_gid)
> +		__field(umode_t, ia_mode)
> +	),
> +	TP_fast_assign(__entry->xid = be32_to_cpu(rqstp->rq_xid);
> +		SVC_RQST_ENDPOINT_ASSIGNMENTS(rqstp);
> +		__entry->fh_hash = knfsd_fh_hash(&fhp->fh_handle);
> +		__entry->gtime_tv_sec = guardtime ? guardtime->tv_sec : 0;
> +		__entry->gtime_tv_nsec = guardtime ? guardtime->tv_nsec : 0;
> +		__entry->ia_valid = iap->ia_valid;
> +		__entry->ia_size = iap->ia_size;
> +		__entry->ia_uid = __kuid_val(iap->ia_uid);
> +		__entry->ia_gid = __kgid_val(iap->ia_gid);
> +		__entry->ia_mode = iap->ia_mode;
> +	),
> +	TP_printk(
> +		"xid=0x%08x fh_hash=0x%08x ia_valid=%s ia_size=%llu ia_mode=0%o ia_uid=%u ia_gid=%u guard_time=%lld.%u",
> +		__entry->xid, __entry->fh_hash, show_ia_valid_flags(__entry->ia_valid),
> +		__entry->ia_size, __entry->ia_mode, __entry->ia_uid, __entry->ia_gid,
> +		__entry->gtime_tv_sec, __entry->gtime_tv_nsec
> +	)
> +)
> +
>  #endif /* _NFSD_TRACE_H */
>  
>  #undef TRACE_INCLUDE_PATH
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index d1156a18a79579bf427fe5809dc93d06e241201e..77ae22abc1a21ec587cf089b2a5f750464b5e985 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -501,7 +501,8 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  	bool		size_change = (iap->ia_valid & ATTR_SIZE);
>  	int		retries;
>  
> -dprintk("nfsd_setattr pacl=%p valid=0x%x\n", attr->na_pacl, iap->ia_valid);

Nit:

These unindented dprintk call sites were introduced by Rick's POSIX
ACL patch series, which is experimental. He's promised to remove them
before he sends a final version of that series.

Your patch can leave them in place, as they will be gone soon by other
means.


> +	trace_nfsd_setattr(rqstp, fhp, iap, guardtime);
> +
>  	if (iap->ia_valid & ATTR_SIZE) {
>  		accmode |= NFSD_MAY_WRITE|NFSD_MAY_OWNER_OVERRIDE;
>  		ftype = S_IFREG;
> @@ -597,7 +598,6 @@ dprintk("nfsd_setattr pacl=%p valid=0x%x\n", attr->na_pacl, iap->ia_valid);
>  						NULL);
>  	}
>  	if (IS_ENABLED(CONFIG_FS_POSIX_ACL) && attr->na_pacl) {
> -dprintk("at set_posix_acl\n");
>  		/*
>  		 * For any file system that is not ACL_SCOPE_FILE_OBJECT,
>  		 * a_count == 0 MUST reply nfserr_inval.
> @@ -612,7 +612,6 @@ dprintk("at set_posix_acl\n");
>  							attr->na_pacl);
>  		else
>  			attr->na_paclerr = -EINVAL;
> -dprintk("set_posix_acl=%d\n", attr->na_paclerr);
>  	}
>  out_fill_attrs:
>  	/*
> diff --git a/include/trace/misc/fs.h b/include/trace/misc/fs.h
> index 738b97f22f3651f2370830037a8f4bfdf9a42ad4..0406ebe2a80a499dfcadb7e63db4d9e4a84d4d64 100644
> --- a/include/trace/misc/fs.h
> +++ b/include/trace/misc/fs.h
> @@ -120,3 +120,24 @@
>  		{ LOOKUP_BENEATH,	"BENEATH" }, \
>  		{ LOOKUP_IN_ROOT,	"IN_ROOT" }, \
>  		{ LOOKUP_CACHED,	"CACHED" })
> +
> +#define show_ia_valid_flags(flags)			\
> +	__print_flags(flags, "|",			\
> +		{ ATTR_MODE,		"MODE" },	\
> +		{ ATTR_UID,		"UID" },	\
> +		{ ATTR_GID,		"GID" },	\
> +		{ ATTR_SIZE,		"SIZE" },	\
> +		{ ATTR_ATIME,		"ATIME" },	\
> +		{ ATTR_MTIME,		"MTIME" },	\
> +		{ ATTR_CTIME,		"CTIME" },	\
> +		{ ATTR_ATIME_SET,	"ATIME_SET" },	\
> +		{ ATTR_MTIME_SET,	"MTIME_SET" },	\
> +		{ ATTR_FORCE,		"FORCE" },	\
> +		{ ATTR_KILL_SUID,	"KILL_SUID" },	\
> +		{ ATTR_KILL_SGID,	"KILL_SGID" },	\
> +		{ ATTR_FILE,		"FILE" },	\
> +		{ ATTR_KILL_PRIV,	"KILL_PRIV" },	\
> +		{ ATTR_OPEN,		"OPEN" },	\
> +		{ ATTR_TIMES_SET,	"TIMES_SET" },	\
> +		{ ATTR_TOUCH,		"TOUCH"},	\
> +		{ ATTR_DELEG,		"DELEG"})
> 


-- 
Chuck Lever