| Message ID | 20190123185929.46507-1-bfoster@redhat.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | xfs_repair: initialize non-leaf finobt blocks with correct magic | expand |
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 *) >
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 *) > >
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.
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 *)
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(-)