On Wed, Aug 01, 2018 at 09:19:49AM -0400, Brian Foster wrote:
> The xfs_defer_ops ->dop_pending list is used to track active
> deferred operations once intents are logged. These items must be
> aborted in the event of an error. The list is populated as intents
> are logged and items are removed as they complete (or are aborted).
>
> Now that xfs_defer_finish() cancels on error, there is no need to
> ever access ->dop_pending outside of xfs_defer_finish(). The list is
> only ever populated after xfs_defer_finish() begins and is either
> completed or cancelled before it returns.
>
> Remove ->dop_pending from xfs_defer_ops and replace it with a local
> list in the xfs_defer_finish() path. Pass the local list to the
> various helpers now that it is not accessible via dfops. Note that
> we have to check for NULL in the abort case as the final tx roll
> occurs outside of the scope of the new local list (once the dfops
> has completed and thus drained the list).
>
> Signed-off-by: Brian Foster <bfoster@xxxxxxxxxx>
/me wipes the sweat off his brow, bfoster having cleaned up all the
weird dfops/firstblock/tp lifetime rules. Thanks a bunch for all this
work! :)
(Will send out for testing and report back, esp. since we now have an
extra week.)
Reviewed-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
--D
> ---
> fs/xfs/libxfs/xfs_defer.c | 129 ++++++++++++++++++--------------------
> fs/xfs/libxfs/xfs_defer.h | 1 -
> fs/xfs/xfs_trace.h | 5 +-
> fs/xfs/xfs_trans.c | 2 +-
> fs/xfs/xfs_trans.h | 1 -
> 5 files changed, 64 insertions(+), 74 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_defer.c b/fs/xfs/libxfs/xfs_defer.c
> index b656a399cd71..d811a85daf6e 100644
> --- a/fs/xfs/libxfs/xfs_defer.c
> +++ b/fs/xfs/libxfs/xfs_defer.c
> @@ -180,7 +180,7 @@ static const struct xfs_defer_op_type *defer_op_types[XFS_DEFER_OPS_TYPE_MAX];
> * the pending list.
> */
> STATIC void
> -xfs_defer_intake_work(
> +xfs_defer_create_intents(
> struct xfs_trans *tp)
> {
> struct xfs_defer_ops *dop = tp->t_dfops;
> @@ -190,20 +190,19 @@ xfs_defer_intake_work(
> list_for_each_entry(dfp, &dop->dop_intake, dfp_list) {
> dfp->dfp_intent = dfp->dfp_type->create_intent(tp,
> dfp->dfp_count);
> - trace_xfs_defer_intake_work(tp->t_mountp, dfp);
> + trace_xfs_defer_create_intent(tp->t_mountp, dfp);
> list_sort(tp->t_mountp, &dfp->dfp_work,
> dfp->dfp_type->diff_items);
> list_for_each(li, &dfp->dfp_work)
> dfp->dfp_type->log_item(tp, dfp->dfp_intent, li);
> }
> -
> - list_splice_tail_init(&dop->dop_intake, &dop->dop_pending);
> }
>
> /* Abort all the intents that were committed. */
> STATIC void
> xfs_defer_trans_abort(
> struct xfs_trans *tp,
> + struct list_head *dop_pending,
> int error)
> {
> struct xfs_defer_ops *dop = tp->t_dfops;
> @@ -212,11 +211,13 @@ xfs_defer_trans_abort(
> trace_xfs_defer_trans_abort(tp->t_mountp, dop, _RET_IP_);
>
> /* Abort intent items that don't have a done item. */
> - list_for_each_entry(dfp, &dop->dop_pending, dfp_list) {
> - trace_xfs_defer_pending_abort(tp->t_mountp, dfp);
> - if (dfp->dfp_intent && !dfp->dfp_done) {
> - dfp->dfp_type->abort_intent(dfp->dfp_intent);
> - dfp->dfp_intent = NULL;
> + if (dop_pending) {
> + list_for_each_entry(dfp, dop_pending, dfp_list) {
> + trace_xfs_defer_pending_abort(tp->t_mountp, dfp);
> + if (dfp->dfp_intent && !dfp->dfp_done) {
> + dfp->dfp_type->abort_intent(dfp->dfp_intent);
> + dfp->dfp_intent = NULL;
> + }
> }
> }
>
> @@ -228,7 +229,8 @@ xfs_defer_trans_abort(
> /* Roll a transaction so we can do some deferred op processing. */
> STATIC int
> xfs_defer_trans_roll(
> - struct xfs_trans **tp)
> + struct xfs_trans **tp,
> + struct list_head *dop_pending)
> {
> struct xfs_buf_log_item *bli;
> struct xfs_inode_log_item *ili;
> @@ -278,7 +280,7 @@ xfs_defer_trans_roll(
> if (error) {
> trace_xfs_defer_trans_roll_error((*tp)->t_mountp,
> (*tp)->t_dfops, error);
> - xfs_defer_trans_abort(*tp, error);
> + xfs_defer_trans_abort(*tp, dop_pending, error);
> return error;
> }
>
> @@ -295,15 +297,6 @@ xfs_defer_trans_roll(
> return error;
> }
>
> -/* Do we have any work items to finish? */
> -bool
> -xfs_defer_has_unfinished_work(
> - struct xfs_trans *tp)
> -{
> - return !list_empty(&tp->t_dfops->dop_pending) ||
> - !list_empty(&tp->t_dfops->dop_intake);
> -}
> -
> /*
> * Reset an already used dfops after finish.
> */
> @@ -311,7 +304,7 @@ static void
> xfs_defer_reset(
> struct xfs_trans *tp)
> {
> - ASSERT(!xfs_defer_has_unfinished_work(tp));
> + ASSERT(list_empty(&tp->t_dfops->dop_intake));
>
> /*
> * Low mode state transfers across transaction rolls to mirror dfops
> @@ -320,6 +313,36 @@ xfs_defer_reset(
> tp->t_flags &= ~XFS_TRANS_LOWMODE;
> }
>
> +/*
> + * Free up any items left in the list.
> + */
> +static void
> +xfs_defer_cancel_list(
> + struct xfs_mount *mp,
> + struct list_head *dop_list)
> +{
> + struct xfs_defer_pending *dfp;
> + struct xfs_defer_pending *pli;
> + struct list_head *pwi;
> + struct list_head *n;
> +
> + /*
> + * Free the pending items. Caller should already have arranged
> + * for the intent items to be released.
> + */
> + list_for_each_entry_safe(dfp, pli, dop_list, dfp_list) {
> + trace_xfs_defer_cancel_list(mp, dfp);
> + list_del(&dfp->dfp_list);
> + list_for_each_safe(pwi, n, &dfp->dfp_work) {
> + list_del(pwi);
> + dfp->dfp_count--;
> + dfp->dfp_type->cancel_item(pwi);
> + }
> + ASSERT(dfp->dfp_count == 0);
> + kmem_free(dfp);
> + }
> +}
> +
> /*
> * Finish all the pending work. This involves logging intent items for
> * any work items that wandered in since the last transaction roll (if
> @@ -338,26 +361,30 @@ xfs_defer_finish_noroll(
> void *state;
> int error = 0;
> void (*cleanup_fn)(struct xfs_trans *, void *, int);
> + LIST_HEAD(dop_pending);
>
> ASSERT((*tp)->t_flags & XFS_TRANS_PERM_LOG_RES);
>
> trace_xfs_defer_finish((*tp)->t_mountp, (*tp)->t_dfops, _RET_IP_);
>
> /* Until we run out of pending work to finish... */
> - while (xfs_defer_has_unfinished_work(*tp)) {
> - /* Log intents for work items sitting in the intake. */
> - xfs_defer_intake_work(*tp);
> + while (!list_empty(&dop_pending) ||
> + !list_empty(&(*tp)->t_dfops->dop_intake)) {
> + /* log intents and pull in intake items */
> + xfs_defer_create_intents(*tp);
> + list_splice_tail_init(&(*tp)->t_dfops->dop_intake,
> + &dop_pending);
>
> /*
> * Roll the transaction.
> */
> - error = xfs_defer_trans_roll(tp);
> + error = xfs_defer_trans_roll(tp, &dop_pending);
> if (error)
> goto out;
>
> /* Log an intent-done item for the first pending item. */
> - dfp = list_first_entry(&(*tp)->t_dfops->dop_pending,
> - struct xfs_defer_pending, dfp_list);
> + dfp = list_first_entry(&dop_pending, struct xfs_defer_pending,
> + dfp_list);
> trace_xfs_defer_pending_finish((*tp)->t_mountp, dfp);
> dfp->dfp_done = dfp->dfp_type->create_done(*tp, dfp->dfp_intent,
> dfp->dfp_count);
> @@ -387,7 +414,7 @@ xfs_defer_finish_noroll(
> */
> if (cleanup_fn)
> cleanup_fn(*tp, state, error);
> - xfs_defer_trans_abort(*tp, error);
> + xfs_defer_trans_abort(*tp, &dop_pending, error);
> goto out;
> }
> }
> @@ -419,6 +446,7 @@ xfs_defer_finish_noroll(
> if (error) {
> trace_xfs_defer_finish_error((*tp)->t_mountp, (*tp)->t_dfops,
> error);
> + xfs_defer_cancel_list((*tp)->t_mountp, &dop_pending);
> xfs_defer_cancel(*tp);
> return error;
> }
> @@ -441,7 +469,7 @@ xfs_defer_finish(
> if (error)
> return error;
> if ((*tp)->t_flags & XFS_TRANS_DIRTY) {
> - error = xfs_defer_trans_roll(tp);
> + error = xfs_defer_trans_roll(tp, NULL);
> if (error)
> return error;
> }
> @@ -449,47 +477,14 @@ xfs_defer_finish(
> return 0;
> }
>
> -/*
> - * Free up any items left in the list.
> - */
> void
> xfs_defer_cancel(
> - struct xfs_trans *tp)
> + struct xfs_trans *tp)
> {
> - struct xfs_defer_ops *dop = tp->t_dfops;
> - struct xfs_defer_pending *dfp;
> - struct xfs_defer_pending *pli;
> - struct list_head *pwi;
> - struct list_head *n;
> -
> - trace_xfs_defer_cancel(NULL, dop, _RET_IP_);
> + struct xfs_mount *mp = tp->t_mountp;
>
> - /*
> - * Free the pending items. Caller should already have arranged
> - * for the intent items to be released.
> - */
> - list_for_each_entry_safe(dfp, pli, &dop->dop_intake, dfp_list) {
> - trace_xfs_defer_intake_cancel(NULL, dfp);
> - list_del(&dfp->dfp_list);
> - list_for_each_safe(pwi, n, &dfp->dfp_work) {
> - list_del(pwi);
> - dfp->dfp_count--;
> - dfp->dfp_type->cancel_item(pwi);
> - }
> - ASSERT(dfp->dfp_count == 0);
> - kmem_free(dfp);
> - }
> - list_for_each_entry_safe(dfp, pli, &dop->dop_pending, dfp_list) {
> - trace_xfs_defer_pending_cancel(NULL, dfp);
> - list_del(&dfp->dfp_list);
> - list_for_each_safe(pwi, n, &dfp->dfp_work) {
> - list_del(pwi);
> - dfp->dfp_count--;
> - dfp->dfp_type->cancel_item(pwi);
> - }
> - ASSERT(dfp->dfp_count == 0);
> - kmem_free(dfp);
> - }
> + trace_xfs_defer_cancel(mp, tp->t_dfops, _RET_IP_);
> + xfs_defer_cancel_list(mp, &tp->t_dfops->dop_intake);
> }
>
> /* Add an item for later deferred processing. */
> @@ -547,7 +542,6 @@ xfs_defer_init(
>
> memset(dop, 0, sizeof(struct xfs_defer_ops));
> INIT_LIST_HEAD(&dop->dop_intake);
> - INIT_LIST_HEAD(&dop->dop_pending);
> if (tp) {
> ASSERT(tp->t_firstblock == NULLFSBLOCK);
> tp->t_dfops = dop;
> @@ -571,7 +565,6 @@ xfs_defer_move(
> ASSERT(dst != src);
>
> list_splice_init(&src->dop_intake, &dst->dop_intake);
> - list_splice_init(&src->dop_pending, &dst->dop_pending);
>
> /*
> * Low free space mode was historically controlled by a dfops field.
> diff --git a/fs/xfs/libxfs/xfs_defer.h b/fs/xfs/libxfs/xfs_defer.h
> index f051c8056141..f091bf3abeaf 100644
> --- a/fs/xfs/libxfs/xfs_defer.h
> +++ b/fs/xfs/libxfs/xfs_defer.h
> @@ -41,7 +41,6 @@ int xfs_defer_finish_noroll(struct xfs_trans **tp);
> int xfs_defer_finish(struct xfs_trans **tp);
> void xfs_defer_cancel(struct xfs_trans *);
> void xfs_defer_init(struct xfs_trans *tp, struct xfs_defer_ops *dop);
> -bool xfs_defer_has_unfinished_work(struct xfs_trans *tp);
> void xfs_defer_move(struct xfs_trans *dtp, struct xfs_trans *stp);
>
> /* Description of a deferred type. */
> diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
> index 8807f1bb814a..fec9cfe3dfb4 100644
> --- a/fs/xfs/xfs_trace.h
> +++ b/fs/xfs/xfs_trace.h
> @@ -2392,9 +2392,8 @@ DEFINE_DEFER_EVENT(xfs_defer_finish_done);
> DEFINE_DEFER_ERROR_EVENT(xfs_defer_trans_roll_error);
> DEFINE_DEFER_ERROR_EVENT(xfs_defer_finish_error);
>
> -DEFINE_DEFER_PENDING_EVENT(xfs_defer_intake_work);
> -DEFINE_DEFER_PENDING_EVENT(xfs_defer_intake_cancel);
> -DEFINE_DEFER_PENDING_EVENT(xfs_defer_pending_cancel);
> +DEFINE_DEFER_PENDING_EVENT(xfs_defer_create_intent);
> +DEFINE_DEFER_PENDING_EVENT(xfs_defer_cancel_list);
> DEFINE_DEFER_PENDING_EVENT(xfs_defer_pending_finish);
> DEFINE_DEFER_PENDING_EVENT(xfs_defer_pending_abort);
>
> diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
> index 7c99aa6c04e2..413e4138357f 100644
> --- a/fs/xfs/xfs_trans.c
> +++ b/fs/xfs/xfs_trans.c
> @@ -929,7 +929,7 @@ __xfs_trans_commit(
> * Finish deferred items on final commit. Only permanent transactions
> * should ever have deferred ops.
> */
> - WARN_ON_ONCE(xfs_defer_has_unfinished_work(tp->t_dfops) &&
> + WARN_ON_ONCE(!list_empty(&tp->t_dfops->dop_intake) &&
> !(tp->t_flags & XFS_TRANS_PERM_LOG_RES));
> if (!regrant && (tp->t_flags & XFS_TRANS_PERM_LOG_RES)) {
> error = xfs_defer_finish_noroll(&tp);
> diff --git a/fs/xfs/xfs_trans.h b/fs/xfs/xfs_trans.h
> index 299656dbf324..1cdc7c0ebeac 100644
> --- a/fs/xfs/xfs_trans.h
> +++ b/fs/xfs/xfs_trans.h
> @@ -96,7 +96,6 @@ void xfs_log_item_init(struct xfs_mount *mp, struct xfs_log_item *item,
> #define XFS_DEFER_OPS_NR_BUFS 2 /* join up to two buffers */
> struct xfs_defer_ops {
> struct list_head dop_intake; /* unlogged pending work */
> - struct list_head dop_pending; /* logged pending work */
> };
>
> /*
> --
> 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
