Re: [PATCH 3/6] xfs: don't include bnobt blocks when reserving free block pool

[Brian Foster <bfoster@redhat.com>]

On Thu, Mar 17, 2022 at 02:21:12PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> xfs_reserve_blocks controls the size of the user-visible free space
> reserve pool.  Given the difference between the current and requested
> pool sizes, it will try to reserve free space from fdblocks.  However,
> the amount requested from fdblocks is also constrained by the amount of
> space that we think xfs_mod_fdblocks will give us.  We'll keep trying to
> reserve space so long as xfs_mod_fdblocks returns ENOSPC.
> 
> In commit fd43cf600cf6, we decided that xfs_mod_fdblocks should not hand
> out the "free space" used by the free space btrees, because some portion
> of the free space btrees hold in reserve space for future btree
> expansion.  Unfortunately, xfs_reserve_blocks' estimation of the number
> of blocks that it could request from xfs_mod_fdblocks was not updated to
> include m_allocbt_blks, so if space is extremely low, the caller hangs.
> 
> Fix this by creating a function to estimate the number of blocks that
> can be reserved from fdblocks, which needs to exclude the set-aside and
> m_allocbt_blks.
> 
> Found by running xfs/306 (which formats a single-AG 20MB filesystem)
> with an fstests configuration that specifies a 1k blocksize and a
> specially crafted log size that will consume 7/8 of the space (17920
> blocks, specifically) in that AG.
> 
> Cc: Brian Foster <bfoster@redhat.com>
> Fixes: fd43cf600cf6 ("xfs: set aside allocation btree blocks from block reservation")
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  fs/xfs/xfs_fsops.c |    7 +++++--
>  fs/xfs/xfs_mount.h |   29 +++++++++++++++++++++++++++++
>  2 files changed, 34 insertions(+), 2 deletions(-)
> 
> 
> diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c
> index 33e26690a8c4..b71799a3acd3 100644
> --- a/fs/xfs/xfs_fsops.c
> +++ b/fs/xfs/xfs_fsops.c
> @@ -433,8 +433,11 @@ xfs_reserve_blocks(
>  	 */
>  	error = -ENOSPC;
>  	do {
> -		free = percpu_counter_sum(&mp->m_fdblocks) -
> -						mp->m_alloc_set_aside;
> +		/*
> +		 * The reservation pool cannot take space that xfs_mod_fdblocks
> +		 * will not give us.
> +		 */

This comment seems unnecessary. I'm not sure what this is telling that
the code doesn't already..?

> +		free = xfs_fdblocks_available(mp);
>  		if (free <= 0)
>  			break;
>  
> diff --git a/fs/xfs/xfs_mount.h b/fs/xfs/xfs_mount.h
> index 00720a02e761..998b54c3c454 100644
> --- a/fs/xfs/xfs_mount.h
> +++ b/fs/xfs/xfs_mount.h
> @@ -479,6 +479,35 @@ extern void	xfs_unmountfs(xfs_mount_t *);
>   */
>  #define XFS_FDBLOCKS_BATCH	1024
>  
> +/*
> + * Estimate the amount of space that xfs_mod_fdblocks might give us without
> + * drawing from the reservation pool.  In other words, estimate the free space
> + * that is available to userspace.
> + *
> + * This quantity is the amount of free space tracked in the on-disk metadata
> + * minus:
> + *
> + * - Delayed allocation reservations
> + * - Per-AG space reservations to guarantee metadata expansion
> + * - Userspace-controlled free space reserve pool
> + *
> + * - Space reserved to ensure that we can always split a bmap btree
> + * - Free space btree blocks that are not available for allocation due to
> + *   per-AG metadata reservations
> + *
> + * The first three are captured in the incore fdblocks counter.
> + */

Hm. Sometimes I wonder if we overdocument things to our own detriment
(reading back my own comments at times suggests I'm terrible at this).
So do we really need to document what other internal reservations are or
are not taken out of ->m_fdblocks here..? I suspect we already have
plenty of sufficient documentation for things like perag res colocated
with the actual code, such that this kind of thing just creates an
external reference that will probably just bitrot as years go by. Can we
reduce this down to just explain how/why this helper has to calculate a
block availability value for blocks that otherwise haven't been
explicitly allocated out of the in-core free block counters?

> +static inline int64_t
> +xfs_fdblocks_available(
> +	struct xfs_mount	*mp)
> +{
> +	int64_t			free = percpu_counter_sum(&mp->m_fdblocks);
> +
> +	free -= mp->m_alloc_set_aside;
> +	free -= atomic64_read(&mp->m_allocbt_blks);
> +	return free;
> +}
> +

FWIW the helper seems fine in context, but will this help us avoid the
duplicate calculation in xfs_mod_fdblocks(), for instance?

Brian

>  extern int	xfs_mod_fdblocks(struct xfs_mount *mp, int64_t delta,
>  				 bool reserved);
>  extern int	xfs_mod_frextents(struct xfs_mount *mp, int64_t delta);
>