| Message ID | 20180730164520.36882-3-bfoster@redhat.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | xfs: condense dfops and automatic relogging | expand |
On Mon, Jul 30, 2018 at 12:45:07PM -0400, Brian Foster wrote: > Log intent recovery is the last user of an external (on-stack) > dfops. The pattern exists because the dfops is used to collect > additional deferred operations queued during the whole recovery > sequence. The dfops is finished with a new transaction after intent > recovery completes. > > We already have a mechanism to create an empty, container-like > transaction to support the scrub infrastructure. We can reuse that > mechanism here to drop the final user of external dfops. This > facilitates folding dfops state (i.e., dop_low) into the > transaction, the elimination of now unused external dfops support > and also eliminates the only caller of __xfs_defer_cancel(). > > Replace the on-stack dfops with an empty transaction and pass it > around to the various helpers that queue and finish deferred > operations during intent recovery. > > Signed-off-by: Brian Foster <bfoster@redhat.com> > --- > fs/xfs/xfs_bmap_item.c | 12 +++++------ > fs/xfs/xfs_bmap_item.h | 3 +-- > fs/xfs/xfs_log_recover.c | 42 ++++++++++++++++++-------------------- > fs/xfs/xfs_refcount_item.c | 12 +++++------ > fs/xfs/xfs_refcount_item.h | 3 +-- > 5 files changed, 34 insertions(+), 38 deletions(-) > > diff --git a/fs/xfs/xfs_bmap_item.c b/fs/xfs/xfs_bmap_item.c > index e1d6c127b07d..992235e7c761 100644 > --- a/fs/xfs/xfs_bmap_item.c > +++ b/fs/xfs/xfs_bmap_item.c > @@ -375,9 +375,8 @@ xfs_bud_init( > */ > int > xfs_bui_recover( > - struct xfs_mount *mp, > - struct xfs_bui_log_item *buip, > - struct xfs_defer_ops *dfops) > + struct xfs_trans *itp, "itp"? Oh, the intent-holder transaction... > + struct xfs_bui_log_item *buip) > { > int error = 0; > unsigned int bui_type; > @@ -393,6 +392,7 @@ xfs_bui_recover( > struct xfs_trans *tp; > struct xfs_inode *ip = NULL; > struct xfs_bmbt_irec irec; > + struct xfs_mount *mp = itp->t_mountp; > > ASSERT(!test_bit(XFS_BUI_RECOVERED, &buip->bui_flags)); > > @@ -446,7 +446,7 @@ xfs_bui_recover( > * finishes them on completion. Transfer current dfops state to this > * transaction and transfer the result back before we return. > */ > - xfs_defer_move(tp->t_dfops, dfops); > + xfs_defer_move(tp->t_dfops, itp->t_dfops); > budp = xfs_trans_get_bud(tp, buip); > > /* Grab the inode. */ > @@ -494,7 +494,7 @@ xfs_bui_recover( > } > > set_bit(XFS_BUI_RECOVERED, &buip->bui_flags); > - xfs_defer_move(dfops, tp->t_dfops); > + xfs_defer_move(itp->t_dfops, tp->t_dfops); > error = xfs_trans_commit(tp); > xfs_iunlock(ip, XFS_ILOCK_EXCL); > xfs_irele(ip); > @@ -502,7 +502,7 @@ xfs_bui_recover( > return error; > > err_inode: > - xfs_defer_move(dfops, tp->t_dfops); > + xfs_defer_move(itp->t_dfops, tp->t_dfops); > xfs_trans_cancel(tp); > if (ip) { > xfs_iunlock(ip, XFS_ILOCK_EXCL); > diff --git a/fs/xfs/xfs_bmap_item.h b/fs/xfs/xfs_bmap_item.h > index fd1a1b13df51..ebf6cd6d33e2 100644 > --- a/fs/xfs/xfs_bmap_item.h > +++ b/fs/xfs/xfs_bmap_item.h > @@ -79,7 +79,6 @@ struct xfs_bud_log_item *xfs_bud_init(struct xfs_mount *, > struct xfs_bui_log_item *); > void xfs_bui_item_free(struct xfs_bui_log_item *); > void xfs_bui_release(struct xfs_bui_log_item *); > -int xfs_bui_recover(struct xfs_mount *mp, struct xfs_bui_log_item *buip, > - struct xfs_defer_ops *dfops); > +int xfs_bui_recover(struct xfs_trans *itp, struct xfs_bui_log_item *buip); > > #endif /* __XFS_BMAP_ITEM_H__ */ > diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c > index 7776fde9430c..6650b790e43b 100644 > --- a/fs/xfs/xfs_log_recover.c > +++ b/fs/xfs/xfs_log_recover.c > @@ -4733,10 +4733,9 @@ xlog_recover_cancel_rui( > /* Recover the CUI if necessary. */ > STATIC int > xlog_recover_process_cui( > - struct xfs_mount *mp, > + struct xfs_trans *tp, This parameter is the intent-placeholder transaction, right? Shouldn't it be "itp"? > struct xfs_ail *ailp, > - struct xfs_log_item *lip, > - struct xfs_defer_ops *dfops) > + struct xfs_log_item *lip) > { > struct xfs_cui_log_item *cuip; > int error; > @@ -4749,7 +4748,7 @@ xlog_recover_process_cui( > return 0; > > spin_unlock(&ailp->ail_lock); > - error = xfs_cui_recover(mp, cuip, dfops); > + error = xfs_cui_recover(tp, cuip); > spin_lock(&ailp->ail_lock); > > return error; > @@ -4774,10 +4773,9 @@ xlog_recover_cancel_cui( > /* Recover the BUI if necessary. */ > STATIC int > xlog_recover_process_bui( > - struct xfs_mount *mp, > + struct xfs_trans *tp, > struct xfs_ail *ailp, > - struct xfs_log_item *lip, > - struct xfs_defer_ops *dfops) > + struct xfs_log_item *lip) > { > struct xfs_bui_log_item *buip; > int error; > @@ -4790,7 +4788,7 @@ xlog_recover_process_bui( > return 0; > > spin_unlock(&ailp->ail_lock); > - error = xfs_bui_recover(mp, buip, dfops); > + error = xfs_bui_recover(tp, buip); > spin_lock(&ailp->ail_lock); > > return error; > @@ -4829,9 +4827,9 @@ static inline bool xlog_item_is_intent(struct xfs_log_item *lip) > /* Take all the collected deferred ops and finish them in order. */ > static int > xlog_finish_defer_ops( > - struct xfs_mount *mp, > - struct xfs_defer_ops *dfops) > + struct xfs_trans *itp) > { > + struct xfs_mount *mp = itp->t_mountp; > struct xfs_trans *tp; > int64_t freeblks; > uint resblks; > @@ -4855,7 +4853,7 @@ xlog_finish_defer_ops( > if (error) > return error; > /* transfer all collected dfops to this transaction */ > - xfs_defer_move(tp->t_dfops, dfops); > + xfs_defer_move(tp->t_dfops, itp->t_dfops); > > return xfs_trans_commit(tp); > } > @@ -4880,22 +4878,25 @@ STATIC int > xlog_recover_process_intents( > struct xlog *log) > { > - struct xfs_defer_ops dfops; > + struct xfs_trans *tp; > struct xfs_ail_cursor cur; > struct xfs_log_item *lip; > struct xfs_ail *ailp; > - int error = 0; > + int error; > #if defined(DEBUG) || defined(XFS_WARN) > xfs_lsn_t last_lsn; > #endif > > + error = xfs_trans_alloc_empty(log->l_mp, &tp); Here's where we allocate the intent-placeholder transaction, so why isn't this named "itp" as well? I think I have a handle on what this patch does, though the variable naming itp/tp is a little confusing. --D > + if (error) > + return error; > + > ailp = log->l_ailp; > spin_lock(&ailp->ail_lock); > lip = xfs_trans_ail_cursor_first(ailp, &cur, 0); > #if defined(DEBUG) || defined(XFS_WARN) > last_lsn = xlog_assign_lsn(log->l_curr_cycle, log->l_curr_block); > #endif > - xfs_defer_init(NULL, &dfops); > while (lip != NULL) { > /* > * We're done when we see something other than an intent. > @@ -4930,12 +4931,10 @@ xlog_recover_process_intents( > error = xlog_recover_process_rui(log->l_mp, ailp, lip); > break; > case XFS_LI_CUI: > - error = xlog_recover_process_cui(log->l_mp, ailp, lip, > - &dfops); > + error = xlog_recover_process_cui(tp, ailp, lip); > break; > case XFS_LI_BUI: > - error = xlog_recover_process_bui(log->l_mp, ailp, lip, > - &dfops); > + error = xlog_recover_process_bui(tp, ailp, lip); > break; > } > if (error) > @@ -4945,10 +4944,9 @@ xlog_recover_process_intents( > out: > xfs_trans_ail_cursor_done(&cur); > spin_unlock(&ailp->ail_lock); > - if (error) > - __xfs_defer_cancel(&dfops); > - else > - error = xlog_finish_defer_ops(log->l_mp, &dfops); > + if (!error) > + error = xlog_finish_defer_ops(tp); > + xfs_trans_cancel(tp); > > return error; > } > diff --git a/fs/xfs/xfs_refcount_item.c b/fs/xfs/xfs_refcount_item.c > index d3582a06626f..d663ea1f4076 100644 > --- a/fs/xfs/xfs_refcount_item.c > +++ b/fs/xfs/xfs_refcount_item.c > @@ -380,9 +380,8 @@ xfs_cud_init( > */ > int > xfs_cui_recover( > - struct xfs_mount *mp, > - struct xfs_cui_log_item *cuip, > - struct xfs_defer_ops *dfops) > + struct xfs_trans *itp, > + struct xfs_cui_log_item *cuip) > { > int i; > int error = 0; > @@ -398,6 +397,7 @@ xfs_cui_recover( > xfs_extlen_t new_len; > struct xfs_bmbt_irec irec; > bool requeue_only = false; > + struct xfs_mount *mp = itp->t_mountp; > > ASSERT(!test_bit(XFS_CUI_RECOVERED, &cuip->cui_flags)); > > @@ -457,7 +457,7 @@ xfs_cui_recover( > * finishes them on completion. Transfer current dfops state to this > * transaction and transfer the result back before we return. > */ > - xfs_defer_move(tp->t_dfops, dfops); > + xfs_defer_move(tp->t_dfops, itp->t_dfops); > cudp = xfs_trans_get_cud(tp, cuip); > > for (i = 0; i < cuip->cui_format.cui_nextents; i++) { > @@ -522,13 +522,13 @@ xfs_cui_recover( > > xfs_refcount_finish_one_cleanup(tp, rcur, error); > set_bit(XFS_CUI_RECOVERED, &cuip->cui_flags); > - xfs_defer_move(dfops, tp->t_dfops); > + xfs_defer_move(itp->t_dfops, tp->t_dfops); > error = xfs_trans_commit(tp); > return error; > > abort_error: > xfs_refcount_finish_one_cleanup(tp, rcur, error); > - xfs_defer_move(dfops, tp->t_dfops); > + xfs_defer_move(itp->t_dfops, tp->t_dfops); > xfs_trans_cancel(tp); > return error; > } > diff --git a/fs/xfs/xfs_refcount_item.h b/fs/xfs/xfs_refcount_item.h > index dd830b69cd1e..98895da52c1f 100644 > --- a/fs/xfs/xfs_refcount_item.h > +++ b/fs/xfs/xfs_refcount_item.h > @@ -82,7 +82,6 @@ struct xfs_cud_log_item *xfs_cud_init(struct xfs_mount *, > struct xfs_cui_log_item *); > void xfs_cui_item_free(struct xfs_cui_log_item *); > void xfs_cui_release(struct xfs_cui_log_item *); > -int xfs_cui_recover(struct xfs_mount *mp, struct xfs_cui_log_item *cuip, > - struct xfs_defer_ops *dfops); > +int xfs_cui_recover(struct xfs_trans *itp, struct xfs_cui_log_item *cuip); > > #endif /* __XFS_REFCOUNT_ITEM_H__ */ > -- > 2.17.1 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > the body of a message to majordomo@vger.kernel.org > 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jul 30, 2018 at 01:09:09PM -0700, Darrick J. Wong wrote: > > - struct xfs_mount *mp, > > - struct xfs_bui_log_item *buip, > > - struct xfs_defer_ops *dfops) > > + struct xfs_trans *itp, > > "itp"? > > Oh, the intent-holder transaction... Maybe we should just call it parent_tp which would be a little less confusing? -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> > + error = xfs_trans_alloc_empty(log->l_mp, &tp); > + if (error) > + return error; > + I think we need a big comment explaining the use of this transaction here. Except for that and any naming issues: Reviewed-by: Christoph Hellwig <hch@lst.de> -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jul 30, 2018 at 01:09:09PM -0700, Darrick J. Wong wrote: > On Mon, Jul 30, 2018 at 12:45:07PM -0400, Brian Foster wrote: > > Log intent recovery is the last user of an external (on-stack) > > dfops. The pattern exists because the dfops is used to collect > > additional deferred operations queued during the whole recovery > > sequence. The dfops is finished with a new transaction after intent > > recovery completes. > > > > We already have a mechanism to create an empty, container-like > > transaction to support the scrub infrastructure. We can reuse that > > mechanism here to drop the final user of external dfops. This > > facilitates folding dfops state (i.e., dop_low) into the > > transaction, the elimination of now unused external dfops support > > and also eliminates the only caller of __xfs_defer_cancel(). > > > > Replace the on-stack dfops with an empty transaction and pass it > > around to the various helpers that queue and finish deferred > > operations during intent recovery. > > > > Signed-off-by: Brian Foster <bfoster@redhat.com> > > --- > > fs/xfs/xfs_bmap_item.c | 12 +++++------ > > fs/xfs/xfs_bmap_item.h | 3 +-- > > fs/xfs/xfs_log_recover.c | 42 ++++++++++++++++++-------------------- > > fs/xfs/xfs_refcount_item.c | 12 +++++------ > > fs/xfs/xfs_refcount_item.h | 3 +-- > > 5 files changed, 34 insertions(+), 38 deletions(-) > > > > diff --git a/fs/xfs/xfs_bmap_item.c b/fs/xfs/xfs_bmap_item.c > > index e1d6c127b07d..992235e7c761 100644 > > --- a/fs/xfs/xfs_bmap_item.c > > +++ b/fs/xfs/xfs_bmap_item.c > > @@ -375,9 +375,8 @@ xfs_bud_init( > > */ > > int > > xfs_bui_recover( > > - struct xfs_mount *mp, > > - struct xfs_bui_log_item *buip, > > - struct xfs_defer_ops *dfops) > > + struct xfs_trans *itp, > > "itp"? > > Oh, the intent-holder transaction... > Yeah.. > > + struct xfs_bui_log_item *buip) > > { > > int error = 0; > > unsigned int bui_type; ... > > diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c > > index 7776fde9430c..6650b790e43b 100644 > > --- a/fs/xfs/xfs_log_recover.c > > +++ b/fs/xfs/xfs_log_recover.c ... > > @@ -4880,22 +4878,25 @@ STATIC int > > xlog_recover_process_intents( > > struct xlog *log) > > { > > - struct xfs_defer_ops dfops; > > + struct xfs_trans *tp; > > struct xfs_ail_cursor cur; > > struct xfs_log_item *lip; > > struct xfs_ail *ailp; > > - int error = 0; > > + int error; > > #if defined(DEBUG) || defined(XFS_WARN) > > xfs_lsn_t last_lsn; > > #endif > > > > + error = xfs_trans_alloc_empty(log->l_mp, &tp); > > Here's where we allocate the intent-placeholder transaction, so why > isn't this named "itp" as well? > I only used itp where tp conflicted with a local transaction (the contexts where we have to transfer state back and forth between two transactions). > I think I have a handle on what this patch does, though the variable > naming itp/tp is a little confusing. > I have no real preference on the naming. I can call it parent_tp as Christoph suggests and use that up and down the chain rather than only in the contexts where both transactions exist if you think that is more clear..? Brian > --D > > > + if (error) > > + return error; > > + > > ailp = log->l_ailp; > > spin_lock(&ailp->ail_lock); > > lip = xfs_trans_ail_cursor_first(ailp, &cur, 0); > > #if defined(DEBUG) || defined(XFS_WARN) > > last_lsn = xlog_assign_lsn(log->l_curr_cycle, log->l_curr_block); > > #endif > > - xfs_defer_init(NULL, &dfops); > > while (lip != NULL) { > > /* > > * We're done when we see something other than an intent. > > @@ -4930,12 +4931,10 @@ xlog_recover_process_intents( > > error = xlog_recover_process_rui(log->l_mp, ailp, lip); > > break; > > case XFS_LI_CUI: > > - error = xlog_recover_process_cui(log->l_mp, ailp, lip, > > - &dfops); > > + error = xlog_recover_process_cui(tp, ailp, lip); > > break; > > case XFS_LI_BUI: > > - error = xlog_recover_process_bui(log->l_mp, ailp, lip, > > - &dfops); > > + error = xlog_recover_process_bui(tp, ailp, lip); > > break; > > } > > if (error) > > @@ -4945,10 +4944,9 @@ xlog_recover_process_intents( > > out: > > xfs_trans_ail_cursor_done(&cur); > > spin_unlock(&ailp->ail_lock); > > - if (error) > > - __xfs_defer_cancel(&dfops); > > - else > > - error = xlog_finish_defer_ops(log->l_mp, &dfops); > > + if (!error) > > + error = xlog_finish_defer_ops(tp); > > + xfs_trans_cancel(tp); > > > > return error; > > } > > diff --git a/fs/xfs/xfs_refcount_item.c b/fs/xfs/xfs_refcount_item.c > > index d3582a06626f..d663ea1f4076 100644 > > --- a/fs/xfs/xfs_refcount_item.c > > +++ b/fs/xfs/xfs_refcount_item.c > > @@ -380,9 +380,8 @@ xfs_cud_init( > > */ > > int > > xfs_cui_recover( > > - struct xfs_mount *mp, > > - struct xfs_cui_log_item *cuip, > > - struct xfs_defer_ops *dfops) > > + struct xfs_trans *itp, > > + struct xfs_cui_log_item *cuip) > > { > > int i; > > int error = 0; > > @@ -398,6 +397,7 @@ xfs_cui_recover( > > xfs_extlen_t new_len; > > struct xfs_bmbt_irec irec; > > bool requeue_only = false; > > + struct xfs_mount *mp = itp->t_mountp; > > > > ASSERT(!test_bit(XFS_CUI_RECOVERED, &cuip->cui_flags)); > > > > @@ -457,7 +457,7 @@ xfs_cui_recover( > > * finishes them on completion. Transfer current dfops state to this > > * transaction and transfer the result back before we return. > > */ > > - xfs_defer_move(tp->t_dfops, dfops); > > + xfs_defer_move(tp->t_dfops, itp->t_dfops); > > cudp = xfs_trans_get_cud(tp, cuip); > > > > for (i = 0; i < cuip->cui_format.cui_nextents; i++) { > > @@ -522,13 +522,13 @@ xfs_cui_recover( > > > > xfs_refcount_finish_one_cleanup(tp, rcur, error); > > set_bit(XFS_CUI_RECOVERED, &cuip->cui_flags); > > - xfs_defer_move(dfops, tp->t_dfops); > > + xfs_defer_move(itp->t_dfops, tp->t_dfops); > > error = xfs_trans_commit(tp); > > return error; > > > > abort_error: > > xfs_refcount_finish_one_cleanup(tp, rcur, error); > > - xfs_defer_move(dfops, tp->t_dfops); > > + xfs_defer_move(itp->t_dfops, tp->t_dfops); > > xfs_trans_cancel(tp); > > return error; > > } > > diff --git a/fs/xfs/xfs_refcount_item.h b/fs/xfs/xfs_refcount_item.h > > index dd830b69cd1e..98895da52c1f 100644 > > --- a/fs/xfs/xfs_refcount_item.h > > +++ b/fs/xfs/xfs_refcount_item.h > > @@ -82,7 +82,6 @@ struct xfs_cud_log_item *xfs_cud_init(struct xfs_mount *, > > struct xfs_cui_log_item *); > > void xfs_cui_item_free(struct xfs_cui_log_item *); > > void xfs_cui_release(struct xfs_cui_log_item *); > > -int xfs_cui_recover(struct xfs_mount *mp, struct xfs_cui_log_item *cuip, > > - struct xfs_defer_ops *dfops); > > +int xfs_cui_recover(struct xfs_trans *itp, struct xfs_cui_log_item *cuip); > > > > #endif /* __XFS_REFCOUNT_ITEM_H__ */ > > -- > > 2.17.1 > > > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > > the body of a message to majordomo@vger.kernel.org > > 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@vger.kernel.org > 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jul 31, 2018 at 07:47:51AM -0400, Brian Foster wrote: > On Mon, Jul 30, 2018 at 01:09:09PM -0700, Darrick J. Wong wrote: > > On Mon, Jul 30, 2018 at 12:45:07PM -0400, Brian Foster wrote: > > > Log intent recovery is the last user of an external (on-stack) > > > dfops. The pattern exists because the dfops is used to collect > > > additional deferred operations queued during the whole recovery > > > sequence. The dfops is finished with a new transaction after intent > > > recovery completes. > > > > > > We already have a mechanism to create an empty, container-like > > > transaction to support the scrub infrastructure. We can reuse that > > > mechanism here to drop the final user of external dfops. This > > > facilitates folding dfops state (i.e., dop_low) into the > > > transaction, the elimination of now unused external dfops support > > > and also eliminates the only caller of __xfs_defer_cancel(). > > > > > > Replace the on-stack dfops with an empty transaction and pass it > > > around to the various helpers that queue and finish deferred > > > operations during intent recovery. > > > > > > Signed-off-by: Brian Foster <bfoster@redhat.com> > > > --- > > > fs/xfs/xfs_bmap_item.c | 12 +++++------ > > > fs/xfs/xfs_bmap_item.h | 3 +-- > > > fs/xfs/xfs_log_recover.c | 42 ++++++++++++++++++-------------------- > > > fs/xfs/xfs_refcount_item.c | 12 +++++------ > > > fs/xfs/xfs_refcount_item.h | 3 +-- > > > 5 files changed, 34 insertions(+), 38 deletions(-) > > > > > > diff --git a/fs/xfs/xfs_bmap_item.c b/fs/xfs/xfs_bmap_item.c > > > index e1d6c127b07d..992235e7c761 100644 > > > --- a/fs/xfs/xfs_bmap_item.c > > > +++ b/fs/xfs/xfs_bmap_item.c > > > @@ -375,9 +375,8 @@ xfs_bud_init( > > > */ > > > int > > > xfs_bui_recover( > > > - struct xfs_mount *mp, > > > - struct xfs_bui_log_item *buip, > > > - struct xfs_defer_ops *dfops) > > > + struct xfs_trans *itp, > > > > "itp"? > > > > Oh, the intent-holder transaction... > > > > Yeah.. > > > > + struct xfs_bui_log_item *buip) > > > { > > > int error = 0; > > > unsigned int bui_type; > ... > > > diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c > > > index 7776fde9430c..6650b790e43b 100644 > > > --- a/fs/xfs/xfs_log_recover.c > > > +++ b/fs/xfs/xfs_log_recover.c > ... > > > @@ -4880,22 +4878,25 @@ STATIC int > > > xlog_recover_process_intents( > > > struct xlog *log) > > > { > > > - struct xfs_defer_ops dfops; > > > + struct xfs_trans *tp; > > > struct xfs_ail_cursor cur; > > > struct xfs_log_item *lip; > > > struct xfs_ail *ailp; > > > - int error = 0; > > > + int error; > > > #if defined(DEBUG) || defined(XFS_WARN) > > > xfs_lsn_t last_lsn; > > > #endif > > > > > > + error = xfs_trans_alloc_empty(log->l_mp, &tp); > > > > Here's where we allocate the intent-placeholder transaction, so why > > isn't this named "itp" as well? > > > > I only used itp where tp conflicted with a local transaction (the > contexts where we have to transfer state back and forth between two > transactions). > > > I think I have a handle on what this patch does, though the variable > > naming itp/tp is a little confusing. > > > > I have no real preference on the naming. I can call it parent_tp as > Christoph suggests and use that up and down the chain rather than only > in the contexts where both transactions exist if you think that is more > clear..? Yes, please. The 'parent_tp' transaction is really just a holder for a dfops list that will eventually get finished with a non-empty transaction, so we should make it as clear as possible in the source code that the parent_tp is not for logging real work. :) --D > Brian > > > --D > > > > > + if (error) > > > + return error; > > > + > > > ailp = log->l_ailp; > > > spin_lock(&ailp->ail_lock); > > > lip = xfs_trans_ail_cursor_first(ailp, &cur, 0); > > > #if defined(DEBUG) || defined(XFS_WARN) > > > last_lsn = xlog_assign_lsn(log->l_curr_cycle, log->l_curr_block); > > > #endif > > > - xfs_defer_init(NULL, &dfops); > > > while (lip != NULL) { > > > /* > > > * We're done when we see something other than an intent. > > > @@ -4930,12 +4931,10 @@ xlog_recover_process_intents( > > > error = xlog_recover_process_rui(log->l_mp, ailp, lip); > > > break; > > > case XFS_LI_CUI: > > > - error = xlog_recover_process_cui(log->l_mp, ailp, lip, > > > - &dfops); > > > + error = xlog_recover_process_cui(tp, ailp, lip); > > > break; > > > case XFS_LI_BUI: > > > - error = xlog_recover_process_bui(log->l_mp, ailp, lip, > > > - &dfops); > > > + error = xlog_recover_process_bui(tp, ailp, lip); > > > break; > > > } > > > if (error) > > > @@ -4945,10 +4944,9 @@ xlog_recover_process_intents( > > > out: > > > xfs_trans_ail_cursor_done(&cur); > > > spin_unlock(&ailp->ail_lock); > > > - if (error) > > > - __xfs_defer_cancel(&dfops); > > > - else > > > - error = xlog_finish_defer_ops(log->l_mp, &dfops); > > > + if (!error) > > > + error = xlog_finish_defer_ops(tp); > > > + xfs_trans_cancel(tp); > > > > > > return error; > > > } > > > diff --git a/fs/xfs/xfs_refcount_item.c b/fs/xfs/xfs_refcount_item.c > > > index d3582a06626f..d663ea1f4076 100644 > > > --- a/fs/xfs/xfs_refcount_item.c > > > +++ b/fs/xfs/xfs_refcount_item.c > > > @@ -380,9 +380,8 @@ xfs_cud_init( > > > */ > > > int > > > xfs_cui_recover( > > > - struct xfs_mount *mp, > > > - struct xfs_cui_log_item *cuip, > > > - struct xfs_defer_ops *dfops) > > > + struct xfs_trans *itp, > > > + struct xfs_cui_log_item *cuip) > > > { > > > int i; > > > int error = 0; > > > @@ -398,6 +397,7 @@ xfs_cui_recover( > > > xfs_extlen_t new_len; > > > struct xfs_bmbt_irec irec; > > > bool requeue_only = false; > > > + struct xfs_mount *mp = itp->t_mountp; > > > > > > ASSERT(!test_bit(XFS_CUI_RECOVERED, &cuip->cui_flags)); > > > > > > @@ -457,7 +457,7 @@ xfs_cui_recover( > > > * finishes them on completion. Transfer current dfops state to this > > > * transaction and transfer the result back before we return. > > > */ > > > - xfs_defer_move(tp->t_dfops, dfops); > > > + xfs_defer_move(tp->t_dfops, itp->t_dfops); > > > cudp = xfs_trans_get_cud(tp, cuip); > > > > > > for (i = 0; i < cuip->cui_format.cui_nextents; i++) { > > > @@ -522,13 +522,13 @@ xfs_cui_recover( > > > > > > xfs_refcount_finish_one_cleanup(tp, rcur, error); > > > set_bit(XFS_CUI_RECOVERED, &cuip->cui_flags); > > > - xfs_defer_move(dfops, tp->t_dfops); > > > + xfs_defer_move(itp->t_dfops, tp->t_dfops); > > > error = xfs_trans_commit(tp); > > > return error; > > > > > > abort_error: > > > xfs_refcount_finish_one_cleanup(tp, rcur, error); > > > - xfs_defer_move(dfops, tp->t_dfops); > > > + xfs_defer_move(itp->t_dfops, tp->t_dfops); > > > xfs_trans_cancel(tp); > > > return error; > > > } > > > diff --git a/fs/xfs/xfs_refcount_item.h b/fs/xfs/xfs_refcount_item.h > > > index dd830b69cd1e..98895da52c1f 100644 > > > --- a/fs/xfs/xfs_refcount_item.h > > > +++ b/fs/xfs/xfs_refcount_item.h > > > @@ -82,7 +82,6 @@ struct xfs_cud_log_item *xfs_cud_init(struct xfs_mount *, > > > struct xfs_cui_log_item *); > > > void xfs_cui_item_free(struct xfs_cui_log_item *); > > > void xfs_cui_release(struct xfs_cui_log_item *); > > > -int xfs_cui_recover(struct xfs_mount *mp, struct xfs_cui_log_item *cuip, > > > - struct xfs_defer_ops *dfops); > > > +int xfs_cui_recover(struct xfs_trans *itp, struct xfs_cui_log_item *cuip); > > > > > > #endif /* __XFS_REFCOUNT_ITEM_H__ */ > > > -- > > > 2.17.1 > > > > > > -- > > > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > > > the body of a message to majordomo@vger.kernel.org > > > 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@vger.kernel.org > > 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@vger.kernel.org > 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/xfs/xfs_bmap_item.c b/fs/xfs/xfs_bmap_item.c index e1d6c127b07d..992235e7c761 100644 --- a/fs/xfs/xfs_bmap_item.c +++ b/fs/xfs/xfs_bmap_item.c @@ -375,9 +375,8 @@ xfs_bud_init( */ int xfs_bui_recover( - struct xfs_mount *mp, - struct xfs_bui_log_item *buip, - struct xfs_defer_ops *dfops) + struct xfs_trans *itp, + struct xfs_bui_log_item *buip) { int error = 0; unsigned int bui_type; @@ -393,6 +392,7 @@ xfs_bui_recover( struct xfs_trans *tp; struct xfs_inode *ip = NULL; struct xfs_bmbt_irec irec; + struct xfs_mount *mp = itp->t_mountp; ASSERT(!test_bit(XFS_BUI_RECOVERED, &buip->bui_flags)); @@ -446,7 +446,7 @@ xfs_bui_recover( * finishes them on completion. Transfer current dfops state to this * transaction and transfer the result back before we return. */ - xfs_defer_move(tp->t_dfops, dfops); + xfs_defer_move(tp->t_dfops, itp->t_dfops); budp = xfs_trans_get_bud(tp, buip); /* Grab the inode. */ @@ -494,7 +494,7 @@ xfs_bui_recover( } set_bit(XFS_BUI_RECOVERED, &buip->bui_flags); - xfs_defer_move(dfops, tp->t_dfops); + xfs_defer_move(itp->t_dfops, tp->t_dfops); error = xfs_trans_commit(tp); xfs_iunlock(ip, XFS_ILOCK_EXCL); xfs_irele(ip); @@ -502,7 +502,7 @@ xfs_bui_recover( return error; err_inode: - xfs_defer_move(dfops, tp->t_dfops); + xfs_defer_move(itp->t_dfops, tp->t_dfops); xfs_trans_cancel(tp); if (ip) { xfs_iunlock(ip, XFS_ILOCK_EXCL); diff --git a/fs/xfs/xfs_bmap_item.h b/fs/xfs/xfs_bmap_item.h index fd1a1b13df51..ebf6cd6d33e2 100644 --- a/fs/xfs/xfs_bmap_item.h +++ b/fs/xfs/xfs_bmap_item.h @@ -79,7 +79,6 @@ struct xfs_bud_log_item *xfs_bud_init(struct xfs_mount *, struct xfs_bui_log_item *); void xfs_bui_item_free(struct xfs_bui_log_item *); void xfs_bui_release(struct xfs_bui_log_item *); -int xfs_bui_recover(struct xfs_mount *mp, struct xfs_bui_log_item *buip, - struct xfs_defer_ops *dfops); +int xfs_bui_recover(struct xfs_trans *itp, struct xfs_bui_log_item *buip); #endif /* __XFS_BMAP_ITEM_H__ */ diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c index 7776fde9430c..6650b790e43b 100644 --- a/fs/xfs/xfs_log_recover.c +++ b/fs/xfs/xfs_log_recover.c @@ -4733,10 +4733,9 @@ xlog_recover_cancel_rui( /* Recover the CUI if necessary. */ STATIC int xlog_recover_process_cui( - struct xfs_mount *mp, + struct xfs_trans *tp, struct xfs_ail *ailp, - struct xfs_log_item *lip, - struct xfs_defer_ops *dfops) + struct xfs_log_item *lip) { struct xfs_cui_log_item *cuip; int error; @@ -4749,7 +4748,7 @@ xlog_recover_process_cui( return 0; spin_unlock(&ailp->ail_lock); - error = xfs_cui_recover(mp, cuip, dfops); + error = xfs_cui_recover(tp, cuip); spin_lock(&ailp->ail_lock); return error; @@ -4774,10 +4773,9 @@ xlog_recover_cancel_cui( /* Recover the BUI if necessary. */ STATIC int xlog_recover_process_bui( - struct xfs_mount *mp, + struct xfs_trans *tp, struct xfs_ail *ailp, - struct xfs_log_item *lip, - struct xfs_defer_ops *dfops) + struct xfs_log_item *lip) { struct xfs_bui_log_item *buip; int error; @@ -4790,7 +4788,7 @@ xlog_recover_process_bui( return 0; spin_unlock(&ailp->ail_lock); - error = xfs_bui_recover(mp, buip, dfops); + error = xfs_bui_recover(tp, buip); spin_lock(&ailp->ail_lock); return error; @@ -4829,9 +4827,9 @@ static inline bool xlog_item_is_intent(struct xfs_log_item *lip) /* Take all the collected deferred ops and finish them in order. */ static int xlog_finish_defer_ops( - struct xfs_mount *mp, - struct xfs_defer_ops *dfops) + struct xfs_trans *itp) { + struct xfs_mount *mp = itp->t_mountp; struct xfs_trans *tp; int64_t freeblks; uint resblks; @@ -4855,7 +4853,7 @@ xlog_finish_defer_ops( if (error) return error; /* transfer all collected dfops to this transaction */ - xfs_defer_move(tp->t_dfops, dfops); + xfs_defer_move(tp->t_dfops, itp->t_dfops); return xfs_trans_commit(tp); } @@ -4880,22 +4878,25 @@ STATIC int xlog_recover_process_intents( struct xlog *log) { - struct xfs_defer_ops dfops; + struct xfs_trans *tp; struct xfs_ail_cursor cur; struct xfs_log_item *lip; struct xfs_ail *ailp; - int error = 0; + int error; #if defined(DEBUG) || defined(XFS_WARN) xfs_lsn_t last_lsn; #endif + error = xfs_trans_alloc_empty(log->l_mp, &tp); + if (error) + return error; + ailp = log->l_ailp; spin_lock(&ailp->ail_lock); lip = xfs_trans_ail_cursor_first(ailp, &cur, 0); #if defined(DEBUG) || defined(XFS_WARN) last_lsn = xlog_assign_lsn(log->l_curr_cycle, log->l_curr_block); #endif - xfs_defer_init(NULL, &dfops); while (lip != NULL) { /* * We're done when we see something other than an intent. @@ -4930,12 +4931,10 @@ xlog_recover_process_intents( error = xlog_recover_process_rui(log->l_mp, ailp, lip); break; case XFS_LI_CUI: - error = xlog_recover_process_cui(log->l_mp, ailp, lip, - &dfops); + error = xlog_recover_process_cui(tp, ailp, lip); break; case XFS_LI_BUI: - error = xlog_recover_process_bui(log->l_mp, ailp, lip, - &dfops); + error = xlog_recover_process_bui(tp, ailp, lip); break; } if (error) @@ -4945,10 +4944,9 @@ xlog_recover_process_intents( out: xfs_trans_ail_cursor_done(&cur); spin_unlock(&ailp->ail_lock); - if (error) - __xfs_defer_cancel(&dfops); - else - error = xlog_finish_defer_ops(log->l_mp, &dfops); + if (!error) + error = xlog_finish_defer_ops(tp); + xfs_trans_cancel(tp); return error; } diff --git a/fs/xfs/xfs_refcount_item.c b/fs/xfs/xfs_refcount_item.c index d3582a06626f..d663ea1f4076 100644 --- a/fs/xfs/xfs_refcount_item.c +++ b/fs/xfs/xfs_refcount_item.c @@ -380,9 +380,8 @@ xfs_cud_init( */ int xfs_cui_recover( - struct xfs_mount *mp, - struct xfs_cui_log_item *cuip, - struct xfs_defer_ops *dfops) + struct xfs_trans *itp, + struct xfs_cui_log_item *cuip) { int i; int error = 0; @@ -398,6 +397,7 @@ xfs_cui_recover( xfs_extlen_t new_len; struct xfs_bmbt_irec irec; bool requeue_only = false; + struct xfs_mount *mp = itp->t_mountp; ASSERT(!test_bit(XFS_CUI_RECOVERED, &cuip->cui_flags)); @@ -457,7 +457,7 @@ xfs_cui_recover( * finishes them on completion. Transfer current dfops state to this * transaction and transfer the result back before we return. */ - xfs_defer_move(tp->t_dfops, dfops); + xfs_defer_move(tp->t_dfops, itp->t_dfops); cudp = xfs_trans_get_cud(tp, cuip); for (i = 0; i < cuip->cui_format.cui_nextents; i++) { @@ -522,13 +522,13 @@ xfs_cui_recover( xfs_refcount_finish_one_cleanup(tp, rcur, error); set_bit(XFS_CUI_RECOVERED, &cuip->cui_flags); - xfs_defer_move(dfops, tp->t_dfops); + xfs_defer_move(itp->t_dfops, tp->t_dfops); error = xfs_trans_commit(tp); return error; abort_error: xfs_refcount_finish_one_cleanup(tp, rcur, error); - xfs_defer_move(dfops, tp->t_dfops); + xfs_defer_move(itp->t_dfops, tp->t_dfops); xfs_trans_cancel(tp); return error; } diff --git a/fs/xfs/xfs_refcount_item.h b/fs/xfs/xfs_refcount_item.h index dd830b69cd1e..98895da52c1f 100644 --- a/fs/xfs/xfs_refcount_item.h +++ b/fs/xfs/xfs_refcount_item.h @@ -82,7 +82,6 @@ struct xfs_cud_log_item *xfs_cud_init(struct xfs_mount *, struct xfs_cui_log_item *); void xfs_cui_item_free(struct xfs_cui_log_item *); void xfs_cui_release(struct xfs_cui_log_item *); -int xfs_cui_recover(struct xfs_mount *mp, struct xfs_cui_log_item *cuip, - struct xfs_defer_ops *dfops); +int xfs_cui_recover(struct xfs_trans *itp, struct xfs_cui_log_item *cuip); #endif /* __XFS_REFCOUNT_ITEM_H__ */
Log intent recovery is the last user of an external (on-stack) dfops. The pattern exists because the dfops is used to collect additional deferred operations queued during the whole recovery sequence. The dfops is finished with a new transaction after intent recovery completes. We already have a mechanism to create an empty, container-like transaction to support the scrub infrastructure. We can reuse that mechanism here to drop the final user of external dfops. This facilitates folding dfops state (i.e., dop_low) into the transaction, the elimination of now unused external dfops support and also eliminates the only caller of __xfs_defer_cancel(). Replace the on-stack dfops with an empty transaction and pass it around to the various helpers that queue and finish deferred operations during intent recovery. Signed-off-by: Brian Foster <bfoster@redhat.com> --- fs/xfs/xfs_bmap_item.c | 12 +++++------ fs/xfs/xfs_bmap_item.h | 3 +-- fs/xfs/xfs_log_recover.c | 42 ++++++++++++++++++-------------------- fs/xfs/xfs_refcount_item.c | 12 +++++------ fs/xfs/xfs_refcount_item.h | 3 +-- 5 files changed, 34 insertions(+), 38 deletions(-)