[PATCH v2 0/3] xfs_repair: more fixes

[PATCH v2 0/3] xfs_repair: more fixes

[Darrick J. Wong]

Hi all,

Two more fixes for repair: first, we actually complain if ag header crc
verification fails.  Second, we now try to propagate enough of an AGFL
so that fixing the freelist should never require splitting the free
space btrees.

If you're going to start using this mess, you probably ought to just
pull from my git trees, which are linked below.

This is an extraordinary way to destroy everything.  Enjoy!
Comments and questions are, as always, welcome.

--D

xfsprogs git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfsprogs-dev.git/log/?h=repair-fixes
---
 repair/agbtree.c  |   78 +++++++++++++++++++++++++++++++++++++++--------------
 repair/bulkload.c |    2 +
 repair/bulkload.h |    3 ++
 repair/scan.c     |    6 ++++
 4 files changed, 69 insertions(+), 20 deletions(-)

[PATCH 1/3] xfs_repair: complain about ag header crc errors

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Repair doesn't complain about crc errors in the AG headers, and it
should.  Otherwise, this gives the admin the wrong impression about the
state of the filesystem after a nomodify check.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Brian Foster <bfoster@redhat.com>
---
 repair/scan.c |    6 ++++++
 1 file changed, 6 insertions(+)


diff --git a/repair/scan.c b/repair/scan.c
index 505cfc53..42b299f7 100644
--- a/repair/scan.c
+++ b/repair/scan.c
@@ -2441,6 +2441,8 @@ scan_ag(
 		objname = _("root superblock");
 		goto out_free_sb;
 	}
