[PATCH] f2fs: Fix f2fs_truncate_partial_nodes ftrace event

[PATCH] f2fs: Fix f2fs_truncate_partial_nodes ftrace event

[Douglas RAILLARD]

From: Douglas Raillard <douglas.raillard@arm.com>

Fix the nid_t field so that its size is correctly reported in the text
format embedded in trace.dat files. As it stands, it is reported as
being of size 4:

        field:nid_t nid[3];     offset:24;      size:4; signed:0;

Instead of 12:

        field:nid_t nid[3];     offset:24;      size:12;        signed:0;

This also fixes the reported offset of subsequent fields so that they
match with the actual struct layout.


Signed-off-by: Douglas Raillard <douglas.raillard@arm.com>
---
 include/trace/events/f2fs.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/trace/events/f2fs.h b/include/trace/events/f2fs.h
index 31d994e6b4ca..8d053838d6cf 100644
--- a/include/trace/events/f2fs.h
+++ b/include/trace/events/f2fs.h
@@ -512,7 +512,7 @@ TRACE_EVENT(f2fs_truncate_partial_nodes,
 	TP_STRUCT__entry(
 		__field(dev_t,	dev)
 		__field(ino_t,	ino)
-		__field(nid_t,	nid[3])
+		__array(nid_t,	nid, 3)
 		__field(int,	depth)
 		__field(int,	err)
 	),
-- 
2.25.1

Re: [PATCH] f2fs: Fix f2fs_truncate_partial_nodes ftrace event

[Mukesh Ojha]

On 3/6/2023 5:55 PM, Douglas RAILLARD wrote:
> From: Douglas Raillard <douglas.raillard@arm.com>
> 
> Fix the nid_t field so that its size is correctly reported in the text
> format embedded in trace.dat files. As it stands, it is reported as
> being of size 4:
> 
>          field:nid_t nid[3];     offset:24;      size:4; signed:0;
> 
> Instead of 12:
> 
>          field:nid_t nid[3];     offset:24;      size:12;        signed:0;
> 
> This also fixes the reported offset of subsequent fields so that they
> match with the actual struct layout.
> 
> 
> Signed-off-by: Douglas Raillard <douglas.raillard@arm.com>
> ---
>   include/trace/events/f2fs.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/trace/events/f2fs.h b/include/trace/events/f2fs.h
> index 31d994e6b4ca..8d053838d6cf 100644
> --- a/include/trace/events/f2fs.h
> +++ b/include/trace/events/f2fs.h
> @@ -512,7 +512,7 @@ TRACE_EVENT(f2fs_truncate_partial_nodes,
>   	TP_STRUCT__entry(
>   		__field(dev_t,	dev)
>   		__field(ino_t,	ino)
> -		__field(nid_t,	nid[3])
> +		__array(nid_t,	nid, 3)


Good catch.

Reviewed-by: Mukesh Ojha <quic_mojha@quicinc.com>

-Mukesh

>   		__field(int,	depth)
>   		__field(int,	err)
>   	),

Re: [PATCH] f2fs: Fix f2fs_truncate_partial_nodes ftrace event

[Steven Rostedt]

On Mon,  6 Mar 2023 12:25:49 +0000
Douglas RAILLARD <douglas.raillard@arm.com> wrote:

> diff --git a/include/trace/events/f2fs.h b/include/trace/events/f2fs.h
> index 31d994e6b4ca..8d053838d6cf 100644
> --- a/include/trace/events/f2fs.h
> +++ b/include/trace/events/f2fs.h
> @@ -512,7 +512,7 @@ TRACE_EVENT(f2fs_truncate_partial_nodes,
>  	TP_STRUCT__entry(
>  		__field(dev_t,	dev)
>  		__field(ino_t,	ino)
> -		__field(nid_t,	nid[3])

That should not have even compiled. I'll see if I can add some tricks to
make that fail.

Thanks,

-- Steve


> +		__array(nid_t,	nid, 3)
>  		__field(int,	depth)
>  		__field(int,	err)
>  	),

Re: [PATCH] f2fs: Fix f2fs_truncate_partial_nodes ftrace event

[Steven Rostedt]

On Mon, 6 Mar 2023 11:15:13 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> > diff --git a/include/trace/events/f2fs.h b/include/trace/events/f2fs.h
> > index 31d994e6b4ca..8d053838d6cf 100644
> > --- a/include/trace/events/f2fs.h
> > +++ b/include/trace/events/f2fs.h
> > @@ -512,7 +512,7 @@ TRACE_EVENT(f2fs_truncate_partial_nodes,
> >  	TP_STRUCT__entry(
> >  		__field(dev_t,	dev)
> >  		__field(ino_t,	ino)
> > -		__field(nid_t,	nid[3])  
> 
> That should not have even compiled. I'll see if I can add some tricks to
> make that fail.

And this patch makes that happen!

-- Steve

diff --git a/include/trace/stages/stage5_get_offsets.h b/include/trace/stages/stage5_get_offsets.h
index ac5c24d3beeb..e30a13be46ba 100644
--- a/include/trace/stages/stage5_get_offsets.h
+++ b/include/trace/stages/stage5_get_offsets.h
@@ -9,17 +9,30 @@
 #undef __entry
 #define __entry entry
 
+/*
+ * Fields should never declare an array: i.e. __field(int, arr[5])
+ * If they do, it will cause issues in parsing and possibly corrupt the
+ * events. To prevent that from happening, test the sizeof() a fictitious
+ * type called "struct _test_no_array_##item" which will fail if "item"
+ * contains array elements (like "arr[5]").
+ *
+ * If you hit this, use __array(int, arr, 5) instead.
+ */
 #undef __field
-#define __field(type, item)
+#define __field(type, item)					\
+	{ (void)sizeof(struct _test_no_array_##item *); }
 
 #undef __field_ext
-#define __field_ext(type, item, filter_type)
+#define __field_ext(type, item, filter_type)			\
+	{ (void)sizeof(struct _test_no_array_##item *); }
 
 #undef __field_struct
-#define __field_struct(type, item)
+#define __field_struct(type, item)				\
+	{ (void)sizeof(struct _test_no_array_##item *); }
 
 #undef __field_struct_ext
-#define __field_struct_ext(type, item, filter_type)
+#define __field_struct_ext(type, item, filter_type)		\
+	{ (void)sizeof(struct _test_no_array_##item *); }
 
 #undef __array
 #define __array(type, item, len)

Re: [PATCH] f2fs: Fix f2fs_truncate_partial_nodes ftrace event

[Chao Yu]

On 2023/3/6 20:25, Douglas RAILLARD wrote:
> From: Douglas Raillard <douglas.raillard@arm.com>
> 
> Fix the nid_t field so that its size is correctly reported in the text
> format embedded in trace.dat files. As it stands, it is reported as
> being of size 4:
> 
>          field:nid_t nid[3];     offset:24;      size:4; signed:0;
> 
> Instead of 12:
> 
>          field:nid_t nid[3];     offset:24;      size:12;        signed:0;
> 
> This also fixes the reported offset of subsequent fields so that they
> match with the actual struct layout.
> 
> 
> Signed-off-by: Douglas Raillard <douglas.raillard@arm.com>

Reviewed-by: Chao Yu <chao@kernel.org>

Thanks,

Re: [f2fs-dev] [PATCH] f2fs: Fix f2fs_truncate_partial_nodes ftrace event

[patchwork-bot+f2fs]

Hello:

This patch was applied to jaegeuk/f2fs.git (dev)
by Jaegeuk Kim <jaegeuk@kernel.org>:

On Mon,  6 Mar 2023 12:25:49 +0000 you wrote:
> From: Douglas Raillard <douglas.raillard@arm.com>
> 
> Fix the nid_t field so that its size is correctly reported in the text
> format embedded in trace.dat files. As it stands, it is reported as
> being of size 4:
> 
>         field:nid_t nid[3];     offset:24;      size:4; signed:0;
> 
> [...]

Here is the summary with links:
  - [f2fs-dev] f2fs: Fix f2fs_truncate_partial_nodes ftrace event
    https://git.kernel.org/jaegeuk/f2fs/c/3ccc99d5a4ea

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [f2fs-dev] [PATCH] f2fs: Fix f2fs_truncate_partial_nodes ftrace event

[Steven Rostedt]

On Tue, 07 Mar 2023 17:40:24 +0000
patchwork-bot+f2fs@kernel.org wrote:

> Hello:
> 
> This patch was applied to jaegeuk/f2fs.git (dev)
> by Jaegeuk Kim <jaegeuk@kernel.org>:
> 
> On Mon,  6 Mar 2023 12:25:49 +0000 you wrote:
> > From: Douglas Raillard <douglas.raillard@arm.com>
> > 
> > Fix the nid_t field so that its size is correctly reported in the text
> > format embedded in trace.dat files. As it stands, it is reported as
> > being of size 4:
> > 
> >         field:nid_t nid[3];     offset:24;      size:4; signed:0;
> > 
> > [...]  
> 
> Here is the summary with links:
>   - [f2fs-dev] f2fs: Fix f2fs_truncate_partial_nodes ftrace event
>     https://git.kernel.org/jaegeuk/f2fs/c/3ccc99d5a4ea
> 
> You are awesome, thank you!

I'm hoping you are not waiting for the merge window to get this in. Please
get it to Linus before rc5 is out, because I plan on adding my patch[1]
which will cause this to fail the build at rc5.

The above is a fix to bug that causes unwanted results. Please get it into
mainline ASAP.

[1] https://patchwork.kernel.org/project/linux-trace-kernel/patch/20230309221302.642e82d9@gandalf.local.home/

-- Steve

Re: [f2fs-dev] [PATCH] f2fs: Fix f2fs_truncate_partial_nodes ftrace event

[Jaegeuk Kim]

On 03/28, Steven Rostedt wrote:
> On Tue, 07 Mar 2023 17:40:24 +0000
> patchwork-bot+f2fs@kernel.org wrote:
> 
> > Hello:
> > 
> > This patch was applied to jaegeuk/f2fs.git (dev)
> > by Jaegeuk Kim <jaegeuk@kernel.org>:
> > 
> > On Mon,  6 Mar 2023 12:25:49 +0000 you wrote:
> > > From: Douglas Raillard <douglas.raillard@arm.com>
> > > 
> > > Fix the nid_t field so that its size is correctly reported in the text
> > > format embedded in trace.dat files. As it stands, it is reported as
> > > being of size 4:
> > > 
> > >         field:nid_t nid[3];     offset:24;      size:4; signed:0;
> > > 
> > > [...]  
> > 
> > Here is the summary with links:
> >   - [f2fs-dev] f2fs: Fix f2fs_truncate_partial_nodes ftrace event
> >     https://git.kernel.org/jaegeuk/f2fs/c/3ccc99d5a4ea
> > 
> > You are awesome, thank you!
> 
> I'm hoping you are not waiting for the merge window to get this in. Please
> get it to Linus before rc5 is out, because I plan on adding my patch[1]
> which will cause this to fail the build at rc5.
> 
> The above is a fix to bug that causes unwanted results. Please get it into
> mainline ASAP.

Thanks for heads-up. I sent a pull request having the above patch to Linus.

https://lore.kernel.org/linux-f2fs-devel/ZCOHd4jYn8zUCEZ0@google.com/T/#u

Thanks,

> 
> [1] https://patchwork.kernel.org/project/linux-trace-kernel/patch/20230309221302.642e82d9@gandalf.local.home/
> 
> -- Steve

Re: [f2fs-dev] [PATCH] f2fs: Fix f2fs_truncate_partial_nodes ftrace event

[Steven Rostedt]

On Tue, 28 Mar 2023 17:36:06 -0700
Jaegeuk Kim <jaegeuk@kernel.org> wrote:

> > I'm hoping you are not waiting for the merge window to get this in. Please
> > get it to Linus before rc5 is out, because I plan on adding my patch[1]
> > which will cause this to fail the build at rc5.
> > 
> > The above is a fix to bug that causes unwanted results. Please get it into
> > mainline ASAP.  
> 
> Thanks for heads-up. I sent a pull request having the above patch to Linus.
> 
> https://lore.kernel.org/linux-f2fs-devel/ZCOHd4jYn8zUCEZ0@google.com/T/#u

Much appreciated!

-- Steve

Re: [f2fs-dev] [PATCH] f2fs: Fix f2fs_truncate_partial_nodes ftrace event

[Linus Torvalds]

On Tue, Mar 28, 2023 at 5:36 PM Jaegeuk Kim <jaegeuk@kernel.org> wrote:
>
> Thanks for heads-up. I sent a pull request having the above patch to Linus.

.. and it's merged now.

               Linus

Re: [PATCH] f2fs: Fix f2fs_truncate_partial_nodes ftrace event

[Kajetan Puchalski]

On Mon, Mar 06, 2023 at 12:25:49PM +0000, Douglas RAILLARD wrote:
> From: Douglas Raillard <douglas.raillard@arm.com>
> 
> Fix the nid_t field so that its size is correctly reported in the text
> format embedded in trace.dat files. As it stands, it is reported as
> being of size 4:
> 
>         field:nid_t nid[3];     offset:24;      size:4; signed:0;
> 
> Instead of 12:
> 
>         field:nid_t nid[3];     offset:24;      size:12;        signed:0;
> 
> This also fixes the reported offset of subsequent fields so that they
> match with the actual struct layout.
> 
> 
> Signed-off-by: Douglas Raillard <douglas.raillard@arm.com>
> ---
>  include/trace/events/f2fs.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/trace/events/f2fs.h b/include/trace/events/f2fs.h
> index 31d994e6b4ca..8d053838d6cf 100644
> --- a/include/trace/events/f2fs.h
> +++ b/include/trace/events/f2fs.h
> @@ -512,7 +512,7 @@ TRACE_EVENT(f2fs_truncate_partial_nodes,
>  	TP_STRUCT__entry(
>  		__field(dev_t,	dev)
>  		__field(ino_t,	ino)
> -		__field(nid_t,	nid[3])
> +		__array(nid_t,	nid, 3)
>  		__field(int,	depth)
>  		__field(int,	err)
>  	),
> -- 
> 2.25.1

Hi,

Just wanted to flag that I noticed this breaks Perfetto tracing on
Android, at least as of Android 13. I'm not sure if it's been fixed in newer
versions. Looks like the version of Perfetto in Android 13 is expecting
the previous (ie broken) field format to be there and its entire ftrace
collector fails as a result:

E/perfetto( 3532): ranslation_table.cc:133 Failed to infer ftrace field type for "f2fs_truncate_partial_nodes.nid" (type:"nid_t nid[3]" size:12 signed:0) (errno: 2, No such file or directory)
I/perfetto( 3640):            probes.cc:65 Hard resetting ftrace state.

For my own purposes I just reverted these two:
* 0b04d4c0542e8573a837b1d81b94209e48723b25 (f2fs: Fix f2fs_truncate_partial_nodes ftrace event)
* f82e7ca019dfad3b006fd3b772f7ac569672db55 (tracing: Error if a trace event has an array for a __field()

and now it works fine so not the biggest deal but this should probably
be addressed, I imagine more likely on the Perfetto side.

Re: [PATCH] f2fs: Fix f2fs_truncate_partial_nodes ftrace event

[Kajetan Puchalski]

On Mon, Aug 14, 2023 at 02:32:53PM +0100, Kajetan Puchalski wrote:
> On Mon, Mar 06, 2023 at 12:25:49PM +0000, Douglas RAILLARD wrote:
> > From: Douglas Raillard <douglas.raillard@arm.com>
> > 
> > Fix the nid_t field so that its size is correctly reported in the text
> > format embedded in trace.dat files. As it stands, it is reported as
> > being of size 4:
> > 
> >         field:nid_t nid[3];     offset:24;      size:4; signed:0;
> > 
> > Instead of 12:
> > 
> >         field:nid_t nid[3];     offset:24;      size:12;        signed:0;
> > 
> > This also fixes the reported offset of subsequent fields so that they
> > match with the actual struct layout.
> > 
> > 
> > Signed-off-by: Douglas Raillard <douglas.raillard@arm.com>
> > ---
> >  include/trace/events/f2fs.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/include/trace/events/f2fs.h b/include/trace/events/f2fs.h
> > index 31d994e6b4ca..8d053838d6cf 100644
> > --- a/include/trace/events/f2fs.h
> > +++ b/include/trace/events/f2fs.h
> > @@ -512,7 +512,7 @@ TRACE_EVENT(f2fs_truncate_partial_nodes,
> >  	TP_STRUCT__entry(
> >  		__field(dev_t,	dev)
> >  		__field(ino_t,	ino)
> > -		__field(nid_t,	nid[3])
> > +		__array(nid_t,	nid, 3)
> >  		__field(int,	depth)
> >  		__field(int,	err)
> >  	),
> > -- 
> > 2.25.1
> 
> Hi,
> 
> Just wanted to flag that I noticed this breaks Perfetto tracing on
> Android, at least as of Android 13. I'm not sure if it's been fixed in newer
> versions. Looks like the version of Perfetto in Android 13 is expecting
> the previous (ie broken) field format to be there and its entire ftrace
> collector fails as a result:
> 
> E/perfetto( 3532): ranslation_table.cc:133 Failed to infer ftrace field type for "f2fs_truncate_partial_nodes.nid" (type:"nid_t nid[3]" size:12 signed:0) (errno: 2, No such file or directory)
> I/perfetto( 3640):            probes.cc:65 Hard resetting ftrace state.
> 
> For my own purposes I just reverted these two:
> * 0b04d4c0542e8573a837b1d81b94209e48723b25 (f2fs: Fix f2fs_truncate_partial_nodes ftrace event)
> * f82e7ca019dfad3b006fd3b772f7ac569672db55 (tracing: Error if a trace event has an array for a __field()
> 
> and now it works fine so not the biggest deal but this should probably
> be addressed, I imagine more likely on the Perfetto side.

Added context here, it is just caused by the parser implementation in Perfetto
being pretty lacking:

https://github.com/google/perfetto/blob/c36c70c1d4a72eafdd257f7a63e55f49fbc3df3d/src/traced/probes/ftrace/proto_translation_table.cc#L255

Re: [PATCH] f2fs: Fix f2fs_truncate_partial_nodes ftrace event

[Jaegeuk Kim]

On 08/14, Kajetan Puchalski wrote:
> On Mon, Aug 14, 2023 at 02:32:53PM +0100, Kajetan Puchalski wrote:
> > On Mon, Mar 06, 2023 at 12:25:49PM +0000, Douglas RAILLARD wrote:
> > > From: Douglas Raillard <douglas.raillard@arm.com>
> > > 
> > > Fix the nid_t field so that its size is correctly reported in the text
> > > format embedded in trace.dat files. As it stands, it is reported as
> > > being of size 4:
> > > 
> > >         field:nid_t nid[3];     offset:24;      size:4; signed:0;
> > > 
> > > Instead of 12:
> > > 
> > >         field:nid_t nid[3];     offset:24;      size:12;        signed:0;
> > > 
> > > This also fixes the reported offset of subsequent fields so that they
> > > match with the actual struct layout.
> > > 
> > > 
> > > Signed-off-by: Douglas Raillard <douglas.raillard@arm.com>
> > > ---
> > >  include/trace/events/f2fs.h | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/include/trace/events/f2fs.h b/include/trace/events/f2fs.h
> > > index 31d994e6b4ca..8d053838d6cf 100644
> > > --- a/include/trace/events/f2fs.h
> > > +++ b/include/trace/events/f2fs.h
> > > @@ -512,7 +512,7 @@ TRACE_EVENT(f2fs_truncate_partial_nodes,
> > >  	TP_STRUCT__entry(
> > >  		__field(dev_t,	dev)
> > >  		__field(ino_t,	ino)
> > > -		__field(nid_t,	nid[3])
> > > +		__array(nid_t,	nid, 3)
> > >  		__field(int,	depth)
> > >  		__field(int,	err)
> > >  	),
> > > -- 
> > > 2.25.1
> > 
> > Hi,
> > 
> > Just wanted to flag that I noticed this breaks Perfetto tracing on
> > Android, at least as of Android 13. I'm not sure if it's been fixed in newer
> > versions. Looks like the version of Perfetto in Android 13 is expecting
> > the previous (ie broken) field format to be there and its entire ftrace
> > collector fails as a result:
> > 
> > E/perfetto( 3532): ranslation_table.cc:133 Failed to infer ftrace field type for "f2fs_truncate_partial_nodes.nid" (type:"nid_t nid[3]" size:12 signed:0) (errno: 2, No such file or directory)
> > I/perfetto( 3640):            probes.cc:65 Hard resetting ftrace state.
> > 
> > For my own purposes I just reverted these two:
> > * 0b04d4c0542e8573a837b1d81b94209e48723b25 (f2fs: Fix f2fs_truncate_partial_nodes ftrace event)
> > * f82e7ca019dfad3b006fd3b772f7ac569672db55 (tracing: Error if a trace event has an array for a __field()
> > 
> > and now it works fine so not the biggest deal but this should probably
> > be addressed, I imagine more likely on the Perfetto side.
> 
> Added context here, it is just caused by the parser implementation in Perfetto
> being pretty lacking:
> 
> https://github.com/google/perfetto/blob/c36c70c1d4a72eafdd257f7a63e55f49fbc3df3d/src/traced/probes/ftrace/proto_translation_table.cc#L255

Hi, I believe this was fixed by
https://android-review.git.corp.google.com/c/platform/external/perfetto/+/2587146