On Tue, Jul 03, 2018 at 08:15:35AM -0700, Darrick J. Wong wrote:
> On Mon, Jul 02, 2018 at 01:38:36PM -0400, Brian Foster wrote:
> > Log recovery passes down a central dfops structure to recovery
> > handlers for bui and cui log items. Each of these handlers allocates
> > and commits a transaction and defers any remaining operations to be
> > completed by the main recovery sequence.
> >
> > Since dfops outlives the transaction in this context, set and clear
> > ->t_dfops appropriately such that the *_finish_item() paths and
> > below (i.e., xfs_bmapi*()) can expect to find the dfops in the
> > transaction without it being committed with the dfops attached. This
> > is required because transaction commit expects that an associated
> > dfops is finished and in this context the dfops may be populated at
> > commit time.
> >
> > Signed-off-by: Brian Foster <bfoster@xxxxxxxxxx>
> > Reviewed-by: Christoph Hellwig <hch@xxxxxx>
> > ---
> >
> > v2:
> > - Add comments to explain unique ->t_dfops usage in bui/cui log item
> > recovery.
> >
> > fs/xfs/xfs_bmap_item.c | 8 ++++++++
> > fs/xfs/xfs_refcount_item.c | 8 ++++++++
>
> General note: realtime rmap (being rooted in an inode) log item replay
> can generate further dfops, so I'll have to wire that up when I pass the
> log recovery dfops collector into xfs_rui_recover, and I'll have to
> remember to attach it to the transaction when I do that.
>
Ok. I'm hoping that a last step to embed ->t_dfops directly in the
transaction may clean up this ugly set/unset pattern a bit. What I'm
thinking is that a transaction roll would end up having to transfer
(splice) the pending dfops items from the current ->t_dfops to the new
one rather than simply copy the pointer. Some of the dfops code has to
become a bit more transaction centric to make sure we don't lose the
dfops pointer, but otherwise if we factor that into a little helper I
think that means these recovery functions would just have to do
something like:
xfs_defer_move(&tp->t_dfops, dfops);
... before committing tp to collect all the deferred items in the dfops
from the caller. Otherwise, the transaction commit would actually run
xfs_defer_finish() itself if the associated dfops has pending work. I
haven't really dug into that yet, though, so there may be more issues to
consider (thoughts appreciated, if that triggers any for you).
Brian
> Otherwise I think this is fine,
> Reviewed-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
>
> --D
>
> > 2 files changed, 16 insertions(+)
> >
> > diff --git a/fs/xfs/xfs_bmap_item.c b/fs/xfs/xfs_bmap_item.c
> > index 956ebd583e27..478bfc798861 100644
> > --- a/fs/xfs/xfs_bmap_item.c
> > +++ b/fs/xfs/xfs_bmap_item.c
> > @@ -441,6 +441,7 @@ xfs_bui_recover(
> > XFS_EXTENTADD_SPACE_RES(mp, XFS_DATA_FORK), 0, 0, &tp);
> > if (error)
> > return error;
> > + tp->t_dfops = dfops;
> > budp = xfs_trans_get_bud(tp, buip);
> >
> > /* Grab the inode. */
> > @@ -487,6 +488,12 @@ xfs_bui_recover(
> > }
> >
> > set_bit(XFS_BUI_RECOVERED, &buip->bui_flags);
> > + /*
> > + * Recovery finishes all deferred ops once intent processing is
> > + * complete. Reset the trans reference because commit expects a finished
> > + * dfops or none at all.
> > + */
> > + tp->t_dfops = NULL;
> > error = xfs_trans_commit(tp);
> > xfs_iunlock(ip, XFS_ILOCK_EXCL);
> > IRELE(ip);
> > @@ -494,6 +501,7 @@ xfs_bui_recover(
> > return error;
> >
> > err_inode:
> > + tp->t_dfops = NULL;
> > xfs_trans_cancel(tp);
> > if (ip) {
> > xfs_iunlock(ip, XFS_ILOCK_EXCL);
> > diff --git a/fs/xfs/xfs_refcount_item.c b/fs/xfs/xfs_refcount_item.c
> > index 472a73e9d331..2064c689bc72 100644
> > --- a/fs/xfs/xfs_refcount_item.c
> > +++ b/fs/xfs/xfs_refcount_item.c
> > @@ -452,6 +452,7 @@ xfs_cui_recover(
> > mp->m_refc_maxlevels * 2, 0, XFS_TRANS_RESERVE, &tp);
> > if (error)
> > return error;
> > + tp->t_dfops = dfops;
> > cudp = xfs_trans_get_cud(tp, cuip);
> >
> > for (i = 0; i < cuip->cui_format.cui_nextents; i++) {
> > @@ -514,11 +515,18 @@ xfs_cui_recover(
> >
> > xfs_refcount_finish_one_cleanup(tp, rcur, error);
> > set_bit(XFS_CUI_RECOVERED, &cuip->cui_flags);
> > + /*
> > + * Recovery finishes all deferred ops once intent processing is
> > + * complete. Reset the trans reference because commit expects a finished
> > + * dfops or none at all.
> > + */
> > + tp->t_dfops = NULL;
> > error = xfs_trans_commit(tp);
> > return error;
> >
> > abort_error:
> > xfs_refcount_finish_one_cleanup(tp, rcur, error);
> > + tp->t_dfops = NULL;
> > xfs_trans_cancel(tp);
> > return error;
> > }
> > --
> > 2.17.1
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message to majordomo@xxxxxxxxxxxxxxx
> > 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
