[PATCH v1 1/2] xfs: fix typo from filesystes to filesystems

[PATCH v1 1/2] xfs: fix typo from filesystes to filesystems

[Xiaole He]

Signed-off-by: Xiaole He <hexiaole@kylinos.cn>
---
 fs/xfs/libxfs/xfs_alloc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
index f8ff81c3de76..7ebef43e3c1f 100644
--- a/fs/xfs/libxfs/xfs_alloc.c
+++ b/fs/xfs/libxfs/xfs_alloc.c
@@ -41,7 +41,7 @@ STATIC int xfs_alloc_ag_vextent_near(xfs_alloc_arg_t *);
 STATIC int xfs_alloc_ag_vextent_size(xfs_alloc_arg_t *);
 
 /*
- * Size of the AGFL.  For CRC-enabled filesystes we steal a couple of slots in
+ * Size of the AGFL.  For CRC-enabled filesystems we steal a couple of slots in
  * the beginning of the block for a proper header with the location information
  * and CRC.
  */
-- 
2.27.0

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

[Xiaole He]

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,
this patch fix the problem and sorry for not notice this problem at
Commit d3e53ab7cdc7fabb8c94137e335634e0ed4691e8 ("xfs: fix inode
reservation space for removing transaction").

Signed-off-by: Xiaole He <hexiaole@kylinos.cn>
---
 fs/xfs/libxfs/xfs_trans_resv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/xfs/libxfs/xfs_trans_resv.c b/fs/xfs/libxfs/xfs_trans_resv.c
index 5b2f27cbdb80..6064fae042e8 100644
--- a/fs/xfs/libxfs/xfs_trans_resv.c
+++ b/fs/xfs/libxfs/xfs_trans_resv.c
@@ -518,7 +518,7 @@ xfs_calc_remove_reservation(
 		max((xfs_calc_inode_res(mp, 2) +
 		     xfs_calc_buf_res(XFS_DIROP_LOG_COUNT(mp),
 				      XFS_FSB_TO_B(mp, 1))),
-		    (xfs_calc_buf_res(4, mp->m_sb.sb_sectsize) +
+		    (xfs_calc_buf_res(5, mp->m_sb.sb_sectsize) +
 		     xfs_calc_buf_res(xfs_allocfree_block_count(mp, 2),
 				      XFS_FSB_TO_B(mp, 1))));
 }
-- 
2.27.0

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

[Dave Chinner]

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