On Thu, Nov 30, 2017 at 01:58:31PM -0500, Brian Foster wrote:
> The tr_ifree transaction handles inode unlinks and inode chunk
> frees. The current transaction calculation does not accurately
> reflect worst case changes to the inode btree, however. The inobt
> portion of the current transaction reservation only covers
> modification of a single inobt buffer (for the particular inode
> record). This is a historical artifact from the days before XFS
> supported full inode chunk removal.
>
> When support for inode chunk removal was added in commit
> 254f6311ed1b ("Implement deletion of inode clusters in XFS."), the
> additional log reservation required for chunk removal was not added
> correctly. The new reservation only considered the header overhead
> of associated buffers rather than the full contents of the btrees
> and AGF and AGFL buffers affected by the transaction. The
> reservation for the free space btrees was subsequently fixed up in
> commit 5fe6abb82f76 ("Add space for inode and allocation btrees to
> ITRUNCATE log reservation"), but the res. for full inobt joins has
> never been added.
>
> Further review of the ifree reservation uncovered a couple more
> problems:
>
> - The undocumented +2 blocks are intended for the AGF and AGFL, but
> are also not sized correctly and should be logged as full sectors
> (not FSBs).
> - The additional single block header is undocumented and serves no
> apparent purpose.
>
> Update xfs_calc_ifree_reservation() to include a full inobt join in
> the reservation calculation. Refactor the undocumented blocks
> appropriately and fix up the comments to reflect the current
> calculation.
>
> Signed-off-by: Brian Foster <bfoster@xxxxxxxxxx>
Looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> ---
> fs/xfs/libxfs/xfs_trans_resv.c | 15 ++++++---------
> 1 file changed, 6 insertions(+), 9 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_trans_resv.c b/fs/xfs/libxfs/xfs_trans_resv.c
> index 6bd916bd35e2..838566b85622 100644
> --- a/fs/xfs/libxfs/xfs_trans_resv.c
> +++ b/fs/xfs/libxfs/xfs_trans_resv.c
> @@ -490,10 +490,9 @@ xfs_calc_symlink_reservation(
> /*
> * In freeing an inode we can modify:
> * the inode being freed: inode size
> - * the super block free inode counter: sector size
> - * the agi hash list and counters: sector size
> - * the inode btree entry: block size
> - * the on disk inode before ours in the agi hash list: inode cluster size
> + * the super block free inode counter, AGF and AGFL: sector size
> + * the on disk inode (agi unlinked list removal)
> + * the inode chunk is marked stale (headers only)
> * the inode btree: max depth * blocksize
> * the allocation btrees: 2 trees * (max depth - 1) * block size
> * the finobt (record insertion, removal or modification)
> @@ -504,12 +503,10 @@ xfs_calc_ifree_reservation(
> {
> return XFS_DQUOT_LOGRES(mp) +
> xfs_calc_inode_res(mp, 1) +
> - xfs_calc_buf_res(1, mp->m_sb.sb_sectsize) +
> - xfs_calc_buf_res(1, XFS_FSB_TO_B(mp, 1)) +
> + xfs_calc_buf_res(3, mp->m_sb.sb_sectsize) +
> xfs_calc_iunlink_remove_reservation(mp) +
> - xfs_calc_buf_res(1, 0) +
> - xfs_calc_buf_res(2 + mp->m_ialloc_blks +
> - mp->m_in_maxlevels, 0) +
> + xfs_calc_buf_res(mp->m_ialloc_blks, 0) +
> + xfs_calc_buf_res(mp->m_in_maxlevels, XFS_FSB_TO_B(mp, 1)) +
> xfs_calc_buf_res(xfs_allocfree_log_count(mp, 1),
> XFS_FSB_TO_B(mp, 1)) +
> xfs_calc_finobt_res(mp, 0, 1);
> --
> 2.13.6
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
