On Fri, Mar 18, 2022 at 08:18:37AM -0400, Brian Foster wrote:
> On Thu, Mar 17, 2022 at 02:21:12PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
> > Fixes: fd43cf600cf6 ("xfs: set aside allocation btree blocks from block reservation")
> > Signed-off-by: Darrick J. Wong <djwong@xxxxxxxxxx>
> > ---
> > 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..?
Yeah, I'll get rid of it.
> > + 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?
Hmm. I suppose I could reduce the comment at the same time that I split
out the code that computes the amount of free space that isn't
available.
> > +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?
It will once I turn that into:
/*
* Estimate the amount of free space that is not available to userspace
* and is not explicitly reserved from the incore fdblocks:
*
* - 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
*/
static inline uint64_t
xfs_fdblocks_unavailable(
struct xfs_mount *mp)
{
return mp->m_alloc_set_aside + atomic64_read(&mp->m_allocbt_blks);
}
/*
* Estimate the amount of space that xfs_mod_fdblocks might give us
* without drawing from any reservation pool. In other words, estimate
* the free space that is available to userspace.
*/
static inline int64_t
xfs_fdblocks_available(
struct xfs_mount *mp)
{
return percpu_counter_sum(&mp->m_fdblocks) -
xfs_fdblocks_unavailable(mp);
}
--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);
> >
>
