Re: [PATCH v2] btrfs: trace use file_inode(file)->i_sb to calculate fs_info

[Filipe Manana <fdmanana@kernel.org>]

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