| Message ID | 20220922054458.40826-14-allison.henderson@oracle.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Parent Pointers | expand |
On Wed, Sep 21, 2022 at 10:44:45PM -0700, allison.henderson@oracle.com wrote: > From: Allison Henderson <allison.henderson@oracle.com> > > We need to add, remove or modify parent pointer attributes during > create/link/unlink/rename operations atomically with the dirents in the > parent directories being modified. This means they need to be modified > in the same transaction as the parent directories, and so we need to add > the required space for the attribute modifications to the transaction > reservations. > > Signed-off-by: Dave Chinner <dchinner@redhat.com> > Signed-off-by: Allison Henderson <allison.henderson@oracle.com> > --- > fs/xfs/libxfs/xfs_trans_resv.c | 135 ++++++++++++++++++++++++++------- > 1 file changed, 106 insertions(+), 29 deletions(-) > > diff --git a/fs/xfs/libxfs/xfs_trans_resv.c b/fs/xfs/libxfs/xfs_trans_resv.c > index 2c4ad6e4bb14..f7799800d556 100644 > --- a/fs/xfs/libxfs/xfs_trans_resv.c > +++ b/fs/xfs/libxfs/xfs_trans_resv.c > @@ -19,6 +19,7 @@ > #include "xfs_trans.h" > #include "xfs_qm.h" > #include "xfs_trans_space.h" > +#include "xfs_attr_item.h" > > #define _ALLOC true > #define _FREE false > @@ -421,28 +422,45 @@ xfs_calc_itruncate_reservation_minlogsize( > } > > /* > - * In renaming a files we can modify: > - * the four inodes involved: 4 * inode size > + * In renaming a files we can modify (t1): > + * the four inodes involved: 5 * inode size ...the *five* inodes involved... Also -- even before parent pointers we could have five inodes involved in a rename transaction, so I think this change needs to be a separate bugfix at the start of the series. Rename isn't experimental, so I can't let this one slide. :/ > * the two directory btrees: 2 * (max depth + v2) * dir block size > * the two directory bmap btrees: 2 * max depth * block size > * And the bmap_finish transaction can free dir and bmap blocks (two sets > - * of bmap blocks) giving: > + * of bmap blocks) giving (t2): > * the agf for the ags in which the blocks live: 3 * sector size > * the agfl for the ags in which the blocks live: 3 * sector size > * the superblock for the free block count: sector size > * the allocation btrees: 3 exts * 2 trees * (2 * max depth - 1) * block size > + * If parent pointers are enabled (t3), then each transaction in the chain > + * must be capable of setting or removing the extended attribute > + * containing the parent information. It must also be able to handle > + * the three xattr intent items that track the progress of the parent > + * pointer update. > */ > STATIC uint > xfs_calc_rename_reservation( > struct xfs_mount *mp) > { > - return XFS_DQUOT_LOGRES(mp) + > - max((xfs_calc_inode_res(mp, 4) + > - xfs_calc_buf_res(2 * XFS_DIROP_LOG_COUNT(mp), > - XFS_FSB_TO_B(mp, 1))), > - (xfs_calc_buf_res(7, mp->m_sb.sb_sectsize) + > - xfs_calc_buf_res(xfs_allocfree_block_count(mp, 3), > - XFS_FSB_TO_B(mp, 1)))); > + unsigned int overhead = XFS_DQUOT_LOGRES(mp); > + struct xfs_trans_resv *resp = M_RES(mp); > + unsigned int t1, t2, t3 = 0; > + > + t1 = xfs_calc_inode_res(mp, 5) + > + xfs_calc_buf_res(2 * XFS_DIROP_LOG_COUNT(mp), > + XFS_FSB_TO_B(mp, 1)); > + > + t2 = xfs_calc_buf_res(7, mp->m_sb.sb_sectsize) + > + xfs_calc_buf_res(xfs_allocfree_block_count(mp, 3), > + XFS_FSB_TO_B(mp, 1)); > + > + if (xfs_has_parent(mp)) { > + t3 = max(resp->tr_attrsetm.tr_logres, > + resp->tr_attrrm.tr_logres); Ooh I like this refactoring of xfs_calc_rename_reservation. :) I guess we now tr_attr{setm,rm} before computing the rename reservation so this is ok. > + overhead += 3 * (sizeof(struct xfs_attri_log_item)); Should the size of the name, newname, and value buffers be added into overhead? They take up log space too. > + } > + > + return overhead + max3(t1, t2, t3); > } > > /* > @@ -909,24 +927,59 @@ xfs_calc_sb_reservation( > return xfs_calc_buf_res(1, mp->m_sb.sb_sectsize); > } > > -void > -xfs_trans_resv_calc( > - struct xfs_mount *mp, > - struct xfs_trans_resv *resp) > +/* > + * Calculate extra space needed for parent pointer attributes > + */ > +STATIC void > +xfs_calc_parent_ptr_reservations( > + struct xfs_mount *mp) > { > - int logcount_adj = 0; > + struct xfs_trans_resv *resp = M_RES(mp); > > - /* > - * The following transactions are logged in physical format and > - * require a permanent reservation on space. > - */ > - resp->tr_write.tr_logres = xfs_calc_write_reservation(mp, false); > - resp->tr_write.tr_logcount = XFS_WRITE_LOG_COUNT; > - resp->tr_write.tr_logflags |= XFS_TRANS_PERM_LOG_RES; > + if (!xfs_has_parent(mp)) > + return; > > - resp->tr_itruncate.tr_logres = xfs_calc_itruncate_reservation(mp, false); > - resp->tr_itruncate.tr_logcount = XFS_ITRUNCATE_LOG_COUNT; > - resp->tr_itruncate.tr_logflags |= XFS_TRANS_PERM_LOG_RES; > + resp->tr_rename.tr_logres += max(resp->tr_attrsetm.tr_logres, > + resp->tr_attrrm.tr_logres); > + resp->tr_rename.tr_logcount += max(resp->tr_attrsetm.tr_logcount, > + resp->tr_attrrm.tr_logcount); Doesn't xfs_calc_rename_reservation add this to tr_rename already? > + > + resp->tr_create.tr_logres += resp->tr_attrsetm.tr_logres; > + resp->tr_create.tr_logcount += resp->tr_attrsetm.tr_logcount; > + > + resp->tr_mkdir.tr_logres += resp->tr_attrsetm.tr_logres; > + resp->tr_mkdir.tr_logcount += resp->tr_attrsetm.tr_logcount; > + > + resp->tr_link.tr_logres += resp->tr_attrsetm.tr_logres; > + resp->tr_link.tr_logcount += resp->tr_attrsetm.tr_logcount; > + > + resp->tr_symlink.tr_logres += resp->tr_attrsetm.tr_logres; > + resp->tr_symlink.tr_logcount += resp->tr_attrsetm.tr_logcount; > + > + resp->tr_remove.tr_logres += resp->tr_attrrm.tr_logres; > + resp->tr_remove.tr_logcount += resp->tr_attrrm.tr_logcount; Shouldn't each of these += additions be made to xfs_calc_{icreate,mkdir,link,symlink,remove}_reservation, respectively? --D > +} > + > +/* > + * Namespace reservations. > + * > + * These get tricky when parent pointers are enabled as we have attribute > + * modifications occurring from within these transactions. Rather than confuse > + * each of these reservation calculations with the conditional attribute > + * reservations, add them here in a clear and concise manner. This assumes that > + * the attribute reservations have already been calculated. > + * > + * Note that we only include the static attribute reservation here; the runtime > + * reservation will have to be modified by the size of the attributes being > + * added/removed/modified. See the comments on the attribute reservation > + * calculations for more details. > + */ > +STATIC void > +xfs_calc_namespace_reservations( > + struct xfs_mount *mp, > + struct xfs_trans_resv *resp) > +{ > + ASSERT(resp->tr_attrsetm.tr_logres > 0); > > resp->tr_rename.tr_logres = xfs_calc_rename_reservation(mp); > resp->tr_rename.tr_logcount = XFS_RENAME_LOG_COUNT; > @@ -948,15 +1001,37 @@ xfs_trans_resv_calc( > resp->tr_create.tr_logcount = XFS_CREATE_LOG_COUNT; > resp->tr_create.tr_logflags |= XFS_TRANS_PERM_LOG_RES; > > + resp->tr_mkdir.tr_logres = xfs_calc_mkdir_reservation(mp); > + resp->tr_mkdir.tr_logcount = XFS_MKDIR_LOG_COUNT; > + resp->tr_mkdir.tr_logflags |= XFS_TRANS_PERM_LOG_RES; > + > + xfs_calc_parent_ptr_reservations(mp); > +} > + > +void > +xfs_trans_resv_calc( > + struct xfs_mount *mp, > + struct xfs_trans_resv *resp) > +{ > + int logcount_adj = 0; > + > + /* > + * The following transactions are logged in physical format and > + * require a permanent reservation on space. > + */ > + resp->tr_write.tr_logres = xfs_calc_write_reservation(mp, false); > + resp->tr_write.tr_logcount = XFS_WRITE_LOG_COUNT; > + resp->tr_write.tr_logflags |= XFS_TRANS_PERM_LOG_RES; > + > + resp->tr_itruncate.tr_logres = xfs_calc_itruncate_reservation(mp, false); > + resp->tr_itruncate.tr_logcount = XFS_ITRUNCATE_LOG_COUNT; > + resp->tr_itruncate.tr_logflags |= XFS_TRANS_PERM_LOG_RES; > + > resp->tr_create_tmpfile.tr_logres = > xfs_calc_create_tmpfile_reservation(mp); > resp->tr_create_tmpfile.tr_logcount = XFS_CREATE_TMPFILE_LOG_COUNT; > resp->tr_create_tmpfile.tr_logflags |= XFS_TRANS_PERM_LOG_RES; > > - resp->tr_mkdir.tr_logres = xfs_calc_mkdir_reservation(mp); > - resp->tr_mkdir.tr_logcount = XFS_MKDIR_LOG_COUNT; > - resp->tr_mkdir.tr_logflags |= XFS_TRANS_PERM_LOG_RES; > - > resp->tr_ifree.tr_logres = xfs_calc_ifree_reservation(mp); > resp->tr_ifree.tr_logcount = XFS_INACTIVE_LOG_COUNT; > resp->tr_ifree.tr_logflags |= XFS_TRANS_PERM_LOG_RES; > @@ -986,6 +1061,8 @@ xfs_trans_resv_calc( > resp->tr_qm_dqalloc.tr_logcount = XFS_WRITE_LOG_COUNT; > resp->tr_qm_dqalloc.tr_logflags |= XFS_TRANS_PERM_LOG_RES; > > + xfs_calc_namespace_reservations(mp, resp); > + > /* > * The following transactions are logged in logical format with > * a default log count. > -- > 2.25.1 >
On Fri, 2022-09-23 at 13:17 -0700, Darrick J. Wong wrote: > On Wed, Sep 21, 2022 at 10:44:45PM -0700, > allison.henderson@oracle.com wrote: > > From: Allison Henderson <allison.henderson@oracle.com> > > > > We need to add, remove or modify parent pointer attributes during > > create/link/unlink/rename operations atomically with the dirents in > > the > > parent directories being modified. This means they need to be > > modified > > in the same transaction as the parent directories, and so we need > > to add > > the required space for the attribute modifications to the > > transaction > > reservations. > > > > Signed-off-by: Dave Chinner <dchinner@redhat.com> > > Signed-off-by: Allison Henderson <allison.henderson@oracle.com> > > --- > > fs/xfs/libxfs/xfs_trans_resv.c | 135 ++++++++++++++++++++++++++--- > > ---- > > 1 file changed, 106 insertions(+), 29 deletions(-) > > > > diff --git a/fs/xfs/libxfs/xfs_trans_resv.c > > b/fs/xfs/libxfs/xfs_trans_resv.c > > index 2c4ad6e4bb14..f7799800d556 100644 > > --- a/fs/xfs/libxfs/xfs_trans_resv.c > > +++ b/fs/xfs/libxfs/xfs_trans_resv.c > > @@ -19,6 +19,7 @@ > > #include "xfs_trans.h" > > #include "xfs_qm.h" > > #include "xfs_trans_space.h" > > +#include "xfs_attr_item.h" > > > > #define _ALLOC true > > #define _FREE false > > @@ -421,28 +422,45 @@ xfs_calc_itruncate_reservation_minlogsize( > > } > > > > /* > > - * In renaming a files we can modify: > > - * the four inodes involved: 4 * inode size > > + * In renaming a files we can modify (t1): > > + * the four inodes involved: 5 * inode size > > ...the *five* inodes involved... > > Also -- even before parent pointers we could have five inodes > involved > in a rename transaction, so I think this change needs to be a > separate > bugfix at the start of the series. Rename isn't experimental, so I > can't let this one slide. :/ I see, ok I will split this one out then > > > * the two directory btrees: 2 * (max depth + v2) * dir block > > size > > * the two directory bmap btrees: 2 * max depth * block size > > * And the bmap_finish transaction can free dir and bmap blocks > > (two sets > > - * of bmap blocks) giving: > > + * of bmap blocks) giving (t2): > > * the agf for the ags in which the blocks live: 3 * sector > > size > > * the agfl for the ags in which the blocks live: 3 * sector > > size > > * the superblock for the free block count: sector size > > * the allocation btrees: 3 exts * 2 trees * (2 * max depth - > > 1) * block size > > + * If parent pointers are enabled (t3), then each transaction in > > the chain > > + * must be capable of setting or removing the extended > > attribute > > + * containing the parent information. It must also be able to > > handle > > + * the three xattr intent items that track the progress of the > > parent > > + * pointer update. > > */ > > STATIC uint > > xfs_calc_rename_reservation( > > struct xfs_mount *mp) > > { > > - return XFS_DQUOT_LOGRES(mp) + > > - max((xfs_calc_inode_res(mp, 4) + > > - xfs_calc_buf_res(2 * XFS_DIROP_LOG_COUNT(mp), > > - XFS_FSB_TO_B(mp, 1))), > > - (xfs_calc_buf_res(7, mp->m_sb.sb_sectsize) + > > - xfs_calc_buf_res(xfs_allocfree_block_count(mp, > > 3), > > - XFS_FSB_TO_B(mp, 1)))); > > + unsigned int overhead = XFS_DQUOT_LOGRES(mp); > > + struct xfs_trans_resv *resp = M_RES(mp); > > + unsigned int t1, t2, t3 = 0; > > + > > + t1 = xfs_calc_inode_res(mp, 5) + > > + xfs_calc_buf_res(2 * XFS_DIROP_LOG_COUNT(mp), > > + XFS_FSB_TO_B(mp, 1)); > > + > > + t2 = xfs_calc_buf_res(7, mp->m_sb.sb_sectsize) + > > + xfs_calc_buf_res(xfs_allocfree_block_count(mp, 3), > > + XFS_FSB_TO_B(mp, 1)); > > + > > + if (xfs_has_parent(mp)) { > > + t3 = max(resp->tr_attrsetm.tr_logres, > > + resp->tr_attrrm.tr_logres); > > Ooh I like this refactoring of xfs_calc_rename_reservation. :) > > I guess we now tr_attr{setm,rm} before computing the rename > reservation > so this is ok. > > > + overhead += 3 * (sizeof(struct > > xfs_attri_log_item)); > > Should the size of the name, newname, and value buffers be added into > overhead? They take up log space too. That would make sense, but we cant really calculate that ahead of time with out just assuming the max size which is up to 64k for the value. Maybe those sizes should be added on after we know what they are? > > > + } > > + > > + return overhead + max3(t1, t2, t3); > > } > > > > /* > > @@ -909,24 +927,59 @@ xfs_calc_sb_reservation( > > return xfs_calc_buf_res(1, mp->m_sb.sb_sectsize); > > } > > > > -void > > -xfs_trans_resv_calc( > > - struct xfs_mount *mp, > > - struct xfs_trans_resv *resp) > > +/* > > + * Calculate extra space needed for parent pointer attributes > > + */ > > +STATIC void > > +xfs_calc_parent_ptr_reservations( > > + struct xfs_mount *mp) > > { > > - int logcount_adj = 0; > > + struct xfs_trans_resv *resp = M_RES(mp); > > > > - /* > > - * The following transactions are logged in physical format > > and > > - * require a permanent reservation on space. > > - */ > > - resp->tr_write.tr_logres = xfs_calc_write_reservation(mp, > > false); > > - resp->tr_write.tr_logcount = XFS_WRITE_LOG_COUNT; > > - resp->tr_write.tr_logflags |= XFS_TRANS_PERM_LOG_RES; > > + if (!xfs_has_parent(mp)) > > + return; > > > > - resp->tr_itruncate.tr_logres = > > xfs_calc_itruncate_reservation(mp, false); > > - resp->tr_itruncate.tr_logcount = XFS_ITRUNCATE_LOG_COUNT; > > - resp->tr_itruncate.tr_logflags |= XFS_TRANS_PERM_LOG_RES; > > + resp->tr_rename.tr_logres += max(resp- > > >tr_attrsetm.tr_logres, > > + resp- > > >tr_attrrm.tr_logres); > > + resp->tr_rename.tr_logcount += max(resp- > > >tr_attrsetm.tr_logcount, > > + resp- > > >tr_attrrm.tr_logcount); > > Doesn't xfs_calc_rename_reservation add this to tr_rename already? Oh, I think we can remove the tr_rename.tr_logres update. But not the logcount update right? xfs_calc_rename_reservation just calculates what tr_rename.tr_logres should be, but it doesnt make any additions. It's that's used over in xfs_calc_namespace_reservations, but we still need to update tr_rename.tr_logcount which is separate field from tr_rename.tr_logres. > > > + > > + resp->tr_create.tr_logres += resp->tr_attrsetm.tr_logres; > > + resp->tr_create.tr_logcount += resp- > > >tr_attrsetm.tr_logcount; > > + > > + resp->tr_mkdir.tr_logres += resp->tr_attrsetm.tr_logres; > > + resp->tr_mkdir.tr_logcount += resp- > > >tr_attrsetm.tr_logcount; > > + > > + resp->tr_link.tr_logres += resp->tr_attrsetm.tr_logres; > > + resp->tr_link.tr_logcount += resp->tr_attrsetm.tr_logcount; > > + > > + resp->tr_symlink.tr_logres += resp->tr_attrsetm.tr_logres; > > + resp->tr_symlink.tr_logcount += resp- > > >tr_attrsetm.tr_logcount; > > + > > + resp->tr_remove.tr_logres += resp->tr_attrrm.tr_logres; > > + resp->tr_remove.tr_logcount += resp->tr_attrrm.tr_logcount; > > Shouldn't each of these += additions be made to > xfs_calc_{icreate,mkdir,link,symlink,remove}_reservation, > respectively? > I suppose we could redo it that way? But then not all of the "+=" would disappear if they worked like xfs_calc_rename_reservation. Just the *.tr_logres. Do we really want separate wrappers for just these oneliners though? Allison > --D > > > +} > > + > > +/* > > + * Namespace reservations. > > + * > > + * These get tricky when parent pointers are enabled as we have > > attribute > > + * modifications occurring from within these transactions. Rather > > than confuse > > + * each of these reservation calculations with the conditional > > attribute > > + * reservations, add them here in a clear and concise manner. This > > assumes that > > + * the attribute reservations have already been calculated. > > + * > > + * Note that we only include the static attribute reservation > > here; the runtime > > + * reservation will have to be modified by the size of the > > attributes being > > + * added/removed/modified. See the comments on the attribute > > reservation > > + * calculations for more details. > > + */ > > +STATIC void > > +xfs_calc_namespace_reservations( > > + struct xfs_mount *mp, > > + struct xfs_trans_resv *resp) > > +{ > > + ASSERT(resp->tr_attrsetm.tr_logres > 0); > > > > resp->tr_rename.tr_logres = > > xfs_calc_rename_reservation(mp); > > resp->tr_rename.tr_logcount = XFS_RENAME_LOG_COUNT; > > @@ -948,15 +1001,37 @@ xfs_trans_resv_calc( > > resp->tr_create.tr_logcount = XFS_CREATE_LOG_COUNT; > > resp->tr_create.tr_logflags |= XFS_TRANS_PERM_LOG_RES; > > > > + resp->tr_mkdir.tr_logres = xfs_calc_mkdir_reservation(mp); > > + resp->tr_mkdir.tr_logcount = XFS_MKDIR_LOG_COUNT; > > + resp->tr_mkdir.tr_logflags |= XFS_TRANS_PERM_LOG_RES; > > + > > + xfs_calc_parent_ptr_reservations(mp); > > +} > > + > > +void > > +xfs_trans_resv_calc( > > + struct xfs_mount *mp, > > + struct xfs_trans_resv *resp) > > +{ > > + int logcount_adj = 0; > > + > > + /* > > + * The following transactions are logged in physical format > > and > > + * require a permanent reservation on space. > > + */ > > + resp->tr_write.tr_logres = xfs_calc_write_reservation(mp, > > false); > > + resp->tr_write.tr_logcount = XFS_WRITE_LOG_COUNT; > > + resp->tr_write.tr_logflags |= XFS_TRANS_PERM_LOG_RES; > > + > > + resp->tr_itruncate.tr_logres = > > xfs_calc_itruncate_reservation(mp, false); > > + resp->tr_itruncate.tr_logcount = XFS_ITRUNCATE_LOG_COUNT; > > + resp->tr_itruncate.tr_logflags |= XFS_TRANS_PERM_LOG_RES; > > + > > resp->tr_create_tmpfile.tr_logres = > > xfs_calc_create_tmpfile_reservation(mp); > > resp->tr_create_tmpfile.tr_logcount = > > XFS_CREATE_TMPFILE_LOG_COUNT; > > resp->tr_create_tmpfile.tr_logflags |= > > XFS_TRANS_PERM_LOG_RES; > > > > - resp->tr_mkdir.tr_logres = xfs_calc_mkdir_reservation(mp); > > - resp->tr_mkdir.tr_logcount = XFS_MKDIR_LOG_COUNT; > > - resp->tr_mkdir.tr_logflags |= XFS_TRANS_PERM_LOG_RES; > > - > > resp->tr_ifree.tr_logres = xfs_calc_ifree_reservation(mp); > > resp->tr_ifree.tr_logcount = XFS_INACTIVE_LOG_COUNT; > > resp->tr_ifree.tr_logflags |= XFS_TRANS_PERM_LOG_RES; > > @@ -986,6 +1061,8 @@ xfs_trans_resv_calc( > > resp->tr_qm_dqalloc.tr_logcount = XFS_WRITE_LOG_COUNT; > > resp->tr_qm_dqalloc.tr_logflags |= XFS_TRANS_PERM_LOG_RES; > > > > + xfs_calc_namespace_reservations(mp, resp); > > + > > /* > > * The following transactions are logged in logical format > > with > > * a default log count. > > -- > > 2.25.1 > >
On Fri, Sep 23, 2022 at 11:53:22PM +0000, Allison Henderson wrote: > On Fri, 2022-09-23 at 13:17 -0700, Darrick J. Wong wrote: > > On Wed, Sep 21, 2022 at 10:44:45PM -0700, > > allison.henderson@oracle.com wrote: > > > From: Allison Henderson <allison.henderson@oracle.com> > > > > > > We need to add, remove or modify parent pointer attributes during > > > create/link/unlink/rename operations atomically with the dirents in > > > the > > > parent directories being modified. This means they need to be > > > modified > > > in the same transaction as the parent directories, and so we need > > > to add > > > the required space for the attribute modifications to the > > > transaction > > > reservations. > > > > > > Signed-off-by: Dave Chinner <dchinner@redhat.com> > > > Signed-off-by: Allison Henderson <allison.henderson@oracle.com> > > > --- > > > fs/xfs/libxfs/xfs_trans_resv.c | 135 ++++++++++++++++++++++++++--- > > > ---- > > > 1 file changed, 106 insertions(+), 29 deletions(-) > > > > > > diff --git a/fs/xfs/libxfs/xfs_trans_resv.c > > > b/fs/xfs/libxfs/xfs_trans_resv.c > > > index 2c4ad6e4bb14..f7799800d556 100644 > > > --- a/fs/xfs/libxfs/xfs_trans_resv.c > > > +++ b/fs/xfs/libxfs/xfs_trans_resv.c > > > @@ -19,6 +19,7 @@ > > > #include "xfs_trans.h" > > > #include "xfs_qm.h" > > > #include "xfs_trans_space.h" > > > +#include "xfs_attr_item.h" > > > > > > #define _ALLOC true > > > #define _FREE false > > > @@ -421,28 +422,45 @@ xfs_calc_itruncate_reservation_minlogsize( > > > } > > > > > > /* > > > - * In renaming a files we can modify: > > > - * the four inodes involved: 4 * inode size > > > + * In renaming a files we can modify (t1): > > > + * the four inodes involved: 5 * inode size > > > > ...the *five* inodes involved... > > > > Also -- even before parent pointers we could have five inodes > > involved > > in a rename transaction, so I think this change needs to be a > > separate > > bugfix at the start of the series. Rename isn't experimental, so I > > can't let this one slide. :/ > I see, ok I will split this one out then > > > > > > * the two directory btrees: 2 * (max depth + v2) * dir block > > > size > > > * the two directory bmap btrees: 2 * max depth * block size > > > * And the bmap_finish transaction can free dir and bmap blocks > > > (two sets > > > - * of bmap blocks) giving: > > > + * of bmap blocks) giving (t2): > > > * the agf for the ags in which the blocks live: 3 * sector > > > size > > > * the agfl for the ags in which the blocks live: 3 * sector > > > size > > > * the superblock for the free block count: sector size > > > * the allocation btrees: 3 exts * 2 trees * (2 * max depth - > > > 1) * block size > > > + * If parent pointers are enabled (t3), then each transaction in > > > the chain > > > + * must be capable of setting or removing the extended > > > attribute > > > + * containing the parent information. It must also be able to > > > handle > > > + * the three xattr intent items that track the progress of the > > > parent > > > + * pointer update. > > > */ > > > STATIC uint > > > xfs_calc_rename_reservation( > > > struct xfs_mount *mp) > > > { > > > - return XFS_DQUOT_LOGRES(mp) + > > > - max((xfs_calc_inode_res(mp, 4) + > > > - xfs_calc_buf_res(2 * XFS_DIROP_LOG_COUNT(mp), > > > - XFS_FSB_TO_B(mp, 1))), > > > - (xfs_calc_buf_res(7, mp->m_sb.sb_sectsize) + > > > - xfs_calc_buf_res(xfs_allocfree_block_count(mp, > > > 3), > > > - XFS_FSB_TO_B(mp, 1)))); > > > + unsigned int overhead = XFS_DQUOT_LOGRES(mp); > > > + struct xfs_trans_resv *resp = M_RES(mp); > > > + unsigned int t1, t2, t3 = 0; > > > + > > > + t1 = xfs_calc_inode_res(mp, 5) + > > > + xfs_calc_buf_res(2 * XFS_DIROP_LOG_COUNT(mp), > > > + XFS_FSB_TO_B(mp, 1)); > > > + > > > + t2 = xfs_calc_buf_res(7, mp->m_sb.sb_sectsize) + > > > + xfs_calc_buf_res(xfs_allocfree_block_count(mp, 3), > > > + XFS_FSB_TO_B(mp, 1)); > > > + > > > + if (xfs_has_parent(mp)) { > > > + t3 = max(resp->tr_attrsetm.tr_logres, > > > + resp->tr_attrrm.tr_logres); > > > > Ooh I like this refactoring of xfs_calc_rename_reservation. :) > > > > I guess we now tr_attr{setm,rm} before computing the rename > > reservation > > so this is ok. > > > > > + overhead += 3 * (sizeof(struct > > > xfs_attri_log_item)); > > > > Should the size of the name, newname, and value buffers be added into > > overhead? They take up log space too. > That would make sense, but we cant really calculate that ahead of time > with out just assuming the max size which is up to 64k for the value. > Maybe those sizes should be added on after we know what they are? They have to be worst case values, but fortunately we know what the worst case is: name: sizeof(ondisk parent ptr structure) + iovec overhead newname: same for rename, and zero everywhere else iiuc? value: 255 + iovec_overhead > > > > > + } > > > + > > > + return overhead + max3(t1, t2, t3); > > > } > > > > > > /* > > > @@ -909,24 +927,59 @@ xfs_calc_sb_reservation( > > > return xfs_calc_buf_res(1, mp->m_sb.sb_sectsize); > > > } > > > > > > -void > > > -xfs_trans_resv_calc( > > > - struct xfs_mount *mp, > > > - struct xfs_trans_resv *resp) > > > +/* > > > + * Calculate extra space needed for parent pointer attributes > > > + */ > > > +STATIC void > > > +xfs_calc_parent_ptr_reservations( > > > + struct xfs_mount *mp) > > > { > > > - int logcount_adj = 0; > > > + struct xfs_trans_resv *resp = M_RES(mp); > > > > > > - /* > > > - * The following transactions are logged in physical format > > > and > > > - * require a permanent reservation on space. > > > - */ > > > - resp->tr_write.tr_logres = xfs_calc_write_reservation(mp, > > > false); > > > - resp->tr_write.tr_logcount = XFS_WRITE_LOG_COUNT; > > > - resp->tr_write.tr_logflags |= XFS_TRANS_PERM_LOG_RES; > > > + if (!xfs_has_parent(mp)) > > > + return; > > > > > > - resp->tr_itruncate.tr_logres = > > > xfs_calc_itruncate_reservation(mp, false); > > > - resp->tr_itruncate.tr_logcount = XFS_ITRUNCATE_LOG_COUNT; > > > - resp->tr_itruncate.tr_logflags |= XFS_TRANS_PERM_LOG_RES; > > > + resp->tr_rename.tr_logres += max(resp- > > > >tr_attrsetm.tr_logres, > > > + resp- > > > >tr_attrrm.tr_logres); > > > + resp->tr_rename.tr_logcount += max(resp- > > > >tr_attrsetm.tr_logcount, > > > + resp- > > > >tr_attrrm.tr_logcount); > > > > Doesn't xfs_calc_rename_reservation add this to tr_rename already? > Oh, I think we can remove the tr_rename.tr_logres update. But not the > logcount update right? Right, though you could update XFS_RENAME_LOG_COUNT (or turn it into a helper function). > xfs_calc_rename_reservation just calculates what tr_rename.tr_logres > should be, but it doesnt make any additions. It's that's used over in > xfs_calc_namespace_reservations, but we still need to update > tr_rename.tr_logcount which is separate field from tr_rename.tr_logres. > > > > > > > + > > > + resp->tr_create.tr_logres += resp->tr_attrsetm.tr_logres; > > > + resp->tr_create.tr_logcount += resp- > > > >tr_attrsetm.tr_logcount; > > > + > > > + resp->tr_mkdir.tr_logres += resp->tr_attrsetm.tr_logres; > > > + resp->tr_mkdir.tr_logcount += resp- > > > >tr_attrsetm.tr_logcount; > > > + > > > + resp->tr_link.tr_logres += resp->tr_attrsetm.tr_logres; > > > + resp->tr_link.tr_logcount += resp->tr_attrsetm.tr_logcount; > > > + > > > + resp->tr_symlink.tr_logres += resp->tr_attrsetm.tr_logres; > > > + resp->tr_symlink.tr_logcount += resp- > > > >tr_attrsetm.tr_logcount; > > > + > > > + resp->tr_remove.tr_logres += resp->tr_attrrm.tr_logres; > > > + resp->tr_remove.tr_logcount += resp->tr_attrrm.tr_logcount; > > > > Shouldn't each of these += additions be made to > > xfs_calc_{icreate,mkdir,link,symlink,remove}_reservation, > > respectively? > > > > I suppose we could redo it that way? But then not all of the "+=" > would disappear if they worked like xfs_calc_rename_reservation. Just > the *.tr_logres. Do we really want separate wrappers for just these > oneliners though? Yes, once those macros start acquiring logic, they ought to turn into static inline helpers. --D > Allison > > > --D > > > > > +} > > > + > > > +/* > > > + * Namespace reservations. > > > + * > > > + * These get tricky when parent pointers are enabled as we have > > > attribute > > > + * modifications occurring from within these transactions. Rather > > > than confuse > > > + * each of these reservation calculations with the conditional > > > attribute > > > + * reservations, add them here in a clear and concise manner. This > > > assumes that > > > + * the attribute reservations have already been calculated. > > > + * > > > + * Note that we only include the static attribute reservation > > > here; the runtime > > > + * reservation will have to be modified by the size of the > > > attributes being > > > + * added/removed/modified. See the comments on the attribute > > > reservation > > > + * calculations for more details. > > > + */ > > > +STATIC void > > > +xfs_calc_namespace_reservations( > > > + struct xfs_mount *mp, > > > + struct xfs_trans_resv *resp) > > > +{ > > > + ASSERT(resp->tr_attrsetm.tr_logres > 0); > > > > > > resp->tr_rename.tr_logres = > > > xfs_calc_rename_reservation(mp); > > > resp->tr_rename.tr_logcount = XFS_RENAME_LOG_COUNT; > > > @@ -948,15 +1001,37 @@ xfs_trans_resv_calc( > > > resp->tr_create.tr_logcount = XFS_CREATE_LOG_COUNT; > > > resp->tr_create.tr_logflags |= XFS_TRANS_PERM_LOG_RES; > > > > > > + resp->tr_mkdir.tr_logres = xfs_calc_mkdir_reservation(mp); > > > + resp->tr_mkdir.tr_logcount = XFS_MKDIR_LOG_COUNT; > > > + resp->tr_mkdir.tr_logflags |= XFS_TRANS_PERM_LOG_RES; > > > + > > > + xfs_calc_parent_ptr_reservations(mp); > > > +} > > > + > > > +void > > > +xfs_trans_resv_calc( > > > + struct xfs_mount *mp, > > > + struct xfs_trans_resv *resp) > > > +{ > > > + int logcount_adj = 0; > > > + > > > + /* > > > + * The following transactions are logged in physical format > > > and > > > + * require a permanent reservation on space. > > > + */ > > > + resp->tr_write.tr_logres = xfs_calc_write_reservation(mp, > > > false); > > > + resp->tr_write.tr_logcount = XFS_WRITE_LOG_COUNT; > > > + resp->tr_write.tr_logflags |= XFS_TRANS_PERM_LOG_RES; > > > + > > > + resp->tr_itruncate.tr_logres = > > > xfs_calc_itruncate_reservation(mp, false); > > > + resp->tr_itruncate.tr_logcount = XFS_ITRUNCATE_LOG_COUNT; > > > + resp->tr_itruncate.tr_logflags |= XFS_TRANS_PERM_LOG_RES; > > > + > > > resp->tr_create_tmpfile.tr_logres = > > > xfs_calc_create_tmpfile_reservation(mp); > > > resp->tr_create_tmpfile.tr_logcount = > > > XFS_CREATE_TMPFILE_LOG_COUNT; > > > resp->tr_create_tmpfile.tr_logflags |= > > > XFS_TRANS_PERM_LOG_RES; > > > > > > - resp->tr_mkdir.tr_logres = xfs_calc_mkdir_reservation(mp); > > > - resp->tr_mkdir.tr_logcount = XFS_MKDIR_LOG_COUNT; > > > - resp->tr_mkdir.tr_logflags |= XFS_TRANS_PERM_LOG_RES; > > > - > > > resp->tr_ifree.tr_logres = xfs_calc_ifree_reservation(mp); > > > resp->tr_ifree.tr_logcount = XFS_INACTIVE_LOG_COUNT; > > > resp->tr_ifree.tr_logflags |= XFS_TRANS_PERM_LOG_RES; > > > @@ -986,6 +1061,8 @@ xfs_trans_resv_calc( > > > resp->tr_qm_dqalloc.tr_logcount = XFS_WRITE_LOG_COUNT; > > > resp->tr_qm_dqalloc.tr_logflags |= XFS_TRANS_PERM_LOG_RES; > > > > > > + xfs_calc_namespace_reservations(mp, resp); > > > + > > > /* > > > * The following transactions are logged in logical format > > > with > > > * a default log count. > > > -- > > > 2.25.1 > > > >
On Mon, 2022-09-26 at 16:53 -0700, Darrick J. Wong wrote: > On Fri, Sep 23, 2022 at 11:53:22PM +0000, Allison Henderson wrote: > > On Fri, 2022-09-23 at 13:17 -0700, Darrick J. Wong wrote: > > > On Wed, Sep 21, 2022 at 10:44:45PM -0700, > > > allison.henderson@oracle.com wrote: > > > > From: Allison Henderson <allison.henderson@oracle.com> > > > > > > > > We need to add, remove or modify parent pointer attributes > > > > during > > > > create/link/unlink/rename operations atomically with the > > > > dirents in > > > > the > > > > parent directories being modified. This means they need to be > > > > modified > > > > in the same transaction as the parent directories, and so we > > > > need > > > > to add > > > > the required space for the attribute modifications to the > > > > transaction > > > > reservations. > > > > > > > > Signed-off-by: Dave Chinner <dchinner@redhat.com> > > > > Signed-off-by: Allison Henderson <allison.henderson@oracle.com> > > > > --- > > > > fs/xfs/libxfs/xfs_trans_resv.c | 135 > > > > ++++++++++++++++++++++++++--- > > > > ---- > > > > 1 file changed, 106 insertions(+), 29 deletions(-) > > > > > > > > diff --git a/fs/xfs/libxfs/xfs_trans_resv.c > > > > b/fs/xfs/libxfs/xfs_trans_resv.c > > > > index 2c4ad6e4bb14..f7799800d556 100644 > > > > --- a/fs/xfs/libxfs/xfs_trans_resv.c > > > > +++ b/fs/xfs/libxfs/xfs_trans_resv.c > > > > @@ -19,6 +19,7 @@ > > > > #include "xfs_trans.h" > > > > #include "xfs_qm.h" > > > > #include "xfs_trans_space.h" > > > > +#include "xfs_attr_item.h" > > > > > > > > #define _ALLOC true > > > > #define _FREE false > > > > @@ -421,28 +422,45 @@ > > > > xfs_calc_itruncate_reservation_minlogsize( > > > > } > > > > > > > > /* > > > > - * In renaming a files we can modify: > > > > - * the four inodes involved: 4 * inode size > > > > + * In renaming a files we can modify (t1): > > > > + * the four inodes involved: 5 * inode size > > > > > > ...the *five* inodes involved... > > > > > > Also -- even before parent pointers we could have five inodes > > > involved > > > in a rename transaction, so I think this change needs to be a > > > separate > > > bugfix at the start of the series. Rename isn't experimental, so > > > I > > > can't let this one slide. :/ > > I see, ok I will split this one out then > > > > > > > > > * the two directory btrees: 2 * (max depth + v2) * dir > > > > block > > > > size > > > > * the two directory bmap btrees: 2 * max depth * block > > > > size > > > > * And the bmap_finish transaction can free dir and bmap > > > > blocks > > > > (two sets > > > > - * of bmap blocks) giving: > > > > + * of bmap blocks) giving (t2): > > > > * the agf for the ags in which the blocks live: 3 * sector > > > > size > > > > * the agfl for the ags in which the blocks live: 3 * > > > > sector > > > > size > > > > * the superblock for the free block count: sector size > > > > * the allocation btrees: 3 exts * 2 trees * (2 * max depth > > > > - > > > > 1) * block size > > > > + * If parent pointers are enabled (t3), then each transaction > > > > in > > > > the chain > > > > + * must be capable of setting or removing the extended > > > > attribute > > > > + * containing the parent information. It must also be able > > > > to > > > > handle > > > > + * the three xattr intent items that track the progress of > > > > the > > > > parent > > > > + * pointer update. > > > > */ > > > > STATIC uint > > > > xfs_calc_rename_reservation( > > > > struct xfs_mount *mp) > > > > { > > > > - return XFS_DQUOT_LOGRES(mp) + > > > > - max((xfs_calc_inode_res(mp, 4) + > > > > - xfs_calc_buf_res(2 * > > > > XFS_DIROP_LOG_COUNT(mp), > > > > - XFS_FSB_TO_B(mp, 1))), > > > > - (xfs_calc_buf_res(7, mp->m_sb.sb_sectsize) > > > > + > > > > - > > > > xfs_calc_buf_res(xfs_allocfree_block_count(mp, > > > > 3), > > > > - XFS_FSB_TO_B(mp, 1)))); > > > > + unsigned int overhead = > > > > XFS_DQUOT_LOGRES(mp); > > > > + struct xfs_trans_resv *resp = M_RES(mp); > > > > + unsigned int t1, t2, t3 = 0; > > > > + > > > > + t1 = xfs_calc_inode_res(mp, 5) + > > > > + xfs_calc_buf_res(2 * XFS_DIROP_LOG_COUNT(mp), > > > > + XFS_FSB_TO_B(mp, 1)); > > > > + > > > > + t2 = xfs_calc_buf_res(7, mp->m_sb.sb_sectsize) + > > > > + xfs_calc_buf_res(xfs_allocfree_block_count(mp, 3), > > > > + XFS_FSB_TO_B(mp, 1)); > > > > + > > > > + if (xfs_has_parent(mp)) { > > > > + t3 = max(resp->tr_attrsetm.tr_logres, > > > > + resp->tr_attrrm.tr_logres); > > > > > > Ooh I like this refactoring of xfs_calc_rename_reservation. :) > > > > > > I guess we now tr_attr{setm,rm} before computing the rename > > > reservation > > > so this is ok. > > > > > > > + overhead += 3 * (sizeof(struct > > > > xfs_attri_log_item)); > > > > > > Should the size of the name, newname, and value buffers be added > > > into > > > overhead? They take up log space too. > > That would make sense, but we cant really calculate that ahead of > > time > > with out just assuming the max size which is up to 64k for the > > value. > > Maybe those sizes should be added on after we know what they are? > > They have to be worst case values, but fortunately we know what the > worst case is: > > name: sizeof(ondisk parent ptr structure) + iovec overhead > newname: same for rename, and zero everywhere else iiuc? > value: 255 + iovec_overhead > Ok, I'll see if I can plumb those through xfs_trans_alloc > > > > > > > + } > > > > + > > > > + return overhead + max3(t1, t2, t3); > > > > } > > > > > > > > /* > > > > @@ -909,24 +927,59 @@ xfs_calc_sb_reservation( > > > > return xfs_calc_buf_res(1, mp->m_sb.sb_sectsize); > > > > } > > > > > > > > -void > > > > -xfs_trans_resv_calc( > > > > - struct xfs_mount *mp, > > > > - struct xfs_trans_resv *resp) > > > > +/* > > > > + * Calculate extra space needed for parent pointer attributes > > > > + */ > > > > +STATIC void > > > > +xfs_calc_parent_ptr_reservations( > > > > + struct xfs_mount *mp) > > > > { > > > > - int logcount_adj = 0; > > > > + struct xfs_trans_resv *resp = M_RES(mp); > > > > > > > > - /* > > > > - * The following transactions are logged in physical > > > > format > > > > and > > > > - * require a permanent reservation on space. > > > > - */ > > > > - resp->tr_write.tr_logres = > > > > xfs_calc_write_reservation(mp, > > > > false); > > > > - resp->tr_write.tr_logcount = XFS_WRITE_LOG_COUNT; > > > > - resp->tr_write.tr_logflags |= XFS_TRANS_PERM_LOG_RES; > > > > + if (!xfs_has_parent(mp)) > > > > + return; > > > > > > > > - resp->tr_itruncate.tr_logres = > > > > xfs_calc_itruncate_reservation(mp, false); > > > > - resp->tr_itruncate.tr_logcount = > > > > XFS_ITRUNCATE_LOG_COUNT; > > > > - resp->tr_itruncate.tr_logflags |= > > > > XFS_TRANS_PERM_LOG_RES; > > > > + resp->tr_rename.tr_logres += max(resp- > > > > > tr_attrsetm.tr_logres, > > > > + resp- > > > > > tr_attrrm.tr_logres); > > > > + resp->tr_rename.tr_logcount += max(resp- > > > > > tr_attrsetm.tr_logcount, > > > > + resp- > > > > > tr_attrrm.tr_logcount); > > > > > > Doesn't xfs_calc_rename_reservation add this to tr_rename > > > already? > > Oh, I think we can remove the tr_rename.tr_logres update. But not > > the > > logcount update right? > > Right, though you could update XFS_RENAME_LOG_COUNT (or turn it into > a > helper function). Hmm, updating the #define would affect non pptr code paths, and it seems a bit small for a helper. I may leave the log count here if it doesnt bother folks, I dont think it's out of place along with the other logcount updates. > > > xfs_calc_rename_reservation just calculates what > > tr_rename.tr_logres > > should be, but it doesnt make any additions. It's that's used over > > in > > xfs_calc_namespace_reservations, but we still need to update > > tr_rename.tr_logcount which is separate field from > > tr_rename.tr_logres. > > > > > > > > > > > + > > > > + resp->tr_create.tr_logres += resp- > > > > >tr_attrsetm.tr_logres; > > > > + resp->tr_create.tr_logcount += resp- > > > > > tr_attrsetm.tr_logcount; > > > > + > > > > + resp->tr_mkdir.tr_logres += resp- > > > > >tr_attrsetm.tr_logres; > > > > + resp->tr_mkdir.tr_logcount += resp- > > > > > tr_attrsetm.tr_logcount; > > > > + > > > > + resp->tr_link.tr_logres += resp->tr_attrsetm.tr_logres; > > > > + resp->tr_link.tr_logcount += resp- > > > > >tr_attrsetm.tr_logcount; > > > > + > > > > + resp->tr_symlink.tr_logres += resp- > > > > >tr_attrsetm.tr_logres; > > > > + resp->tr_symlink.tr_logcount += resp- > > > > > tr_attrsetm.tr_logcount; > > > > + > > > > + resp->tr_remove.tr_logres += resp->tr_attrrm.tr_logres; > > > > + resp->tr_remove.tr_logcount += resp- > > > > >tr_attrrm.tr_logcount; > > > > > > Shouldn't each of these += additions be made to > > > xfs_calc_{icreate,mkdir,link,symlink,remove}_reservation, > > > respectively? > > > > > > > I suppose we could redo it that way? But then not all of the "+=" > > would disappear if they worked like xfs_calc_rename_reservation. > > Just > > the *.tr_logres. Do we really want separate wrappers for just > > these > > oneliners though? > > Yes, once those macros start acquiring logic, they ought to turn into > static inline helpers. Ok, I will split those off as needed when I go through the xfs_trans_alloc modifications then. Thanks! > > --D > > > Allison > > > > > --D > > > > > > > +} > > > > + > > > > +/* > > > > + * Namespace reservations. > > > > + * > > > > + * These get tricky when parent pointers are enabled as we > > > > have > > > > attribute > > > > + * modifications occurring from within these transactions. > > > > Rather > > > > than confuse > > > > + * each of these reservation calculations with the conditional > > > > attribute > > > > + * reservations, add them here in a clear and concise manner. > > > > This > > > > assumes that > > > > + * the attribute reservations have already been calculated. > > > > + * > > > > + * Note that we only include the static attribute reservation > > > > here; the runtime > > > > + * reservation will have to be modified by the size of the > > > > attributes being > > > > + * added/removed/modified. See the comments on the attribute > > > > reservation > > > > + * calculations for more details. > > > > + */ > > > > +STATIC void > > > > +xfs_calc_namespace_reservations( > > > > + struct xfs_mount *mp, > > > > + struct xfs_trans_resv *resp) > > > > +{ > > > > + ASSERT(resp->tr_attrsetm.tr_logres > 0); > > > > > > > > resp->tr_rename.tr_logres = > > > > xfs_calc_rename_reservation(mp); > > > > resp->tr_rename.tr_logcount = XFS_RENAME_LOG_COUNT; > > > > @@ -948,15 +1001,37 @@ xfs_trans_resv_calc( > > > > resp->tr_create.tr_logcount = XFS_CREATE_LOG_COUNT; > > > > resp->tr_create.tr_logflags |= XFS_TRANS_PERM_LOG_RES; > > > > > > > > + resp->tr_mkdir.tr_logres = > > > > xfs_calc_mkdir_reservation(mp); > > > > + resp->tr_mkdir.tr_logcount = XFS_MKDIR_LOG_COUNT; > > > > + resp->tr_mkdir.tr_logflags |= XFS_TRANS_PERM_LOG_RES; > > > > + > > > > + xfs_calc_parent_ptr_reservations(mp); > > > > +} > > > > + > > > > +void > > > > +xfs_trans_resv_calc( > > > > + struct xfs_mount *mp, > > > > + struct xfs_trans_resv *resp) > > > > +{ > > > > + int logcount_adj = 0; > > > > + > > > > + /* > > > > + * The following transactions are logged in physical > > > > format > > > > and > > > > + * require a permanent reservation on space. > > > > + */ > > > > + resp->tr_write.tr_logres = > > > > xfs_calc_write_reservation(mp, > > > > false); > > > > + resp->tr_write.tr_logcount = XFS_WRITE_LOG_COUNT; > > > > + resp->tr_write.tr_logflags |= XFS_TRANS_PERM_LOG_RES; > > > > + > > > > + resp->tr_itruncate.tr_logres = > > > > xfs_calc_itruncate_reservation(mp, false); > > > > + resp->tr_itruncate.tr_logcount = > > > > XFS_ITRUNCATE_LOG_COUNT; > > > > + resp->tr_itruncate.tr_logflags |= > > > > XFS_TRANS_PERM_LOG_RES; > > > > + > > > > resp->tr_create_tmpfile.tr_logres = > > > > xfs_calc_create_tmpfile_reservation(mp) > > > > ; > > > > resp->tr_create_tmpfile.tr_logcount = > > > > XFS_CREATE_TMPFILE_LOG_COUNT; > > > > resp->tr_create_tmpfile.tr_logflags |= > > > > XFS_TRANS_PERM_LOG_RES; > > > > > > > > - resp->tr_mkdir.tr_logres = > > > > xfs_calc_mkdir_reservation(mp); > > > > - resp->tr_mkdir.tr_logcount = XFS_MKDIR_LOG_COUNT; > > > > - resp->tr_mkdir.tr_logflags |= XFS_TRANS_PERM_LOG_RES; > > > > - > > > > resp->tr_ifree.tr_logres = > > > > xfs_calc_ifree_reservation(mp); > > > > resp->tr_ifree.tr_logcount = XFS_INACTIVE_LOG_COUNT; > > > > resp->tr_ifree.tr_logflags |= XFS_TRANS_PERM_LOG_RES; > > > > @@ -986,6 +1061,8 @@ xfs_trans_resv_calc( > > > > resp->tr_qm_dqalloc.tr_logcount = XFS_WRITE_LOG_COUNT; > > > > resp->tr_qm_dqalloc.tr_logflags |= > > > > XFS_TRANS_PERM_LOG_RES; > > > > > > > > + xfs_calc_namespace_reservations(mp, resp); > > > > + > > > > /* > > > > * The following transactions are logged in logical > > > > format > > > > with > > > > * a default log count. > > > > -- > > > > 2.25.1 > > > > > >
On Tue, Sep 27, 2022 at 08:04:06PM +0000, Allison Henderson wrote: > On Mon, 2022-09-26 at 16:53 -0700, Darrick J. Wong wrote: > > On Fri, Sep 23, 2022 at 11:53:22PM +0000, Allison Henderson wrote: > > > On Fri, 2022-09-23 at 13:17 -0700, Darrick J. Wong wrote: > > > > On Wed, Sep 21, 2022 at 10:44:45PM -0700, > > > > allison.henderson@oracle.com wrote: > > > > > From: Allison Henderson <allison.henderson@oracle.com> > > > > > > > > > > We need to add, remove or modify parent pointer attributes > > > > > during > > > > > create/link/unlink/rename operations atomically with the > > > > > dirents in > > > > > the > > > > > parent directories being modified. This means they need to be > > > > > modified > > > > > in the same transaction as the parent directories, and so we > > > > > need > > > > > to add > > > > > the required space for the attribute modifications to the > > > > > transaction > > > > > reservations. > > > > > > > > > > Signed-off-by: Dave Chinner <dchinner@redhat.com> > > > > > Signed-off-by: Allison Henderson <allison.henderson@oracle.com> > > > > > --- > > > > > fs/xfs/libxfs/xfs_trans_resv.c | 135 > > > > > ++++++++++++++++++++++++++--- > > > > > ---- > > > > > 1 file changed, 106 insertions(+), 29 deletions(-) > > > > > > > > > > diff --git a/fs/xfs/libxfs/xfs_trans_resv.c > > > > > b/fs/xfs/libxfs/xfs_trans_resv.c > > > > > index 2c4ad6e4bb14..f7799800d556 100644 > > > > > --- a/fs/xfs/libxfs/xfs_trans_resv.c > > > > > +++ b/fs/xfs/libxfs/xfs_trans_resv.c > > > > > @@ -19,6 +19,7 @@ > > > > > #include "xfs_trans.h" > > > > > #include "xfs_qm.h" > > > > > #include "xfs_trans_space.h" > > > > > +#include "xfs_attr_item.h" > > > > > > > > > > #define _ALLOC true > > > > > #define _FREE false > > > > > @@ -421,28 +422,45 @@ > > > > > xfs_calc_itruncate_reservation_minlogsize( > > > > > } > > > > > > > > > > /* > > > > > - * In renaming a files we can modify: > > > > > - * the four inodes involved: 4 * inode size > > > > > + * In renaming a files we can modify (t1): > > > > > + * the four inodes involved: 5 * inode size > > > > > > > > ...the *five* inodes involved... > > > > > > > > Also -- even before parent pointers we could have five inodes > > > > involved > > > > in a rename transaction, so I think this change needs to be a > > > > separate > > > > bugfix at the start of the series. Rename isn't experimental, so > > > > I > > > > can't let this one slide. :/ > > > I see, ok I will split this one out then > > > > > > > > > > > > * the two directory btrees: 2 * (max depth + v2) * dir > > > > > block > > > > > size > > > > > * the two directory bmap btrees: 2 * max depth * block > > > > > size > > > > > * And the bmap_finish transaction can free dir and bmap > > > > > blocks > > > > > (two sets > > > > > - * of bmap blocks) giving: > > > > > + * of bmap blocks) giving (t2): > > > > > * the agf for the ags in which the blocks live: 3 * sector > > > > > size > > > > > * the agfl for the ags in which the blocks live: 3 * > > > > > sector > > > > > size > > > > > * the superblock for the free block count: sector size > > > > > * the allocation btrees: 3 exts * 2 trees * (2 * max depth > > > > > - > > > > > 1) * block size > > > > > + * If parent pointers are enabled (t3), then each transaction > > > > > in > > > > > the chain > > > > > + * must be capable of setting or removing the extended > > > > > attribute > > > > > + * containing the parent information. It must also be able > > > > > to > > > > > handle > > > > > + * the three xattr intent items that track the progress of > > > > > the > > > > > parent > > > > > + * pointer update. > > > > > */ > > > > > STATIC uint > > > > > xfs_calc_rename_reservation( > > > > > struct xfs_mount *mp) > > > > > { > > > > > - return XFS_DQUOT_LOGRES(mp) + > > > > > - max((xfs_calc_inode_res(mp, 4) + > > > > > - xfs_calc_buf_res(2 * > > > > > XFS_DIROP_LOG_COUNT(mp), > > > > > - XFS_FSB_TO_B(mp, 1))), > > > > > - (xfs_calc_buf_res(7, mp->m_sb.sb_sectsize) > > > > > + > > > > > - > > > > > xfs_calc_buf_res(xfs_allocfree_block_count(mp, > > > > > 3), > > > > > - XFS_FSB_TO_B(mp, 1)))); > > > > > + unsigned int overhead = > > > > > XFS_DQUOT_LOGRES(mp); > > > > > + struct xfs_trans_resv *resp = M_RES(mp); > > > > > + unsigned int t1, t2, t3 = 0; > > > > > + > > > > > + t1 = xfs_calc_inode_res(mp, 5) + > > > > > + xfs_calc_buf_res(2 * XFS_DIROP_LOG_COUNT(mp), > > > > > + XFS_FSB_TO_B(mp, 1)); > > > > > + > > > > > + t2 = xfs_calc_buf_res(7, mp->m_sb.sb_sectsize) + > > > > > + xfs_calc_buf_res(xfs_allocfree_block_count(mp, 3), > > > > > + XFS_FSB_TO_B(mp, 1)); > > > > > + > > > > > + if (xfs_has_parent(mp)) { > > > > > + t3 = max(resp->tr_attrsetm.tr_logres, > > > > > + resp->tr_attrrm.tr_logres); > > > > > > > > Ooh I like this refactoring of xfs_calc_rename_reservation. :) > > > > > > > > I guess we now tr_attr{setm,rm} before computing the rename > > > > reservation > > > > so this is ok. > > > > > > > > > + overhead += 3 * (sizeof(struct > > > > > xfs_attri_log_item)); > > > > > > > > Should the size of the name, newname, and value buffers be added > > > > into > > > > overhead? They take up log space too. > > > That would make sense, but we cant really calculate that ahead of > > > time > > > with out just assuming the max size which is up to 64k for the > > > value. > > > Maybe those sizes should be added on after we know what they are? > > > > They have to be worst case values, but fortunately we know what the > > worst case is: > > > > name: sizeof(ondisk parent ptr structure) + iovec overhead > > newname: same for rename, and zero everywhere else iiuc? > > value: 255 + iovec_overhead > > > Ok, I'll see if I can plumb those through xfs_trans_alloc > > > > > > > > > > + } > > > > > + > > > > > + return overhead + max3(t1, t2, t3); > > > > > } > > > > > > > > > > /* > > > > > @@ -909,24 +927,59 @@ xfs_calc_sb_reservation( > > > > > return xfs_calc_buf_res(1, mp->m_sb.sb_sectsize); > > > > > } > > > > > > > > > > -void > > > > > -xfs_trans_resv_calc( > > > > > - struct xfs_mount *mp, > > > > > - struct xfs_trans_resv *resp) > > > > > +/* > > > > > + * Calculate extra space needed for parent pointer attributes > > > > > + */ > > > > > +STATIC void > > > > > +xfs_calc_parent_ptr_reservations( > > > > > + struct xfs_mount *mp) > > > > > { > > > > > - int logcount_adj = 0; > > > > > + struct xfs_trans_resv *resp = M_RES(mp); > > > > > > > > > > - /* > > > > > - * The following transactions are logged in physical > > > > > format > > > > > and > > > > > - * require a permanent reservation on space. > > > > > - */ > > > > > - resp->tr_write.tr_logres = > > > > > xfs_calc_write_reservation(mp, > > > > > false); > > > > > - resp->tr_write.tr_logcount = XFS_WRITE_LOG_COUNT; > > > > > - resp->tr_write.tr_logflags |= XFS_TRANS_PERM_LOG_RES; > > > > > + if (!xfs_has_parent(mp)) > > > > > + return; > > > > > > > > > > - resp->tr_itruncate.tr_logres = > > > > > xfs_calc_itruncate_reservation(mp, false); > > > > > - resp->tr_itruncate.tr_logcount = > > > > > XFS_ITRUNCATE_LOG_COUNT; > > > > > - resp->tr_itruncate.tr_logflags |= > > > > > XFS_TRANS_PERM_LOG_RES; > > > > > + resp->tr_rename.tr_logres += max(resp- > > > > > > tr_attrsetm.tr_logres, > > > > > + resp- > > > > > > tr_attrrm.tr_logres); > > > > > + resp->tr_rename.tr_logcount += max(resp- > > > > > > tr_attrsetm.tr_logcount, > > > > > + resp- > > > > > > tr_attrrm.tr_logcount); > > > > > > > > Doesn't xfs_calc_rename_reservation add this to tr_rename > > > > already? > > > Oh, I think we can remove the tr_rename.tr_logres update. But not > > > the > > > logcount update right? > > > > Right, though you could update XFS_RENAME_LOG_COUNT (or turn it into > > a > > helper function). > Hmm, updating the #define would affect non pptr code paths, and it > seems a bit small for a helper. I may leave the log count here if it > doesnt bother folks, I dont think it's out of place along with the > other logcount updates. How do you mean? static inline unsigned int xfs_rename_log_count( struct xfs_mount *mp, struct xfs_trans_resv *resp)) { /* One for the rename, one more for freeing blocks */ unsigned int ret = 2; /* * Pre-reserve enough log reservation to handle the transaction * rolling needed to remove or add one parent pointer. * * XXX: Should we reserve enough space to handle the tx rolls * resulting from removing /and/ adding a pptr? */ if (xfs_has_parent(mp)) ret += max(resp->tr_attrsetm.tr_logcount, resp->tr_attrrm.tr_logcount); return ret; } --D > > > > > > xfs_calc_rename_reservation just calculates what > > > tr_rename.tr_logres > > > should be, but it doesnt make any additions. It's that's used over > > > in > > > xfs_calc_namespace_reservations, but we still need to update > > > tr_rename.tr_logcount which is separate field from > > > tr_rename.tr_logres. > > > > > > > > > > > > > > > + > > > > > + resp->tr_create.tr_logres += resp- > > > > > >tr_attrsetm.tr_logres; > > > > > + resp->tr_create.tr_logcount += resp- > > > > > > tr_attrsetm.tr_logcount; > > > > > + > > > > > + resp->tr_mkdir.tr_logres += resp- > > > > > >tr_attrsetm.tr_logres; > > > > > + resp->tr_mkdir.tr_logcount += resp- > > > > > > tr_attrsetm.tr_logcount; > > > > > + > > > > > + resp->tr_link.tr_logres += resp->tr_attrsetm.tr_logres; > > > > > + resp->tr_link.tr_logcount += resp- > > > > > >tr_attrsetm.tr_logcount; > > > > > + > > > > > + resp->tr_symlink.tr_logres += resp- > > > > > >tr_attrsetm.tr_logres; > > > > > + resp->tr_symlink.tr_logcount += resp- > > > > > > tr_attrsetm.tr_logcount; > > > > > + > > > > > + resp->tr_remove.tr_logres += resp->tr_attrrm.tr_logres; > > > > > + resp->tr_remove.tr_logcount += resp- > > > > > >tr_attrrm.tr_logcount; > > > > > > > > Shouldn't each of these += additions be made to > > > > xfs_calc_{icreate,mkdir,link,symlink,remove}_reservation, > > > > respectively? > > > > > > > > > > I suppose we could redo it that way? But then not all of the "+=" > > > would disappear if they worked like xfs_calc_rename_reservation. > > > Just > > > the *.tr_logres. Do we really want separate wrappers for just > > > these > > > oneliners though? > > > > Yes, once those macros start acquiring logic, they ought to turn into > > static inline helpers. > Ok, I will split those off as needed when I go through the > xfs_trans_alloc modifications then. > > Thanks! > > > > > --D > > > > > Allison > > > > > > > --D > > > > > > > > > +} > > > > > + > > > > > +/* > > > > > + * Namespace reservations. > > > > > + * > > > > > + * These get tricky when parent pointers are enabled as we > > > > > have > > > > > attribute > > > > > + * modifications occurring from within these transactions. > > > > > Rather > > > > > than confuse > > > > > + * each of these reservation calculations with the conditional > > > > > attribute > > > > > + * reservations, add them here in a clear and concise manner. > > > > > This > > > > > assumes that > > > > > + * the attribute reservations have already been calculated. > > > > > + * > > > > > + * Note that we only include the static attribute reservation > > > > > here; the runtime > > > > > + * reservation will have to be modified by the size of the > > > > > attributes being > > > > > + * added/removed/modified. See the comments on the attribute > > > > > reservation > > > > > + * calculations for more details. > > > > > + */ > > > > > +STATIC void > > > > > +xfs_calc_namespace_reservations( > > > > > + struct xfs_mount *mp, > > > > > + struct xfs_trans_resv *resp) > > > > > +{ > > > > > + ASSERT(resp->tr_attrsetm.tr_logres > 0); > > > > > > > > > > resp->tr_rename.tr_logres = > > > > > xfs_calc_rename_reservation(mp); > > > > > resp->tr_rename.tr_logcount = XFS_RENAME_LOG_COUNT; > > > > > @@ -948,15 +1001,37 @@ xfs_trans_resv_calc( > > > > > resp->tr_create.tr_logcount = XFS_CREATE_LOG_COUNT; > > > > > resp->tr_create.tr_logflags |= XFS_TRANS_PERM_LOG_RES; > > > > > > > > > > + resp->tr_mkdir.tr_logres = > > > > > xfs_calc_mkdir_reservation(mp); > > > > > + resp->tr_mkdir.tr_logcount = XFS_MKDIR_LOG_COUNT; > > > > > + resp->tr_mkdir.tr_logflags |= XFS_TRANS_PERM_LOG_RES; > > > > > + > > > > > + xfs_calc_parent_ptr_reservations(mp); > > > > > +} > > > > > + > > > > > +void > > > > > +xfs_trans_resv_calc( > > > > > + struct xfs_mount *mp, > > > > > + struct xfs_trans_resv *resp) > > > > > +{ > > > > > + int logcount_adj = 0; > > > > > + > > > > > + /* > > > > > + * The following transactions are logged in physical > > > > > format > > > > > and > > > > > + * require a permanent reservation on space. > > > > > + */ > > > > > + resp->tr_write.tr_logres = > > > > > xfs_calc_write_reservation(mp, > > > > > false); > > > > > + resp->tr_write.tr_logcount = XFS_WRITE_LOG_COUNT; > > > > > + resp->tr_write.tr_logflags |= XFS_TRANS_PERM_LOG_RES; > > > > > + > > > > > + resp->tr_itruncate.tr_logres = > > > > > xfs_calc_itruncate_reservation(mp, false); > > > > > + resp->tr_itruncate.tr_logcount = > > > > > XFS_ITRUNCATE_LOG_COUNT; > > > > > + resp->tr_itruncate.tr_logflags |= > > > > > XFS_TRANS_PERM_LOG_RES; > > > > > + > > > > > resp->tr_create_tmpfile.tr_logres = > > > > > xfs_calc_create_tmpfile_reservation(mp) > > > > > ; > > > > > resp->tr_create_tmpfile.tr_logcount = > > > > > XFS_CREATE_TMPFILE_LOG_COUNT; > > > > > resp->tr_create_tmpfile.tr_logflags |= > > > > > XFS_TRANS_PERM_LOG_RES; > > > > > > > > > > - resp->tr_mkdir.tr_logres = > > > > > xfs_calc_mkdir_reservation(mp); > > > > > - resp->tr_mkdir.tr_logcount = XFS_MKDIR_LOG_COUNT; > > > > > - resp->tr_mkdir.tr_logflags |= XFS_TRANS_PERM_LOG_RES; > > > > > - > > > > > resp->tr_ifree.tr_logres = > > > > > xfs_calc_ifree_reservation(mp); > > > > > resp->tr_ifree.tr_logcount = XFS_INACTIVE_LOG_COUNT; > > > > > resp->tr_ifree.tr_logflags |= XFS_TRANS_PERM_LOG_RES; > > > > > @@ -986,6 +1061,8 @@ xfs_trans_resv_calc( > > > > > resp->tr_qm_dqalloc.tr_logcount = XFS_WRITE_LOG_COUNT; > > > > > resp->tr_qm_dqalloc.tr_logflags |= > > > > > XFS_TRANS_PERM_LOG_RES; > > > > > > > > > > + xfs_calc_namespace_reservations(mp, resp); > > > > > + > > > > > /* > > > > > * The following transactions are logged in logical > > > > > format > > > > > with > > > > > * a default log count. > > > > > -- > > > > > 2.25.1 > > > > > > > > >
diff --git a/fs/xfs/libxfs/xfs_trans_resv.c b/fs/xfs/libxfs/xfs_trans_resv.c index 2c4ad6e4bb14..f7799800d556 100644 --- a/fs/xfs/libxfs/xfs_trans_resv.c +++ b/fs/xfs/libxfs/xfs_trans_resv.c @@ -19,6 +19,7 @@ #include "xfs_trans.h" #include "xfs_qm.h" #include "xfs_trans_space.h" +#include "xfs_attr_item.h" #define _ALLOC true #define _FREE false @@ -421,28 +422,45 @@ xfs_calc_itruncate_reservation_minlogsize( } /* - * In renaming a files we can modify: - * the four inodes involved: 4 * inode size + * In renaming a files we can modify (t1): + * the four inodes involved: 5 * inode size * the two directory btrees: 2 * (max depth + v2) * dir block size * the two directory bmap btrees: 2 * max depth * block size * And the bmap_finish transaction can free dir and bmap blocks (two sets - * of bmap blocks) giving: + * of bmap blocks) giving (t2): * the agf for the ags in which the blocks live: 3 * sector size * the agfl for the ags in which the blocks live: 3 * sector size * the superblock for the free block count: sector size * the allocation btrees: 3 exts * 2 trees * (2 * max depth - 1) * block size + * If parent pointers are enabled (t3), then each transaction in the chain + * must be capable of setting or removing the extended attribute + * containing the parent information. It must also be able to handle + * the three xattr intent items that track the progress of the parent + * pointer update. */ STATIC uint xfs_calc_rename_reservation( struct xfs_mount *mp) { - return XFS_DQUOT_LOGRES(mp) + - max((xfs_calc_inode_res(mp, 4) + - xfs_calc_buf_res(2 * XFS_DIROP_LOG_COUNT(mp), - XFS_FSB_TO_B(mp, 1))), - (xfs_calc_buf_res(7, mp->m_sb.sb_sectsize) + - xfs_calc_buf_res(xfs_allocfree_block_count(mp, 3), - XFS_FSB_TO_B(mp, 1)))); + unsigned int overhead = XFS_DQUOT_LOGRES(mp); + struct xfs_trans_resv *resp = M_RES(mp); + unsigned int t1, t2, t3 = 0; + + t1 = xfs_calc_inode_res(mp, 5) + + xfs_calc_buf_res(2 * XFS_DIROP_LOG_COUNT(mp), + XFS_FSB_TO_B(mp, 1)); + + t2 = xfs_calc_buf_res(7, mp->m_sb.sb_sectsize) + + xfs_calc_buf_res(xfs_allocfree_block_count(mp, 3), + XFS_FSB_TO_B(mp, 1)); + + if (xfs_has_parent(mp)) { + t3 = max(resp->tr_attrsetm.tr_logres, + resp->tr_attrrm.tr_logres); + overhead += 3 * (sizeof(struct xfs_attri_log_item)); + } + + return overhead + max3(t1, t2, t3); } /* @@ -909,24 +927,59 @@ xfs_calc_sb_reservation( return xfs_calc_buf_res(1, mp->m_sb.sb_sectsize); } -void -xfs_trans_resv_calc( - struct xfs_mount *mp, - struct xfs_trans_resv *resp) +/* + * Calculate extra space needed for parent pointer attributes + */ +STATIC void +xfs_calc_parent_ptr_reservations( + struct xfs_mount *mp) { - int logcount_adj = 0; + struct xfs_trans_resv *resp = M_RES(mp); - /* - * The following transactions are logged in physical format and - * require a permanent reservation on space. - */ - resp->tr_write.tr_logres = xfs_calc_write_reservation(mp, false); - resp->tr_write.tr_logcount = XFS_WRITE_LOG_COUNT; - resp->tr_write.tr_logflags |= XFS_TRANS_PERM_LOG_RES; + if (!xfs_has_parent(mp)) + return; - resp->tr_itruncate.tr_logres = xfs_calc_itruncate_reservation(mp, false); - resp->tr_itruncate.tr_logcount = XFS_ITRUNCATE_LOG_COUNT; - resp->tr_itruncate.tr_logflags |= XFS_TRANS_PERM_LOG_RES; + resp->tr_rename.tr_logres += max(resp->tr_attrsetm.tr_logres, + resp->tr_attrrm.tr_logres); + resp->tr_rename.tr_logcount += max(resp->tr_attrsetm.tr_logcount, + resp->tr_attrrm.tr_logcount); + + resp->tr_create.tr_logres += resp->tr_attrsetm.tr_logres; + resp->tr_create.tr_logcount += resp->tr_attrsetm.tr_logcount; + + resp->tr_mkdir.tr_logres += resp->tr_attrsetm.tr_logres; + resp->tr_mkdir.tr_logcount += resp->tr_attrsetm.tr_logcount; + + resp->tr_link.tr_logres += resp->tr_attrsetm.tr_logres; + resp->tr_link.tr_logcount += resp->tr_attrsetm.tr_logcount; + + resp->tr_symlink.tr_logres += resp->tr_attrsetm.tr_logres; + resp->tr_symlink.tr_logcount += resp->tr_attrsetm.tr_logcount; + + resp->tr_remove.tr_logres += resp->tr_attrrm.tr_logres; + resp->tr_remove.tr_logcount += resp->tr_attrrm.tr_logcount; +} + +/* + * Namespace reservations. + * + * These get tricky when parent pointers are enabled as we have attribute + * modifications occurring from within these transactions. Rather than confuse + * each of these reservation calculations with the conditional attribute + * reservations, add them here in a clear and concise manner. This assumes that + * the attribute reservations have already been calculated. + * + * Note that we only include the static attribute reservation here; the runtime + * reservation will have to be modified by the size of the attributes being + * added/removed/modified. See the comments on the attribute reservation + * calculations for more details. + */ +STATIC void +xfs_calc_namespace_reservations( + struct xfs_mount *mp, + struct xfs_trans_resv *resp) +{ + ASSERT(resp->tr_attrsetm.tr_logres > 0); resp->tr_rename.tr_logres = xfs_calc_rename_reservation(mp); resp->tr_rename.tr_logcount = XFS_RENAME_LOG_COUNT; @@ -948,15 +1001,37 @@ xfs_trans_resv_calc( resp->tr_create.tr_logcount = XFS_CREATE_LOG_COUNT; resp->tr_create.tr_logflags |= XFS_TRANS_PERM_LOG_RES; + resp->tr_mkdir.tr_logres = xfs_calc_mkdir_reservation(mp); + resp->tr_mkdir.tr_logcount = XFS_MKDIR_LOG_COUNT; + resp->tr_mkdir.tr_logflags |= XFS_TRANS_PERM_LOG_RES; + + xfs_calc_parent_ptr_reservations(mp); +} + +void +xfs_trans_resv_calc( + struct xfs_mount *mp, + struct xfs_trans_resv *resp) +{ + int logcount_adj = 0; + + /* + * The following transactions are logged in physical format and + * require a permanent reservation on space. + */ + resp->tr_write.tr_logres = xfs_calc_write_reservation(mp, false); + resp->tr_write.tr_logcount = XFS_WRITE_LOG_COUNT; + resp->tr_write.tr_logflags |= XFS_TRANS_PERM_LOG_RES; + + resp->tr_itruncate.tr_logres = xfs_calc_itruncate_reservation(mp, false); + resp->tr_itruncate.tr_logcount = XFS_ITRUNCATE_LOG_COUNT; + resp->tr_itruncate.tr_logflags |= XFS_TRANS_PERM_LOG_RES; + resp->tr_create_tmpfile.tr_logres = xfs_calc_create_tmpfile_reservation(mp); resp->tr_create_tmpfile.tr_logcount = XFS_CREATE_TMPFILE_LOG_COUNT; resp->tr_create_tmpfile.tr_logflags |= XFS_TRANS_PERM_LOG_RES; - resp->tr_mkdir.tr_logres = xfs_calc_mkdir_reservation(mp); - resp->tr_mkdir.tr_logcount = XFS_MKDIR_LOG_COUNT; - resp->tr_mkdir.tr_logflags |= XFS_TRANS_PERM_LOG_RES; - resp->tr_ifree.tr_logres = xfs_calc_ifree_reservation(mp); resp->tr_ifree.tr_logcount = XFS_INACTIVE_LOG_COUNT; resp->tr_ifree.tr_logflags |= XFS_TRANS_PERM_LOG_RES; @@ -986,6 +1061,8 @@ xfs_trans_resv_calc( resp->tr_qm_dqalloc.tr_logcount = XFS_WRITE_LOG_COUNT; resp->tr_qm_dqalloc.tr_logflags |= XFS_TRANS_PERM_LOG_RES; + xfs_calc_namespace_reservations(mp, resp); + /* * The following transactions are logged in logical format with * a default log count.