Re: [PATCH v1 2/2] libxfs: fix reservation space for removing transaction

[Dave Chinner <david@fromorbit.com>]

On Thu, Feb 09, 2023 at 11:16:37AM +0800, Xiaole He wrote:
> In libxfs/xfs_trans_resv.c:
> 
> /* libxfs/xfs_trans_resv.c begin */
>  1 /*
>  2  * For removing a directory entry we can modify:
>  3  *    the parent directory inode: inode size
>  4  *    the removed inode: inode size
>  5  *    the directory btree could join: (max depth + v2) * dir block size
>  6  *    the directory bmap btree could join or split: (max depth + v2) * blocksize
>  7  * And the bmap_finish transaction can free the dir and bmap blocks giving:
>  8  *    the agf for the ag in which the blocks live: 2 * sector size
>  9  *    the agfl for the ag in which the blocks live: 2 * sector size
> 10  *    the superblock for the free block count: sector size
> 11  ...
> 12  */
> 13 STATIC uint
> 14 xfs_calc_remove_reservation(
> 15         struct xfs_mount        *mp)
> 16 {
> 17         return XFS_DQUOT_LOGRES(mp) +
> 18                 xfs_calc_iunlink_add_reservation(mp) +
> 19                 max((xfs_calc_inode_res(mp, 2) +
> 20                      xfs_calc_buf_res(XFS_DIROP_LOG_COUNT(mp),
> 21                                       XFS_FSB_TO_B(mp, 1))),
> 22                     (xfs_calc_buf_res(4, mp->m_sb.sb_sectsize) +
> 23 	...
> 24 }
> /* libxfs/xfs_trans_resv.c end */
> 
> Above lines 8-10 indicates there has 5 sector size of space to be
> reserved, but the above line 22 only reserve 4 sector size of space,

We don't log the superblock when lazycount=1 (i.e. default behaviour
for all v5 filesystems and almost all v4 filesystems created in the
last 15 years). Hence a count of 4 is actually correct for almost
all the production XFS filesystems out there.

Even ignoring that little detail, I don't think the comment you are
referencing reflects how extent freeing works these days. There is
no "bmap_finish" transaction in a directory entry remove operation
anymore - that was replaced long ago by generic intent-based
deferred log operations. Those deferred ops only process a single
extent freeing at a time (i.e. inserting a single new freespace
extent into the freespace btrees), so I don't think this comment
reflects reality anymore.

Now that we have minimum 64MB log sizes in mkfs, and a historical
reservation calculation for min log size calculations, we can reduce
the transaction reservation sizes down to match the current reality
without having to care about it changing minimum log sizes.

Hence I think it's time we actually fixed these "bmap_finish
multiple extent free" over-estimations to reflect the current
reality of only processing a single extent per intent. That means
fixing the comments and the code. :)

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com