All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v4 0/3] xfs: set aside allocation btree blocks from block reservation
@ 2021-04-23 13:10 Brian Foster
  2021-04-23 13:10 ` [PATCH v4 1/3] xfs: unconditionally read all AGFs on mounts with perag reservation Brian Foster
                   ` (2 more replies)
  0 siblings, 3 replies; 18+ messages in thread
From: Brian Foster @ 2021-04-23 13:10 UTC (permalink / raw)
  To: linux-xfs

Hi all,

Here's v4 of the proposed allocbt block set aside fix. The patches have
been swizzled around a bit based on previous feedback to try and avoid
confusion over what bits are perag res related and not. This hopefully
facilitates analysis when it comes time to evaluate potential changes to
remove the current perag res mount time AG header scan behavior on which
this currently depends. Otherwise the actual code changes from v3 are
fairly light and documented in the changelog below. Thoughts, reviews,
flames appreciated.

Brian

v4:
- Fix up perag res logic to not skip pagf init on partial res failure.
- Split up set aside patch into separate counter mechanism and set aside
  policy patches.
- Drop unnecessary ->m_has_agresv flag as pagf's are always initialized
  on filesystems with active reservations.
v3: https://lore.kernel.org/linux-xfs/20210318161707.723742-1-bfoster@redhat.com/
- Use a mount flag for easy detection of active perag reservation.
- Filter rmapbt blocks from allocbt block accounting.
v2: https://lore.kernel.org/linux-xfs/20210222152108.896178-1-bfoster@redhat.com/
- Use an atomic counter instead of a percpu counter.
v1: https://lore.kernel.org/linux-xfs/20210217132339.651020-1-bfoster@redhat.com/

Brian Foster (3):
  xfs: unconditionally read all AGFs on mounts with perag reservation
  xfs: introduce in-core global counter of allocbt blocks
  xfs: set aside allocation btree blocks from block reservation

 fs/xfs/libxfs/xfs_ag_resv.c     | 34 ++++++++++++++++++++++-----------
 fs/xfs/libxfs/xfs_alloc.c       | 12 ++++++++++++
 fs/xfs/libxfs/xfs_alloc_btree.c |  2 ++
 fs/xfs/xfs_mount.c              | 15 ++++++++++++++-
 fs/xfs/xfs_mount.h              |  6 ++++++
 5 files changed, 57 insertions(+), 12 deletions(-)

-- 
2.26.3


^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH v4 1/3] xfs: unconditionally read all AGFs on mounts with perag reservation
  2021-04-23 13:10 [PATCH v4 0/3] xfs: set aside allocation btree blocks from block reservation Brian Foster
@ 2021-04-23 13:10 ` Brian Foster
  2021-04-27 10:22   ` Chandan Babu R
                     ` (2 more replies)
  2021-04-23 13:10 ` [PATCH v4 2/3] xfs: introduce in-core global counter of allocbt blocks Brian Foster
  2021-04-23 13:10 ` [PATCH v4 3/3] xfs: set aside allocation btree blocks from block reservation Brian Foster
  2 siblings, 3 replies; 18+ messages in thread
From: Brian Foster @ 2021-04-23 13:10 UTC (permalink / raw)
  To: linux-xfs

perag reservation is enabled at mount time on a per AG basis. The
upcoming change to set aside allocbt blocks from block reservation
requires a populated allocbt counter as soon as possible after mount
to be fully effective against large perag reservations. Therefore as
a preparation step, initialize the pagf on all mounts where at least
one reservation is active. Note that this already occurs to some
degree on most default format filesystems as reservation requirement
calculations already depend on the AGF or AGI, depending on the
reservation type.

Signed-off-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/libxfs/xfs_ag_resv.c | 34 +++++++++++++++++++++++-----------
 1 file changed, 23 insertions(+), 11 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_ag_resv.c b/fs/xfs/libxfs/xfs_ag_resv.c
index 6c5f8d10589c..e32a1833d523 100644
--- a/fs/xfs/libxfs/xfs_ag_resv.c
+++ b/fs/xfs/libxfs/xfs_ag_resv.c
@@ -253,7 +253,8 @@ xfs_ag_resv_init(
 	xfs_agnumber_t			agno = pag->pag_agno;
 	xfs_extlen_t			ask;
 	xfs_extlen_t			used;
-	int				error = 0;
+	int				error = 0, error2;
+	bool				has_resv = false;
 
 	/* Create the metadata reservation. */
 	if (pag->pag_meta_resv.ar_asked == 0) {
@@ -291,6 +292,8 @@ xfs_ag_resv_init(
 			if (error)
 				goto out;
 		}
+		if (ask)
+			has_resv = true;
 	}
 
 	/* Create the RMAPBT metadata reservation */
@@ -304,19 +307,28 @@ xfs_ag_resv_init(
 		error = __xfs_ag_resv_init(pag, XFS_AG_RESV_RMAPBT, ask, used);
 		if (error)
 			goto out;
+		if (ask)
+			has_resv = true;
 	}
 
-#ifdef DEBUG
-	/* need to read in the AGF for the ASSERT below to work */
-	error = xfs_alloc_pagf_init(pag->pag_mount, tp, pag->pag_agno, 0);
-	if (error)
-		return error;
-
-	ASSERT(xfs_perag_resv(pag, XFS_AG_RESV_METADATA)->ar_reserved +
-	       xfs_perag_resv(pag, XFS_AG_RESV_RMAPBT)->ar_reserved <=
-	       pag->pagf_freeblks + pag->pagf_flcount);
-#endif
 out:
+	/*
+	 * Initialize the pagf if we have at least one active reservation on the
+	 * AG. This may have occurred already via reservation calculation, but
+	 * fall back to an explicit init to ensure the in-core allocbt usage
+	 * counters are initialized as soon as possible. This is important
+	 * because filesystems with large perag reservations are susceptible to
+	 * free space reservation problems that the allocbt counter is used to
+	 * address.
+	 */
+	if (has_resv) {
+		error2 = xfs_alloc_pagf_init(mp, tp, pag->pag_agno, 0);
+		if (error2)
+			return error2;
+		ASSERT(xfs_perag_resv(pag, XFS_AG_RESV_METADATA)->ar_reserved +
+		       xfs_perag_resv(pag, XFS_AG_RESV_RMAPBT)->ar_reserved <=
+		       pag->pagf_freeblks + pag->pagf_flcount);
+	}
 	return error;
 }
 
-- 
2.26.3


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH v4 2/3] xfs: introduce in-core global counter of allocbt blocks
  2021-04-23 13:10 [PATCH v4 0/3] xfs: set aside allocation btree blocks from block reservation Brian Foster
  2021-04-23 13:10 ` [PATCH v4 1/3] xfs: unconditionally read all AGFs on mounts with perag reservation Brian Foster
@ 2021-04-23 13:10 ` Brian Foster
  2021-04-27 10:28   ` Chandan Babu R
                     ` (2 more replies)
  2021-04-23 13:10 ` [PATCH v4 3/3] xfs: set aside allocation btree blocks from block reservation Brian Foster
  2 siblings, 3 replies; 18+ messages in thread
From: Brian Foster @ 2021-04-23 13:10 UTC (permalink / raw)
  To: linux-xfs

Introduce an in-core counter to track the sum of all allocbt blocks
used by the filesystem. This value is currently tracked per-ag via
the ->agf_btreeblks field in the AGF, which also happens to include
rmapbt blocks. A global, in-core count of allocbt blocks is required
to identify the subset of global ->m_fdblocks that consists of
unavailable blocks currently used for allocation btrees. To support
this calculation at block reservation time, construct a similar
global counter for allocbt blocks, populate it on first read of each
AGF and update it as allocbt blocks are used and released.

Signed-off-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/libxfs/xfs_alloc.c       | 12 ++++++++++++
 fs/xfs/libxfs/xfs_alloc_btree.c |  2 ++
 fs/xfs/xfs_mount.h              |  6 ++++++
 3 files changed, 20 insertions(+)

diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
index aaa19101bb2a..144e2d68245c 100644
--- a/fs/xfs/libxfs/xfs_alloc.c
+++ b/fs/xfs/libxfs/xfs_alloc.c
@@ -3036,6 +3036,7 @@ xfs_alloc_read_agf(
 	struct xfs_agf		*agf;		/* ag freelist header */
 	struct xfs_perag	*pag;		/* per allocation group data */
 	int			error;
+	uint32_t		allocbt_blks;
 
 	trace_xfs_alloc_read_agf(mp, agno);
 
@@ -3066,6 +3067,17 @@ xfs_alloc_read_agf(
 		pag->pagf_refcount_level = be32_to_cpu(agf->agf_refcount_level);
 		pag->pagf_init = 1;
 		pag->pagf_agflreset = xfs_agfl_needs_reset(mp, agf);
+
+		/*
+		 * Update the global in-core allocbt block counter. Filter
+		 * rmapbt blocks from the on-disk counter because those are
+		 * managed by perag reservation.
+		 */
+		if (pag->pagf_btreeblks > be32_to_cpu(agf->agf_rmap_blocks)) {
+			allocbt_blks = pag->pagf_btreeblks -
+					be32_to_cpu(agf->agf_rmap_blocks);
+			atomic64_add(allocbt_blks, &mp->m_allocbt_blks);
+		}
 	}
 #ifdef DEBUG
 	else if (!XFS_FORCED_SHUTDOWN(mp)) {
diff --git a/fs/xfs/libxfs/xfs_alloc_btree.c b/fs/xfs/libxfs/xfs_alloc_btree.c
index 8e01231b308e..9f5a45f7baed 100644
--- a/fs/xfs/libxfs/xfs_alloc_btree.c
+++ b/fs/xfs/libxfs/xfs_alloc_btree.c
@@ -71,6 +71,7 @@ xfs_allocbt_alloc_block(
 		return 0;
 	}
 
+	atomic64_inc(&cur->bc_mp->m_allocbt_blks);
 	xfs_extent_busy_reuse(cur->bc_mp, cur->bc_ag.agno, bno, 1, false);
 
 	xfs_trans_agbtree_delta(cur->bc_tp, 1);
@@ -95,6 +96,7 @@ xfs_allocbt_free_block(
 	if (error)
 		return error;
 
+	atomic64_dec(&cur->bc_mp->m_allocbt_blks);
 	xfs_extent_busy_insert(cur->bc_tp, be32_to_cpu(agf->agf_seqno), bno, 1,
 			      XFS_EXTENT_BUSY_SKIP_DISCARD);
 	xfs_trans_agbtree_delta(cur->bc_tp, -1);
diff --git a/fs/xfs/xfs_mount.h b/fs/xfs/xfs_mount.h
index 81829d19596e..bb67274ee23f 100644
--- a/fs/xfs/xfs_mount.h
+++ b/fs/xfs/xfs_mount.h
@@ -170,6 +170,12 @@ typedef struct xfs_mount {
 	 * extents or anything related to the rt device.
 	 */
 	struct percpu_counter	m_delalloc_blks;
+	/*
+	 * Global count of allocation btree blocks in use across all AGs. Only
+	 * used when perag reservation is enabled. Helps prevent block
+	 * reservation from attempting to reserve allocation btree blocks.
+	 */
+	atomic64_t		m_allocbt_blks;
 
 	struct radix_tree_root	m_perag_tree;	/* per-ag accounting info */
 	spinlock_t		m_perag_lock;	/* lock for m_perag_tree */
-- 
2.26.3


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH v4 3/3] xfs: set aside allocation btree blocks from block reservation
  2021-04-23 13:10 [PATCH v4 0/3] xfs: set aside allocation btree blocks from block reservation Brian Foster
  2021-04-23 13:10 ` [PATCH v4 1/3] xfs: unconditionally read all AGFs on mounts with perag reservation Brian Foster
  2021-04-23 13:10 ` [PATCH v4 2/3] xfs: introduce in-core global counter of allocbt blocks Brian Foster
@ 2021-04-23 13:10 ` Brian Foster
  2021-04-27 10:29   ` Chandan Babu R
                     ` (2 more replies)
  2 siblings, 3 replies; 18+ messages in thread
From: Brian Foster @ 2021-04-23 13:10 UTC (permalink / raw)
  To: linux-xfs

The blocks used for allocation btrees (bnobt and countbt) are
technically considered free space. This is because as free space is
used, allocbt blocks are removed and naturally become available for
traditional allocation. However, this means that a significant
portion of free space may consist of in-use btree blocks if free
space is severely fragmented.

On large filesystems with large perag reservations, this can lead to
a rare but nasty condition where a significant amount of physical
free space is available, but the majority of actual usable blocks
consist of in-use allocbt blocks. We have a record of a (~12TB, 32
AG) filesystem with multiple AGs in a state with ~2.5GB or so free
blocks tracked across ~300 total allocbt blocks, but effectively at
100% full because the the free space is entirely consumed by
refcountbt perag reservation.

Such a large perag reservation is by design on large filesystems.
The problem is that because the free space is so fragmented, this AG
contributes the 300 or so allocbt blocks to the global counters as
free space. If this pattern repeats across enough AGs, the
filesystem lands in a state where global block reservation can
outrun physical block availability. For example, a streaming
buffered write on the affected filesystem continues to allow delayed
allocation beyond the point where writeback starts to fail due to
physical block allocation failures. The expected behavior is for the
delalloc block reservation to fail gracefully with -ENOSPC before
physical block allocation failure is a possibility.

To address this problem, set aside in-use allocbt blocks at
reservation time and thus ensure they cannot be reserved until truly
available for physical allocation. This allows alloc btree metadata
to continue to reside in free space, but dynamically adjusts
reservation availability based on internal state. Note that the
logic requires that the allocbt counter is fully populated at
reservation time before it is fully effective. We currently rely on
the mount time AGF scan in the perag reservation initialization code
for this dependency on filesystems where it's most important (i.e.
with active perag reservations).

Signed-off-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/xfs_mount.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
index cb1e2c4702c3..bdfee1943796 100644
--- a/fs/xfs/xfs_mount.c
+++ b/fs/xfs/xfs_mount.c
@@ -1188,6 +1188,7 @@ xfs_mod_fdblocks(
 	int64_t			lcounter;
 	long long		res_used;
 	s32			batch;
+	uint64_t		set_aside;
 
 	if (delta > 0) {
 		/*
@@ -1227,8 +1228,20 @@ xfs_mod_fdblocks(
 	else
 		batch = XFS_FDBLOCKS_BATCH;
 
+	/*
+	 * Set aside allocbt blocks because these blocks are tracked as free
+	 * space but not available for allocation. Technically this means that a
+	 * single reservation cannot consume all remaining free space, but the
+	 * ratio of allocbt blocks to usable free blocks should be rather small.
+	 * The tradeoff without this is that filesystems that maintain high
+	 * perag block reservations can over reserve physical block availability
+	 * and fail physical allocation, which leads to much more serious
+	 * problems (i.e. transaction abort, pagecache discards, etc.) than
+	 * slightly premature -ENOSPC.
+	 */
+	set_aside = mp->m_alloc_set_aside + atomic64_read(&mp->m_allocbt_blks);
 	percpu_counter_add_batch(&mp->m_fdblocks, delta, batch);
-	if (__percpu_counter_compare(&mp->m_fdblocks, mp->m_alloc_set_aside,
+	if (__percpu_counter_compare(&mp->m_fdblocks, set_aside,
 				     XFS_FDBLOCKS_BATCH) >= 0) {
 		/* we had space! */
 		return 0;
-- 
2.26.3


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [PATCH v4 1/3] xfs: unconditionally read all AGFs on mounts with perag reservation
  2021-04-23 13:10 ` [PATCH v4 1/3] xfs: unconditionally read all AGFs on mounts with perag reservation Brian Foster
@ 2021-04-27 10:22   ` Chandan Babu R
  2021-04-27 21:36   ` Allison Henderson
  2021-04-28  4:12   ` Darrick J. Wong
  2 siblings, 0 replies; 18+ messages in thread
From: Chandan Babu R @ 2021-04-27 10:22 UTC (permalink / raw)
  To: Brian Foster; +Cc: linux-xfs

On 23 Apr 2021 at 18:40, Brian Foster wrote:
> perag reservation is enabled at mount time on a per AG basis. The
> upcoming change to set aside allocbt blocks from block reservation
> requires a populated allocbt counter as soon as possible after mount
> to be fully effective against large perag reservations. Therefore as
> a preparation step, initialize the pagf on all mounts where at least
> one reservation is active. Note that this already occurs to some
> degree on most default format filesystems as reservation requirement
> calculations already depend on the AGF or AGI, depending on the
> reservation type.
>

The changes look good to me.

Reviewed-by: Chandan Babu R <chandanrlinux@gmail.com>

> Signed-off-by: Brian Foster <bfoster@redhat.com>
> ---
>  fs/xfs/libxfs/xfs_ag_resv.c | 34 +++++++++++++++++++++++-----------
>  1 file changed, 23 insertions(+), 11 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_ag_resv.c b/fs/xfs/libxfs/xfs_ag_resv.c
> index 6c5f8d10589c..e32a1833d523 100644
> --- a/fs/xfs/libxfs/xfs_ag_resv.c
> +++ b/fs/xfs/libxfs/xfs_ag_resv.c
> @@ -253,7 +253,8 @@ xfs_ag_resv_init(
>  	xfs_agnumber_t			agno = pag->pag_agno;
>  	xfs_extlen_t			ask;
>  	xfs_extlen_t			used;
> -	int				error = 0;
> +	int				error = 0, error2;
> +	bool				has_resv = false;
>  
>  	/* Create the metadata reservation. */
>  	if (pag->pag_meta_resv.ar_asked == 0) {
> @@ -291,6 +292,8 @@ xfs_ag_resv_init(
>  			if (error)
>  				goto out;
>  		}
> +		if (ask)
> +			has_resv = true;
>  	}
>  
>  	/* Create the RMAPBT metadata reservation */
> @@ -304,19 +307,28 @@ xfs_ag_resv_init(
>  		error = __xfs_ag_resv_init(pag, XFS_AG_RESV_RMAPBT, ask, used);
>  		if (error)
>  			goto out;
> +		if (ask)
> +			has_resv = true;
>  	}
>  
> -#ifdef DEBUG
> -	/* need to read in the AGF for the ASSERT below to work */
> -	error = xfs_alloc_pagf_init(pag->pag_mount, tp, pag->pag_agno, 0);
> -	if (error)
> -		return error;
> -
> -	ASSERT(xfs_perag_resv(pag, XFS_AG_RESV_METADATA)->ar_reserved +
> -	       xfs_perag_resv(pag, XFS_AG_RESV_RMAPBT)->ar_reserved <=
> -	       pag->pagf_freeblks + pag->pagf_flcount);
> -#endif
>  out:
> +	/*
> +	 * Initialize the pagf if we have at least one active reservation on the
> +	 * AG. This may have occurred already via reservation calculation, but
> +	 * fall back to an explicit init to ensure the in-core allocbt usage
> +	 * counters are initialized as soon as possible. This is important
> +	 * because filesystems with large perag reservations are susceptible to
> +	 * free space reservation problems that the allocbt counter is used to
> +	 * address.
> +	 */
> +	if (has_resv) {
> +		error2 = xfs_alloc_pagf_init(mp, tp, pag->pag_agno, 0);
> +		if (error2)
> +			return error2;
> +		ASSERT(xfs_perag_resv(pag, XFS_AG_RESV_METADATA)->ar_reserved +
> +		       xfs_perag_resv(pag, XFS_AG_RESV_RMAPBT)->ar_reserved <=
> +		       pag->pagf_freeblks + pag->pagf_flcount);
> +	}
>  	return error;
>  }

-- 
chandan

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v4 2/3] xfs: introduce in-core global counter of allocbt blocks
  2021-04-23 13:10 ` [PATCH v4 2/3] xfs: introduce in-core global counter of allocbt blocks Brian Foster
@ 2021-04-27 10:28   ` Chandan Babu R
  2021-04-27 11:33     ` Brian Foster
  2021-04-27 21:37   ` Allison Henderson
  2021-04-28  4:15   ` Darrick J. Wong
  2 siblings, 1 reply; 18+ messages in thread
From: Chandan Babu R @ 2021-04-27 10:28 UTC (permalink / raw)
  To: Brian Foster; +Cc: linux-xfs

On 23 Apr 2021 at 18:40, Brian Foster wrote:
> Introduce an in-core counter to track the sum of all allocbt blocks
> used by the filesystem. This value is currently tracked per-ag via
> the ->agf_btreeblks field in the AGF, which also happens to include
> rmapbt blocks. A global, in-core count of allocbt blocks is required
> to identify the subset of global ->m_fdblocks that consists of
> unavailable blocks currently used for allocation btrees. To support
> this calculation at block reservation time, construct a similar
> global counter for allocbt blocks, populate it on first read of each
> AGF and update it as allocbt blocks are used and released.
>
> Signed-off-by: Brian Foster <bfoster@redhat.com>
> ---
>  fs/xfs/libxfs/xfs_alloc.c       | 12 ++++++++++++
>  fs/xfs/libxfs/xfs_alloc_btree.c |  2 ++
>  fs/xfs/xfs_mount.h              |  6 ++++++
>  3 files changed, 20 insertions(+)
>
> diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
> index aaa19101bb2a..144e2d68245c 100644
> --- a/fs/xfs/libxfs/xfs_alloc.c
> +++ b/fs/xfs/libxfs/xfs_alloc.c
> @@ -3036,6 +3036,7 @@ xfs_alloc_read_agf(
>  	struct xfs_agf		*agf;		/* ag freelist header */
>  	struct xfs_perag	*pag;		/* per allocation group data */
>  	int			error;
> +	uint32_t		allocbt_blks;
>
>  	trace_xfs_alloc_read_agf(mp, agno);
>
> @@ -3066,6 +3067,17 @@ xfs_alloc_read_agf(
>  		pag->pagf_refcount_level = be32_to_cpu(agf->agf_refcount_level);
>  		pag->pagf_init = 1;
>  		pag->pagf_agflreset = xfs_agfl_needs_reset(mp, agf);
> +
> +		/*
> +		 * Update the global in-core allocbt block counter. Filter
> +		 * rmapbt blocks from the on-disk counter because those are
> +		 * managed by perag reservation.
> +		 */
> +		if (pag->pagf_btreeblks > be32_to_cpu(agf->agf_rmap_blocks)) {

pag->pagf_btreeblks gets incremented everytime a block is allocated to refill
AGFL (via xfs_alloc_get_freelist()). Apart from the allobt trees, blocks for
Rmap btree also get allocated from AGFL. Hence pag->pagf_btreeblks must be
larger than agf->agf_rmap_blocks.

Can you please describe the scenario in which pag->pagf_btreeblks has a value
that is <= agf->agf_rmap_blocks?

--
chandan

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v4 3/3] xfs: set aside allocation btree blocks from block reservation
  2021-04-23 13:10 ` [PATCH v4 3/3] xfs: set aside allocation btree blocks from block reservation Brian Foster
@ 2021-04-27 10:29   ` Chandan Babu R
  2021-04-27 21:37   ` Allison Henderson
  2021-04-28  4:12   ` Darrick J. Wong
  2 siblings, 0 replies; 18+ messages in thread
From: Chandan Babu R @ 2021-04-27 10:29 UTC (permalink / raw)
  To: Brian Foster; +Cc: linux-xfs

On 23 Apr 2021 at 18:40, Brian Foster wrote:
> The blocks used for allocation btrees (bnobt and countbt) are
> technically considered free space. This is because as free space is
> used, allocbt blocks are removed and naturally become available for
> traditional allocation. However, this means that a significant
> portion of free space may consist of in-use btree blocks if free
> space is severely fragmented.
>
> On large filesystems with large perag reservations, this can lead to
> a rare but nasty condition where a significant amount of physical
> free space is available, but the majority of actual usable blocks
> consist of in-use allocbt blocks. We have a record of a (~12TB, 32
> AG) filesystem with multiple AGs in a state with ~2.5GB or so free
> blocks tracked across ~300 total allocbt blocks, but effectively at
> 100% full because the the free space is entirely consumed by
> refcountbt perag reservation.
>
> Such a large perag reservation is by design on large filesystems.
> The problem is that because the free space is so fragmented, this AG
> contributes the 300 or so allocbt blocks to the global counters as
> free space. If this pattern repeats across enough AGs, the
> filesystem lands in a state where global block reservation can
> outrun physical block availability. For example, a streaming
> buffered write on the affected filesystem continues to allow delayed
> allocation beyond the point where writeback starts to fail due to
> physical block allocation failures. The expected behavior is for the
> delalloc block reservation to fail gracefully with -ENOSPC before
> physical block allocation failure is a possibility.
>
> To address this problem, set aside in-use allocbt blocks at
> reservation time and thus ensure they cannot be reserved until truly
> available for physical allocation. This allows alloc btree metadata
> to continue to reside in free space, but dynamically adjusts
> reservation availability based on internal state. Note that the
> logic requires that the allocbt counter is fully populated at
> reservation time before it is fully effective. We currently rely on
> the mount time AGF scan in the perag reservation initialization code
> for this dependency on filesystems where it's most important (i.e.
> with active perag reservations).
>

The changes look good to me.

Reviewed-by: Chandan Babu R <chandanrlinux@gmail.com>

-- 
chandan

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v4 2/3] xfs: introduce in-core global counter of allocbt blocks
  2021-04-27 10:28   ` Chandan Babu R
@ 2021-04-27 11:33     ` Brian Foster
  2021-04-27 13:22       ` Chandan Babu R
  0 siblings, 1 reply; 18+ messages in thread
From: Brian Foster @ 2021-04-27 11:33 UTC (permalink / raw)
  To: Chandan Babu R; +Cc: linux-xfs

On Tue, Apr 27, 2021 at 03:58:16PM +0530, Chandan Babu R wrote:
> On 23 Apr 2021 at 18:40, Brian Foster wrote:
> > Introduce an in-core counter to track the sum of all allocbt blocks
> > used by the filesystem. This value is currently tracked per-ag via
> > the ->agf_btreeblks field in the AGF, which also happens to include
> > rmapbt blocks. A global, in-core count of allocbt blocks is required
> > to identify the subset of global ->m_fdblocks that consists of
> > unavailable blocks currently used for allocation btrees. To support
> > this calculation at block reservation time, construct a similar
> > global counter for allocbt blocks, populate it on first read of each
> > AGF and update it as allocbt blocks are used and released.
> >
> > Signed-off-by: Brian Foster <bfoster@redhat.com>
> > ---
> >  fs/xfs/libxfs/xfs_alloc.c       | 12 ++++++++++++
> >  fs/xfs/libxfs/xfs_alloc_btree.c |  2 ++
> >  fs/xfs/xfs_mount.h              |  6 ++++++
> >  3 files changed, 20 insertions(+)
> >
> > diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
> > index aaa19101bb2a..144e2d68245c 100644
> > --- a/fs/xfs/libxfs/xfs_alloc.c
> > +++ b/fs/xfs/libxfs/xfs_alloc.c
> > @@ -3036,6 +3036,7 @@ xfs_alloc_read_agf(
> >  	struct xfs_agf		*agf;		/* ag freelist header */
> >  	struct xfs_perag	*pag;		/* per allocation group data */
> >  	int			error;
> > +	uint32_t		allocbt_blks;
> >
> >  	trace_xfs_alloc_read_agf(mp, agno);
> >
> > @@ -3066,6 +3067,17 @@ xfs_alloc_read_agf(
> >  		pag->pagf_refcount_level = be32_to_cpu(agf->agf_refcount_level);
> >  		pag->pagf_init = 1;
> >  		pag->pagf_agflreset = xfs_agfl_needs_reset(mp, agf);
> > +
> > +		/*
> > +		 * Update the global in-core allocbt block counter. Filter
> > +		 * rmapbt blocks from the on-disk counter because those are
> > +		 * managed by perag reservation.
> > +		 */
> > +		if (pag->pagf_btreeblks > be32_to_cpu(agf->agf_rmap_blocks)) {
> 
> pag->pagf_btreeblks gets incremented everytime a block is allocated to refill
> AGFL (via xfs_alloc_get_freelist()). Apart from the allobt trees, blocks for
> Rmap btree also get allocated from AGFL. Hence pag->pagf_btreeblks must be
> larger than agf->agf_rmap_blocks.
> 

This function is actually to consume a block from the AGFL (as opposed
to refill it).

> Can you please describe the scenario in which pag->pagf_btreeblks has a value
> that is <= agf->agf_rmap_blocks?
> 

Ah, this was just an initialization quirk. I originally had an assert
here and based the logic on the assumption that pagf_btreeblks >=
agf_rmap_blocks, but alas:

# mkfs.xfs -f -mrmapbt <dev>
...
# xfs_db -c "agf 0" -c "p rmapblocks" -c "p btreeblks" <dev>
rmapblocks = 1
btreeblks = 0
#

Brian

> --
> chandan
> 


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v4 2/3] xfs: introduce in-core global counter of allocbt blocks
  2021-04-27 11:33     ` Brian Foster
@ 2021-04-27 13:22       ` Chandan Babu R
  0 siblings, 0 replies; 18+ messages in thread
From: Chandan Babu R @ 2021-04-27 13:22 UTC (permalink / raw)
  To: Brian Foster; +Cc: linux-xfs

On 27 Apr 2021 at 17:03, Brian Foster wrote:
> On Tue, Apr 27, 2021 at 03:58:16PM +0530, Chandan Babu R wrote:
>> On 23 Apr 2021 at 18:40, Brian Foster wrote:
>> > Introduce an in-core counter to track the sum of all allocbt blocks
>> > used by the filesystem. This value is currently tracked per-ag via
>> > the ->agf_btreeblks field in the AGF, which also happens to include
>> > rmapbt blocks. A global, in-core count of allocbt blocks is required
>> > to identify the subset of global ->m_fdblocks that consists of
>> > unavailable blocks currently used for allocation btrees. To support
>> > this calculation at block reservation time, construct a similar
>> > global counter for allocbt blocks, populate it on first read of each
>> > AGF and update it as allocbt blocks are used and released.
>> >
>> > Signed-off-by: Brian Foster <bfoster@redhat.com>
>> > ---
>> >  fs/xfs/libxfs/xfs_alloc.c       | 12 ++++++++++++
>> >  fs/xfs/libxfs/xfs_alloc_btree.c |  2 ++
>> >  fs/xfs/xfs_mount.h              |  6 ++++++
>> >  3 files changed, 20 insertions(+)
>> >
>> > diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
>> > index aaa19101bb2a..144e2d68245c 100644
>> > --- a/fs/xfs/libxfs/xfs_alloc.c
>> > +++ b/fs/xfs/libxfs/xfs_alloc.c
>> > @@ -3036,6 +3036,7 @@ xfs_alloc_read_agf(
>> >  	struct xfs_agf		*agf;		/* ag freelist header */
>> >  	struct xfs_perag	*pag;		/* per allocation group data */
>> >  	int			error;
>> > +	uint32_t		allocbt_blks;
>> >
>> >  	trace_xfs_alloc_read_agf(mp, agno);
>> >
>> > @@ -3066,6 +3067,17 @@ xfs_alloc_read_agf(
>> >  		pag->pagf_refcount_level = be32_to_cpu(agf->agf_refcount_level);
>> >  		pag->pagf_init = 1;
>> >  		pag->pagf_agflreset = xfs_agfl_needs_reset(mp, agf);
>> > +
>> > +		/*
>> > +		 * Update the global in-core allocbt block counter. Filter
>> > +		 * rmapbt blocks from the on-disk counter because those are
>> > +		 * managed by perag reservation.
>> > +		 */
>> > +		if (pag->pagf_btreeblks > be32_to_cpu(agf->agf_rmap_blocks)) {
>> 
>> pag->pagf_btreeblks gets incremented everytime a block is allocated to refill
>> AGFL (via xfs_alloc_get_freelist()). Apart from the allobt trees, blocks for
>> Rmap btree also get allocated from AGFL. Hence pag->pagf_btreeblks must be
>> larger than agf->agf_rmap_blocks.
>> 
>
> This function is actually to consume a block from the AGFL (as opposed
> to refill it).

Sorry, I meant to say "allocate a block from AGFL".

>
>> Can you please describe the scenario in which pag->pagf_btreeblks has a value
>> that is <= agf->agf_rmap_blocks?
>> 
>
> Ah, this was just an initialization quirk. I originally had an assert
> here and based the logic on the assumption that pagf_btreeblks >=
> agf_rmap_blocks, but alas:
>
> # mkfs.xfs -f -mrmapbt <dev>
> ...
> # xfs_db -c "agf 0" -c "p rmapblocks" -c "p btreeblks" <dev>
> rmapblocks = 1
> btreeblks = 0
> #

Thanks for clarifying my doubt.

The patch looks good to me.

Reviewed-by: Chandan Babu R <chandanrlinux@gmail.com>

-- 
chandan

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v4 1/3] xfs: unconditionally read all AGFs on mounts with perag reservation
  2021-04-23 13:10 ` [PATCH v4 1/3] xfs: unconditionally read all AGFs on mounts with perag reservation Brian Foster
  2021-04-27 10:22   ` Chandan Babu R
@ 2021-04-27 21:36   ` Allison Henderson
  2021-04-28  4:12   ` Darrick J. Wong
  2 siblings, 0 replies; 18+ messages in thread
From: Allison Henderson @ 2021-04-27 21:36 UTC (permalink / raw)
  To: Brian Foster, linux-xfs



On 4/23/21 6:10 AM, Brian Foster wrote:
> perag reservation is enabled at mount time on a per AG basis. The
> upcoming change to set aside allocbt blocks from block reservation
> requires a populated allocbt counter as soon as possible after mount
> to be fully effective against large perag reservations. Therefore as
> a preparation step, initialize the pagf on all mounts where at least
> one reservation is active. Note that this already occurs to some
> degree on most default format filesystems as reservation requirement
> calculations already depend on the AGF or AGI, depending on the
> reservation type.
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>
Looks ok to me
Reviewed-by: Allison Henderson <allison.henderson@oracle.com>

> ---
>   fs/xfs/libxfs/xfs_ag_resv.c | 34 +++++++++++++++++++++++-----------
>   1 file changed, 23 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_ag_resv.c b/fs/xfs/libxfs/xfs_ag_resv.c
> index 6c5f8d10589c..e32a1833d523 100644
> --- a/fs/xfs/libxfs/xfs_ag_resv.c
> +++ b/fs/xfs/libxfs/xfs_ag_resv.c
> @@ -253,7 +253,8 @@ xfs_ag_resv_init(
>   	xfs_agnumber_t			agno = pag->pag_agno;
>   	xfs_extlen_t			ask;
>   	xfs_extlen_t			used;
> -	int				error = 0;
> +	int				error = 0, error2;
> +	bool				has_resv = false;
>   
>   	/* Create the metadata reservation. */
>   	if (pag->pag_meta_resv.ar_asked == 0) {
> @@ -291,6 +292,8 @@ xfs_ag_resv_init(
>   			if (error)
>   				goto out;
>   		}
> +		if (ask)
> +			has_resv = true;
>   	}
>   
>   	/* Create the RMAPBT metadata reservation */
> @@ -304,19 +307,28 @@ xfs_ag_resv_init(
>   		error = __xfs_ag_resv_init(pag, XFS_AG_RESV_RMAPBT, ask, used);
>   		if (error)
>   			goto out;
> +		if (ask)
> +			has_resv = true;
>   	}
>   
> -#ifdef DEBUG
> -	/* need to read in the AGF for the ASSERT below to work */
> -	error = xfs_alloc_pagf_init(pag->pag_mount, tp, pag->pag_agno, 0);
> -	if (error)
> -		return error;
> -
> -	ASSERT(xfs_perag_resv(pag, XFS_AG_RESV_METADATA)->ar_reserved +
> -	       xfs_perag_resv(pag, XFS_AG_RESV_RMAPBT)->ar_reserved <=
> -	       pag->pagf_freeblks + pag->pagf_flcount);
> -#endif
>   out:
> +	/*
> +	 * Initialize the pagf if we have at least one active reservation on the
> +	 * AG. This may have occurred already via reservation calculation, but
> +	 * fall back to an explicit init to ensure the in-core allocbt usage
> +	 * counters are initialized as soon as possible. This is important
> +	 * because filesystems with large perag reservations are susceptible to
> +	 * free space reservation problems that the allocbt counter is used to
> +	 * address.
> +	 */
> +	if (has_resv) {
> +		error2 = xfs_alloc_pagf_init(mp, tp, pag->pag_agno, 0);
> +		if (error2)
> +			return error2;
> +		ASSERT(xfs_perag_resv(pag, XFS_AG_RESV_METADATA)->ar_reserved +
> +		       xfs_perag_resv(pag, XFS_AG_RESV_RMAPBT)->ar_reserved <=
> +		       pag->pagf_freeblks + pag->pagf_flcount);
> +	}
>   	return error;
>   }
>   
> 

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v4 2/3] xfs: introduce in-core global counter of allocbt blocks
  2021-04-23 13:10 ` [PATCH v4 2/3] xfs: introduce in-core global counter of allocbt blocks Brian Foster
  2021-04-27 10:28   ` Chandan Babu R
@ 2021-04-27 21:37   ` Allison Henderson
  2021-04-28  4:15   ` Darrick J. Wong
  2 siblings, 0 replies; 18+ messages in thread
From: Allison Henderson @ 2021-04-27 21:37 UTC (permalink / raw)
  To: Brian Foster, linux-xfs



On 4/23/21 6:10 AM, Brian Foster wrote:
> Introduce an in-core counter to track the sum of all allocbt blocks
> used by the filesystem. This value is currently tracked per-ag via
> the ->agf_btreeblks field in the AGF, which also happens to include
> rmapbt blocks. A global, in-core count of allocbt blocks is required
> to identify the subset of global ->m_fdblocks that consists of
> unavailable blocks currently used for allocation btrees. To support
> this calculation at block reservation time, construct a similar
> global counter for allocbt blocks, populate it on first read of each
> AGF and update it as allocbt blocks are used and released.
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>
OK, makes sense
Reviewed-by: Allison Henderson <allison.henderson@oracle.com>

> ---
>   fs/xfs/libxfs/xfs_alloc.c       | 12 ++++++++++++
>   fs/xfs/libxfs/xfs_alloc_btree.c |  2 ++
>   fs/xfs/xfs_mount.h              |  6 ++++++
>   3 files changed, 20 insertions(+)
> 
> diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
> index aaa19101bb2a..144e2d68245c 100644
> --- a/fs/xfs/libxfs/xfs_alloc.c
> +++ b/fs/xfs/libxfs/xfs_alloc.c
> @@ -3036,6 +3036,7 @@ xfs_alloc_read_agf(
>   	struct xfs_agf		*agf;		/* ag freelist header */
>   	struct xfs_perag	*pag;		/* per allocation group data */
>   	int			error;
> +	uint32_t		allocbt_blks;
>   
>   	trace_xfs_alloc_read_agf(mp, agno);
>   
> @@ -3066,6 +3067,17 @@ xfs_alloc_read_agf(
>   		pag->pagf_refcount_level = be32_to_cpu(agf->agf_refcount_level);
>   		pag->pagf_init = 1;
>   		pag->pagf_agflreset = xfs_agfl_needs_reset(mp, agf);
> +
> +		/*
> +		 * Update the global in-core allocbt block counter. Filter
> +		 * rmapbt blocks from the on-disk counter because those are
> +		 * managed by perag reservation.
> +		 */
> +		if (pag->pagf_btreeblks > be32_to_cpu(agf->agf_rmap_blocks)) {
> +			allocbt_blks = pag->pagf_btreeblks -
> +					be32_to_cpu(agf->agf_rmap_blocks);
> +			atomic64_add(allocbt_blks, &mp->m_allocbt_blks);
> +		}
>   	}
>   #ifdef DEBUG
>   	else if (!XFS_FORCED_SHUTDOWN(mp)) {
> diff --git a/fs/xfs/libxfs/xfs_alloc_btree.c b/fs/xfs/libxfs/xfs_alloc_btree.c
> index 8e01231b308e..9f5a45f7baed 100644
> --- a/fs/xfs/libxfs/xfs_alloc_btree.c
> +++ b/fs/xfs/libxfs/xfs_alloc_btree.c
> @@ -71,6 +71,7 @@ xfs_allocbt_alloc_block(
>   		return 0;
>   	}
>   
> +	atomic64_inc(&cur->bc_mp->m_allocbt_blks);
>   	xfs_extent_busy_reuse(cur->bc_mp, cur->bc_ag.agno, bno, 1, false);
>   
>   	xfs_trans_agbtree_delta(cur->bc_tp, 1);
> @@ -95,6 +96,7 @@ xfs_allocbt_free_block(
>   	if (error)
>   		return error;
>   
> +	atomic64_dec(&cur->bc_mp->m_allocbt_blks);
>   	xfs_extent_busy_insert(cur->bc_tp, be32_to_cpu(agf->agf_seqno), bno, 1,
>   			      XFS_EXTENT_BUSY_SKIP_DISCARD);
>   	xfs_trans_agbtree_delta(cur->bc_tp, -1);
> diff --git a/fs/xfs/xfs_mount.h b/fs/xfs/xfs_mount.h
> index 81829d19596e..bb67274ee23f 100644
> --- a/fs/xfs/xfs_mount.h
> +++ b/fs/xfs/xfs_mount.h
> @@ -170,6 +170,12 @@ typedef struct xfs_mount {
>   	 * extents or anything related to the rt device.
>   	 */
>   	struct percpu_counter	m_delalloc_blks;
> +	/*
> +	 * Global count of allocation btree blocks in use across all AGs. Only
> +	 * used when perag reservation is enabled. Helps prevent block
> +	 * reservation from attempting to reserve allocation btree blocks.
> +	 */
> +	atomic64_t		m_allocbt_blks;
>   
>   	struct radix_tree_root	m_perag_tree;	/* per-ag accounting info */
>   	spinlock_t		m_perag_lock;	/* lock for m_perag_tree */
> 

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v4 3/3] xfs: set aside allocation btree blocks from block reservation
  2021-04-23 13:10 ` [PATCH v4 3/3] xfs: set aside allocation btree blocks from block reservation Brian Foster
  2021-04-27 10:29   ` Chandan Babu R
@ 2021-04-27 21:37   ` Allison Henderson
  2021-04-28  4:12   ` Darrick J. Wong
  2 siblings, 0 replies; 18+ messages in thread
From: Allison Henderson @ 2021-04-27 21:37 UTC (permalink / raw)
  To: Brian Foster, linux-xfs



On 4/23/21 6:10 AM, Brian Foster wrote:
> The blocks used for allocation btrees (bnobt and countbt) are
> technically considered free space. This is because as free space is
> used, allocbt blocks are removed and naturally become available for
> traditional allocation. However, this means that a significant
> portion of free space may consist of in-use btree blocks if free
> space is severely fragmented.
> 
> On large filesystems with large perag reservations, this can lead to
> a rare but nasty condition where a significant amount of physical
> free space is available, but the majority of actual usable blocks
> consist of in-use allocbt blocks. We have a record of a (~12TB, 32
> AG) filesystem with multiple AGs in a state with ~2.5GB or so free
> blocks tracked across ~300 total allocbt blocks, but effectively at
> 100% full because the the free space is entirely consumed by
> refcountbt perag reservation.
> 
> Such a large perag reservation is by design on large filesystems.
> The problem is that because the free space is so fragmented, this AG
> contributes the 300 or so allocbt blocks to the global counters as
> free space. If this pattern repeats across enough AGs, the
> filesystem lands in a state where global block reservation can
> outrun physical block availability. For example, a streaming
> buffered write on the affected filesystem continues to allow delayed
> allocation beyond the point where writeback starts to fail due to
> physical block allocation failures. The expected behavior is for the
> delalloc block reservation to fail gracefully with -ENOSPC before
> physical block allocation failure is a possibility.
> 
> To address this problem, set aside in-use allocbt blocks at
> reservation time and thus ensure they cannot be reserved until truly
> available for physical allocation. This allows alloc btree metadata
> to continue to reside in free space, but dynamically adjusts
> reservation availability based on internal state. Note that the
> logic requires that the allocbt counter is fully populated at
> reservation time before it is fully effective. We currently rely on
> the mount time AGF scan in the perag reservation initialization code
> for this dependency on filesystems where it's most important (i.e.
> with active perag reservations).
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>
OK, makes sense, thanks for the comments!
Reviewed-by: Allison Henderson <allison.henderson@oracle.com>
> ---
>   fs/xfs/xfs_mount.c | 15 ++++++++++++++-
>   1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
> index cb1e2c4702c3..bdfee1943796 100644
> --- a/fs/xfs/xfs_mount.c
> +++ b/fs/xfs/xfs_mount.c
> @@ -1188,6 +1188,7 @@ xfs_mod_fdblocks(
>   	int64_t			lcounter;
>   	long long		res_used;
>   	s32			batch;
> +	uint64_t		set_aside;
>   
>   	if (delta > 0) {
>   		/*
> @@ -1227,8 +1228,20 @@ xfs_mod_fdblocks(
>   	else
>   		batch = XFS_FDBLOCKS_BATCH;
>   
> +	/*
> +	 * Set aside allocbt blocks because these blocks are tracked as free
> +	 * space but not available for allocation. Technically this means that a
> +	 * single reservation cannot consume all remaining free space, but the
> +	 * ratio of allocbt blocks to usable free blocks should be rather small.
> +	 * The tradeoff without this is that filesystems that maintain high
> +	 * perag block reservations can over reserve physical block availability
> +	 * and fail physical allocation, which leads to much more serious
> +	 * problems (i.e. transaction abort, pagecache discards, etc.) than
> +	 * slightly premature -ENOSPC.
> +	 */
> +	set_aside = mp->m_alloc_set_aside + atomic64_read(&mp->m_allocbt_blks);
>   	percpu_counter_add_batch(&mp->m_fdblocks, delta, batch);
> -	if (__percpu_counter_compare(&mp->m_fdblocks, mp->m_alloc_set_aside,
> +	if (__percpu_counter_compare(&mp->m_fdblocks, set_aside,
>   				     XFS_FDBLOCKS_BATCH) >= 0) {
>   		/* we had space! */
>   		return 0;
> 

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v4 1/3] xfs: unconditionally read all AGFs on mounts with perag reservation
  2021-04-23 13:10 ` [PATCH v4 1/3] xfs: unconditionally read all AGFs on mounts with perag reservation Brian Foster
  2021-04-27 10:22   ` Chandan Babu R
  2021-04-27 21:36   ` Allison Henderson
@ 2021-04-28  4:12   ` Darrick J. Wong
  2 siblings, 0 replies; 18+ messages in thread
From: Darrick J. Wong @ 2021-04-28  4:12 UTC (permalink / raw)
  To: Brian Foster; +Cc: linux-xfs

On Fri, Apr 23, 2021 at 09:10:48AM -0400, Brian Foster wrote:
> perag reservation is enabled at mount time on a per AG basis. The
> upcoming change to set aside allocbt blocks from block reservation
> requires a populated allocbt counter as soon as possible after mount
> to be fully effective against large perag reservations. Therefore as
> a preparation step, initialize the pagf on all mounts where at least
> one reservation is active. Note that this already occurs to some
> degree on most default format filesystems as reservation requirement
> calculations already depend on the AGF or AGI, depending on the
> reservation type.
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>

Looks good to me too,
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  fs/xfs/libxfs/xfs_ag_resv.c | 34 +++++++++++++++++++++++-----------
>  1 file changed, 23 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_ag_resv.c b/fs/xfs/libxfs/xfs_ag_resv.c
> index 6c5f8d10589c..e32a1833d523 100644
> --- a/fs/xfs/libxfs/xfs_ag_resv.c
> +++ b/fs/xfs/libxfs/xfs_ag_resv.c
> @@ -253,7 +253,8 @@ xfs_ag_resv_init(
>  	xfs_agnumber_t			agno = pag->pag_agno;
>  	xfs_extlen_t			ask;
>  	xfs_extlen_t			used;
> -	int				error = 0;
> +	int				error = 0, error2;
> +	bool				has_resv = false;
>  
>  	/* Create the metadata reservation. */
>  	if (pag->pag_meta_resv.ar_asked == 0) {
> @@ -291,6 +292,8 @@ xfs_ag_resv_init(
>  			if (error)
>  				goto out;
>  		}
> +		if (ask)
> +			has_resv = true;
>  	}
>  
>  	/* Create the RMAPBT metadata reservation */
> @@ -304,19 +307,28 @@ xfs_ag_resv_init(
>  		error = __xfs_ag_resv_init(pag, XFS_AG_RESV_RMAPBT, ask, used);
>  		if (error)
>  			goto out;
> +		if (ask)
> +			has_resv = true;
>  	}
>  
> -#ifdef DEBUG
> -	/* need to read in the AGF for the ASSERT below to work */
> -	error = xfs_alloc_pagf_init(pag->pag_mount, tp, pag->pag_agno, 0);
> -	if (error)
> -		return error;
> -
> -	ASSERT(xfs_perag_resv(pag, XFS_AG_RESV_METADATA)->ar_reserved +
> -	       xfs_perag_resv(pag, XFS_AG_RESV_RMAPBT)->ar_reserved <=
> -	       pag->pagf_freeblks + pag->pagf_flcount);
> -#endif
>  out:
> +	/*
> +	 * Initialize the pagf if we have at least one active reservation on the
> +	 * AG. This may have occurred already via reservation calculation, but
> +	 * fall back to an explicit init to ensure the in-core allocbt usage
> +	 * counters are initialized as soon as possible. This is important
> +	 * because filesystems with large perag reservations are susceptible to
> +	 * free space reservation problems that the allocbt counter is used to
> +	 * address.
> +	 */
> +	if (has_resv) {
> +		error2 = xfs_alloc_pagf_init(mp, tp, pag->pag_agno, 0);
> +		if (error2)
> +			return error2;
> +		ASSERT(xfs_perag_resv(pag, XFS_AG_RESV_METADATA)->ar_reserved +
> +		       xfs_perag_resv(pag, XFS_AG_RESV_RMAPBT)->ar_reserved <=
> +		       pag->pagf_freeblks + pag->pagf_flcount);
> +	}
>  	return error;
>  }
>  
> -- 
> 2.26.3
> 

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v4 3/3] xfs: set aside allocation btree blocks from block reservation
  2021-04-23 13:10 ` [PATCH v4 3/3] xfs: set aside allocation btree blocks from block reservation Brian Foster
  2021-04-27 10:29   ` Chandan Babu R
  2021-04-27 21:37   ` Allison Henderson
@ 2021-04-28  4:12   ` Darrick J. Wong
  2 siblings, 0 replies; 18+ messages in thread
From: Darrick J. Wong @ 2021-04-28  4:12 UTC (permalink / raw)
  To: Brian Foster; +Cc: linux-xfs

On Fri, Apr 23, 2021 at 09:10:50AM -0400, Brian Foster wrote:
> The blocks used for allocation btrees (bnobt and countbt) are
> technically considered free space. This is because as free space is
> used, allocbt blocks are removed and naturally become available for
> traditional allocation. However, this means that a significant
> portion of free space may consist of in-use btree blocks if free
> space is severely fragmented.
> 
> On large filesystems with large perag reservations, this can lead to
> a rare but nasty condition where a significant amount of physical
> free space is available, but the majority of actual usable blocks
> consist of in-use allocbt blocks. We have a record of a (~12TB, 32
> AG) filesystem with multiple AGs in a state with ~2.5GB or so free
> blocks tracked across ~300 total allocbt blocks, but effectively at
> 100% full because the the free space is entirely consumed by
> refcountbt perag reservation.
> 
> Such a large perag reservation is by design on large filesystems.
> The problem is that because the free space is so fragmented, this AG
> contributes the 300 or so allocbt blocks to the global counters as
> free space. If this pattern repeats across enough AGs, the
> filesystem lands in a state where global block reservation can
> outrun physical block availability. For example, a streaming
> buffered write on the affected filesystem continues to allow delayed
> allocation beyond the point where writeback starts to fail due to
> physical block allocation failures. The expected behavior is for the
> delalloc block reservation to fail gracefully with -ENOSPC before
> physical block allocation failure is a possibility.
> 
> To address this problem, set aside in-use allocbt blocks at
> reservation time and thus ensure they cannot be reserved until truly
> available for physical allocation. This allows alloc btree metadata
> to continue to reside in free space, but dynamically adjusts
> reservation availability based on internal state. Note that the
> logic requires that the allocbt counter is fully populated at
> reservation time before it is fully effective. We currently rely on
> the mount time AGF scan in the perag reservation initialization code
> for this dependency on filesystems where it's most important (i.e.
> with active perag reservations).
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>

<nod>
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  fs/xfs/xfs_mount.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
> index cb1e2c4702c3..bdfee1943796 100644
> --- a/fs/xfs/xfs_mount.c
> +++ b/fs/xfs/xfs_mount.c
> @@ -1188,6 +1188,7 @@ xfs_mod_fdblocks(
>  	int64_t			lcounter;
>  	long long		res_used;
>  	s32			batch;
> +	uint64_t		set_aside;
>  
>  	if (delta > 0) {
>  		/*
> @@ -1227,8 +1228,20 @@ xfs_mod_fdblocks(
>  	else
>  		batch = XFS_FDBLOCKS_BATCH;
>  
> +	/*
> +	 * Set aside allocbt blocks because these blocks are tracked as free
> +	 * space but not available for allocation. Technically this means that a
> +	 * single reservation cannot consume all remaining free space, but the
> +	 * ratio of allocbt blocks to usable free blocks should be rather small.
> +	 * The tradeoff without this is that filesystems that maintain high
> +	 * perag block reservations can over reserve physical block availability
> +	 * and fail physical allocation, which leads to much more serious
> +	 * problems (i.e. transaction abort, pagecache discards, etc.) than
> +	 * slightly premature -ENOSPC.
> +	 */
> +	set_aside = mp->m_alloc_set_aside + atomic64_read(&mp->m_allocbt_blks);
>  	percpu_counter_add_batch(&mp->m_fdblocks, delta, batch);
> -	if (__percpu_counter_compare(&mp->m_fdblocks, mp->m_alloc_set_aside,
> +	if (__percpu_counter_compare(&mp->m_fdblocks, set_aside,
>  				     XFS_FDBLOCKS_BATCH) >= 0) {
>  		/* we had space! */
>  		return 0;
> -- 
> 2.26.3
> 

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v4 2/3] xfs: introduce in-core global counter of allocbt blocks
  2021-04-23 13:10 ` [PATCH v4 2/3] xfs: introduce in-core global counter of allocbt blocks Brian Foster
  2021-04-27 10:28   ` Chandan Babu R
  2021-04-27 21:37   ` Allison Henderson
@ 2021-04-28  4:15   ` Darrick J. Wong
  2021-04-28 15:01     ` Brian Foster
  2 siblings, 1 reply; 18+ messages in thread
From: Darrick J. Wong @ 2021-04-28  4:15 UTC (permalink / raw)
  To: Brian Foster; +Cc: linux-xfs

On Fri, Apr 23, 2021 at 09:10:49AM -0400, Brian Foster wrote:
> Introduce an in-core counter to track the sum of all allocbt blocks
> used by the filesystem. This value is currently tracked per-ag via
> the ->agf_btreeblks field in the AGF, which also happens to include
> rmapbt blocks. A global, in-core count of allocbt blocks is required
> to identify the subset of global ->m_fdblocks that consists of
> unavailable blocks currently used for allocation btrees. To support
> this calculation at block reservation time, construct a similar
> global counter for allocbt blocks, populate it on first read of each
> AGF and update it as allocbt blocks are used and released.
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>
> ---
>  fs/xfs/libxfs/xfs_alloc.c       | 12 ++++++++++++
>  fs/xfs/libxfs/xfs_alloc_btree.c |  2 ++
>  fs/xfs/xfs_mount.h              |  6 ++++++
>  3 files changed, 20 insertions(+)
> 
> diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
> index aaa19101bb2a..144e2d68245c 100644
> --- a/fs/xfs/libxfs/xfs_alloc.c
> +++ b/fs/xfs/libxfs/xfs_alloc.c
> @@ -3036,6 +3036,7 @@ xfs_alloc_read_agf(
>  	struct xfs_agf		*agf;		/* ag freelist header */
>  	struct xfs_perag	*pag;		/* per allocation group data */
>  	int			error;
> +	uint32_t		allocbt_blks;
>  
>  	trace_xfs_alloc_read_agf(mp, agno);
>  
> @@ -3066,6 +3067,17 @@ xfs_alloc_read_agf(
>  		pag->pagf_refcount_level = be32_to_cpu(agf->agf_refcount_level);
>  		pag->pagf_init = 1;
>  		pag->pagf_agflreset = xfs_agfl_needs_reset(mp, agf);
> +
> +		/*
> +		 * Update the global in-core allocbt block counter. Filter
> +		 * rmapbt blocks from the on-disk counter because those are
> +		 * managed by perag reservation.
> +		 */
> +		if (pag->pagf_btreeblks > be32_to_cpu(agf->agf_rmap_blocks)) {

As pointed out elsewhere in the thread, agf_rmap_blocks counts the total
number of blocks in the rmapbt (whereas agf_btreeblks counts the number
of non-root blocks in all three free space btrees).  Does this need a
change?

	int delta = (int)pag->pagf_btreeblks - (be32_to_cpu(...) - 1);
	if (delta > 0)
		atomic64_add(delta, &mp->m_allocbt_blks);

--D

> +			allocbt_blks = pag->pagf_btreeblks -
> +					be32_to_cpu(agf->agf_rmap_blocks);
> +			atomic64_add(allocbt_blks, &mp->m_allocbt_blks);
> +		}
>  	}
>  #ifdef DEBUG
>  	else if (!XFS_FORCED_SHUTDOWN(mp)) {
> diff --git a/fs/xfs/libxfs/xfs_alloc_btree.c b/fs/xfs/libxfs/xfs_alloc_btree.c
> index 8e01231b308e..9f5a45f7baed 100644
> --- a/fs/xfs/libxfs/xfs_alloc_btree.c
> +++ b/fs/xfs/libxfs/xfs_alloc_btree.c
> @@ -71,6 +71,7 @@ xfs_allocbt_alloc_block(
>  		return 0;
>  	}
>  
> +	atomic64_inc(&cur->bc_mp->m_allocbt_blks);
>  	xfs_extent_busy_reuse(cur->bc_mp, cur->bc_ag.agno, bno, 1, false);
>  
>  	xfs_trans_agbtree_delta(cur->bc_tp, 1);
> @@ -95,6 +96,7 @@ xfs_allocbt_free_block(
>  	if (error)
>  		return error;
>  
> +	atomic64_dec(&cur->bc_mp->m_allocbt_blks);
>  	xfs_extent_busy_insert(cur->bc_tp, be32_to_cpu(agf->agf_seqno), bno, 1,
>  			      XFS_EXTENT_BUSY_SKIP_DISCARD);
>  	xfs_trans_agbtree_delta(cur->bc_tp, -1);
> diff --git a/fs/xfs/xfs_mount.h b/fs/xfs/xfs_mount.h
> index 81829d19596e..bb67274ee23f 100644
> --- a/fs/xfs/xfs_mount.h
> +++ b/fs/xfs/xfs_mount.h
> @@ -170,6 +170,12 @@ typedef struct xfs_mount {
>  	 * extents or anything related to the rt device.
>  	 */
>  	struct percpu_counter	m_delalloc_blks;
> +	/*
> +	 * Global count of allocation btree blocks in use across all AGs. Only
> +	 * used when perag reservation is enabled. Helps prevent block
> +	 * reservation from attempting to reserve allocation btree blocks.
> +	 */
> +	atomic64_t		m_allocbt_blks;
>  
>  	struct radix_tree_root	m_perag_tree;	/* per-ag accounting info */
>  	spinlock_t		m_perag_lock;	/* lock for m_perag_tree */
> -- 
> 2.26.3
> 

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v4 2/3] xfs: introduce in-core global counter of allocbt blocks
  2021-04-28  4:15   ` Darrick J. Wong
@ 2021-04-28 15:01     ` Brian Foster
  2021-04-28 15:29       ` Brian Foster
  0 siblings, 1 reply; 18+ messages in thread
From: Brian Foster @ 2021-04-28 15:01 UTC (permalink / raw)
  To: Darrick J. Wong; +Cc: linux-xfs

On Tue, Apr 27, 2021 at 09:15:09PM -0700, Darrick J. Wong wrote:
> On Fri, Apr 23, 2021 at 09:10:49AM -0400, Brian Foster wrote:
> > Introduce an in-core counter to track the sum of all allocbt blocks
> > used by the filesystem. This value is currently tracked per-ag via
> > the ->agf_btreeblks field in the AGF, which also happens to include
> > rmapbt blocks. A global, in-core count of allocbt blocks is required
> > to identify the subset of global ->m_fdblocks that consists of
> > unavailable blocks currently used for allocation btrees. To support
> > this calculation at block reservation time, construct a similar
> > global counter for allocbt blocks, populate it on first read of each
> > AGF and update it as allocbt blocks are used and released.
> > 
> > Signed-off-by: Brian Foster <bfoster@redhat.com>
> > ---
> >  fs/xfs/libxfs/xfs_alloc.c       | 12 ++++++++++++
> >  fs/xfs/libxfs/xfs_alloc_btree.c |  2 ++
> >  fs/xfs/xfs_mount.h              |  6 ++++++
> >  3 files changed, 20 insertions(+)
> > 
> > diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
> > index aaa19101bb2a..144e2d68245c 100644
> > --- a/fs/xfs/libxfs/xfs_alloc.c
> > +++ b/fs/xfs/libxfs/xfs_alloc.c
> > @@ -3036,6 +3036,7 @@ xfs_alloc_read_agf(
> >  	struct xfs_agf		*agf;		/* ag freelist header */
> >  	struct xfs_perag	*pag;		/* per allocation group data */
> >  	int			error;
> > +	uint32_t		allocbt_blks;
> >  
> >  	trace_xfs_alloc_read_agf(mp, agno);
> >  
> > @@ -3066,6 +3067,17 @@ xfs_alloc_read_agf(
> >  		pag->pagf_refcount_level = be32_to_cpu(agf->agf_refcount_level);
> >  		pag->pagf_init = 1;
> >  		pag->pagf_agflreset = xfs_agfl_needs_reset(mp, agf);
> > +
> > +		/*
> > +		 * Update the global in-core allocbt block counter. Filter
> > +		 * rmapbt blocks from the on-disk counter because those are
> > +		 * managed by perag reservation.
> > +		 */
> > +		if (pag->pagf_btreeblks > be32_to_cpu(agf->agf_rmap_blocks)) {
> 
> As pointed out elsewhere in the thread, agf_rmap_blocks counts the total
> number of blocks in the rmapbt (whereas agf_btreeblks counts the number
> of non-root blocks in all three free space btrees).  Does this need a
> change?
> 
> 	int delta = (int)pag->pagf_btreeblks - (be32_to_cpu(...) - 1);
> 	if (delta > 0)
> 		atomic64_add(delta, &mp->m_allocbt_blks);
>

Hm yes, this makes more sense. Will fix and update the comment..

Brian
 
> --D
> 
> > +			allocbt_blks = pag->pagf_btreeblks -
> > +					be32_to_cpu(agf->agf_rmap_blocks);
> > +			atomic64_add(allocbt_blks, &mp->m_allocbt_blks);
> > +		}
> >  	}
> >  #ifdef DEBUG
> >  	else if (!XFS_FORCED_SHUTDOWN(mp)) {
> > diff --git a/fs/xfs/libxfs/xfs_alloc_btree.c b/fs/xfs/libxfs/xfs_alloc_btree.c
> > index 8e01231b308e..9f5a45f7baed 100644
> > --- a/fs/xfs/libxfs/xfs_alloc_btree.c
> > +++ b/fs/xfs/libxfs/xfs_alloc_btree.c
> > @@ -71,6 +71,7 @@ xfs_allocbt_alloc_block(
> >  		return 0;
> >  	}
> >  
> > +	atomic64_inc(&cur->bc_mp->m_allocbt_blks);
> >  	xfs_extent_busy_reuse(cur->bc_mp, cur->bc_ag.agno, bno, 1, false);
> >  
> >  	xfs_trans_agbtree_delta(cur->bc_tp, 1);
> > @@ -95,6 +96,7 @@ xfs_allocbt_free_block(
> >  	if (error)
> >  		return error;
> >  
> > +	atomic64_dec(&cur->bc_mp->m_allocbt_blks);
> >  	xfs_extent_busy_insert(cur->bc_tp, be32_to_cpu(agf->agf_seqno), bno, 1,
> >  			      XFS_EXTENT_BUSY_SKIP_DISCARD);
> >  	xfs_trans_agbtree_delta(cur->bc_tp, -1);
> > diff --git a/fs/xfs/xfs_mount.h b/fs/xfs/xfs_mount.h
> > index 81829d19596e..bb67274ee23f 100644
> > --- a/fs/xfs/xfs_mount.h
> > +++ b/fs/xfs/xfs_mount.h
> > @@ -170,6 +170,12 @@ typedef struct xfs_mount {
> >  	 * extents or anything related to the rt device.
> >  	 */
> >  	struct percpu_counter	m_delalloc_blks;
> > +	/*
> > +	 * Global count of allocation btree blocks in use across all AGs. Only
> > +	 * used when perag reservation is enabled. Helps prevent block
> > +	 * reservation from attempting to reserve allocation btree blocks.
> > +	 */
> > +	atomic64_t		m_allocbt_blks;
> >  
> >  	struct radix_tree_root	m_perag_tree;	/* per-ag accounting info */
> >  	spinlock_t		m_perag_lock;	/* lock for m_perag_tree */
> > -- 
> > 2.26.3
> > 
> 


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v4 2/3] xfs: introduce in-core global counter of allocbt blocks
  2021-04-28 15:01     ` Brian Foster
@ 2021-04-28 15:29       ` Brian Foster
  2021-04-28 16:12         ` Darrick J. Wong
  0 siblings, 1 reply; 18+ messages in thread
From: Brian Foster @ 2021-04-28 15:29 UTC (permalink / raw)
  To: Darrick J. Wong; +Cc: linux-xfs

On Wed, Apr 28, 2021 at 11:01:24AM -0400, Brian Foster wrote:
> On Tue, Apr 27, 2021 at 09:15:09PM -0700, Darrick J. Wong wrote:
> > On Fri, Apr 23, 2021 at 09:10:49AM -0400, Brian Foster wrote:
> > > Introduce an in-core counter to track the sum of all allocbt blocks
> > > used by the filesystem. This value is currently tracked per-ag via
> > > the ->agf_btreeblks field in the AGF, which also happens to include
> > > rmapbt blocks. A global, in-core count of allocbt blocks is required
> > > to identify the subset of global ->m_fdblocks that consists of
> > > unavailable blocks currently used for allocation btrees. To support
> > > this calculation at block reservation time, construct a similar
> > > global counter for allocbt blocks, populate it on first read of each
> > > AGF and update it as allocbt blocks are used and released.
> > > 
> > > Signed-off-by: Brian Foster <bfoster@redhat.com>
> > > ---
> > >  fs/xfs/libxfs/xfs_alloc.c       | 12 ++++++++++++
> > >  fs/xfs/libxfs/xfs_alloc_btree.c |  2 ++
> > >  fs/xfs/xfs_mount.h              |  6 ++++++
> > >  3 files changed, 20 insertions(+)
> > > 
> > > diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
> > > index aaa19101bb2a..144e2d68245c 100644
> > > --- a/fs/xfs/libxfs/xfs_alloc.c
> > > +++ b/fs/xfs/libxfs/xfs_alloc.c
> > > @@ -3036,6 +3036,7 @@ xfs_alloc_read_agf(
> > >  	struct xfs_agf		*agf;		/* ag freelist header */
> > >  	struct xfs_perag	*pag;		/* per allocation group data */
> > >  	int			error;
> > > +	uint32_t		allocbt_blks;
> > >  
> > >  	trace_xfs_alloc_read_agf(mp, agno);
> > >  
> > > @@ -3066,6 +3067,17 @@ xfs_alloc_read_agf(
> > >  		pag->pagf_refcount_level = be32_to_cpu(agf->agf_refcount_level);
> > >  		pag->pagf_init = 1;
> > >  		pag->pagf_agflreset = xfs_agfl_needs_reset(mp, agf);
> > > +
> > > +		/*
> > > +		 * Update the global in-core allocbt block counter. Filter
> > > +		 * rmapbt blocks from the on-disk counter because those are
> > > +		 * managed by perag reservation.
> > > +		 */
> > > +		if (pag->pagf_btreeblks > be32_to_cpu(agf->agf_rmap_blocks)) {
> > 
> > As pointed out elsewhere in the thread, agf_rmap_blocks counts the total
> > number of blocks in the rmapbt (whereas agf_btreeblks counts the number
> > of non-root blocks in all three free space btrees).  Does this need a
> > change?
> > 
> > 	int delta = (int)pag->pagf_btreeblks - (be32_to_cpu(...) - 1);
> > 	if (delta > 0)
> > 		atomic64_add(delta, &mp->m_allocbt_blks);
> >
> 
> Hm yes, this makes more sense. Will fix and update the comment..
> 

I ended up with the following:

		...
                /*
                 * Update the in-core allocbt counter. Filter out the rmapbt
                 * subset of the btreeblks counter because the rmapbt is managed
                 * by perag reservation. Subtract one for the rmapbt root block
                 * because the rmap counter includes it while the btreeblks
                 * counter only tracks non-root blocks.
                 */
                allocbt_blks = pag->pagf_btreeblks;
                if (xfs_sb_version_hasrmapbt(&mp->m_sb))
                        allocbt_blks -= be32_to_cpu(agf->agf_rmap_blocks) - 1;
                if (allocbt_blks > 0)
                        atomic64_add(allocbt_blks, &mp->m_allocbt_blks);
		...

Any thoughts before I post v5?

Brian

> Brian
>  
> > --D
> > 
> > > +			allocbt_blks = pag->pagf_btreeblks -
> > > +					be32_to_cpu(agf->agf_rmap_blocks);
> > > +			atomic64_add(allocbt_blks, &mp->m_allocbt_blks);
> > > +		}
> > >  	}
> > >  #ifdef DEBUG
> > >  	else if (!XFS_FORCED_SHUTDOWN(mp)) {
> > > diff --git a/fs/xfs/libxfs/xfs_alloc_btree.c b/fs/xfs/libxfs/xfs_alloc_btree.c
> > > index 8e01231b308e..9f5a45f7baed 100644
> > > --- a/fs/xfs/libxfs/xfs_alloc_btree.c
> > > +++ b/fs/xfs/libxfs/xfs_alloc_btree.c
> > > @@ -71,6 +71,7 @@ xfs_allocbt_alloc_block(
> > >  		return 0;
> > >  	}
> > >  
> > > +	atomic64_inc(&cur->bc_mp->m_allocbt_blks);
> > >  	xfs_extent_busy_reuse(cur->bc_mp, cur->bc_ag.agno, bno, 1, false);
> > >  
> > >  	xfs_trans_agbtree_delta(cur->bc_tp, 1);
> > > @@ -95,6 +96,7 @@ xfs_allocbt_free_block(
> > >  	if (error)
> > >  		return error;
> > >  
> > > +	atomic64_dec(&cur->bc_mp->m_allocbt_blks);
> > >  	xfs_extent_busy_insert(cur->bc_tp, be32_to_cpu(agf->agf_seqno), bno, 1,
> > >  			      XFS_EXTENT_BUSY_SKIP_DISCARD);
> > >  	xfs_trans_agbtree_delta(cur->bc_tp, -1);
> > > diff --git a/fs/xfs/xfs_mount.h b/fs/xfs/xfs_mount.h
> > > index 81829d19596e..bb67274ee23f 100644
> > > --- a/fs/xfs/xfs_mount.h
> > > +++ b/fs/xfs/xfs_mount.h
> > > @@ -170,6 +170,12 @@ typedef struct xfs_mount {
> > >  	 * extents or anything related to the rt device.
> > >  	 */
> > >  	struct percpu_counter	m_delalloc_blks;
> > > +	/*
> > > +	 * Global count of allocation btree blocks in use across all AGs. Only
> > > +	 * used when perag reservation is enabled. Helps prevent block
> > > +	 * reservation from attempting to reserve allocation btree blocks.
> > > +	 */
> > > +	atomic64_t		m_allocbt_blks;
> > >  
> > >  	struct radix_tree_root	m_perag_tree;	/* per-ag accounting info */
> > >  	spinlock_t		m_perag_lock;	/* lock for m_perag_tree */
> > > -- 
> > > 2.26.3
> > > 
> > 


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v4 2/3] xfs: introduce in-core global counter of allocbt blocks
  2021-04-28 15:29       ` Brian Foster
@ 2021-04-28 16:12         ` Darrick J. Wong
  0 siblings, 0 replies; 18+ messages in thread
From: Darrick J. Wong @ 2021-04-28 16:12 UTC (permalink / raw)
  To: Brian Foster; +Cc: linux-xfs

On Wed, Apr 28, 2021 at 11:29:57AM -0400, Brian Foster wrote:
> On Wed, Apr 28, 2021 at 11:01:24AM -0400, Brian Foster wrote:
> > On Tue, Apr 27, 2021 at 09:15:09PM -0700, Darrick J. Wong wrote:
> > > On Fri, Apr 23, 2021 at 09:10:49AM -0400, Brian Foster wrote:
> > > > Introduce an in-core counter to track the sum of all allocbt blocks
> > > > used by the filesystem. This value is currently tracked per-ag via
> > > > the ->agf_btreeblks field in the AGF, which also happens to include
> > > > rmapbt blocks. A global, in-core count of allocbt blocks is required
> > > > to identify the subset of global ->m_fdblocks that consists of
> > > > unavailable blocks currently used for allocation btrees. To support
> > > > this calculation at block reservation time, construct a similar
> > > > global counter for allocbt blocks, populate it on first read of each
> > > > AGF and update it as allocbt blocks are used and released.
> > > > 
> > > > Signed-off-by: Brian Foster <bfoster@redhat.com>
> > > > ---
> > > >  fs/xfs/libxfs/xfs_alloc.c       | 12 ++++++++++++
> > > >  fs/xfs/libxfs/xfs_alloc_btree.c |  2 ++
> > > >  fs/xfs/xfs_mount.h              |  6 ++++++
> > > >  3 files changed, 20 insertions(+)
> > > > 
> > > > diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
> > > > index aaa19101bb2a..144e2d68245c 100644
> > > > --- a/fs/xfs/libxfs/xfs_alloc.c
> > > > +++ b/fs/xfs/libxfs/xfs_alloc.c
> > > > @@ -3036,6 +3036,7 @@ xfs_alloc_read_agf(
> > > >  	struct xfs_agf		*agf;		/* ag freelist header */
> > > >  	struct xfs_perag	*pag;		/* per allocation group data */
> > > >  	int			error;
> > > > +	uint32_t		allocbt_blks;
> > > >  
> > > >  	trace_xfs_alloc_read_agf(mp, agno);
> > > >  
> > > > @@ -3066,6 +3067,17 @@ xfs_alloc_read_agf(
> > > >  		pag->pagf_refcount_level = be32_to_cpu(agf->agf_refcount_level);
> > > >  		pag->pagf_init = 1;
> > > >  		pag->pagf_agflreset = xfs_agfl_needs_reset(mp, agf);
> > > > +
> > > > +		/*
> > > > +		 * Update the global in-core allocbt block counter. Filter
> > > > +		 * rmapbt blocks from the on-disk counter because those are
> > > > +		 * managed by perag reservation.
> > > > +		 */
> > > > +		if (pag->pagf_btreeblks > be32_to_cpu(agf->agf_rmap_blocks)) {
> > > 
> > > As pointed out elsewhere in the thread, agf_rmap_blocks counts the total
> > > number of blocks in the rmapbt (whereas agf_btreeblks counts the number
> > > of non-root blocks in all three free space btrees).  Does this need a
> > > change?
> > > 
> > > 	int delta = (int)pag->pagf_btreeblks - (be32_to_cpu(...) - 1);
> > > 	if (delta > 0)
> > > 		atomic64_add(delta, &mp->m_allocbt_blks);
> > >
> > 
> > Hm yes, this makes more sense. Will fix and update the comment..
> > 
> 
> I ended up with the following:
> 
> 		...
>                 /*
>                  * Update the in-core allocbt counter. Filter out the rmapbt
>                  * subset of the btreeblks counter because the rmapbt is managed
>                  * by perag reservation. Subtract one for the rmapbt root block
>                  * because the rmap counter includes it while the btreeblks
>                  * counter only tracks non-root blocks.
>                  */
>                 allocbt_blks = pag->pagf_btreeblks;
>                 if (xfs_sb_version_hasrmapbt(&mp->m_sb))
>                         allocbt_blks -= be32_to_cpu(agf->agf_rmap_blocks) - 1;
>                 if (allocbt_blks > 0)
>                         atomic64_add(allocbt_blks, &mp->m_allocbt_blks);
> 		...
> 
> Any thoughts before I post v5?

Looks reasonable to me. :)

--D

> Brian
> 
> > Brian
> >  
> > > --D
> > > 
> > > > +			allocbt_blks = pag->pagf_btreeblks -
> > > > +					be32_to_cpu(agf->agf_rmap_blocks);
> > > > +			atomic64_add(allocbt_blks, &mp->m_allocbt_blks);
> > > > +		}
> > > >  	}
> > > >  #ifdef DEBUG
> > > >  	else if (!XFS_FORCED_SHUTDOWN(mp)) {
> > > > diff --git a/fs/xfs/libxfs/xfs_alloc_btree.c b/fs/xfs/libxfs/xfs_alloc_btree.c
> > > > index 8e01231b308e..9f5a45f7baed 100644
> > > > --- a/fs/xfs/libxfs/xfs_alloc_btree.c
> > > > +++ b/fs/xfs/libxfs/xfs_alloc_btree.c
> > > > @@ -71,6 +71,7 @@ xfs_allocbt_alloc_block(
> > > >  		return 0;
> > > >  	}
> > > >  
> > > > +	atomic64_inc(&cur->bc_mp->m_allocbt_blks);
> > > >  	xfs_extent_busy_reuse(cur->bc_mp, cur->bc_ag.agno, bno, 1, false);
> > > >  
> > > >  	xfs_trans_agbtree_delta(cur->bc_tp, 1);
> > > > @@ -95,6 +96,7 @@ xfs_allocbt_free_block(
> > > >  	if (error)
> > > >  		return error;
> > > >  
> > > > +	atomic64_dec(&cur->bc_mp->m_allocbt_blks);
> > > >  	xfs_extent_busy_insert(cur->bc_tp, be32_to_cpu(agf->agf_seqno), bno, 1,
> > > >  			      XFS_EXTENT_BUSY_SKIP_DISCARD);
> > > >  	xfs_trans_agbtree_delta(cur->bc_tp, -1);
> > > > diff --git a/fs/xfs/xfs_mount.h b/fs/xfs/xfs_mount.h
> > > > index 81829d19596e..bb67274ee23f 100644
> > > > --- a/fs/xfs/xfs_mount.h
> > > > +++ b/fs/xfs/xfs_mount.h
> > > > @@ -170,6 +170,12 @@ typedef struct xfs_mount {
> > > >  	 * extents or anything related to the rt device.
> > > >  	 */
> > > >  	struct percpu_counter	m_delalloc_blks;
> > > > +	/*
> > > > +	 * Global count of allocation btree blocks in use across all AGs. Only
> > > > +	 * used when perag reservation is enabled. Helps prevent block
> > > > +	 * reservation from attempting to reserve allocation btree blocks.
> > > > +	 */
> > > > +	atomic64_t		m_allocbt_blks;
> > > >  
> > > >  	struct radix_tree_root	m_perag_tree;	/* per-ag accounting info */
> > > >  	spinlock_t		m_perag_lock;	/* lock for m_perag_tree */
> > > > -- 
> > > > 2.26.3
> > > > 
> > > 
> 

^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2021-04-28 16:12 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-23 13:10 [PATCH v4 0/3] xfs: set aside allocation btree blocks from block reservation Brian Foster
2021-04-23 13:10 ` [PATCH v4 1/3] xfs: unconditionally read all AGFs on mounts with perag reservation Brian Foster
2021-04-27 10:22   ` Chandan Babu R
2021-04-27 21:36   ` Allison Henderson
2021-04-28  4:12   ` Darrick J. Wong
2021-04-23 13:10 ` [PATCH v4 2/3] xfs: introduce in-core global counter of allocbt blocks Brian Foster
2021-04-27 10:28   ` Chandan Babu R
2021-04-27 11:33     ` Brian Foster
2021-04-27 13:22       ` Chandan Babu R
2021-04-27 21:37   ` Allison Henderson
2021-04-28  4:15   ` Darrick J. Wong
2021-04-28 15:01     ` Brian Foster
2021-04-28 15:29       ` Brian Foster
2021-04-28 16:12         ` Darrick J. Wong
2021-04-23 13:10 ` [PATCH v4 3/3] xfs: set aside allocation btree blocks from block reservation Brian Foster
2021-04-27 10:29   ` Chandan Babu R
2021-04-27 21:37   ` Allison Henderson
2021-04-28  4:12   ` Darrick J. Wong

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.