Re: [PATCH] xfs: preserve rmapbt swapext block reservation from freed blocks

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Tue, Jun 02, 2020 at 03:33:06PM -0400, Brian Foster wrote:
> On Tue, Jun 02, 2020 at 11:29:10AM -0700, Darrick J. Wong wrote:
> > On Tue, Jun 02, 2020 at 02:02:06PM -0400, Brian Foster wrote:
> > > The rmapbt extent swap algorithm remaps individual extents between
> > > the source inode and the target to trigger reverse mapping metadata
> > > updates. If either inode straddles a format or other bmap allocation
> > > boundary, the individual unmap and map cycles can trigger repeated
> > > bmap block allocations and frees as the extent count bounces back
> > > and forth across the boundary. While net block usage is bound across
> > > the swap operation, this behavior can prematurely exhaust the
> > > transaction block reservation because it continuously drains as the
> > > transaction rolls. Each allocation accounts against the reservation
> > > and each free returns to global free space on transaction roll.
> > > 
> > > The previous workaround to this problem attempted to detect this
> > > boundary condition and provide surplus block reservation to
> > > acommodate it. This is insufficient because more remaps can occur
> > > than implied by the extent counts; if start offset boundaries are
> > > not aligned between the two inodes, for example.
> > > 
> > > To address this problem more generically and dynamically, add a
> > > transaction accounting mode that returns freed blocks to the
> > > transaction reservation instead of the superblock counters on
> > > transaction roll and use it when the rmapbt based algorithm is
> > > active. This allows the chain of remap transactions to preserve the
> > > block reservation based own its own frees and prevent premature
> > > exhaustion regardless of the remap pattern. Note that this is only
> > > safe for superblocks with lazy sb accounting, but the latter is
> > > required for v5 supers and the rmap feature depends on v5.
> > > 
> > > Fixes: b3fed434822d0 ("xfs: account format bouncing into rmapbt swapext tx reservation")
> > > Root-caused-by: Darrick J. Wong <darrick.wong@oracle.com>
> > > Signed-off-by: Brian Foster <bfoster@redhat.com>
> > > ---
> > > 
> > > v1:
> > > - Use a transaction flag to isolate behavior to rmapbt swapext.
> > > rfc: https://lore.kernel.org/linux-xfs/20200522171828.53440-1-bfoster@redhat.com/
> > > 
> > >  fs/xfs/libxfs/xfs_shared.h |  1 +
> > >  fs/xfs/xfs_bmap_util.c     | 18 +++++++++---------
> > >  fs/xfs/xfs_trans.c         | 13 ++++++++++++-
> > >  3 files changed, 22 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/fs/xfs/libxfs/xfs_shared.h b/fs/xfs/libxfs/xfs_shared.h
> > > index c45acbd3add9..708feb8eac76 100644
> > > --- a/fs/xfs/libxfs/xfs_shared.h
> > > +++ b/fs/xfs/libxfs/xfs_shared.h
> > > @@ -65,6 +65,7 @@ void	xfs_log_get_max_trans_res(struct xfs_mount *mp,
> > >  #define XFS_TRANS_DQ_DIRTY	0x10	/* at least one dquot in trx dirty */
> > >  #define XFS_TRANS_RESERVE	0x20    /* OK to use reserved data blocks */
> > >  #define XFS_TRANS_NO_WRITECOUNT 0x40	/* do not elevate SB writecount */
> > > +#define XFS_TRANS_RES_FDBLKS	0x80	/* reserve newly freed blocks */
> > >  /*
> > >   * LOWMODE is used by the allocator to activate the lowspace algorithm - when
> > >   * free space is running low the extent allocator may choose to allocate an
> > > diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
> > > index f37f5cc4b19f..afdc7f8e0e70 100644
> > > --- a/fs/xfs/xfs_bmap_util.c
> > > +++ b/fs/xfs/xfs_bmap_util.c
> > > @@ -1567,6 +1567,7 @@ xfs_swap_extents(
> > >  	int			lock_flags;
> > >  	uint64_t		f;
> > >  	int			resblks = 0;
> > > +	unsigned int		flags = 0;
> > >  
> > >  	/*
> > >  	 * Lock the inodes against other IO, page faults and truncate to
> > > @@ -1630,17 +1631,16 @@ xfs_swap_extents(
> > >  		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 either inode straddles a bmapbt block allocation boundary,
> > > +		 * the rmapbt algorithm triggers repeated allocs and frees as
> > > +		 * extents are remapped. This can exhaust the block reservation
> > > +		 * prematurely and cause shutdown. Return freed blocks to the
> > > +		 * transaction reservation to counter this behavior.
> > >  		 */
> > > -		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);
> > > +		flags |= XFS_TRANS_RES_FDBLKS;
> > >  	}
> > > -	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_write, resblks, 0, 0, &tp);
> > > +	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_write, resblks, 0, flags,
> > > +				&tp);
> > >  	if (error)
> > >  		goto out_unlock;
> > >  
> > > diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
> > > index 3c94e5ff4316..2040f2df58b5 100644
> > > --- a/fs/xfs/xfs_trans.c
> > > +++ b/fs/xfs/xfs_trans.c
> > > @@ -107,7 +107,8 @@ xfs_trans_dup(
> > >  
> > >  	ntp->t_flags = XFS_TRANS_PERM_LOG_RES |
> > >  		       (tp->t_flags & XFS_TRANS_RESERVE) |
> > > -		       (tp->t_flags & XFS_TRANS_NO_WRITECOUNT);
> > > +		       (tp->t_flags & XFS_TRANS_NO_WRITECOUNT) |
> > > +		       (tp->t_flags & XFS_TRANS_RES_FDBLKS);
> > 
> > At some point I wonder if we'd be better off with a #define mask that
> > covers all the flags that we preserve on transaction roll.
> > 
> > >  	/* We gave our writer reference to the new transaction */
> > >  	tp->t_flags |= XFS_TRANS_NO_WRITECOUNT;
> > >  	ntp->t_ticket = xfs_log_ticket_get(tp->t_ticket);
> > > @@ -365,6 +366,16 @@ 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 && (tp->t_flags & XFS_TRANS_RES_FDBLKS)) {
> > > +			/*
> > > +			 * Return freed blocks directly to the reservation
> > > +			 * instead of the global pool. This is used by chains of
> > > +			 * transaction rolls that repeatedly free and allocate
> > > +			 * blocks. Only safe with lazy sb accounting.
> > > +			 */
> > > +			ASSERT(xfs_sb_version_haslazysbcount(&mp->m_sb));
> > 
> > Shouldn't we check this at xfs_trans_alloc time so that it's immediately
> > obvious when someone screws up?
> > 
> 
> I dumped it here just because the assert was more simple, but sure, we
> could move it to xfs_trans_alloc() and assert that lazy sb accounting is
> enabled if the flag is passed.

<nod>

> > > +			tp->t_blk_res += delta;
> > 
> > What happens if t_blk_res + delta would overflow t_blk_res?  Can you
> > make some (probably contrived) scenario where this is possible?
> > 
> 
> Hmm.. perhaps if this were set on a transaction that truncated a large
> file? It's not clear to me if it could happen on swapext, but we could
> try to be defensive regardless and cap it to some max value. I have
> another variant of this around that tracks the original reservation
> count in a new ->t_blk_res_base field. Alternatively we could just cap
> the addition to something like (UINT_MAX - tp->t_blk_res), since this
> isolated use case probably isn't worth extending xfs_trans for. Hm?

I think it's best not to leave a lurking logic bomb, particularly
because you'd roll over to a weirdly tiny t_blk_res value that could be
smaller than t_blk_res_used.  I think this'll do:

	int blkres_delta = max(UINT_MAX - tp->t_blk_res, delta);

	tp->t_blk_res += blkres_delta;
	delta -= blkres_delta;

	<and then later>

	tp->t_fdblocks_delta += delta;

(Pretend I worked out any potential integer handling bugs in that...)

> > I'm also a little surprised that you don't subtract delta from
> > t_blk_res_used (at least until t_blk_res_used == 0).  Doing it this way
> > means that we'll ratchet up t_blk_res_used and t_blk_res every time we
> > ping pong, which feels a little strange.  But maybe you can elaborate?
> > 
> 
> My impression was that the common case is that one transaction consumes
> a block, the next frees a block, and the cycle repeats. Therefore,
> ->t_blk_res ends up similarly toggling back and forth over however many
> transactions rather than ratcheting up forever because xfs_trans_dup()
> shrinks ->t_blk_res and ->t_blk_res_used starts at zero on each new
> transaction.

Oh, right.  I forgot that it does that.  Ok, never mind then. :)

> I think there's a number of different ways to achieve the same net
> accounting effect. I briefly considered adding a ->t_blk_res_freed
> counter, but it wasn't clear that buys us anything. We could subtract
> from ->t_blk_res_used first, but we still have to fall back if that's
> zero so that just adds more code. It's also a logic wart since it
> assumes the order of frees and allocs within the same transaction and
> IMO slightly obfuscates the meaning of the flag by indirectly reducing
> block usage vs. directly adding free blocks to the reservation, but I
> suppose that bit is subjective. Since the purpose is really to affect a
> chain of transactions vs any particular one, it just seemed that the
> simplest and most predictable approach was to add freed blocks directly
> to the reservation.

<nod> Ok, I'm convinced.

--D

> Brian
> 
> > --D
> > 
> > > +			delta = 0;
> > >  		}
> > >  		tp->t_fdblocks_delta += delta;
> > >  		if (xfs_sb_version_haslazysbcount(&mp->m_sb))
> > > -- 
> > > 2.21.1
> > > 
> > 
>