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

["Darrick J. Wong" <djwong@kernel.org>]

On Fri, Mar 18, 2022 at 08:19:48AM -0400, Brian Foster wrote:
> 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).

In the end, I deleted xfs_fdblocks_avail* so the only new function is
the xfs_fdblocks_unavailable() that I mentioned in the last reply.

It does make the patches smaller...

--D

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