On Wed, Sep 09, 2020 at 06:19:11PM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@xxxxxxxxxx>
>
> Lots of the transaction reservation code reserves space for an
> extent allocation. It is inconsistently implemented, and many of
> them get it wrong. Introduce a new function to calculate the log
> space reservation for adding or removing an extent from the free
> space btrees.
>
> This function reserves space for logging the AGF, the AGFL and the
> free space btrees, avoiding the need to account for them seperately
> in every reservation that manipulates free space.
>
> Convert the EFI recovery reservation to use this transaction
> reservation as EFI recovery only needs to manipulate the free space
> index.
>
> Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
> ---
> fs/xfs/libxfs/xfs_trans_resv.c | 19 ++++++++++++++++++-
> 1 file changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/fs/xfs/libxfs/xfs_trans_resv.c b/fs/xfs/libxfs/xfs_trans_resv.c
> index da2ec052ac0a..621ddb277dfa 100644
> --- a/fs/xfs/libxfs/xfs_trans_resv.c
> +++ b/fs/xfs/libxfs/xfs_trans_resv.c
> @@ -79,6 +79,23 @@ xfs_allocfree_log_count(
> return blocks;
> }
>
> +/*
> + * Log reservation required to add or remove a single extent to the free space
> + * btrees. This requires modifying:
> + *
> + * the agf header: 1 sector
> + * the agfl header: 1 sector
> + * the allocation btrees: 2 trees * (max depth - 1) * block size
Nit, but the xfs_allocfree_log_count() helper this uses clearly
indicates reservation for up to four trees. It might be worth referring
to that here just to minimize spreading details all over the place that
are likely to become stale or inconsistent over time.
> + */
> +uint
> +xfs_allocfree_extent_res(
> + struct xfs_mount *mp)
> +{
> + return xfs_calc_buf_res(2, mp->m_sb.sb_sectsize) +
> + xfs_calc_buf_res(xfs_allocfree_log_count(mp, 1),
Why calculate for a single extent when an EFI can refer to multiple
extents? I thought the max was 2, but the extent free portion of the
itruncate calculation actually uses an op count of 4. The reason for
that is not immediately clear to me. It actually accounts 4 agf/agfl
blocks as well, so perhaps there's a wrong assumption somewhere. FWIW,
the truncate code allows 2 unmaps per transaction and the
xfs_extent_free_defer_type struct limits the dfp to 16. I suspect the
latter is not relevant for the current code.
Either way, multiple extents are factored into the current freeing
reservation and the extent freeing code at runtime (dfops) and during
recovery both appear to iterate on an extent count (potentially > 1) per
transaction. The itruncate comment, for reference (I also just noticed
that the subsequent patch modifies this comment, so you're presumably
aware of this mess):
/*
* ...
* And the bmap_finish transaction can free the blocks and bmap blocks (t2):
* the agf for each of the ags: 4 * sector size
* the agfl for each of the ags: 4 * sector size
* the super block to reflect the freed blocks: sector size
* worst case split in allocation btrees per extent assuming 4 extents:
* 4 exts * 2 trees * (2 * max depth - 1) * block size
* ...
*/
Brian
> + XFS_FSB_TO_B(mp, 1));
> +}
> +
> /*
> * Logging inodes is really tricksy. They are logged in memory format,
> * which means that what we write into the log doesn't directly translate into
> @@ -922,7 +939,7 @@ xfs_trans_resv_calc(
> * EFI recovery is itruncate minus the initial transaction that logs
> * logs the EFI.
> */
> - resp->tr_efi.tr_logres = resp->tr_itruncate.tr_logres;
> + resp->tr_efi.tr_logres = xfs_allocfree_extent_res(mp);
> resp->tr_efi.tr_logcount = resp->tr_itruncate.tr_logcount - 1;
> resp->tr_efi.tr_logflags |= XFS_TRANS_PERM_LOG_RES;
>
> --
> 2.28.0
>