+	if (sbbuf->b_error == -EFSBADCRC)
+		do_warn(_("superblock has bad CRC for ag %d\n"), agno);
 	libxfs_sb_from_disk(sb, sbbuf->b_addr);
 
 	error = salvage_buffer(mp->m_dev,
@@ -2450,6 +2452,8 @@ scan_ag(
 		objname = _("agf block");
 		goto out_free_sbbuf;
 	}
+	if (agfbuf->b_error == -EFSBADCRC)
+		do_warn(_("agf has bad CRC for ag %d\n"), agno);
 	agf = agfbuf->b_addr;
 
 	error = salvage_buffer(mp->m_dev,
@@ -2459,6 +2463,8 @@ scan_ag(
 		objname = _("agi block");
 		goto out_free_agfbuf;
 	}
+	if (agibuf->b_error == -EFSBADCRC)
+		do_warn(_("agi has bad CRC for ag %d\n"), agno);
 	agi = agibuf->b_addr;
 
 	/* fix up bad ag headers */

Re: [PATCH 1/3] xfs_repair: complain about ag header crc errors

[Allison Collins]

On 7/2/20 8:26 AM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Repair doesn't complain about crc errors in the AG headers, and it
> should.  Otherwise, this gives the admin the wrong impression about the
> state of the filesystem after a nomodify check.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> Reviewed-by: Brian Foster <bfoster@redhat.com>
Looks fine
Reviewed-by: Allison Collins <allison.henderson@oracle.com>

> ---
>   repair/scan.c |    6 ++++++
>   1 file changed, 6 insertions(+)
> 
> 
> diff --git a/repair/scan.c b/repair/scan.c
> index 505cfc53..42b299f7 100644
> --- a/repair/scan.c
> +++ b/repair/scan.c
> @@ -2441,6 +2441,8 @@ scan_ag(
>   		objname = _("root superblock");
>   		goto out_free_sb;
>   	}
> +	if (sbbuf->b_error == -EFSBADCRC)
> +		do_warn(_("superblock has bad CRC for ag %d\n"), agno);
>   	libxfs_sb_from_disk(sb, sbbuf->b_addr);
>   
>   	error = salvage_buffer(mp->m_dev,
> @@ -2450,6 +2452,8 @@ scan_ag(
>   		objname = _("agf block");
>   		goto out_free_sbbuf;
>   	}
> +	if (agfbuf->b_error == -EFSBADCRC)
> +		do_warn(_("agf has bad CRC for ag %d\n"), agno);
>   	agf = agfbuf->b_addr;
>   
>   	error = salvage_buffer(mp->m_dev,
> @@ -2459,6 +2463,8 @@ scan_ag(
>   		objname = _("agi block");
>   		goto out_free_agfbuf;
>   	}
> +	if (agibuf->b_error == -EFSBADCRC)
> +		do_warn(_("agi has bad CRC for ag %d\n"), agno);
>   	agi = agibuf->b_addr;
>   
>   	/* fix up bad ag headers */
> 

Re: [PATCH 1/3] xfs_repair: complain about ag header crc errors

[Christoph Hellwig]

On Thu, Jul 02, 2020 at 08:26:56AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Repair doesn't complain about crc errors in the AG headers, and it
> should.  Otherwise, this gives the admin the wrong impression about the
> state of the filesystem after a nomodify check.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> Reviewed-by: Brian Foster <bfoster@redhat.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 2/3] xfs_repair: simplify free space btree calculations in init_freespace_cursors

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Add a summary variable to the bulkload structure so that we can track
the number of blocks that have been reserved for a particular (btree)
bulkload operation.  Doing so enables us to simplify the logic in
init_freespace_cursors that deals with figuring out how many more blocks
we need to fill the bnobt/cntbt properly.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 repair/agbtree.c  |   33 +++++++++++++++++----------------
 repair/bulkload.c |    2 ++
 repair/bulkload.h |    3 +++
 3 files changed, 22 insertions(+), 16 deletions(-)


diff --git a/repair/agbtree.c b/repair/agbtree.c
index 339b1489..de8015ec 100644
--- a/repair/agbtree.c
+++ b/repair/agbtree.c
@@ -217,8 +217,6 @@ init_freespace_cursors(
 	struct bt_rebuild	*btr_bno,
 	struct bt_rebuild	*btr_cnt)
 {
-	unsigned int		bno_blocks;
-	unsigned int		cnt_blocks;
 	int			error;
 
 	init_rebuild(sc, &XFS_RMAP_OINFO_AG, free_space, btr_bno);
@@ -244,9 +242,7 @@ init_freespace_cursors(
 	 */
 	do {
 		unsigned int	num_freeblocks;
-
-		bno_blocks = btr_bno->bload.nr_blocks;
-		cnt_blocks = btr_cnt->bload.nr_blocks;
+		int		delta_bno, delta_cnt;
 
 		/* Compute how many bnobt blocks we'll need. */
 		error = -libxfs_btree_bload_compute_geometry(btr_bno->cur,
@@ -262,25 +258,30 @@ _("Unable to compute free space by block btree geometry, error %d.\n"), -error);
 			do_error(
 _("Unable to compute free space by length btree geometry, error %d.\n"), -error);
 
+		/*
+		 * Compute the deficit between the number of blocks reserved
+		 * and the number of blocks we think we need for the btree.
+		 */
+		delta_bno = (int)btr_bno->newbt.nr_reserved -
+				 btr_bno->bload.nr_blocks;
+		delta_cnt = (int)btr_cnt->newbt.nr_reserved -
+				 btr_cnt->bload.nr_blocks;
+
 		/* We don't need any more blocks, so we're done. */
-		if (bno_blocks >= btr_bno->bload.nr_blocks &&
-		    cnt_blocks >= btr_cnt->bload.nr_blocks)
+		if (delta_bno >= 0 && delta_cnt >= 0) {
+			*extra_blocks = delta_bno + delta_cnt;
 			break;
+		}
 
 		/* Allocate however many more blocks we need this time. */
-		if (bno_blocks < btr_bno->bload.nr_blocks)
-			reserve_btblocks(sc->mp, agno, btr_bno,
-					btr_bno->bload.nr_blocks - bno_blocks);
-		if (cnt_blocks < btr_cnt->bload.nr_blocks)
-			reserve_btblocks(sc->mp, agno, btr_cnt,
-					btr_cnt->bload.nr_blocks - cnt_blocks);
+		if (delta_bno < 0)
+			reserve_btblocks(sc->mp, agno, btr_bno, -delta_bno);
+		if (delta_cnt < 0)
+			reserve_btblocks(sc->mp, agno, btr_cnt, -delta_cnt);
 
 		/* Ok, now how many free space records do we have? */
 		*nr_extents = count_bno_extents_blocks(agno, &num_freeblocks);
 	} while (1);
-
-	*extra_blocks = (bno_blocks - btr_bno->bload.nr_blocks) +
-			(cnt_blocks - btr_cnt->bload.nr_blocks);
 }
 
 /* Rebuild the free space btrees. */
diff --git a/repair/bulkload.c b/repair/bulkload.c
index 81d67e62..8dd0a0c3 100644
--- a/repair/bulkload.c
+++ b/repair/bulkload.c
@@ -40,6 +40,8 @@ bulkload_add_blocks(
 	resv->len = len;
 	resv->used = 0;
 	list_add_tail(&resv->list, &bkl->resv_list);
+	bkl->nr_reserved += len;
+
 	return 0;
 }
 
diff --git a/repair/bulkload.h b/repair/bulkload.h
index 01f67279..a84e99b8 100644
--- a/repair/bulkload.h
+++ b/repair/bulkload.h
@@ -41,6 +41,9 @@ struct bulkload {
 
 	/* The last reservation we allocated from. */
 	struct bulkload_resv	*last_resv;
+
+	/* Number of blocks reserved via resv_list. */
+	unsigned int		nr_reserved;
 };
 
 #define for_each_bulkload_reservation(bkl, resv, n)	\

Re: [PATCH 2/3] xfs_repair: simplify free space btree calculations in init_freespace_cursors

[Allison Collins]

On 7/2/20 8:27 AM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Add a summary variable to the bulkload structure so that we can track
> the number of blocks that have been reserved for a particular (btree)
> bulkload operation.  Doing so enables us to simplify the logic in
> init_freespace_cursors that deals with figuring out how many more blocks
> we need to fill the bnobt/cntbt properly.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Alrighty, looks good
Reviewed-by: Allison Collins <allison.henderson@oracle.com>

> ---
>   repair/agbtree.c  |   33 +++++++++++++++++----------------
>   repair/bulkload.c |    2 ++
>   repair/bulkload.h |    3 +++
>   3 files changed, 22 insertions(+), 16 deletions(-)
> 
> 
> diff --git a/repair/agbtree.c b/repair/agbtree.c
> index 339b1489..de8015ec 100644
> --- a/repair/agbtree.c
> +++ b/repair/agbtree.c
> @@ -217,8 +217,6 @@ init_freespace_cursors(
>   	struct bt_rebuild	*btr_bno,
>   	struct bt_rebuild	*btr_cnt)
>   {
> -	unsigned int		bno_blocks;
> -	unsigned int		cnt_blocks;
>   	int			error;
>   
>   	init_rebuild(sc, &XFS_RMAP_OINFO_AG, free_space, btr_bno);
> @@ -244,9 +242,7 @@ init_freespace_cursors(
>   	 */
>   	do {
>   		unsigned int	num_freeblocks;
> -
> -		bno_blocks = btr_bno->bload.nr_blocks;
> -		cnt_blocks = btr_cnt->bload.nr_blocks;
> +		int		delta_bno, delta_cnt;
>   
>   		/* Compute how many bnobt blocks we'll need. */
>   		error = -libxfs_btree_bload_compute_geometry(btr_bno->cur,
> @@ -262,25 +258,30 @@ _("Unable to compute free space by block btree geometry, error %d.\n"), -error);
>   			do_error(
>   _("Unable to compute free space by length btree geometry, error %d.\n"), -error);
>   
> +		/*
> +		 * Compute the deficit between the number of blocks reserved
> +		 * and the number of blocks we think we need for the btree.
> +		 */
> +		delta_bno = (int)btr_bno->newbt.nr_reserved -
> +				 btr_bno->bload.nr_blocks;
> +		delta_cnt = (int)btr_cnt->newbt.nr_reserved -
> +				 btr_cnt->bload.nr_blocks;
> +
>   		/* We don't need any more blocks, so we're done. */
> -		if (bno_blocks >= btr_bno->bload.nr_blocks &&
> -		    cnt_blocks >= btr_cnt->bload.nr_blocks)
> +		if (delta_bno >= 0 && delta_cnt >= 0) {
> +			*extra_blocks = delta_bno + delta_cnt;
>   			break;
> +		}
>   
>   		/* Allocate however many more blocks we need this time. */
> -		if (bno_blocks < btr_bno->bload.nr_blocks)
> -			reserve_btblocks(sc->mp, agno, btr_bno,
> -					btr_bno->bload.nr_blocks - bno_blocks);
> -		if (cnt_blocks < btr_cnt->bload.nr_blocks)
> -			reserve_btblocks(sc->mp, agno, btr_cnt,
> -					btr_cnt->bload.nr_blocks - cnt_blocks);
> +		if (delta_bno < 0)
> +			reserve_btblocks(sc->mp, agno, btr_bno, -delta_bno);
> +		if (delta_cnt < 0)
> +			reserve_btblocks(sc->mp, agno, btr_cnt, -delta_cnt);
>   
>   		/* Ok, now how many free space records do we have? */
>   		*nr_extents = count_bno_extents_blocks(agno, &num_freeblocks);
>   	} while (1);
> -
> -	*extra_blocks = (bno_blocks - btr_bno->bload.nr_blocks) +
> -			(cnt_blocks - btr_cnt->bload.nr_blocks);
>   }
>   
>   /* Rebuild the free space btrees. */
> diff --git a/repair/bulkload.c b/repair/bulkload.c
> index 81d67e62..8dd0a0c3 100644
> --- a/repair/bulkload.c
> +++ b/repair/bulkload.c
> @@ -40,6 +40,8 @@ bulkload_add_blocks(
>   	resv->len = len;
>   	resv->used = 0;
>   	list_add_tail(&resv->list, &bkl->resv_list);
> +	bkl->nr_reserved += len;
> +
>   	return 0;
>   }
>   
> diff --git a/repair/bulkload.h b/repair/bulkload.h
> index 01f67279..a84e99b8 100644
> --- a/repair/bulkload.h
> +++ b/repair/bulkload.h
> @@ -41,6 +41,9 @@ struct bulkload {
>   
>   	/* The last reservation we allocated from. */
>   	struct bulkload_resv	*last_resv;
> +
> +	/* Number of blocks reserved via resv_list. */
> +	unsigned int		nr_reserved;
>   };
>   
>   #define for_each_bulkload_reservation(bkl, resv, n)	\
> 

Re: [PATCH 2/3] xfs_repair: simplify free space btree calculations in init_freespace_cursors

[Brian Foster]

On Thu, Jul 02, 2020 at 08:27:03AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Add a summary variable to the bulkload structure so that we can track
> the number of blocks that have been reserved for a particular (btree)
> bulkload operation.  Doing so enables us to simplify the logic in
> init_freespace_cursors that deals with figuring out how many more blocks
> we need to fill the bnobt/cntbt properly.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---

Nice simplification:

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  repair/agbtree.c  |   33 +++++++++++++++++----------------
>  repair/bulkload.c |    2 ++
>  repair/bulkload.h |    3 +++
>  3 files changed, 22 insertions(+), 16 deletions(-)
> 
> 
> diff --git a/repair/agbtree.c b/repair/agbtree.c
> index 339b1489..de8015ec 100644
> --- a/repair/agbtree.c
> +++ b/repair/agbtree.c
> @@ -217,8 +217,6 @@ init_freespace_cursors(
>  	struct bt_rebuild	*btr_bno,
>  	struct bt_rebuild	*btr_cnt)
>  {
> -	unsigned int		bno_blocks;
> -	unsigned int		cnt_blocks;
>  	int			error;
>  
>  	init_rebuild(sc, &XFS_RMAP_OINFO_AG, free_space, btr_bno);
> @@ -244,9 +242,7 @@ init_freespace_cursors(
>  	 */
>  	do {
>  		unsigned int	num_freeblocks;
> -
> -		bno_blocks = btr_bno->bload.nr_blocks;
> -		cnt_blocks = btr_cnt->bload.nr_blocks;
> +		int		delta_bno, delta_cnt;
>  
>  		/* Compute how many bnobt blocks we'll need. */
>  		error = -libxfs_btree_bload_compute_geometry(btr_bno->cur,
> @@ -262,25 +258,30 @@ _("Unable to compute free space by block btree geometry, error %d.\n"), -error);
>  			do_error(
>  _("Unable to compute free space by length btree geometry, error %d.\n"), -error);
>  
> +		/*
> +		 * Compute the deficit between the number of blocks reserved
> +		 * and the number of blocks we think we need for the btree.
> +		 */
> +		delta_bno = (int)btr_bno->newbt.nr_reserved -
> +				 btr_bno->bload.nr_blocks;
> +		delta_cnt = (int)btr_cnt->newbt.nr_reserved -
> +				 btr_cnt->bload.nr_blocks;
> +
>  		/* We don't need any more blocks, so we're done. */
> -		if (bno_blocks >= btr_bno->bload.nr_blocks &&
> -		    cnt_blocks >= btr_cnt->bload.nr_blocks)
> +		if (delta_bno >= 0 && delta_cnt >= 0) {
> +			*extra_blocks = delta_bno + delta_cnt;
>  			break;
> +		}
>  
>  		/* Allocate however many more blocks we need this time. */
> -		if (bno_blocks < btr_bno->bload.nr_blocks)
> -			reserve_btblocks(sc->mp, agno, btr_bno,
> -					btr_bno->bload.nr_blocks - bno_blocks);
> -		if (cnt_blocks < btr_cnt->bload.nr_blocks)
> -			reserve_btblocks(sc->mp, agno, btr_cnt,
> -					btr_cnt->bload.nr_blocks - cnt_blocks);
> +		if (delta_bno < 0)
> +			reserve_btblocks(sc->mp, agno, btr_bno, -delta_bno);
> +		if (delta_cnt < 0)
> +			reserve_btblocks(sc->mp, agno, btr_cnt, -delta_cnt);
>  
>  		/* Ok, now how many free space records do we have? */
>  		*nr_extents = count_bno_extents_blocks(agno, &num_freeblocks);
>  	} while (1);
> -
> -	*extra_blocks = (bno_blocks - btr_bno->bload.nr_blocks) +
> -			(cnt_blocks - btr_cnt->bload.nr_blocks);
>  }
>  
>  /* Rebuild the free space btrees. */
> diff --git a/repair/bulkload.c b/repair/bulkload.c
> index 81d67e62..8dd0a0c3 100644
> --- a/repair/bulkload.c
> +++ b/repair/bulkload.c
> @@ -40,6 +40,8 @@ bulkload_add_blocks(
>  	resv->len = len;
>  	resv->used = 0;
>  	list_add_tail(&resv->list, &bkl->resv_list);
> +	bkl->nr_reserved += len;
> +
>  	return 0;
>  }
>  
> diff --git a/repair/bulkload.h b/repair/bulkload.h
> index 01f67279..a84e99b8 100644
> --- a/repair/bulkload.h
> +++ b/repair/bulkload.h
> @@ -41,6 +41,9 @@ struct bulkload {
>  
>  	/* The last reservation we allocated from. */
>  	struct bulkload_resv	*last_resv;
> +
> +	/* Number of blocks reserved via resv_list. */
> +	unsigned int		nr_reserved;
>  };
>  
>  #define for_each_bulkload_reservation(bkl, resv, n)	\
> 

Re: [PATCH 2/3] xfs_repair: simplify free space btree calculations in init_freespace_cursors

[Christoph Hellwig]

On Thu, Jul 02, 2020 at 08:27:03AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Add a summary variable to the bulkload structure so that we can track
> the number of blocks that have been reserved for a particular (btree)
> bulkload operation.  Doing so enables us to simplify the logic in
> init_freespace_cursors that deals with figuring out how many more blocks
> we need to fill the bnobt/cntbt properly.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 3/3] xfs_repair: try to fill the AGFL before we fix the freelist

[Darrick J. Wong]

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
@@ -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);

Re: [PATCH 3/3] xfs_repair: try to fill the AGFL before we fix the freelist

[Brian Foster]

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);
> 

Re: [PATCH 3/3] xfs_repair: try to fill the AGFL before we fix the freelist

[Darrick J. Wong]

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);
> > 
> 

Re: [PATCH 3/3] xfs_repair: try to fill the AGFL before we fix the freelist

[Brian Foster]

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);
> > > 
> > 
> 

[PATCH v2 3/3] xfs_repair: try to fill the AGFL before we fix the freelist

[Darrick J. Wong]

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>
Reviewed-by: Brian Foster <bfoster@redhat.com>
---
v2: tweak the reserve_agblocks return logic a bit
---
 repair/agbtree.c |   52 ++++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 44 insertions(+), 8 deletions(-)

diff --git a/repair/agbtree.c b/repair/agbtree.c
index de8015ec..69821745 100644
--- a/repair/agbtree.c
+++ b/repair/agbtree.c
@@ -64,9 +64,12 @@ consume_freespace(
 	}
 }
 
-/* Reserve blocks for the new per-AG structures. */
-static void
-reserve_btblocks(
+/*
+ * Reserve blocks for the new per-AG structures.  Returns true if all blocks
+ * were allocated, and false if we ran out of space.
+ */
+static bool
+reserve_agblocks(
 	struct xfs_mount	*mp,
 	xfs_agnumber_t		agno,
 	struct bt_rebuild	*btr,
@@ -86,8 +89,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 +112,20 @@ _("error - not enough free space in filesystem\n"));
 	fprintf(stderr, "blocks_allocated = %d\n",
 		blocks_allocated);
 #endif
+	return blocks_allocated == nr_blocks;
+}
+
+static inline void
+reserve_btblocks(
+	struct xfs_mount	*mp,
+	xfs_agnumber_t		agno,
+	struct bt_rebuild	*btr,
+	uint32_t		nr_blocks)
+{
+	if (!reserve_agblocks(mp, agno, btr, 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,32 @@ _("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 &&
+		    !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);

Re: [PATCH v2 3/3] xfs_repair: try to fill the AGFL before we fix the freelist

[Christoph Hellwig]

On Wed, Jul 08, 2020 at 08:34:55AM -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>
> Reviewed-by: Brian Foster <bfoster@redhat.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>