| Message ID | 20230306122549.236561-1-douglas.raillard@arm.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 3ccc99d5a4eac7ef3595d8d0cb284c7c5710daec |
| Headers | show |
| Series | [f2fs-dev] f2fs: Fix f2fs_truncate_partial_nodes ftrace event | expand |
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) > ),
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) > ),
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)
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,
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!
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
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
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
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
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.
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
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
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) ),