From: "Darrick J. Wong" <darrick.wong@oracle.com>
To: Brian Foster <bfoster@redhat.com>
Cc: linux-xfs@vger.kernel.org
Subject: Re: [PATCH v2 14/15] xfs: drop unnecessary xfs_defer_finish() dfops parameter
Date: Tue, 24 Jul 2018 13:55:09 -0700 [thread overview]
Message-ID: <20180724205509.GM4813@magnolia> (raw)
In-Reply-To: <20180723130414.47980-15-bfoster@redhat.com>
On Mon, Jul 23, 2018 at 09:04:13AM -0400, Brian Foster wrote:
> Every caller of xfs_defer_finish() now passes the transaction and
> its associated ->t_dfops. The xfs_defer_ops parameter is therefore
> no longer necessary and can be removed.
>
> Since most xfs_defer_finish() callers also have to consider
> xfs_defer_cancel() on error, update the latter to also receive the
> transaction for consistency. The log recovery code contains an
> outlier case that cancels a dfops directly without an available
> transaction. Retain an internal wrapper to support this outlier case
> for the time being.
>
> Signed-off-by: Brian Foster <bfoster@redhat.com>
Looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
--D
> ---
> fs/xfs/libxfs/xfs_attr.c | 27 +++++++++++++--------------
> fs/xfs/libxfs/xfs_attr_remote.c | 8 ++++----
> fs/xfs/libxfs/xfs_defer.c | 7 +++----
> fs/xfs/libxfs/xfs_defer.h | 4 ++--
> fs/xfs/xfs_bmap_util.c | 4 ++--
> fs/xfs/xfs_dquot.c | 4 ++--
> fs/xfs/xfs_inode.c | 4 ++--
> fs/xfs/xfs_log_recover.c | 2 +-
> fs/xfs/xfs_reflink.c | 8 ++++----
> fs/xfs/xfs_trans.c | 13 ++++++++++---
> fs/xfs/xfs_trans.h | 3 +++
> 11 files changed, 46 insertions(+), 38 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> index 3e98f0af389c..3deb5cdadf08 100644
> --- a/fs/xfs/libxfs/xfs_attr.c
> +++ b/fs/xfs/libxfs/xfs_attr.c
> @@ -322,7 +322,7 @@ xfs_attr_set(
> xfs_trans_bhold(args.trans, leaf_bp);
> xfs_defer_bjoin(args.trans->t_dfops, leaf_bp);
> xfs_defer_ijoin(args.trans->t_dfops, dp);
> - error = xfs_defer_finish(&args.trans, args.trans->t_dfops);
> + error = xfs_defer_finish(&args.trans);
> if (error)
> goto out;
>
> @@ -591,7 +591,7 @@ xfs_attr_leaf_addname(
> if (error)
> goto out_defer_cancel;
> xfs_defer_ijoin(args->trans->t_dfops, dp);
> - error = xfs_defer_finish(&args->trans, args->trans->t_dfops);
> + error = xfs_defer_finish(&args->trans);
> if (error)
> goto out_defer_cancel;
>
> @@ -680,7 +680,7 @@ xfs_attr_leaf_addname(
> if (error)
> goto out_defer_cancel;
> xfs_defer_ijoin(args->trans->t_dfops, dp);
> - error = xfs_defer_finish(&args->trans, args->trans->t_dfops);
> + error = xfs_defer_finish(&args->trans);
> if (error)
> goto out_defer_cancel;
> }
> @@ -698,7 +698,7 @@ xfs_attr_leaf_addname(
> }
> return error;
> out_defer_cancel:
> - xfs_defer_cancel(args->trans->t_dfops);
> + xfs_defer_cancel(args->trans);
> return error;
> }
>
> @@ -744,13 +744,13 @@ xfs_attr_leaf_removename(
> if (error)
> goto out_defer_cancel;
> xfs_defer_ijoin(args->trans->t_dfops, dp);
> - error = xfs_defer_finish(&args->trans, args->trans->t_dfops);
> + error = xfs_defer_finish(&args->trans);
> if (error)
> goto out_defer_cancel;
> }
> return 0;
> out_defer_cancel:
> - xfs_defer_cancel(args->trans->t_dfops);
> + xfs_defer_cancel(args->trans);
> return error;
> }
>
> @@ -871,8 +871,7 @@ xfs_attr_node_addname(
> if (error)
> goto out_defer_cancel;
> xfs_defer_ijoin(args->trans->t_dfops, dp);
> - error = xfs_defer_finish(&args->trans,
> - args->trans->t_dfops);
> + error = xfs_defer_finish(&args->trans);
> if (error)
> goto out_defer_cancel;
>
> @@ -897,7 +896,7 @@ xfs_attr_node_addname(
> if (error)
> goto out_defer_cancel;
> xfs_defer_ijoin(args->trans->t_dfops, dp);
> - error = xfs_defer_finish(&args->trans, args->trans->t_dfops);
> + error = xfs_defer_finish(&args->trans);
> if (error)
> goto out_defer_cancel;
> } else {
> @@ -994,7 +993,7 @@ xfs_attr_node_addname(
> if (error)
> goto out_defer_cancel;
> xfs_defer_ijoin(args->trans->t_dfops, dp);
> - error = xfs_defer_finish(&args->trans, args->trans->t_dfops);
> + error = xfs_defer_finish(&args->trans);
> if (error)
> goto out_defer_cancel;
> }
> @@ -1023,7 +1022,7 @@ xfs_attr_node_addname(
> return error;
> return retval;
> out_defer_cancel:
> - xfs_defer_cancel(args->trans->t_dfops);
> + xfs_defer_cancel(args->trans);
> goto out;
> }
>
> @@ -1118,7 +1117,7 @@ xfs_attr_node_removename(
> if (error)
> goto out_defer_cancel;
> xfs_defer_ijoin(args->trans->t_dfops, dp);
> - error = xfs_defer_finish(&args->trans, args->trans->t_dfops);
> + error = xfs_defer_finish(&args->trans);
> if (error)
> goto out_defer_cancel;
> /*
> @@ -1150,7 +1149,7 @@ xfs_attr_node_removename(
> if (error)
> goto out_defer_cancel;
> xfs_defer_ijoin(args->trans->t_dfops, dp);
> - error = xfs_defer_finish(&args->trans, args->trans->t_dfops);
> + error = xfs_defer_finish(&args->trans);
> if (error)
> goto out_defer_cancel;
> } else
> @@ -1162,7 +1161,7 @@ xfs_attr_node_removename(
> xfs_da_state_free(state);
> return error;
> out_defer_cancel:
> - xfs_defer_cancel(args->trans->t_dfops);
> + xfs_defer_cancel(args->trans);
> goto out;
> }
>
> diff --git a/fs/xfs/libxfs/xfs_attr_remote.c b/fs/xfs/libxfs/xfs_attr_remote.c
> index 0fbfb740949e..77ca38586913 100644
> --- a/fs/xfs/libxfs/xfs_attr_remote.c
> +++ b/fs/xfs/libxfs/xfs_attr_remote.c
> @@ -487,7 +487,7 @@ xfs_attr_rmtval_set(
> if (error)
> goto out_defer_cancel;
> xfs_defer_ijoin(args->trans->t_dfops, dp);
> - error = xfs_defer_finish(&args->trans, args->trans->t_dfops);
> + error = xfs_defer_finish(&args->trans);
> if (error)
> goto out_defer_cancel;
>
> @@ -555,7 +555,7 @@ xfs_attr_rmtval_set(
> ASSERT(valuelen == 0);
> return 0;
> out_defer_cancel:
> - xfs_defer_cancel(args->trans->t_dfops);
> + xfs_defer_cancel(args->trans);
> return error;
> }
>
> @@ -628,7 +628,7 @@ xfs_attr_rmtval_remove(
> if (error)
> goto out_defer_cancel;
> xfs_defer_ijoin(args->trans->t_dfops, args->dp);
> - error = xfs_defer_finish(&args->trans, args->trans->t_dfops);
> + error = xfs_defer_finish(&args->trans);
> if (error)
> goto out_defer_cancel;
>
> @@ -641,6 +641,6 @@ xfs_attr_rmtval_remove(
> }
> return 0;
> out_defer_cancel:
> - xfs_defer_cancel(args->trans->t_dfops);
> + xfs_defer_cancel(args->trans);
> return error;
> }
> diff --git a/fs/xfs/libxfs/xfs_defer.c b/fs/xfs/libxfs/xfs_defer.c
> index b63cc9e730da..cbee0a86c978 100644
> --- a/fs/xfs/libxfs/xfs_defer.c
> +++ b/fs/xfs/libxfs/xfs_defer.c
> @@ -342,9 +342,9 @@ xfs_defer_reset(
> */
> int
> xfs_defer_finish(
> - struct xfs_trans **tp,
> - struct xfs_defer_ops *dop)
> + struct xfs_trans **tp)
> {
> + struct xfs_defer_ops *dop = (*tp)->t_dfops;
> struct xfs_defer_pending *dfp;
> struct list_head *li;
> struct list_head *n;
> @@ -353,7 +353,6 @@ xfs_defer_finish(
> void (*cleanup_fn)(struct xfs_trans *, void *, int);
>
> ASSERT((*tp)->t_flags & XFS_TRANS_PERM_LOG_RES);
> - ASSERT((*tp)->t_dfops == dop);
>
> trace_xfs_defer_finish((*tp)->t_mountp, dop, _RET_IP_);
>
> @@ -454,7 +453,7 @@ xfs_defer_finish(
> * Free up any items left in the list.
> */
> void
> -xfs_defer_cancel(
> +__xfs_defer_cancel(
> struct xfs_defer_ops *dop)
> {
> struct xfs_defer_pending *dfp;
> diff --git a/fs/xfs/libxfs/xfs_defer.h b/fs/xfs/libxfs/xfs_defer.h
> index 35507ca9a148..56f927803940 100644
> --- a/fs/xfs/libxfs/xfs_defer.h
> +++ b/fs/xfs/libxfs/xfs_defer.h
> @@ -48,8 +48,8 @@ enum xfs_defer_ops_type {
>
> void xfs_defer_add(struct xfs_defer_ops *dop, enum xfs_defer_ops_type type,
> struct list_head *h);
> -int xfs_defer_finish(struct xfs_trans **tp, struct xfs_defer_ops *dop);
> -void xfs_defer_cancel(struct xfs_defer_ops *dop);
> +int xfs_defer_finish(struct xfs_trans **tp);
> +void __xfs_defer_cancel(struct xfs_defer_ops *dop);
> void xfs_defer_init(struct xfs_trans *tp, struct xfs_defer_ops *dop);
> bool xfs_defer_has_unfinished_work(struct xfs_defer_ops *dop);
> int xfs_defer_ijoin(struct xfs_defer_ops *dop, struct xfs_inode *ip);
> diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
> index 6b0f31402a81..7b40e9fef381 100644
> --- a/fs/xfs/xfs_bmap_util.c
> +++ b/fs/xfs/xfs_bmap_util.c
> @@ -1624,7 +1624,7 @@ xfs_swap_extent_rmap(
> goto out_defer;
>
> xfs_defer_ijoin(tp->t_dfops, ip);
> - error = xfs_defer_finish(tpp, tp->t_dfops);
> + error = xfs_defer_finish(tpp);
> tp = *tpp;
> if (error)
> goto out_defer;
> @@ -1645,7 +1645,7 @@ xfs_swap_extent_rmap(
> return 0;
>
> out_defer:
> - xfs_defer_cancel(tp->t_dfops);
> + xfs_defer_cancel(tp);
> out:
> trace_xfs_swap_extent_rmap_error(ip, error, _RET_IP_);
> tip->i_d.di_flags2 = tip_flags2;
> diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
> index a57d5e8c3118..da5c55cec966 100644
> --- a/fs/xfs/xfs_dquot.c
> +++ b/fs/xfs/xfs_dquot.c
> @@ -368,7 +368,7 @@ xfs_dquot_disk_alloc(
> xfs_trans_brelse(tp, bp);
> goto error1;
> }
> - error = xfs_defer_finish(tpp, tp->t_dfops);
> + error = xfs_defer_finish(tpp);
> tp = *tpp;
> if (error) {
> xfs_buf_relse(bp);
> @@ -378,7 +378,7 @@ xfs_dquot_disk_alloc(
> return 0;
>
> error1:
> - xfs_defer_cancel(tp->t_dfops);
> + xfs_defer_cancel(tp);
> error0:
> return error;
> }
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index c47183a2f167..0e4bd559a6a7 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -1570,7 +1570,7 @@ xfs_itruncate_extents_flags(
> * reservation and commit the old transaction.
> */
> xfs_defer_ijoin(tp->t_dfops, ip);
> - error = xfs_defer_finish(&tp, tp->t_dfops);
> + error = xfs_defer_finish(&tp);
> if (error)
> goto out_bmap_cancel;
>
> @@ -1606,7 +1606,7 @@ xfs_itruncate_extents_flags(
> * the transaction can be properly aborted. We just need to make sure
> * we're not holding any resources that we were not when we came in.
> */
> - xfs_defer_cancel(tp->t_dfops);
> + xfs_defer_cancel(tp);
> goto out;
> }
>
> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> index 265e1f561157..94908a4019e1 100644
> --- a/fs/xfs/xfs_log_recover.c
> +++ b/fs/xfs/xfs_log_recover.c
> @@ -4946,7 +4946,7 @@ xlog_recover_process_intents(
> xfs_trans_ail_cursor_done(&cur);
> spin_unlock(&ailp->ail_lock);
> if (error)
> - xfs_defer_cancel(&dfops);
> + __xfs_defer_cancel(&dfops);
> else
> error = xlog_finish_defer_ops(log->l_mp, &dfops);
>
> diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
> index 865ba615dd76..cddde219630a 100644
> --- a/fs/xfs/xfs_reflink.c
> +++ b/fs/xfs/xfs_reflink.c
> @@ -518,9 +518,9 @@ xfs_reflink_cancel_cow_blocks(
>
> /* Roll the transaction */
> xfs_defer_ijoin((*tpp)->t_dfops, ip);
> - error = xfs_defer_finish(tpp, (*tpp)->t_dfops);
> + error = xfs_defer_finish(tpp);
> if (error) {
> - xfs_defer_cancel((*tpp)->t_dfops);
> + xfs_defer_cancel(*tpp);
> break;
> }
>
> @@ -716,7 +716,7 @@ xfs_reflink_end_cow(
> xfs_bmap_del_extent_cow(ip, &icur, &got, &del);
>
> xfs_defer_ijoin(tp->t_dfops, ip);
> - error = xfs_defer_finish(&tp, tp->t_dfops);
> + error = xfs_defer_finish(&tp);
> if (error)
> goto out_cancel;
> if (!xfs_iext_get_extent(ifp, &icur, &got))
> @@ -1077,7 +1077,7 @@ xfs_reflink_remap_extent(
> next_extent:
> /* Process all the deferred stuff. */
> xfs_defer_ijoin(tp->t_dfops, ip);
> - error = xfs_defer_finish(&tp, tp->t_dfops);
> + error = xfs_defer_finish(&tp);
> if (error)
> goto out_cancel;
> }
> diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
> index 412c8d236c71..cd553aa9ecb0 100644
> --- a/fs/xfs/xfs_trans.c
> +++ b/fs/xfs/xfs_trans.c
> @@ -933,9 +933,9 @@ __xfs_trans_commit(
>
> /* finish deferred items on final commit */
> if (!regrant && tp->t_dfops) {
> - error = xfs_defer_finish(&tp, tp->t_dfops);
> + error = xfs_defer_finish(&tp);
> if (error) {
> - xfs_defer_cancel(tp->t_dfops);
> + xfs_defer_cancel(tp);
> goto out_unreserve;
> }
> }
> @@ -1030,7 +1030,7 @@ xfs_trans_cancel(
> trace_xfs_trans_cancel(tp, _RET_IP_);
>
> if (tp->t_dfops)
> - xfs_defer_cancel(tp->t_dfops);
> + xfs_defer_cancel(tp);
>
> /*
> * See if the caller is relying on us to shut down the
> @@ -1111,3 +1111,10 @@ xfs_trans_roll(
> tres.tr_logflags = XFS_TRANS_PERM_LOG_RES;
> return xfs_trans_reserve(*tpp, &tres, 0, 0);
> }
> +
> +void
> +xfs_defer_cancel(
> + struct xfs_trans *tp)
> +{
> + __xfs_defer_cancel(tp->t_dfops);
> +}
> diff --git a/fs/xfs/xfs_trans.h b/fs/xfs/xfs_trans.h
> index dc79e3c1d3e8..5170e89bec02 100644
> --- a/fs/xfs/xfs_trans.h
> +++ b/fs/xfs/xfs_trans.h
> @@ -214,6 +214,9 @@ xfs_trans_read_buf(
> flags, bpp, ops);
> }
>
> +/* cancel dfops associated with a transaction */
> +void xfs_defer_cancel(struct xfs_trans *);
> +
> struct xfs_buf *xfs_trans_getsb(xfs_trans_t *, struct xfs_mount *, int);
>
> void xfs_trans_brelse(xfs_trans_t *, struct xfs_buf *);
> --
> 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
next prev parent reply other threads:[~2018-07-24 22:33 UTC|newest]
Thread overview: 57+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-07-23 13:03 [PATCH v2 00/15] xfs: embed dfops in the transaction Brian Foster
2018-07-23 13:04 ` [PATCH v2 01/15] xfs: pull up dfops from xfs_itruncate_extents() Brian Foster
2018-07-23 20:37 ` Bill O'Donnell
2018-07-24 20:27 ` Darrick J. Wong
2018-07-23 13:04 ` [PATCH v2 02/15] xfs: use ->t_dfops in log recovery intent processing Brian Foster
2018-07-23 20:38 ` Bill O'Donnell
2018-07-24 20:27 ` Darrick J. Wong
2018-07-23 13:04 ` [PATCH v2 03/15] xfs: fix transaction leak on remote attr set/remove failure Brian Foster
2018-07-23 20:39 ` Bill O'Donnell
2018-07-24 20:27 ` Darrick J. Wong
2018-07-23 13:04 ` [PATCH v2 04/15] xfs: make deferred processing safe for embedded dfops Brian Foster
2018-07-23 20:45 ` Bill O'Donnell
2018-07-24 20:45 ` Darrick J. Wong
2018-07-23 13:04 ` [PATCH v2 05/15] xfs: remove unused deferred ops committed field Brian Foster
2018-07-23 20:46 ` Bill O'Donnell
2018-07-24 20:28 ` Darrick J. Wong
2018-07-23 13:04 ` [PATCH v2 06/15] xfs: reset dfops to initial state after finish Brian Foster
2018-07-24 12:53 ` Bill O'Donnell
2018-07-24 20:46 ` Darrick J. Wong
2018-07-23 13:04 ` [PATCH v2 07/15] xfs: pack holes in xfs_defer_ops and xfs_trans Brian Foster
2018-07-23 20:48 ` Bill O'Donnell
2018-07-24 20:46 ` Darrick J. Wong
2018-07-23 13:04 ` [PATCH v2 08/15] xfs: support embedded dfops in transaction Brian Foster
2018-07-24 12:56 ` Bill O'Donnell
2018-07-24 20:51 ` Darrick J. Wong
2018-07-25 5:04 ` Christoph Hellwig
2018-07-23 13:04 ` [PATCH v2 09/15] xfs: use internal dfops in cow blocks cancel Brian Foster
2018-07-23 20:49 ` Bill O'Donnell
2018-07-24 20:53 ` Darrick J. Wong
2018-07-25 5:04 ` Christoph Hellwig
2018-07-23 13:04 ` [PATCH v2 10/15] xfs: use internal dfops in attr code Brian Foster
2018-07-23 20:51 ` Bill O'Donnell
2018-07-24 20:53 ` Darrick J. Wong
2018-07-25 5:04 ` Christoph Hellwig
2018-07-23 13:04 ` [PATCH v2 11/15] xfs: use internal dfops during [b|c]ui recovery Brian Foster
2018-07-24 13:02 ` Bill O'Donnell
2018-07-24 20:53 ` Darrick J. Wong
2018-07-25 5:05 ` Christoph Hellwig
2018-07-23 13:04 ` [PATCH v2 12/15] xfs: remove all boilerplate defer init/finish code Brian Foster
2018-07-24 13:11 ` Bill O'Donnell
2018-07-24 20:54 ` Darrick J. Wong
2018-07-25 5:07 ` Christoph Hellwig
2018-07-23 13:04 ` [PATCH v2 13/15] xfs: remove unnecessary dfops init calls in xattr code Brian Foster
2018-07-24 13:13 ` Bill O'Donnell
2018-07-24 20:54 ` Darrick J. Wong
2018-07-25 5:07 ` Christoph Hellwig
2018-07-23 13:04 ` [PATCH v2 14/15] xfs: drop unnecessary xfs_defer_finish() dfops parameter Brian Foster
2018-07-24 13:16 ` Bill O'Donnell
2018-07-24 20:55 ` Darrick J. Wong [this message]
2018-07-25 5:11 ` Christoph Hellwig
2018-07-25 11:09 ` Brian Foster
2018-07-25 11:34 ` Christoph Hellwig
2018-07-23 13:04 ` [PATCH v2 15/15] xfs: bypass final dfops roll in trans commit path Brian Foster
2018-07-24 13:25 ` Bill O'Donnell
2018-07-24 20:55 ` Darrick J. Wong
2018-07-25 5:16 ` Christoph Hellwig
2018-07-25 11:12 ` [PATCH v3 " Brian Foster
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20180724205509.GM4813@magnolia \
--to=darrick.wong@oracle.com \
--cc=bfoster@redhat.com \
--cc=linux-xfs@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).