On Mon, Jul 30, 2018 at 12:45:17PM -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>
> ---
> fs/xfs/libxfs/xfs_defer.c | 80 ++++++++++++++++++++-------------------
> fs/xfs/libxfs/xfs_defer.h | 3 +-
> fs/xfs/xfs_trace.h | 1 -
> fs/xfs/xfs_trans.h | 1 -
> 4 files changed, 43 insertions(+), 42 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_defer.c b/fs/xfs/libxfs/xfs_defer.c
> index 66848ede62c0..6bf792e2d61b 100644
> --- a/fs/xfs/libxfs/xfs_defer.c
> +++ b/fs/xfs/libxfs/xfs_defer.c
> @@ -174,6 +174,8 @@
>
> static const struct xfs_defer_op_type *defer_op_types[XFS_DEFER_OPS_TYPE_MAX];
>
> +static void __xfs_defer_cancel(struct list_head *);
> +
> /*
> * For each pending item in the intake list, log its intent item and the
> * associated extents, then add the entire intake list to the end of
> @@ -181,7 +183,8 @@ static const struct xfs_defer_op_type *defer_op_types[XFS_DEFER_OPS_TYPE_MAX];
> */
> STATIC void
> xfs_defer_intake_work(
> - struct xfs_trans *tp)
> + struct xfs_trans *tp,
> + struct list_head *dop_pending)
> {
> struct xfs_defer_ops *dop = tp->t_dfops;
> struct list_head *li;
> @@ -197,13 +200,14 @@ xfs_defer_intake_work(
> dfp->dfp_type->log_item(tp, dfp->dfp_intent, li);
> }
>
> - list_splice_tail_init(&dop->dop_intake, &dop->dop_pending);
> + list_splice_tail_init(&dop->dop_intake, 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 +216,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) {
/me kinda wonders if you could just make xfs_defer_finish pass an empty
list_head into xfs_defer_trans_roll to avoid this extra indent, but I
don't care all that much and maybe we should save the stack space. :)
> + 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 +234,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;
> @@ -272,7 +279,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;
> }
>
> @@ -292,9 +299,10 @@ xfs_defer_trans_roll(
> /* Do we have any work items to finish? */
> bool
> xfs_defer_has_unfinished_work(
> - struct xfs_trans *tp)
> + struct xfs_trans *tp,
> + struct list_head *dop_pending)
> {
> - return !list_empty(&tp->t_dfops->dop_pending) ||
> + return !list_empty(dop_pending) ||
> !list_empty(&tp->t_dfops->dop_intake);
> }
>
> @@ -305,7 +313,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
> @@ -332,26 +340,27 @@ 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)) {
> + while (xfs_defer_has_unfinished_work(*tp, &dop_pending)) {
<urk> see discussion below.
> /* Log intents for work items sitting in the intake. */
> - xfs_defer_intake_work(*tp);
> + xfs_defer_intake_work(*tp, &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);
> @@ -381,7 +390,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;
> }
> }
> @@ -413,6 +422,7 @@ xfs_defer_finish_noroll(
> if (error) {
> trace_xfs_defer_finish_error((*tp)->t_mountp, (*tp)->t_dfops,
> error);
> + __xfs_defer_cancel(&dop_pending);
> xfs_defer_cancel(*tp);
> return error;
> }
> @@ -435,7 +445,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;
> }
> @@ -446,34 +456,20 @@ xfs_defer_finish(
> /*
> * Free up any items left in the list.
> */
> -void
> -xfs_defer_cancel(
> - struct xfs_trans *tp)
> +static void
> +__xfs_defer_cancel(
Maybe this should be xfs_defer_cancel_list?
> + struct list_head *dop_list)
> {
> - 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_);
> -
> /*
> * 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) {
> + list_for_each_entry_safe(dfp, pli, dop_list, dfp_list) {
> trace_xfs_defer_pending_cancel(NULL, dfp);
trace_xfs_defer_cancel_list?
Maybe it's worth passing a *mp into this function so we can pass it to
the tracepoint?
> list_del(&dfp->dfp_list);
> list_for_each_safe(pwi, n, &dfp->dfp_work) {
> @@ -486,6 +482,14 @@ xfs_defer_cancel(
> }
> }
>
> +void
> +xfs_defer_cancel(
> + struct xfs_trans *tp)
> +{
> + trace_xfs_defer_cancel(NULL, tp->t_dfops, _RET_IP_);
That NULL can be tp->t_mountp, right? Since tp should never be NULL?
> + __xfs_defer_cancel(&tp->t_dfops->dop_intake);
> +}
> +
> /* Add an item for later deferred processing. */
> void
> xfs_defer_add(
> @@ -541,7 +545,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;
> @@ -565,7 +568,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..363af16328cb 100644
> --- a/fs/xfs/libxfs/xfs_defer.h
> +++ b/fs/xfs/libxfs/xfs_defer.h
> @@ -41,7 +41,8 @@ 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);
> +bool xfs_defer_has_unfinished_work(struct xfs_trans *tp,
> + struct list_head *dop_pending);
Eww, I dislike exposing this implementation detail in a public
interface. Assuming you take my suggestion in patch 1 to ASSERT if
xfs_trans_{cancel,commit} encounter unfinished deferred work and a
transaction we can't roll, there will be only three callers of this
function, and only one of them knows about the pending item list.
Perhaps the list_empty(dop_pending) check can be open-coded in
xfs_defer_finish_noroll?
--D
> 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..6b55bbc09578 100644
> --- a/fs/xfs/xfs_trace.h
> +++ b/fs/xfs/xfs_trace.h
> @@ -2393,7 +2393,6 @@ 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_pending_finish);
> DEFINE_DEFER_PENDING_EVENT(xfs_defer_pending_abort);
> 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
