| Message ID | 159370362968.3579756.14752877317465395252.stgit@magnolia (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xfs_repair: more fixes | expand |
On Thu, Jul 02, 2020 at 08:27:09AM -0700, Darrick J. Wong wrote: > From: Darrick J. Wong <darrick.wong@oracle.com> > > In commit 9851fd79bfb1, we added a slight amount of slack to the free > space btrees being reconstructed so that the initial fix_freelist call > (which is run against a totally empty AGFL) would never have to split > either free space btree in order to populate the free list. > > The new btree bulk loading code in xfs_repair can re-create this > situation because it can set the slack values to zero if the filesystem > is very full. However, these days repair has the infrastructure needed > to ensure that overestimations of the btree block counts end up on the > AGFL or get freed back into the filesystem at the end of phase 5. > > Fix this problem by reserving extra blocks in the bnobt reservation, and > checking that there are enough overages in the bnobt/cntbt fakeroots to > populate the AGFL with the minimum number of blocks it needs to handle a > split in the bno/cnt/rmap btrees. > > Note that we reserve blocks for the new bnobt/cntbt/AGFL at the very end > of the reservation steps in phase 5, so the extra allocation should not > cause repair to fail if it can't find blocks for btrees. > > Fixes: 9851fd79bfb1 ("repair: AGFL rebuild fails if btree split required") > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> > --- > repair/agbtree.c | 51 ++++++++++++++++++++++++++++++++++++++++++++------- > 1 file changed, 44 insertions(+), 7 deletions(-) > > > diff --git a/repair/agbtree.c b/repair/agbtree.c > index de8015ec..9f64d54b 100644 > --- a/repair/agbtree.c > +++ b/repair/agbtree.c ... > @@ -268,16 +288,33 @@ _("Unable to compute free space by length btree geometry, error %d.\n"), -error) > btr_cnt->bload.nr_blocks; > > /* We don't need any more blocks, so we're done. */ > - if (delta_bno >= 0 && delta_cnt >= 0) { > + if (delta_bno >= 0 && delta_cnt >= 0 && > + delta_bno + delta_cnt >= agfl_goal) { > *extra_blocks = delta_bno + delta_cnt; > break; > } > > /* Allocate however many more blocks we need this time. */ > - if (delta_bno < 0) > + if (delta_bno < 0) { > reserve_btblocks(sc->mp, agno, btr_bno, -delta_bno); > - if (delta_cnt < 0) > + delta_bno = 0; > + } > + if (delta_cnt < 0) { > reserve_btblocks(sc->mp, agno, btr_cnt, -delta_cnt); > + delta_cnt = 0; > + } > + > + /* > + * Try to fill the bnobt cursor with extra blocks to populate > + * the AGFL. If we don't get all the blocks we want, stop > + * trying to fill the AGFL because the AG is totally out of > + * space. > + */ > + agfl_wanted = agfl_goal - (delta_bno + delta_cnt); > + if (agfl_wanted > 0 && > + agfl_wanted != reserve_agblocks(sc->mp, agno, btr_bno, > + agfl_wanted)) > + agfl_goal = 0; Nit: can we split off the function call so it's not embedded in the if condition? With that tweak: Reviewed-by: Brian Foster <bfoster@redhat.com> > > /* Ok, now how many free space records do we have? */ > *nr_extents = count_bno_extents_blocks(agno, &num_freeblocks); >
On Tue, Jul 07, 2020 at 08:59:06AM -0400, Brian Foster wrote: > On Thu, Jul 02, 2020 at 08:27:09AM -0700, Darrick J. Wong wrote: > > From: Darrick J. Wong <darrick.wong@oracle.com> > > > > In commit 9851fd79bfb1, we added a slight amount of slack to the free > > space btrees being reconstructed so that the initial fix_freelist call > > (which is run against a totally empty AGFL) would never have to split > > either free space btree in order to populate the free list. > > > > The new btree bulk loading code in xfs_repair can re-create this > > situation because it can set the slack values to zero if the filesystem > > is very full. However, these days repair has the infrastructure needed > > to ensure that overestimations of the btree block counts end up on the > > AGFL or get freed back into the filesystem at the end of phase 5. > > > > Fix this problem by reserving extra blocks in the bnobt reservation, and > > checking that there are enough overages in the bnobt/cntbt fakeroots to > > populate the AGFL with the minimum number of blocks it needs to handle a > > split in the bno/cnt/rmap btrees. > > > > Note that we reserve blocks for the new bnobt/cntbt/AGFL at the very end > > of the reservation steps in phase 5, so the extra allocation should not > > cause repair to fail if it can't find blocks for btrees. > > > > Fixes: 9851fd79bfb1 ("repair: AGFL rebuild fails if btree split required") > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> > > --- > > repair/agbtree.c | 51 ++++++++++++++++++++++++++++++++++++++++++++------- > > 1 file changed, 44 insertions(+), 7 deletions(-) > > > > > > diff --git a/repair/agbtree.c b/repair/agbtree.c > > index de8015ec..9f64d54b 100644 > > --- a/repair/agbtree.c > > +++ b/repair/agbtree.c > ... > > @@ -268,16 +288,33 @@ _("Unable to compute free space by length btree geometry, error %d.\n"), -error) > > btr_cnt->bload.nr_blocks; > > > > /* We don't need any more blocks, so we're done. */ > > - if (delta_bno >= 0 && delta_cnt >= 0) { > > + if (delta_bno >= 0 && delta_cnt >= 0 && > > + delta_bno + delta_cnt >= agfl_goal) { > > *extra_blocks = delta_bno + delta_cnt; > > break; > > } > > > > /* Allocate however many more blocks we need this time. */ > > - if (delta_bno < 0) > > + if (delta_bno < 0) { > > reserve_btblocks(sc->mp, agno, btr_bno, -delta_bno); > > - if (delta_cnt < 0) > > + delta_bno = 0; > > + } > > + if (delta_cnt < 0) { > > reserve_btblocks(sc->mp, agno, btr_cnt, -delta_cnt); > > + delta_cnt = 0; > > + } > > + > > + /* > > + * Try to fill the bnobt cursor with extra blocks to populate > > + * the AGFL. If we don't get all the blocks we want, stop > > + * trying to fill the AGFL because the AG is totally out of > > + * space. > > + */ > > + agfl_wanted = agfl_goal - (delta_bno + delta_cnt); > > + if (agfl_wanted > 0 && > > + agfl_wanted != reserve_agblocks(sc->mp, agno, btr_bno, > > + agfl_wanted)) > > + agfl_goal = 0; > > Nit: can we split off the function call so it's not embedded in the if > condition? With that tweak: It occurs to me that we don't care how much we fall short of the requested allocation. I could change reserve_agblocks to return true if it got all the blocks it was asked to get, and then that becomes: if (agfl_wanted > 0 && !reserve_agblocks(sc->mp, agno, btr_bno, agfl_wanted) agfl_goal = 0; How does that sound? --D > Reviewed-by: Brian Foster <bfoster@redhat.com> > > > > > /* Ok, now how many free space records do we have? */ > > *nr_extents = count_bno_extents_blocks(agno, &num_freeblocks); > > >
On Tue, Jul 07, 2020 at 07:07:08AM -0700, Darrick J. Wong wrote: > On Tue, Jul 07, 2020 at 08:59:06AM -0400, Brian Foster wrote: > > On Thu, Jul 02, 2020 at 08:27:09AM -0700, Darrick J. Wong wrote: > > > From: Darrick J. Wong <darrick.wong@oracle.com> > > > > > > In commit 9851fd79bfb1, we added a slight amount of slack to the free > > > space btrees being reconstructed so that the initial fix_freelist call > > > (which is run against a totally empty AGFL) would never have to split > > > either free space btree in order to populate the free list. > > > > > > The new btree bulk loading code in xfs_repair can re-create this > > > situation because it can set the slack values to zero if the filesystem > > > is very full. However, these days repair has the infrastructure needed > > > to ensure that overestimations of the btree block counts end up on the > > > AGFL or get freed back into the filesystem at the end of phase 5. > > > > > > Fix this problem by reserving extra blocks in the bnobt reservation, and > > > checking that there are enough overages in the bnobt/cntbt fakeroots to > > > populate the AGFL with the minimum number of blocks it needs to handle a > > > split in the bno/cnt/rmap btrees. > > > > > > Note that we reserve blocks for the new bnobt/cntbt/AGFL at the very end > > > of the reservation steps in phase 5, so the extra allocation should not > > > cause repair to fail if it can't find blocks for btrees. > > > > > > Fixes: 9851fd79bfb1 ("repair: AGFL rebuild fails if btree split required") > > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> > > > --- > > > repair/agbtree.c | 51 ++++++++++++++++++++++++++++++++++++++++++++------- > > > 1 file changed, 44 insertions(+), 7 deletions(-) > > > > > > > > > diff --git a/repair/agbtree.c b/repair/agbtree.c > > > index de8015ec..9f64d54b 100644 > > > --- a/repair/agbtree.c > > > +++ b/repair/agbtree.c > > ... > > > @@ -268,16 +288,33 @@ _("Unable to compute free space by length btree geometry, error %d.\n"), -error) > > > btr_cnt->bload.nr_blocks; > > > > > > /* We don't need any more blocks, so we're done. */ > > > - if (delta_bno >= 0 && delta_cnt >= 0) { > > > + if (delta_bno >= 0 && delta_cnt >= 0 && > > > + delta_bno + delta_cnt >= agfl_goal) { > > > *extra_blocks = delta_bno + delta_cnt; > > > break; > > > } > > > > > > /* Allocate however many more blocks we need this time. */ > > > - if (delta_bno < 0) > > > + if (delta_bno < 0) { > > > reserve_btblocks(sc->mp, agno, btr_bno, -delta_bno); > > > - if (delta_cnt < 0) > > > + delta_bno = 0; > > > + } > > > + if (delta_cnt < 0) { > > > reserve_btblocks(sc->mp, agno, btr_cnt, -delta_cnt); > > > + delta_cnt = 0; > > > + } > > > + > > > + /* > > > + * Try to fill the bnobt cursor with extra blocks to populate > > > + * the AGFL. If we don't get all the blocks we want, stop > > > + * trying to fill the AGFL because the AG is totally out of > > > + * space. > > > + */ > > > + agfl_wanted = agfl_goal - (delta_bno + delta_cnt); > > > + if (agfl_wanted > 0 && > > > + agfl_wanted != reserve_agblocks(sc->mp, agno, btr_bno, > > > + agfl_wanted)) > > > + agfl_goal = 0; > > > > Nit: can we split off the function call so it's not embedded in the if > > condition? With that tweak: > > It occurs to me that we don't care how much we fall short of the > requested allocation. I could change reserve_agblocks to return true if > it got all the blocks it was asked to get, and then that becomes: > > if (agfl_wanted > 0 && > !reserve_agblocks(sc->mp, agno, btr_bno, agfl_wanted) > agfl_goal = 0; > > How does that sound? > Looks readable enough to me, thanks. Brian > --D > > > Reviewed-by: Brian Foster <bfoster@redhat.com> > > > > > > > > /* Ok, now how many free space records do we have? */ > > > *nr_extents = count_bno_extents_blocks(agno, &num_freeblocks); > > > > > >
diff --git a/repair/agbtree.c b/repair/agbtree.c index de8015ec..9f64d54b 100644 --- a/repair/agbtree.c +++ b/repair/agbtree.c @@ -65,8 +65,8 @@ consume_freespace( } /* Reserve blocks for the new per-AG structures. */ -static void -reserve_btblocks( +static uint32_t +reserve_agblocks( struct xfs_mount *mp, xfs_agnumber_t agno, struct bt_rebuild *btr, @@ -86,8 +86,7 @@ reserve_btblocks( */ ext_ptr = findfirst_bcnt_extent(agno); if (!ext_ptr) - do_error( -_("error - not enough free space in filesystem\n")); + break; /* Use up the extent we've got. */ len = min(ext_ptr->ex_blockcount, nr_blocks - blocks_allocated); @@ -110,6 +109,23 @@ _("error - not enough free space in filesystem\n")); fprintf(stderr, "blocks_allocated = %d\n", blocks_allocated); #endif + return blocks_allocated; +} + +static inline void +reserve_btblocks( + struct xfs_mount *mp, + xfs_agnumber_t agno, + struct bt_rebuild *btr, + uint32_t nr_blocks) +{ + uint32_t got; + + got = reserve_agblocks(mp, agno, btr, nr_blocks); + if (got != nr_blocks) + do_error( + _("error - not enough free space in filesystem, AG %u\n"), + agno); } /* Feed one of the new btree blocks to the bulk loader. */ @@ -217,8 +233,11 @@ init_freespace_cursors( struct bt_rebuild *btr_bno, struct bt_rebuild *btr_cnt) { + unsigned int agfl_goal; int error; + agfl_goal = libxfs_alloc_min_freelist(sc->mp, NULL); + init_rebuild(sc, &XFS_RMAP_OINFO_AG, free_space, btr_bno); init_rebuild(sc, &XFS_RMAP_OINFO_AG, free_space, btr_cnt); @@ -243,6 +262,7 @@ init_freespace_cursors( do { unsigned int num_freeblocks; int delta_bno, delta_cnt; + int agfl_wanted; /* Compute how many bnobt blocks we'll need. */ error = -libxfs_btree_bload_compute_geometry(btr_bno->cur, @@ -268,16 +288,33 @@ _("Unable to compute free space by length btree geometry, error %d.\n"), -error) btr_cnt->bload.nr_blocks; /* We don't need any more blocks, so we're done. */ - if (delta_bno >= 0 && delta_cnt >= 0) { + if (delta_bno >= 0 && delta_cnt >= 0 && + delta_bno + delta_cnt >= agfl_goal) { *extra_blocks = delta_bno + delta_cnt; break; } /* Allocate however many more blocks we need this time. */ - if (delta_bno < 0) + if (delta_bno < 0) { reserve_btblocks(sc->mp, agno, btr_bno, -delta_bno); - if (delta_cnt < 0) + delta_bno = 0; + } + if (delta_cnt < 0) { reserve_btblocks(sc->mp, agno, btr_cnt, -delta_cnt); + delta_cnt = 0; + } + + /* + * Try to fill the bnobt cursor with extra blocks to populate + * the AGFL. If we don't get all the blocks we want, stop + * trying to fill the AGFL because the AG is totally out of + * space. + */ + agfl_wanted = agfl_goal - (delta_bno + delta_cnt); + if (agfl_wanted > 0 && + agfl_wanted != reserve_agblocks(sc->mp, agno, btr_bno, + agfl_wanted)) + agfl_goal = 0; /* Ok, now how many free space records do we have? */ *nr_extents = count_bno_extents_blocks(agno, &num_freeblocks);