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

[Goldwyn Rodrigues <rgoldwyn@suse.de>]

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