Re: [RFC PATCH] xfs: transfer freed blocks to blk res when lazy accounting

[Brian Foster <bfoster@redhat.com>]

On Fri, May 22, 2020 at 06:36:14PM -0700, Darrick J. Wong wrote:
> On Fri, May 22, 2020 at 01:18:28PM -0400, Brian Foster wrote:
> > Signed-off-by: Brian Foster <bfoster@redhat.com>
> > ---
> > 
> > Darrick mentioned on IRC a few days ago that he'd seen an issue that
> > looked similar to the problem with the rmapbt based extent swap
> > algorithm when the associated inodes happen to bounce between extent and
> > btree format. That problem caused repeated bmapbt block allocations and
> > frees that exhausted the transaction block reservation across the
> > sequence of transaction rolls. The workaround for that was to use an
> > oversized block reservation, but that is not a generic or efficient
> > solution.
> > 
> > I was originally playing around with some hacks to set an optional base
> > block reservation on the transaction that we would attempt to replenish
> > across transaction roll sequences as the block reservation depletes, but
> > eventually noticed that there isn't much difference between stuffing
> > block frees in the transaction reservation counter vs. the delta counter
> > when lazy sb accounting is enabled (which is required for v5 supers). As
> > such, the following patch seems to address the rmapbt issue in my
> > isolated tests.
> > 
> > I think one tradeoff with this logic is that chains of rolling/freeing
> > transactions would now aggregate freed space until the final transaction
> > commits vs. as transactions roll. It's not immediately clear to me how
> > much of an issue that is, but it sounds a bit dicey when considering
> > things like truncates of large files. This behavior could still be tied
> > to a transaction flag to restrict its use to situations like rmapbt
> > swapext, however. Anyways, this is mostly untested outside of the extent
> > swap use case so I wanted to throw this on the list as an RFC for now
> > and see if anybody has thoughts or other ideas.
> 
> Hmm, well, this /would/ fix the immediate problem of running out of
> block reservation, but I wonder if there are other weird subtleties.
> If we're nearly out of space and we're mounted with -odiscard and the
> disk is really slow at processing discard, can we encounter weird
> failure cases where we end up stuck waiting for the extent busy tree to
> say that one of our pingponged blocks is ok to use again?
> 

Yeah, I think something like that could happen. I don't think it should
be a failure scenario though as the busy extent list should involve a
log force and retry in the worst case. Either way, we could always
mitigate risk by making this an optional accounting mode for particular
(extent swap) transactions...

> In the meantime, I noticed that xfs/227 on a pmem fs (or possibly
> anything with synchronous writes?) and reflink+rmap enabled seemed to
> fail pretty consistently.  In a hastily done and incomprehensi{ve,ble}
> survey I noted that I couldn't make the disastrous pingpong happen if
> there were more than ~4 blocks in the bmapbt, so maybe this would help
> there.
> 

Do you mean with this patch or with current upstream? I don't see
xfs/227 failures on my current setups (this patch passed a weekend auto
test run), but I'll have to retry with something synchronous...

BTW, is xfs/227 related to the problem you had mentioned on IRC? I
wasn't quite sure what operation was involved with whatever error report
you had. xfs/227 looks like an xfs_fsr test, so I'd have thought the
upstream workaround would have addressed that.. (though I see some attr
ops in there as well so perhaps this is related to the attr fork..?).

Brian

> In unrelated news, I also tried fixing the log recovery defer ops chain
> transactions to absorb the unused block reservations that the
> xfs_*i_item_recover functions created, but that just made fdblocks be
> wrong.  But it didn't otherwise blow up! :P
> 
> --D
> 
> > Brian
> > 
> >  fs/xfs/xfs_bmap_util.c | 11 -----------
> >  fs/xfs/xfs_trans.c     |  4 ++++
> >  2 files changed, 4 insertions(+), 11 deletions(-)
> > 
> > diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
> > index f37f5cc4b19f..74b3bad6c414 100644
> > --- a/fs/xfs/xfs_bmap_util.c
> > +++ b/fs/xfs/xfs_bmap_util.c
> > @@ -1628,17 +1628,6 @@ xfs_swap_extents(
> >  		 */
> >  		resblks = XFS_SWAP_RMAP_SPACE_RES(mp, ipnext, w);
> >  		resblks +=  XFS_SWAP_RMAP_SPACE_RES(mp, tipnext, w);
> > -
> > -		/*
> > -		 * Handle the corner case where either inode might straddle the
> > -		 * btree format boundary. If so, the inode could bounce between
> > -		 * btree <-> extent format on unmap -> remap cycles, freeing and
> > -		 * allocating a bmapbt block each time.
> > -		 */
> > -		if (ipnext == (XFS_IFORK_MAXEXT(ip, w) + 1))
> > -			resblks += XFS_IFORK_MAXEXT(ip, w);
> > -		if (tipnext == (XFS_IFORK_MAXEXT(tip, w) + 1))
> > -			resblks += XFS_IFORK_MAXEXT(tip, w);
> >  	}
> >  	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_write, resblks, 0, 0, &tp);
> >  	if (error)
> > diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
> > index 28b983ff8b11..b421d27445c1 100644
> > --- a/fs/xfs/xfs_trans.c
> > +++ b/fs/xfs/xfs_trans.c
> > @@ -370,6 +370,10 @@ xfs_trans_mod_sb(
> >  			tp->t_blk_res_used += (uint)-delta;
> >  			if (tp->t_blk_res_used > tp->t_blk_res)
> >  				xfs_force_shutdown(mp, SHUTDOWN_CORRUPT_INCORE);
> > +		} else if (delta > 0 &&
> > +			   xfs_sb_version_haslazysbcount(&mp->m_sb)) {
> > +			tp->t_blk_res += delta;
> > +			delta = 0;
> >  		}
> >  		tp->t_fdblocks_delta += delta;
> >  		if (xfs_sb_version_haslazysbcount(&mp->m_sb))
> > -- 
> > 2.21.1
> > 
>