* [PATCH v2] btrfs: trace use file_inode(file)->i_sb to calculate fs_info @ 2026-02-19 1:49 Goldwyn Rodrigues 2026-02-19 9:18 ` Filipe Manana 0 siblings, 1 reply; 5+ messages in thread From: Goldwyn Rodrigues @ 2026-02-19 1:49 UTC (permalink / raw) To: linux-btrfs If overlay is used on top of btrfs, dentry->d_sb translates to overlay's super block and fsid assignment will lead to a crash. Use file_inode(file)->i_sb to always get btrfs_sb. Changes since v1: Changed subject to include trace Use file_inode() to get inode pointer Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com> Reviewed-by: Qu Wenruo <wqu@suse.com> Reviewed-by: Boris Burkov <boris@bur.io> --- include/trace/events/btrfs.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/include/trace/events/btrfs.h b/include/trace/events/btrfs.h index 125bdc166bfe..92118df217b4 100644 --- a/include/trace/events/btrfs.h +++ b/include/trace/events/btrfs.h @@ -770,9 +770,9 @@ TRACE_EVENT(btrfs_sync_file, TP_fast_assign( const struct dentry *dentry = file->f_path.dentry; - const struct inode *inode = d_inode(dentry); + const struct inode *inode = file_inode(file); - TP_fast_assign_fsid(btrfs_sb(file->f_path.dentry->d_sb)); + TP_fast_assign_fsid(btrfs_sb(inode->i_sb)); __entry->ino = btrfs_ino(BTRFS_I(inode)); __entry->parent = btrfs_ino(BTRFS_I(d_inode(dentry->d_parent))); __entry->datasync = datasync; -- 2.53.0 -- Goldwyn ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2] btrfs: trace use file_inode(file)->i_sb to calculate fs_info 2026-02-19 1:49 [PATCH v2] btrfs: trace use file_inode(file)->i_sb to calculate fs_info Goldwyn Rodrigues @ 2026-02-19 9:18 ` Filipe Manana 2026-02-19 11:52 ` Filipe Manana 0 siblings, 1 reply; 5+ messages in thread From: Filipe Manana @ 2026-02-19 9:18 UTC (permalink / raw) To: Goldwyn Rodrigues; +Cc: linux-btrfs On Thu, Feb 19, 2026 at 1:50 AM Goldwyn Rodrigues <rgoldwyn@suse.de> wrote: > > If overlay is used on top of btrfs, dentry->d_sb translates to overlay's > super block and fsid assignment will lead to a crash. > > Use file_inode(file)->i_sb to always get btrfs_sb. > > Changes since v1: > Changed subject to include trace > Use file_inode() to get inode pointer Information about what changes between patch versions doesn't go into the change log, it goes below the line marked as "---", as that's irrelevant information to have in git, it's only useful for patch reviews. This subject: "btrfs: trace use file_inode(file)->i_sb to calculate fs_info" is also odd, using a C expression, not saying where (which trace event) and not saying what problem are we fixing but rather how are we fixing the problem. I suggest something much more clear and concise such as: "btrfs: fix a crash in the trace event btrfs_sync_file()" One further comment below. > > Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com> > Reviewed-by: Qu Wenruo <wqu@suse.com> > Reviewed-by: Boris Burkov <boris@bur.io> > --- > include/trace/events/btrfs.h | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/include/trace/events/btrfs.h b/include/trace/events/btrfs.h > index 125bdc166bfe..92118df217b4 100644 > --- a/include/trace/events/btrfs.h > +++ b/include/trace/events/btrfs.h > @@ -770,9 +770,9 @@ TRACE_EVENT(btrfs_sync_file, > > TP_fast_assign( > const struct dentry *dentry = file->f_path.dentry; Shouldn't we also use file_dentry(file) here? I think we should, otherwise we get the same bug that was fixed in: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=de17e793b104d690e1d007dfc5cb6b4f649598ca > - const struct inode *inode = d_inode(dentry); > + const struct inode *inode = file_inode(file); > > - TP_fast_assign_fsid(btrfs_sb(file->f_path.dentry->d_sb)); > + TP_fast_assign_fsid(btrfs_sb(inode->i_sb)); > __entry->ino = btrfs_ino(BTRFS_I(inode)); > __entry->parent = btrfs_ino(BTRFS_I(d_inode(dentry->d_parent))); And here, why didn't you replace d_inode() with file_inode() like above? Thanks. > __entry->datasync = datasync; > -- > 2.53.0 > > > -- > Goldwyn > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] btrfs: trace use file_inode(file)->i_sb to calculate fs_info 2026-02-19 9:18 ` Filipe Manana @ 2026-02-19 11:52 ` Filipe Manana 2026-02-25 13:31 ` Goldwyn Rodrigues 0 siblings, 1 reply; 5+ messages in thread From: Filipe Manana @ 2026-02-19 11:52 UTC (permalink / raw) To: Goldwyn Rodrigues; +Cc: linux-btrfs On Thu, Feb 19, 2026 at 9:18 AM Filipe Manana <fdmanana@kernel.org> wrote: > > On Thu, Feb 19, 2026 at 1:50 AM Goldwyn Rodrigues <rgoldwyn@suse.de> wrote: > > > > If overlay is used on top of btrfs, dentry->d_sb translates to overlay's > > super block and fsid assignment will lead to a crash. > > > > Use file_inode(file)->i_sb to always get btrfs_sb. > > > > Changes since v1: > > Changed subject to include trace > > Use file_inode() to get inode pointer > > Information about what changes between patch versions doesn't go into > the change log, it goes below the line marked as "---", as that's > irrelevant information to have in git, it's only useful for patch > reviews. > > This subject: > > "btrfs: trace use file_inode(file)->i_sb to calculate fs_info" > > is also odd, using a C expression, not saying where (which trace > event) and not saying what problem are we fixing but rather how are we > fixing the problem. > > I suggest something much more clear and concise such as: > > "btrfs: fix a crash in the trace event btrfs_sync_file()" > > One further comment below. > > > > > Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com> > > Reviewed-by: Qu Wenruo <wqu@suse.com> > > Reviewed-by: Boris Burkov <boris@bur.io> > > --- > > include/trace/events/btrfs.h | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/include/trace/events/btrfs.h b/include/trace/events/btrfs.h > > index 125bdc166bfe..92118df217b4 100644 > > --- a/include/trace/events/btrfs.h > > +++ b/include/trace/events/btrfs.h > > @@ -770,9 +770,9 @@ TRACE_EVENT(btrfs_sync_file, > > > > TP_fast_assign( > > const struct dentry *dentry = file->f_path.dentry; > > Shouldn't we also use file_dentry(file) here? > > I think we should, otherwise we get the same bug that was fixed in: > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=de17e793b104d690e1d007dfc5cb6b4f649598ca > > > - const struct inode *inode = d_inode(dentry); > > + const struct inode *inode = file_inode(file); > > > > - TP_fast_assign_fsid(btrfs_sb(file->f_path.dentry->d_sb)); > > + TP_fast_assign_fsid(btrfs_sb(inode->i_sb)); > > __entry->ino = btrfs_ino(BTRFS_I(inode)); > > __entry->parent = btrfs_ino(BTRFS_I(d_inode(dentry->d_parent))); > > And here, why didn't you replace d_inode() with file_inode() like above? Actually I meant using dget_parent(dentry) to get the parent here, so it should be: btrfs_ino(BTRFS_I(d_inode(dget_parent(dentry))) After the following replacement at the top as mentioned before: const struct dentry *dentry = file->f_path.dentry; with const struct dentry *dentry = file_dentry(file); > > Thanks. > > > __entry->datasync = datasync; > > -- > > 2.53.0 > > > > > > -- > > Goldwyn > > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] btrfs: trace use file_inode(file)->i_sb to calculate fs_info 2026-02-19 11:52 ` Filipe Manana @ 2026-02-25 13:31 ` Goldwyn Rodrigues 2026-02-25 13:46 ` Filipe Manana 0 siblings, 1 reply; 5+ messages in thread From: Goldwyn Rodrigues @ 2026-02-25 13:31 UTC (permalink / raw) To: Filipe Manana; +Cc: linux-btrfs On 11:52 19/02, Filipe Manana wrote: > On Thu, Feb 19, 2026 at 9:18 AM Filipe Manana <fdmanana@kernel.org> wrote: > > > > On Thu, Feb 19, 2026 at 1:50 AM Goldwyn Rodrigues <rgoldwyn@suse.de> wrote: > > > > > > If overlay is used on top of btrfs, dentry->d_sb translates to overlay's > > > super block and fsid assignment will lead to a crash. > > > > > > Use file_inode(file)->i_sb to always get btrfs_sb. > > > > > > Changes since v1: > > > Changed subject to include trace > > > Use file_inode() to get inode pointer > > > > Information about what changes between patch versions doesn't go into > > the change log, it goes below the line marked as "---", as that's > > irrelevant information to have in git, it's only useful for patch > > reviews. > > > > This subject: > > > > "btrfs: trace use file_inode(file)->i_sb to calculate fs_info" > > > > is also odd, using a C expression, not saying where (which trace > > event) and not saying what problem are we fixing but rather how are we > > fixing the problem. > > > > I suggest something much more clear and concise such as: > > > > "btrfs: fix a crash in the trace event btrfs_sync_file()" > > Understood. > > One further comment below. Response inline.. > > > > > > > > Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com> > > > Reviewed-by: Qu Wenruo <wqu@suse.com> > > > Reviewed-by: Boris Burkov <boris@bur.io> > > > --- > > > include/trace/events/btrfs.h | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > diff --git a/include/trace/events/btrfs.h b/include/trace/events/btrfs.h > > > index 125bdc166bfe..92118df217b4 100644 > > > --- a/include/trace/events/btrfs.h > > > +++ b/include/trace/events/btrfs.h > > > @@ -770,9 +770,9 @@ TRACE_EVENT(btrfs_sync_file, > > > > > > TP_fast_assign( > > > const struct dentry *dentry = file->f_path.dentry; > > > > Shouldn't we also use file_dentry(file) here? > > > > I think we should, otherwise we get the same bug that was fixed in: > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=de17e793b104d690e1d007dfc5cb6b4f649598ca > > No. file_dentry() is obsolete now. Check the function implementation and the comment before it. > > > - const struct inode *inode = d_inode(dentry); > > > + const struct inode *inode = file_inode(file); > > > > > > - TP_fast_assign_fsid(btrfs_sb(file->f_path.dentry->d_sb)); > > > + TP_fast_assign_fsid(btrfs_sb(inode->i_sb)); > > > __entry->ino = btrfs_ino(BTRFS_I(inode)); > > > __entry->parent = btrfs_ino(BTRFS_I(d_inode(dentry->d_parent))); > > > > And here, why didn't you replace d_inode() with file_inode() like above? > > Actually I meant using dget_parent(dentry) to get the parent here, so > it should be: > > btrfs_ino(BTRFS_I(d_inode(dget_parent(dentry))) > > After the following replacement at the top as mentioned before: > > const struct dentry *dentry = file->f_path.dentry; > > with > > const struct dentry *dentry = file_dentry(file); .. this would need to be followed with a dput(). This is quite a lot of computation for a ftrace function. Since we are just printing the inode number of the parent, can we just assign inode->i_ino as opposed to btrfs_ino()? I understand it would show the overlayfs inode number as opposed to the BTRFS one, and may not be the same, but isn't that you would look for when you are debugging? -- Goldwyn ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] btrfs: trace use file_inode(file)->i_sb to calculate fs_info 2026-02-25 13:31 ` Goldwyn Rodrigues @ 2026-02-25 13:46 ` Filipe Manana 0 siblings, 0 replies; 5+ messages in thread From: Filipe Manana @ 2026-02-25 13:46 UTC (permalink / raw) To: Goldwyn Rodrigues; +Cc: linux-btrfs On Wed, Feb 25, 2026 at 1:31 PM Goldwyn Rodrigues <rgoldwyn@suse.de> wrote: > > On 11:52 19/02, Filipe Manana wrote: > > On Thu, Feb 19, 2026 at 9:18 AM Filipe Manana <fdmanana@kernel.org> wrote: > > > > > > On Thu, Feb 19, 2026 at 1:50 AM Goldwyn Rodrigues <rgoldwyn@suse.de> wrote: > > > > > > > > If overlay is used on top of btrfs, dentry->d_sb translates to overlay's > > > > super block and fsid assignment will lead to a crash. > > > > > > > > Use file_inode(file)->i_sb to always get btrfs_sb. > > > > > > > > Changes since v1: > > > > Changed subject to include trace > > > > Use file_inode() to get inode pointer > > > > > > Information about what changes between patch versions doesn't go into > > > the change log, it goes below the line marked as "---", as that's > > > irrelevant information to have in git, it's only useful for patch > > > reviews. > > > > > > This subject: > > > > > > "btrfs: trace use file_inode(file)->i_sb to calculate fs_info" > > > > > > is also odd, using a C expression, not saying where (which trace > > > event) and not saying what problem are we fixing but rather how are we > > > fixing the problem. > > > > > > I suggest something much more clear and concise such as: > > > > > > "btrfs: fix a crash in the trace event btrfs_sync_file()" > > > > > Understood. > > > > One further comment below. > > Response inline.. > > > > > > > > > > > > Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com> > > > > Reviewed-by: Qu Wenruo <wqu@suse.com> > > > > Reviewed-by: Boris Burkov <boris@bur.io> > > > > --- > > > > include/trace/events/btrfs.h | 4 ++-- > > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/include/trace/events/btrfs.h b/include/trace/events/btrfs.h > > > > index 125bdc166bfe..92118df217b4 100644 > > > > --- a/include/trace/events/btrfs.h > > > > +++ b/include/trace/events/btrfs.h > > > > @@ -770,9 +770,9 @@ TRACE_EVENT(btrfs_sync_file, > > > > > > > > TP_fast_assign( > > > > const struct dentry *dentry = file->f_path.dentry; > > > > > > Shouldn't we also use file_dentry(file) here? > > > > > > I think we should, otherwise we get the same bug that was fixed in: > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=de17e793b104d690e1d007dfc5cb6b4f649598ca > > > > > No. file_dentry() is obsolete now. Check the function implementation and > the comment before it. > > > > > - const struct inode *inode = d_inode(dentry); > > > > + const struct inode *inode = file_inode(file); > > > > > > > > - TP_fast_assign_fsid(btrfs_sb(file->f_path.dentry->d_sb)); > > > > + TP_fast_assign_fsid(btrfs_sb(inode->i_sb)); > > > > __entry->ino = btrfs_ino(BTRFS_I(inode)); > > > > __entry->parent = btrfs_ino(BTRFS_I(d_inode(dentry->d_parent))); > > > > > > And here, why didn't you replace d_inode() with file_inode() like above? > > > > Actually I meant using dget_parent(dentry) to get the parent here, so > > it should be: > > > > btrfs_ino(BTRFS_I(d_inode(dget_parent(dentry))) > > > > After the following replacement at the top as mentioned before: > > > > const struct dentry *dentry = file->f_path.dentry; > > > > with > > > > const struct dentry *dentry = file_dentry(file); > > .. this would need to be followed with a dput(). > > This is quite a lot of computation for a ftrace function. Since we are > just printing the inode number of the parent, can we just assign > inode->i_ino as opposed to btrfs_ino()? I understand it would show the > overlayfs inode number as opposed to the BTRFS one, and may not be the > same, but isn't that you would look for when you are debugging? When I look at the trace, I only care about the btrfs inode number, that's what helps me debug btrfs issues - the overlayfs inode number is useless for what I need. > > -- > Goldwyn ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2026-02-25 13:46 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2026-02-19 1:49 [PATCH v2] btrfs: trace use file_inode(file)->i_sb to calculate fs_info Goldwyn Rodrigues 2026-02-19 9:18 ` Filipe Manana 2026-02-19 11:52 ` Filipe Manana 2026-02-25 13:31 ` Goldwyn Rodrigues 2026-02-25 13:46 ` Filipe Manana
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox