Re: [PATCH 5/6] xfs: don't report reserved bnobt space as available

[Brian Foster <bfoster@redhat.com>]

On Thu, Mar 17, 2022 at 02:21:23PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> On a modern filesystem, we don't allow userspace to allocate blocks for
> data storage from the per-AG space reservations, the user-controlled
> reservation pool that prevents ENOSPC in the middle of internal
> operations, or the internal per-AG set-aside that prevents ENOSPC.
> Since we now consider free space btree blocks as unavailable for
> allocation for data storage, we shouldn't report those blocks via statfs
> either.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  fs/xfs/xfs_fsops.c |    3 +--
>  fs/xfs/xfs_mount.h |   13 +++++++++++++
>  fs/xfs/xfs_super.c |    4 +---
>  3 files changed, 15 insertions(+), 5 deletions(-)
> 
> 
...
> diff --git a/fs/xfs/xfs_mount.h b/fs/xfs/xfs_mount.h
> index 998b54c3c454..74e9b8558162 100644
> --- a/fs/xfs/xfs_mount.h
> +++ b/fs/xfs/xfs_mount.h
> @@ -508,6 +508,19 @@ xfs_fdblocks_available(
>  	return free;
>  }
>  
> +/* Same as above, but don't take the slow path. */
> +static inline int64_t
> +xfs_fdblocks_available_fast(
> +	struct xfs_mount	*mp)
> +{
> +	int64_t			free;
> +
> +	free = percpu_counter_read_positive(&mp->m_fdblocks);
> +	free -= mp->m_alloc_set_aside;
> +	free -= atomic64_read(&mp->m_allocbt_blks);
> +	return free;
> +}
> +

No objection to the behavior change, but the point of the helper should
be to simplify things and reduce duplication. Here it seems we're going
to continue duplicating the set aside calculation, just in separate
helpers because different contexts apparently have different ways of
reading the free space counters (?).

If that's the case and we want an _available() helper, can we create a
single helper that takes the fdblocks count as a parameter and returns
the final "available" value so the helper can be used more broadly and
consistently? (Or factor out the common bits into an internal helper and
turn these two into simple parameter passing wrappers if you really want
to keep the api as such).

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);
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index d84714e4e46a..7b6c147e63c4 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -791,7 +791,6 @@ xfs_fs_statfs(
>  	uint64_t		fakeinos, id;
>  	uint64_t		icount;
>  	uint64_t		ifree;
> -	uint64_t		fdblocks;
>  	xfs_extlen_t		lsize;
>  	int64_t			ffree;
>  
> @@ -806,7 +805,6 @@ xfs_fs_statfs(
>  
>  	icount = percpu_counter_sum(&mp->m_icount);
>  	ifree = percpu_counter_sum(&mp->m_ifree);
> -	fdblocks = percpu_counter_sum(&mp->m_fdblocks);
>  
>  	spin_lock(&mp->m_sb_lock);
>  	statp->f_bsize = sbp->sb_blocksize;
> @@ -815,7 +813,7 @@ xfs_fs_statfs(
>  	spin_unlock(&mp->m_sb_lock);
>  
>  	/* make sure statp->f_bfree does not underflow */
> -	statp->f_bfree = max_t(int64_t, fdblocks - mp->m_alloc_set_aside, 0);
> +	statp->f_bfree = max_t(int64_t, xfs_fdblocks_available(mp), 0);
>  	statp->f_bavail = statp->f_bfree;
>  
>  	fakeinos = XFS_FSB_TO_INO(mp, statp->f_bfree);
>