Re: [PATCH] xfs: fix file_path handling in tracepoints

[Christoph Hellwig <hch@infradead.org>]

On Wed, Jul 10, 2024 at 10:43:53PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Since file_path() takes the output buffer as one of its arguments, we
> might as well have it format directly into the tracepoint's char array
> instead of wasting stack space.

This looks sensible to me, but..

The nicest way to format the interesting parts of a file path is to
simply use the magic %pD printk specificer, which removes the entire
need for an extra buffer.  It would be kinda nice to use that for
tracing, but I can't see how to accomodate the users of the binary
trace buffer with that.  Adding Steven and the trace list for
comments.

> -		__array(char, pathname, 256)
> +		__array(char, pathname, MAXNAMELEN)
>  	),
>  	TP_fast_assign(
> -		char		pathname[257];
>  		char		*path;
>  
>  		__entry->ino = file_inode(xf->file)->i_ino;
> +		path = file_path(xf->file, __entry->pathname, MAXNAMELEN);
>  		if (IS_ERR(path))
> +			strncpy(__entry->pathname, "(unknown)",
> +					sizeof(__entry->pathname));
>  	),
>  	TP_printk("xfino 0x%lx path '%s'",
>  		  __entry->ino,
> diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
> index 56c8333a470bb..0dfb698a43aa4 100644
> --- a/fs/xfs/xfs_trace.h
> +++ b/fs/xfs/xfs_trace.h
> @@ -4715,20 +4715,18 @@ TRACE_EVENT(xmbuf_create,
>  	TP_STRUCT__entry(
>  		__field(dev_t, dev)
>  		__field(unsigned long, ino)
> -		__array(char, pathname, 256)
> +		__array(char, pathname, MAXNAMELEN)
>  	),
>  	TP_fast_assign(
> -		char		pathname[257];
>  		char		*path;
>  		struct file	*file = btp->bt_file;
>  
>  		__entry->dev = btp->bt_mount->m_super->s_dev;
>  		__entry->ino = file_inode(file)->i_ino;
> -		memset(pathname, 0, sizeof(pathname));
> -		path = file_path(file, pathname, sizeof(pathname) - 1);
> +		path = file_path(file, __entry->pathname, MAXNAMELEN);
>  		if (IS_ERR(path))
> -			path = "(unknown)";
> -		strncpy(__entry->pathname, path, sizeof(__entry->pathname));
> +			strncpy(__entry->pathname, "(unknown)",
> +					sizeof(__entry->pathname));
>  	),
>  	TP_printk("dev %d:%d xmino 0x%lx path '%s'",
>  		  MAJOR(__entry->dev), MINOR(__entry->dev),
---end quoted text---

----- End forwarded message -----