diff mbox series

[01/17] xfs: fix incorrect decoding in xchk_btree_cur_fsbno

Message ID 163424262046.756780.2366797746965376855.stgit@magnolia (mailing list archive)
State Superseded, archived
Headers show
Series xfs: support dynamic btree cursor height | expand

Commit Message

Darrick J. Wong Oct. 14, 2021, 8:17 p.m. UTC 
From: Darrick J. Wong <djwong@kernel.org>

During review of subsequent patches, Dave and I noticed that this
function doesn't work quite right -- accessing cur->bc_ino depends on
the ROOT_IN_INODE flag, not LONG_PTRS.  Fix that and the parentheses
isssue.  While we're at it, remove the piece that accesses cur->bc_ag,
because block 0 of an AG is never part of a btree.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/scrub/trace.c |    7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

Comments

Dave Chinner Oct. 14, 2021, 10:48 p.m. UTC | #1 
On Thu, Oct 14, 2021 at 01:17:00PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> During review of subsequent patches, Dave and I noticed that this
> function doesn't work quite right -- accessing cur->bc_ino depends on
> the ROOT_IN_INODE flag, not LONG_PTRS.  Fix that and the parentheses
> isssue.  While we're at it, remove the piece that accesses cur->bc_ag,
> because block 0 of an AG is never part of a btree.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  fs/xfs/scrub/trace.c |    7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> 
> diff --git a/fs/xfs/scrub/trace.c b/fs/xfs/scrub/trace.c
> index c0ef53fe6611..93c13763c15e 100644
> --- a/fs/xfs/scrub/trace.c
> +++ b/fs/xfs/scrub/trace.c
> @@ -24,10 +24,11 @@ xchk_btree_cur_fsbno(
>  	if (level < cur->bc_nlevels && cur->bc_bufs[level])
>  		return XFS_DADDR_TO_FSB(cur->bc_mp,
>  				xfs_buf_daddr(cur->bc_bufs[level]));
> -	if (level == cur->bc_nlevels - 1 && cur->bc_flags & XFS_BTREE_LONG_PTRS)
> +
> +	if (level == cur->bc_nlevels - 1 &&
> +	    (cur->bc_flags & XFS_BTREE_ROOT_IN_INODE))
>  		return XFS_INO_TO_FSB(cur->bc_mp, cur->bc_ino.ip->i_ino);

Ok.

> -	if (!(cur->bc_flags & XFS_BTREE_LONG_PTRS))
> -		return XFS_AGB_TO_FSB(cur->bc_mp, cur->bc_ag.pag->pag_agno, 0);
> +

But doesn't this break the tracing code on short pointers as the
tracing code does:

	TP_fast_assign(
		xfs_fsblock_t fsbno = xchk_btree_cur_fsbno(cur, level);
		...
		__entry->agno = XFS_FSB_TO_AGNO(cur->bc_mp, fsbno);


i.e. the tracing will no longer give the correct agno for per-ag
cursors that don't have any buffers attached to them at the current
level?

Cheers,

Dave.
Darrick J. Wong Oct. 14, 2021, 11:05 p.m. UTC | #2 
On Fri, Oct 15, 2021 at 09:48:13AM +1100, Dave Chinner wrote:
> On Thu, Oct 14, 2021 at 01:17:00PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > During review of subsequent patches, Dave and I noticed that this
> > function doesn't work quite right -- accessing cur->bc_ino depends on
> > the ROOT_IN_INODE flag, not LONG_PTRS.  Fix that and the parentheses
> > isssue.  While we're at it, remove the piece that accesses cur->bc_ag,
> > because block 0 of an AG is never part of a btree.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> >  fs/xfs/scrub/trace.c |    7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> > 
> > 
> > diff --git a/fs/xfs/scrub/trace.c b/fs/xfs/scrub/trace.c
> > index c0ef53fe6611..93c13763c15e 100644
> > --- a/fs/xfs/scrub/trace.c
> > +++ b/fs/xfs/scrub/trace.c
> > @@ -24,10 +24,11 @@ xchk_btree_cur_fsbno(
> >  	if (level < cur->bc_nlevels && cur->bc_bufs[level])
> >  		return XFS_DADDR_TO_FSB(cur->bc_mp,
> >  				xfs_buf_daddr(cur->bc_bufs[level]));
> > -	if (level == cur->bc_nlevels - 1 && cur->bc_flags & XFS_BTREE_LONG_PTRS)
> > +
> > +	if (level == cur->bc_nlevels - 1 &&
> > +	    (cur->bc_flags & XFS_BTREE_ROOT_IN_INODE))
> >  		return XFS_INO_TO_FSB(cur->bc_mp, cur->bc_ino.ip->i_ino);
> 
> Ok.
> 
> > -	if (!(cur->bc_flags & XFS_BTREE_LONG_PTRS))
> > -		return XFS_AGB_TO_FSB(cur->bc_mp, cur->bc_ag.pag->pag_agno, 0);
> > +
> 
> But doesn't this break the tracing code on short pointers as the
> tracing code does:
> 
> 	TP_fast_assign(
> 		xfs_fsblock_t fsbno = xchk_btree_cur_fsbno(cur, level);
> 		...
> 		__entry->agno = XFS_FSB_TO_AGNO(cur->bc_mp, fsbno);
> 
> 
> i.e. the tracing will no longer give the correct agno for per-ag
> cursors that don't have any buffers attached to them at the current
> level?

Hmmm.  By that logic, maybe we should get rid of the (level ==
cur->bc_nlevels - 1) check entirely so that any cursor without a buffer
attached at that level will always provide *some* kind of breadcrumb to
the tracepoints?

I almost did that instead, except for the consideration that if you're
tracing the online fsck code, you should /probably/ have at least one of
xchk_stop/xrep_attempt/xchk_done included in the filter list so you can
be certain of what the kernel is checking.

--D

> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
Dave Chinner Oct. 14, 2021, 11:17 p.m. UTC | #3 
On Thu, Oct 14, 2021 at 04:05:49PM -0700, Darrick J. Wong wrote:
> On Fri, Oct 15, 2021 at 09:48:13AM +1100, Dave Chinner wrote:
> > On Thu, Oct 14, 2021 at 01:17:00PM -0700, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <djwong@kernel.org>
> > > 
> > > During review of subsequent patches, Dave and I noticed that this
> > > function doesn't work quite right -- accessing cur->bc_ino depends on
> > > the ROOT_IN_INODE flag, not LONG_PTRS.  Fix that and the parentheses
> > > isssue.  While we're at it, remove the piece that accesses cur->bc_ag,
> > > because block 0 of an AG is never part of a btree.
> > > 
> > > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > > ---
> > >  fs/xfs/scrub/trace.c |    7 ++++---
> > >  1 file changed, 4 insertions(+), 3 deletions(-)
> > > 
> > > 
> > > diff --git a/fs/xfs/scrub/trace.c b/fs/xfs/scrub/trace.c
> > > index c0ef53fe6611..93c13763c15e 100644
> > > --- a/fs/xfs/scrub/trace.c
> > > +++ b/fs/xfs/scrub/trace.c
> > > @@ -24,10 +24,11 @@ xchk_btree_cur_fsbno(
> > >  	if (level < cur->bc_nlevels && cur->bc_bufs[level])
> > >  		return XFS_DADDR_TO_FSB(cur->bc_mp,
> > >  				xfs_buf_daddr(cur->bc_bufs[level]));
> > > -	if (level == cur->bc_nlevels - 1 && cur->bc_flags & XFS_BTREE_LONG_PTRS)
> > > +
> > > +	if (level == cur->bc_nlevels - 1 &&
> > > +	    (cur->bc_flags & XFS_BTREE_ROOT_IN_INODE))
> > >  		return XFS_INO_TO_FSB(cur->bc_mp, cur->bc_ino.ip->i_ino);
> > 
> > Ok.
> > 
> > > -	if (!(cur->bc_flags & XFS_BTREE_LONG_PTRS))
> > > -		return XFS_AGB_TO_FSB(cur->bc_mp, cur->bc_ag.pag->pag_agno, 0);
> > > +
> > 
> > But doesn't this break the tracing code on short pointers as the
> > tracing code does:
> > 
> > 	TP_fast_assign(
> > 		xfs_fsblock_t fsbno = xchk_btree_cur_fsbno(cur, level);
> > 		...
> > 		__entry->agno = XFS_FSB_TO_AGNO(cur->bc_mp, fsbno);
> > 
> > 
> > i.e. the tracing will no longer give the correct agno for per-ag
> > cursors that don't have any buffers attached to them at the current
> > level?
> 
> Hmmm.  By that logic, maybe we should get rid of the (level ==
> cur->bc_nlevels - 1) check entirely so that any cursor without a buffer
> attached at that level will always provide *some* kind of breadcrumb to
> the tracepoints?

Perhaps so.

> I almost did that instead, except for the consideration that if you're
> tracing the online fsck code, you should /probably/ have at least one of
> xchk_stop/xrep_attempt/xchk_done included in the filter list so you can
> be certain of what the kernel is checking.

It doesn't worry me how we resolve this - I just pointed it out
because it's not mentioned in the commit message and I wanted to
make sure it wasn't an oversight. If there are better ways to get
this information from tracepoints and XFS_FSB_TO_AGNO() does
something safe and obvious with NULLFSBLOCK (e.g. ends up as -1)
then as long as there's mention of this change in the commit message
I'm fine with it as it is.

Cheers,

Dave.
Darrick J. Wong Oct. 15, 2021, 12:37 a.m. UTC | #4 
On Fri, Oct 15, 2021 at 10:17:02AM +1100, Dave Chinner wrote:
> On Thu, Oct 14, 2021 at 04:05:49PM -0700, Darrick J. Wong wrote:
> > On Fri, Oct 15, 2021 at 09:48:13AM +1100, Dave Chinner wrote:
> > > On Thu, Oct 14, 2021 at 01:17:00PM -0700, Darrick J. Wong wrote:
> > > > From: Darrick J. Wong <djwong@kernel.org>
> > > > 
> > > > During review of subsequent patches, Dave and I noticed that this
> > > > function doesn't work quite right -- accessing cur->bc_ino depends on
> > > > the ROOT_IN_INODE flag, not LONG_PTRS.  Fix that and the parentheses
> > > > isssue.  While we're at it, remove the piece that accesses cur->bc_ag,
> > > > because block 0 of an AG is never part of a btree.
> > > > 
> > > > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > > > ---
> > > >  fs/xfs/scrub/trace.c |    7 ++++---
> > > >  1 file changed, 4 insertions(+), 3 deletions(-)
> > > > 
> > > > 
> > > > diff --git a/fs/xfs/scrub/trace.c b/fs/xfs/scrub/trace.c
> > > > index c0ef53fe6611..93c13763c15e 100644
> > > > --- a/fs/xfs/scrub/trace.c
> > > > +++ b/fs/xfs/scrub/trace.c
> > > > @@ -24,10 +24,11 @@ xchk_btree_cur_fsbno(
> > > >  	if (level < cur->bc_nlevels && cur->bc_bufs[level])
> > > >  		return XFS_DADDR_TO_FSB(cur->bc_mp,
> > > >  				xfs_buf_daddr(cur->bc_bufs[level]));
> > > > -	if (level == cur->bc_nlevels - 1 && cur->bc_flags & XFS_BTREE_LONG_PTRS)
> > > > +
> > > > +	if (level == cur->bc_nlevels - 1 &&
> > > > +	    (cur->bc_flags & XFS_BTREE_ROOT_IN_INODE))
> > > >  		return XFS_INO_TO_FSB(cur->bc_mp, cur->bc_ino.ip->i_ino);
> > > 
> > > Ok.
> > > 
> > > > -	if (!(cur->bc_flags & XFS_BTREE_LONG_PTRS))
> > > > -		return XFS_AGB_TO_FSB(cur->bc_mp, cur->bc_ag.pag->pag_agno, 0);
> > > > +
> > > 
> > > But doesn't this break the tracing code on short pointers as the
> > > tracing code does:
> > > 
> > > 	TP_fast_assign(
> > > 		xfs_fsblock_t fsbno = xchk_btree_cur_fsbno(cur, level);
> > > 		...
> > > 		__entry->agno = XFS_FSB_TO_AGNO(cur->bc_mp, fsbno);
> > > 
> > > 
> > > i.e. the tracing will no longer give the correct agno for per-ag
> > > cursors that don't have any buffers attached to them at the current
> > > level?
> > 
> > Hmmm.  By that logic, maybe we should get rid of the (level ==
> > cur->bc_nlevels - 1) check entirely so that any cursor without a buffer
> > attached at that level will always provide *some* kind of breadcrumb to
> > the tracepoints?
> 
> Perhaps so.
> 
> > I almost did that instead, except for the consideration that if you're
> > tracing the online fsck code, you should /probably/ have at least one of
> > xchk_stop/xrep_attempt/xchk_done included in the filter list so you can
> > be certain of what the kernel is checking.
> 
> It doesn't worry me how we resolve this - I just pointed it out
> because it's not mentioned in the commit message and I wanted to
> make sure it wasn't an oversight. If there are better ways to get
> this information from tracepoints and XFS_FSB_TO_AGNO() does
> something safe and obvious with NULLFSBLOCK (e.g. ends up as -1)
> then as long as there's mention of this change in the commit message
> I'm fine with it as it is.

Ok.  I'll add the following to the commit message:

"Note: This changes the btree scrubber tracepoints behavior -- if the
cursor has no buffer for a certain level, it will always report
NULLFSBLOCK.  It is assumed that anyone tracing the online fsck code
will also be tracing xchk_start/xchk_done or otherwise be aware of what
exactly is being scrubbed."

--D

> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
diff mbox series

Patch

diff --git a/fs/xfs/scrub/trace.c b/fs/xfs/scrub/trace.c
index c0ef53fe6611..93c13763c15e 100644
--- a/fs/xfs/scrub/trace.c
+++ b/fs/xfs/scrub/trace.c
@@ -24,10 +24,11 @@  xchk_btree_cur_fsbno(
 	if (level < cur->bc_nlevels && cur->bc_bufs[level])
 		return XFS_DADDR_TO_FSB(cur->bc_mp,
 				xfs_buf_daddr(cur->bc_bufs[level]));
-	if (level == cur->bc_nlevels - 1 && cur->bc_flags & XFS_BTREE_LONG_PTRS)
+
+	if (level == cur->bc_nlevels - 1 &&
+	    (cur->bc_flags & XFS_BTREE_ROOT_IN_INODE))
 		return XFS_INO_TO_FSB(cur->bc_mp, cur->bc_ino.ip->i_ino);
-	if (!(cur->bc_flags & XFS_BTREE_LONG_PTRS))
-		return XFS_AGB_TO_FSB(cur->bc_mp, cur->bc_ag.pag->pag_agno, 0);
+
 	return NULLFSBLOCK;
 }