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

["Darrick J. Wong" <djwong@kernel.org>]

On Wed, Aug 10, 2022 at 09:49:10AM +0800, 何小乐 wrote:
> Thank you for review, Darrick.
> Yes, this problem(reserve 1 inode size rather than 2) exists in both
> the userspace utility 'xfsprogs' and kernel source 'xfs-linux'. 
> 
> The reason that sent patch against 'xfsprogs' only is that I'm not
> sure whether the patch is correct(whether there need reserve one more
> inode size for the removed inode or not) and hope the maintainers

Yep -- we do need to have enough space to log the parent directory inode
as well as the child that's being removed from the directory.

> helping to review for its correctness, if it's correct, I'm willing to
> send another patch against kernel source 'xfs-linux.
> 
> So, Darric, you said 'The logic looks correct.' means the patch is
> correct, rather than the original logic that reserving 1 inode size
> for removing transaction is correct, right? Hopelly I did not
> misunderstand.

Yes, the patch looks correct to me.  Please send us the kernel version,
so that we can land it in the kernel (and then xfsprogs). :)

--D

> 
> 
> 
> 
> 
> 
> 
> At 2022-08-10 04:44:12, "Darrick J. Wong" <djwong@kernel.org> wrote:
> >On Tue, Aug 02, 2022 at 11:18:06AM +0800, Xiaole He wrote:
> >> From: hexiaole <hexiaole@kylinos.cn>
> >> 
> >> In 'libxfs/xfs_trans_resv.c', the comment for transaction of removing a
> >> directory entry writes:
> >> 
> >> /* libxfs/xfs_trans_resv.c begin */
> >> /*
> >>  * For removing a directory entry we can modify:
> >>  *    the parent directory inode: inode size
> >>  *    the removed inode: inode size
> >> ...
> >> /* libxfs/xfs_trans_resv.c end */
> >> 
> >> There has 2 inode size of space to be reserverd, but the actual code
> >> for inode reservation space writes:
> >> 
> >> /* libxfs/xfs_trans_resv.c begin */
> >> /*
> >>  * For removing a directory entry we can modify:
> >>  *    the parent directory inode: inode size
> >>  *    the removed inode: inode size
> >> ...
> >> xfs_calc_remove_reservation(
> >>         struct xfs_mount        *mp)
> >> {
> >>         return XFS_DQUOT_LOGRES(mp) +
> >>                 xfs_calc_iunlink_add_reservation(mp) +
> >>                 max((xfs_calc_inode_res(mp, 1) +
> >> ...
> >> /* libxfs/xfs_trans_resv.c end */
> >> 
> >> There only count for 1 inode size to be reserved in
> >> 'xfs_calc_inode_res(mp, 1)', rather than 2.
> >
> >The logic looks correct.  Why is this patch against xfsprogs, though?
> >
> >--D
> >
> >> Signed-off-by: hexiaole <hexiaole@kylinos.cn>
> >> ---
> >>  libxfs/xfs_trans_resv.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >> 
> >> diff --git a/libxfs/xfs_trans_resv.c b/libxfs/xfs_trans_resv.c
> >> index d4a9f69e..797176d7 100644
> >> --- a/libxfs/xfs_trans_resv.c
> >> +++ b/libxfs/xfs_trans_resv.c
> >> @@ -514,7 +514,7 @@ xfs_calc_remove_reservation(
> >>  {
> >>  	return XFS_DQUOT_LOGRES(mp) +
> >>  		xfs_calc_iunlink_add_reservation(mp) +
> >> -		max((xfs_calc_inode_res(mp, 1) +
> >> +		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) +
> >> -- 
> >> 2.27.0
> >>