From: Douglas Raillard <[email protected]>
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 <[email protected]>
---
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
On 3/6/2023 5:55 PM, Douglas RAILLARD wrote:
> From: Douglas Raillard <[email protected]>
>
> 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 <[email protected]>
> ---
> 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 <[email protected]>
-Mukesh
> __field(int, depth)
> __field(int, err)
> ),
On Mon, 6 Mar 2023 12:25:49 +0000
Douglas RAILLARD <[email protected]> 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)
> ),
On Mon, 6 Mar 2023 11:15:13 -0500
Steven Rostedt <[email protected]> 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)
On 2023/3/6 20:25, Douglas RAILLARD wrote:
> From: Douglas Raillard <[email protected]>
>
> 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 <[email protected]>
Reviewed-by: Chao Yu <[email protected]>
Thanks,
Hello:
This patch was applied to jaegeuk/f2fs.git (dev)
by Jaegeuk Kim <[email protected]>:
On Mon, 6 Mar 2023 12:25:49 +0000 you wrote:
> From: Douglas Raillard <[email protected]>
>
> 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
On Tue, 07 Mar 2023 17:40:24 +0000
[email protected] wrote:
> Hello:
>
> This patch was applied to jaegeuk/f2fs.git (dev)
> by Jaegeuk Kim <[email protected]>:
>
> On Mon, 6 Mar 2023 12:25:49 +0000 you wrote:
> > From: Douglas Raillard <[email protected]>
> >
> > 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/[email protected]/
-- Steve
On 03/28, Steven Rostedt wrote:
> On Tue, 07 Mar 2023 17:40:24 +0000
> [email protected] wrote:
>
> > Hello:
> >
> > This patch was applied to jaegeuk/f2fs.git (dev)
> > by Jaegeuk Kim <[email protected]>:
> >
> > On Mon, 6 Mar 2023 12:25:49 +0000 you wrote:
> > > From: Douglas Raillard <[email protected]>
> > >
> > > 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/[email protected]/T/#u
Thanks,
>
> [1] https://patchwork.kernel.org/project/linux-trace-kernel/patch/[email protected]/
>
> -- Steve
On Tue, 28 Mar 2023 17:36:06 -0700
Jaegeuk Kim <[email protected]> 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/[email protected]/T/#u
Much appreciated!
-- Steve
On Tue, Mar 28, 2023 at 5:36 PM Jaegeuk Kim <[email protected]> wrote:
>
> Thanks for heads-up. I sent a pull request having the above patch to Linus.
.. and it's merged now.
Linus
On Mon, Mar 06, 2023 at 12:25:49PM +0000, Douglas RAILLARD wrote:
> From: Douglas Raillard <[email protected]>
>
> 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 <[email protected]>
> ---
> 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.
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 <[email protected]>
> >
> > 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 <[email protected]>
> > ---
> > 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
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 <[email protected]>
> > >
> > > 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 <[email protected]>
> > > ---
> > > 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