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@xxxxxxxxxx>
> Signed-off-by: Brian Foster <bfoster@xxxxxxxxxx>
> ---
>
> v1:
> - Use a transaction flag to isolate behavior to rmapbt swapext.
> rfc: https://lore.kernel.org/linux-xfs/20200522171828.53440-1-bfoster@xxxxxxxxxx/
>
> 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?
> + 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?
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?
--D
> + delta = 0;
> }
> tp->t_fdblocks_delta += delta;
> if (xfs_sb_version_haslazysbcount(&mp->m_sb))
> --
> 2.21.1
>
