Re: [PATCH 4/6] xfs: fix infinite loop when reserving free block pool

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

On Thu, Mar 24, 2022 at 08:11:47AM +1100, Dave Chinner wrote:
> On Sun, Mar 20, 2022 at 09:43:49AM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > Don't spin in an infinite loop trying to reserve blocks -- if we can't
> > do it after 30 tries, we're racing with a nearly full filesystem, so
> > just give up.
> > 
> > Cc: Brian Foster <bfoster@redhat.com>
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > Reviewed-by: Brian Foster <bfoster@redhat.com>
> > ---
> >  fs/xfs/xfs_fsops.c |   12 ++++++++++--
> >  1 file changed, 10 insertions(+), 2 deletions(-)
> > 
> > 
> > diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c
> > index 710e857bb825..615334e4f689 100644
> > --- a/fs/xfs/xfs_fsops.c
> > +++ b/fs/xfs/xfs_fsops.c
> > @@ -379,6 +379,7 @@ xfs_reserve_blocks(
> >  	int64_t			fdblks_delta = 0;
> >  	uint64_t		request;
> >  	int64_t			free;
> > +	unsigned int		tries;
> >  	int			error = 0;
> >  
> >  	/* If inval is null, report current values and return */
> > @@ -430,9 +431,16 @@ xfs_reserve_blocks(
> >  	 * If the request is larger than the current reservation, reserve the
> >  	 * blocks before we update the reserve counters. Sample m_fdblocks and
> >  	 * perform a partial reservation if the request exceeds free space.
> > +	 *
> > +	 * The loop body estimates how many blocks it can request from fdblocks
> > +	 * to stash in the reserve pool.  This is a classic TOCTOU race since
> > +	 * fdblocks updates are not always coordinated via m_sb_lock.  We also
> > +	 * cannot tell if @free remaining unchanged between iterations is due
> > +	 * to an idle system or freed blocks being consumed immediately, so
> > +	 * we'll try a finite number of times to satisfy the request.
> >  	 */
> >  	error = -ENOSPC;
> > -	do {
> > +	for (tries = 0; tries < 30 && error == -ENOSPC; tries++) {
> >  		free = percpu_counter_sum(&mp->m_fdblocks) -
> >  						xfs_fdblocks_unavailable(mp);
> >  		if (free <= 0)
> > @@ -459,7 +467,7 @@ xfs_reserve_blocks(
> >  		spin_unlock(&mp->m_sb_lock);
> >  		error = xfs_mod_fdblocks(mp, -fdblks_delta, 0);
> >  		spin_lock(&mp->m_sb_lock);
> > -	} while (error == -ENOSPC);
> > +	}
> 
> So the problem here is that if we don't get all of the reservation,
> we get none of it, so we try again. This seems like we should simply
> punch the error back out to userspace and let it retry rather than
> try a few times and then fail anyway. Either way, userspace has to
> handle failure to reserve enough blocks.
> 
> The other alternative here is that we just change the reserve block
> limit when ENOSPC occurs and allow the xfs_mod_fdblocks() refill
> code just fill up the pool as space is freed. That would greatly
> simplify the code - we do a single attempt to reserve the new space,
> and if it fails we don't care because the reserve space gets
> refilled before free space is made available again.  I think that's
> preferable to having an arbitrary number of retries like this that's
> going to end up failing anyway.

How about I try the following tomorrow?

spin_lock(...);
mp->m_resblks = request;
free = percpu_counter_sum(&mp->m_fdblocks) - xfs_fdblocks_unavailable(mp);
if (request < mp->m_resblks_avail && free > 0) {
	ask = min(request - mp->m_resblks_avail, free);

	spin_unlock(...);
	error = xfs_mod_fdblocks(mp, -ask, 0);
	spin_lock(...);

	if (!error)
		mp->m_resblks_avail += ask;
}
spin_unlock(...);
return error;

--D

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com