* [PATCH] xfs_repair: initialize non-leaf finobt blocks with correct magic
@ 2019-01-23 18:59 Brian Foster
2019-01-23 19:32 ` Eric Sandeen
2019-01-23 20:51 ` Dave Chinner
0 siblings, 2 replies; 4+ messages in thread
From: Brian Foster @ 2019-01-23 18:59 UTC (permalink / raw)
To: linux-xfs; +Cc: Lucas Stach
The free inode btree construction code in xfs_repair has a bug where
any non-leaf nodes outside of the leftmost block at the associated
level in the tree are incorrectly initialized with the inobt magic
value. Update the prop_ino_cursor() path responsible for growing the
non-leaf portion of the inode btrees to use the btnum of the
specific tree being generated rather than the hardcoded inode btree
type.
Signed-off-by: Brian Foster <bfoster@redhat.com>
Reported-by: Lucas Stach <l.stach@pengutronix.de>
Root-caused-by: Lucas Stach <l.stach@pengutronix.de>
---
repair/phase5.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/repair/phase5.c b/repair/phase5.c
index 85d1f4fb..05333f11 100644
--- a/repair/phase5.c
+++ b/repair/phase5.c
@@ -983,7 +983,7 @@ init_ino_cursor(xfs_mount_t *mp, xfs_agnumber_t agno, bt_status_t *btree_curs,
static void
prop_ino_cursor(xfs_mount_t *mp, xfs_agnumber_t agno, bt_status_t *btree_curs,
- xfs_agino_t startino, int level)
+ xfs_btnum_t btnum, xfs_agino_t startino, int level)
{
struct xfs_btree_block *bt_hdr;
xfs_inobt_key_t *bt_key;
@@ -1005,7 +1005,7 @@ prop_ino_cursor(xfs_mount_t *mp, xfs_agnumber_t agno, bt_status_t *btree_curs,
* first path up the left side of the tree
* where the agbno's are already set up
*/
- prop_ino_cursor(mp, agno, btree_curs, startino, level);
+ prop_ino_cursor(mp, agno, btree_curs, btnum, startino, level);
}
if (be16_to_cpu(bt_hdr->bb_numrecs) ==
@@ -1041,7 +1041,7 @@ prop_ino_cursor(xfs_mount_t *mp, xfs_agnumber_t agno, bt_status_t *btree_curs,
lptr->buf_p->b_ops = &xfs_inobt_buf_ops;
bt_hdr = XFS_BUF_TO_BLOCK(lptr->buf_p);
memset(bt_hdr, 0, mp->m_sb.sb_blocksize);
- libxfs_btree_init_block(mp, lptr->buf_p, XFS_BTNUM_INO,
+ libxfs_btree_init_block(mp, lptr->buf_p, btnum,
level, 0, agno, 0);
bt_hdr->bb_u.s.bb_leftsib = cpu_to_be32(lptr->prev_agbno);
@@ -1049,7 +1049,7 @@ prop_ino_cursor(xfs_mount_t *mp, xfs_agnumber_t agno, bt_status_t *btree_curs,
/*
* propagate extent record for first extent in new block up
*/
- prop_ino_cursor(mp, agno, btree_curs, startino, level);
+ prop_ino_cursor(mp, agno, btree_curs, btnum, startino, level);
}
/*
* add inode info to current block
@@ -1201,7 +1201,7 @@ build_ino_tree(xfs_mount_t *mp, xfs_agnumber_t agno,
lptr->modulo--;
if (lptr->num_recs_pb > 0)
- prop_ino_cursor(mp, agno, btree_curs,
+ prop_ino_cursor(mp, agno, btree_curs, btnum,
ino_rec->ino_startnum, 0);
bt_rec = (xfs_inobt_rec_t *)
--
2.17.2
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] xfs_repair: initialize non-leaf finobt blocks with correct magic
2019-01-23 18:59 [PATCH] xfs_repair: initialize non-leaf finobt blocks with correct magic Brian Foster
@ 2019-01-23 19:32 ` Eric Sandeen
2019-01-23 20:49 ` Brian Foster
2019-01-23 20:51 ` Dave Chinner
1 sibling, 1 reply; 4+ messages in thread
From: Eric Sandeen @ 2019-01-23 19:32 UTC (permalink / raw)
To: Brian Foster, linux-xfs; +Cc: Lucas Stach
On 1/23/19 12:59 PM, Brian Foster wrote:
> The free inode btree construction code in xfs_repair has a bug where
> any non-leaf nodes outside of the leftmost block at the associated
> level in the tree are incorrectly initialized with the inobt magic
> value. Update the prop_ino_cursor() path responsible for growing the
> non-leaf portion of the inode btrees to use the btnum of the
> specific tree being generated rather than the hardcoded inode btree
> type.
>
> Signed-off-by: Brian Foster <bfoster@redhat.com>
> Reported-by: Lucas Stach <l.stach@pengutronix.de>
> Root-caused-by: Lucas Stach <l.stach@pengutronix.de>
Looks good to me,
Reviewed-by: Eric Sandeen <sandeen@redhat.com>
Interesting how the other btrees (rmap, refcount) have their own
prop_$FOO_cursor() functions, but prop_ino_cursor() shares code
for the 2 trees.
> ---
> repair/phase5.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/repair/phase5.c b/repair/phase5.c
> index 85d1f4fb..05333f11 100644
> --- a/repair/phase5.c
> +++ b/repair/phase5.c
> @@ -983,7 +983,7 @@ init_ino_cursor(xfs_mount_t *mp, xfs_agnumber_t agno, bt_status_t *btree_curs,
>
> static void
> prop_ino_cursor(xfs_mount_t *mp, xfs_agnumber_t agno, bt_status_t *btree_curs,
> - xfs_agino_t startino, int level)
> + xfs_btnum_t btnum, xfs_agino_t startino, int level)
> {
> struct xfs_btree_block *bt_hdr;
> xfs_inobt_key_t *bt_key;
> @@ -1005,7 +1005,7 @@ prop_ino_cursor(xfs_mount_t *mp, xfs_agnumber_t agno, bt_status_t *btree_curs,
> * first path up the left side of the tree
> * where the agbno's are already set up
> */
> - prop_ino_cursor(mp, agno, btree_curs, startino, level);
> + prop_ino_cursor(mp, agno, btree_curs, btnum, startino, level);
> }
>
> if (be16_to_cpu(bt_hdr->bb_numrecs) ==
> @@ -1041,7 +1041,7 @@ prop_ino_cursor(xfs_mount_t *mp, xfs_agnumber_t agno, bt_status_t *btree_curs,
> lptr->buf_p->b_ops = &xfs_inobt_buf_ops;
> bt_hdr = XFS_BUF_TO_BLOCK(lptr->buf_p);
> memset(bt_hdr, 0, mp->m_sb.sb_blocksize);
> - libxfs_btree_init_block(mp, lptr->buf_p, XFS_BTNUM_INO,
> + libxfs_btree_init_block(mp, lptr->buf_p, btnum,
> level, 0, agno, 0);
>
> bt_hdr->bb_u.s.bb_leftsib = cpu_to_be32(lptr->prev_agbno);
> @@ -1049,7 +1049,7 @@ prop_ino_cursor(xfs_mount_t *mp, xfs_agnumber_t agno, bt_status_t *btree_curs,
> /*
> * propagate extent record for first extent in new block up
> */
> - prop_ino_cursor(mp, agno, btree_curs, startino, level);
> + prop_ino_cursor(mp, agno, btree_curs, btnum, startino, level);
> }
> /*
> * add inode info to current block
> @@ -1201,7 +1201,7 @@ build_ino_tree(xfs_mount_t *mp, xfs_agnumber_t agno,
> lptr->modulo--;
>
> if (lptr->num_recs_pb > 0)
> - prop_ino_cursor(mp, agno, btree_curs,
> + prop_ino_cursor(mp, agno, btree_curs, btnum,
> ino_rec->ino_startnum, 0);
>
> bt_rec = (xfs_inobt_rec_t *)
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] xfs_repair: initialize non-leaf finobt blocks with correct magic
2019-01-23 19:32 ` Eric Sandeen
@ 2019-01-23 20:49 ` Brian Foster
0 siblings, 0 replies; 4+ messages in thread
From: Brian Foster @ 2019-01-23 20:49 UTC (permalink / raw)
To: Eric Sandeen; +Cc: linux-xfs, Lucas Stach
On Wed, Jan 23, 2019 at 01:32:32PM -0600, Eric Sandeen wrote:
> On 1/23/19 12:59 PM, Brian Foster wrote:
> > The free inode btree construction code in xfs_repair has a bug where
> > any non-leaf nodes outside of the leftmost block at the associated
> > level in the tree are incorrectly initialized with the inobt magic
> > value. Update the prop_ino_cursor() path responsible for growing the
> > non-leaf portion of the inode btrees to use the btnum of the
> > specific tree being generated rather than the hardcoded inode btree
> > type.
> >
> > Signed-off-by: Brian Foster <bfoster@redhat.com>
> > Reported-by: Lucas Stach <l.stach@pengutronix.de>
> > Root-caused-by: Lucas Stach <l.stach@pengutronix.de>
>
> Looks good to me,
>
> Reviewed-by: Eric Sandeen <sandeen@redhat.com>
>
> Interesting how the other btrees (rmap, refcount) have their own
> prop_$FOO_cursor() functions, but prop_ino_cursor() shares code
> for the 2 trees.
>
I think the fact that this inobt reconstruction code already existed at
the time the finobt was added kind of dictated the code sharing
approach. The underlying format is almost exactly the same, we otherwise
just skip the records we don't care about (without any free inodes).
Thanks for the review. BTW, this is probably something we want to
backport to whatever stable variants we have going. Let me know if I
should send additional patches for that (though I suspect this would
apply as is).
Brian
> > ---
> > repair/phase5.c | 10 +++++-----
> > 1 file changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/repair/phase5.c b/repair/phase5.c
> > index 85d1f4fb..05333f11 100644
> > --- a/repair/phase5.c
> > +++ b/repair/phase5.c
> > @@ -983,7 +983,7 @@ init_ino_cursor(xfs_mount_t *mp, xfs_agnumber_t agno, bt_status_t *btree_curs,
> >
> > static void
> > prop_ino_cursor(xfs_mount_t *mp, xfs_agnumber_t agno, bt_status_t *btree_curs,
> > - xfs_agino_t startino, int level)
> > + xfs_btnum_t btnum, xfs_agino_t startino, int level)
> > {
> > struct xfs_btree_block *bt_hdr;
> > xfs_inobt_key_t *bt_key;
> > @@ -1005,7 +1005,7 @@ prop_ino_cursor(xfs_mount_t *mp, xfs_agnumber_t agno, bt_status_t *btree_curs,
> > * first path up the left side of the tree
> > * where the agbno's are already set up
> > */
> > - prop_ino_cursor(mp, agno, btree_curs, startino, level);
> > + prop_ino_cursor(mp, agno, btree_curs, btnum, startino, level);
> > }
> >
> > if (be16_to_cpu(bt_hdr->bb_numrecs) ==
> > @@ -1041,7 +1041,7 @@ prop_ino_cursor(xfs_mount_t *mp, xfs_agnumber_t agno, bt_status_t *btree_curs,
> > lptr->buf_p->b_ops = &xfs_inobt_buf_ops;
> > bt_hdr = XFS_BUF_TO_BLOCK(lptr->buf_p);
> > memset(bt_hdr, 0, mp->m_sb.sb_blocksize);
> > - libxfs_btree_init_block(mp, lptr->buf_p, XFS_BTNUM_INO,
> > + libxfs_btree_init_block(mp, lptr->buf_p, btnum,
> > level, 0, agno, 0);
> >
> > bt_hdr->bb_u.s.bb_leftsib = cpu_to_be32(lptr->prev_agbno);
> > @@ -1049,7 +1049,7 @@ prop_ino_cursor(xfs_mount_t *mp, xfs_agnumber_t agno, bt_status_t *btree_curs,
> > /*
> > * propagate extent record for first extent in new block up
> > */
> > - prop_ino_cursor(mp, agno, btree_curs, startino, level);
> > + prop_ino_cursor(mp, agno, btree_curs, btnum, startino, level);
> > }
> > /*
> > * add inode info to current block
> > @@ -1201,7 +1201,7 @@ build_ino_tree(xfs_mount_t *mp, xfs_agnumber_t agno,
> > lptr->modulo--;
> >
> > if (lptr->num_recs_pb > 0)
> > - prop_ino_cursor(mp, agno, btree_curs,
> > + prop_ino_cursor(mp, agno, btree_curs, btnum,
> > ino_rec->ino_startnum, 0);
> >
> > bt_rec = (xfs_inobt_rec_t *)
> >
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] xfs_repair: initialize non-leaf finobt blocks with correct magic
2019-01-23 18:59 [PATCH] xfs_repair: initialize non-leaf finobt blocks with correct magic Brian Foster
2019-01-23 19:32 ` Eric Sandeen
@ 2019-01-23 20:51 ` Dave Chinner
1 sibling, 0 replies; 4+ messages in thread
From: Dave Chinner @ 2019-01-23 20:51 UTC (permalink / raw)
To: Brian Foster; +Cc: linux-xfs, Lucas Stach
On Wed, Jan 23, 2019 at 01:59:29PM -0500, Brian Foster wrote:
> The free inode btree construction code in xfs_repair has a bug where
> any non-leaf nodes outside of the leftmost block at the associated
> level in the tree are incorrectly initialized with the inobt magic
> value. Update the prop_ino_cursor() path responsible for growing the
> non-leaf portion of the inode btrees to use the btnum of the
> specific tree being generated rather than the hardcoded inode btree
> type.
>
> Signed-off-by: Brian Foster <bfoster@redhat.com>
> Reported-by: Lucas Stach <l.stach@pengutronix.de>
> Root-caused-by: Lucas Stach <l.stach@pengutronix.de>
Looks fine, but one minor thing I noticed when looking at the other
cursor functions that handled different tree types was that they
assert the valid btnums at the start of the prop_?_cursor()
function. I note that build_ino_tree() has a similar assert:
ASSERT(btnum == XFS_BTNUM_INO || btnum == XFS_BTNUM_FINO);
might be worth adding just for consistency with the rest of the
code?
Other than that,
Reviewed-by: Dave Chinner <dchinner@redhat.com>
-Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-01-23 20:51 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-01-23 18:59 [PATCH] xfs_repair: initialize non-leaf finobt blocks with correct magic Brian Foster
2019-01-23 19:32 ` Eric Sandeen
2019-01-23 20:49 ` Brian Foster
2019-01-23 20:51 ` Dave Chinner
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox