Re: [PATCH 5/6] xfs: flush speculative space allocations when we run out of quota

From: "Darrick J. Wong" <djwong@kernel.org>
To: Dave Chinner <david@fromorbit.com>
Cc: linux-xfs@vger.kernel.org
Subject: Re: [PATCH 5/6] xfs: flush speculative space allocations when we run out of quota
Date: Mon, 11 Jan 2021 17:31:26 -0800	[thread overview]
Message-ID: <20210112013126.GJ1164246@magnolia> (raw)
In-Reply-To: <20210112012249.GP331610@dread.disaster.area>

On Tue, Jan 12, 2021 at 12:22:49PM +1100, Dave Chinner wrote:
> On Mon, Jan 11, 2021 at 03:23:05PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > If a fs modification (creation, file write, reflink, etc.) is unable to
> > reserve enough quota to handle the modification, try clearing whatever
> > space the filesystem might have been hanging onto in the hopes of
> > speeding up the filesystem.  The flushing behavior will become
> > particularly important when we add deferred inode inactivation because
> > that will increase the amount of space that isn't actively tied to user
> > data.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> >  fs/xfs/xfs_bmap_util.c |   16 ++++++++++++++++
> >  fs/xfs/xfs_file.c      |    2 +-
> >  fs/xfs/xfs_icache.c    |    9 +++++++--
> >  fs/xfs/xfs_icache.h    |    2 +-
> >  fs/xfs/xfs_inode.c     |   17 +++++++++++++++++
> >  fs/xfs/xfs_ioctl.c     |    2 ++
> >  fs/xfs/xfs_iomap.c     |   20 +++++++++++++++++++-
> >  fs/xfs/xfs_reflink.c   |   40 +++++++++++++++++++++++++++++++++++++---
> >  fs/xfs/xfs_trace.c     |    1 +
> >  fs/xfs/xfs_trace.h     |   40 ++++++++++++++++++++++++++++++++++++++++
> >  10 files changed, 141 insertions(+), 8 deletions(-)
> > 
> > 
> > diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
> > index 7371a7f7c652..437fdc8a8fbd 100644
> > --- a/fs/xfs/xfs_bmap_util.c
> > +++ b/fs/xfs/xfs_bmap_util.c
> > @@ -761,6 +761,7 @@ xfs_alloc_file_space(
> >  	 */
> >  	while (allocatesize_fsb && !error) {
> >  		xfs_fileoff_t	s, e;
> > +		bool		cleared_space = false;
> >  
> >  		/*
> >  		 * Determine space reservations for data/realtime.
> > @@ -803,6 +804,7 @@ xfs_alloc_file_space(
> >  		/*
> >  		 * Allocate and setup the transaction.
> >  		 */
> > +retry:
> >  		error = xfs_trans_alloc(mp, &M_RES(mp)->tr_write, resblks,
> >  				resrtextents, 0, &tp);
> >  
> > @@ -819,6 +821,20 @@ xfs_alloc_file_space(
> >  		xfs_ilock(ip, XFS_ILOCK_EXCL);
> >  		error = xfs_trans_reserve_quota_nblks(tp, ip, qblocks,
> >  						      0, quota_flag);
> > +		/*
> > +		 * We weren't able to reserve enough quota to handle fallocate.
> > +		 * Flush any disk space that was being held in the hopes of
> > +		 * speeding up the filesystem.  We hold the IOLOCK so we cannot
> > +		 * do a synchronous scan.
> > +		 */
> > +		if ((error == -ENOSPC || error == -EDQUOT) && !cleared_space) {
> > +			xfs_trans_cancel(tp);
> > +			xfs_iunlock(ip, XFS_ILOCK_EXCL);
> > +			cleared_space = xfs_inode_free_quota_blocks(ip, false);
> > +			if (cleared_space)
> > +				goto retry;
> > +			return error;
> 
> Can't say I'm a fan of repeating this everywhere.  Can we move this
> into xfs_trans_reserve_quota_nblks() with a "retry" flag such that
> we do:
> 
> 	error = xfs_trans_reserve_quota_nblks(tp, ip, qblocks, 0,
> 					quota_flag, &retry);
> 	if (error) {
> 		/* tp already cancelled, inode unlocked */
> 		return error;
> 	}
> 	if (retry) {
> 		/* tp already cancelled, inode unlocked */
> 		goto retry;
> 	}

Assuming you'd be interested in the same kind of change being applied to
the next patch (kick the inode reclaim from xfs_trans_reserve on ENOSPC)
then yes, that seems like a nice cleanup.

--D

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