From: "Darrick J. Wong" <darrick.wong@oracle.com>
To: Dave Chinner <david@fromorbit.com>
Cc: linux-xfs@vger.kernel.org
Subject: Re: [PATCH 06/24] xfs: call xfs_buf_iodone directly
Date: Fri, 22 May 2020 14:53:36 -0700 [thread overview]
Message-ID: <20200522215336.GJ8230@magnolia> (raw)
In-Reply-To: <20200522035029.3022405-7-david@fromorbit.com>
On Fri, May 22, 2020 at 01:50:11PM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> All unmarked dirty buffers should be in the AIL and have log items
> attached to them. Hence when they are written, we will run a
> callback to remove the item from the AIL if appropriate. Now that
> we've handled inode and dquot buffers, all remaining calls are to
> xfs_buf_iodone() and so we can hard code this rather than use an
> indirect call.
>
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
> fs/xfs/xfs_buf.c | 24 +++++++++---------------
> fs/xfs/xfs_buf.h | 6 +-----
> fs/xfs/xfs_buf_item.c | 38 +++++++++-----------------------------
> fs/xfs/xfs_buf_item.h | 2 +-
> fs/xfs/xfs_trans_buf.c | 9 +--------
> 5 files changed, 21 insertions(+), 58 deletions(-)
>
> diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> index b89685ce8519d..5fc83637f7aff 100644
> --- a/fs/xfs/xfs_buf.c
> +++ b/fs/xfs/xfs_buf.c
> @@ -658,7 +658,6 @@ xfs_buf_find(
> */
> if (bp->b_flags & XBF_STALE) {
> ASSERT((bp->b_flags & _XBF_DELWRI_Q) == 0);
> - ASSERT(bp->b_iodone == NULL);
> bp->b_flags &= _XBF_KMEM | _XBF_PAGES;
> bp->b_ops = NULL;
> }
> @@ -1194,10 +1193,13 @@ xfs_buf_ioend(
> if (!bp->b_error && bp->b_io_error)
> xfs_buf_ioerror(bp, bp->b_io_error);
>
> - /* Only validate buffers that were read without errors */
> - if (read && !bp->b_error && bp->b_ops) {
> - ASSERT(!bp->b_iodone);
> - bp->b_ops->verify_read(bp);
> + if (read) {
> + if (!bp->b_error && bp->b_ops)
> + bp->b_ops->verify_read(bp);
> + if (!bp->b_error)
> + bp->b_flags |= XBF_DONE;
> + xfs_buf_ioend_finish(bp);
> + return;
> }
>
> if (!bp->b_error) {
> @@ -1205,9 +1207,6 @@ xfs_buf_ioend(
> bp->b_flags |= XBF_DONE;
> }
>
> - if (read)
> - goto out_finish;
> -
> /*
> * If this is a log recovery buffer, we aren't doing transactional IO
> * yet so we need to let it handle IO completions.
> @@ -1229,13 +1228,8 @@ xfs_buf_ioend(
> return;
> }
>
> - if (bp->b_iodone) {
> - (*(bp->b_iodone))(bp);
> - return;
> - }
> -
> -out_finish:
> - xfs_buf_ioend_finish(bp);
> + /* dirty buffers always need to be removed from the AIL */
> + xfs_buf_dirty_iodone(bp);
> }
>
> static void
> diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h
> index c5fe4c48c9080..a127d56d5eff0 100644
> --- a/fs/xfs/xfs_buf.h
> +++ b/fs/xfs/xfs_buf.h
> @@ -18,6 +18,7 @@
> /*
> * Base types
> */
> +struct xfs_buf;
>
> #define XFS_BUF_DADDR_NULL ((xfs_daddr_t) (-1LL))
>
> @@ -103,10 +104,6 @@ typedef struct xfs_buftarg {
> struct ratelimit_state bt_ioerror_rl;
> } xfs_buftarg_t;
>
> -struct xfs_buf;
> -typedef void (*xfs_buf_iodone_t)(struct xfs_buf *);
> -
> -
> #define XB_PAGES 2
>
> struct xfs_buf_map {
> @@ -159,7 +156,6 @@ typedef struct xfs_buf {
> xfs_buftarg_t *b_target; /* buffer target (device) */
> void *b_addr; /* virtual address of buffer */
> struct work_struct b_ioend_work;
> - xfs_buf_iodone_t b_iodone; /* I/O completion function */
> struct completion b_iowait; /* queue for I/O waiters */
> struct xfs_buf_log_item *b_log_item;
> struct list_head b_li_list; /* Log items list head */
> diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
> index a42cdf9ccc47d..c2e7d14e35c66 100644
> --- a/fs/xfs/xfs_buf_item.c
> +++ b/fs/xfs/xfs_buf_item.c
> @@ -460,7 +460,6 @@ xfs_buf_item_unpin(
> xfs_buf_do_callbacks(bp);
> bp->b_log_item = NULL;
> list_del_init(&bp->b_li_list);
> - bp->b_iodone = NULL;
> } else {
> xfs_trans_ail_delete(lip, SHUTDOWN_LOG_IO_ERROR);
> xfs_buf_item_relse(bp);
> @@ -936,11 +935,7 @@ xfs_buf_item_free(
> }
>
> /*
> - * This is called when the buf log item is no longer needed. It should
> - * free the buf log item associated with the given buffer and clear
> - * the buffer's pointer to the buf log item. If there are no more
> - * items in the list, clear the b_iodone field of the buffer (see
> - * xfs_buf_attach_iodone() below).
> + * xfs_buf_item_relse() is called when the buf log item is no longer needed.
> */
> void
> xfs_buf_item_relse(
> @@ -952,9 +947,6 @@ xfs_buf_item_relse(
> ASSERT(!test_bit(XFS_LI_IN_AIL, &bip->bli_item.li_flags));
>
> bp->b_log_item = NULL;
> - if (list_empty(&bp->b_li_list))
> - bp->b_iodone = NULL;
> -
> xfs_buf_rele(bp);
> xfs_buf_item_free(bip);
> }
> @@ -962,10 +954,7 @@ xfs_buf_item_relse(
>
> /*
> * Add the given log item with its callback to the list of callbacks
> - * to be called when the buffer's I/O completes. If it is not set
> - * already, set the buffer's b_iodone() routine to be
> - * xfs_buf_iodone_callbacks() and link the log item into the list of
> - * items rooted at b_li_list.
> + * to be called when the buffer's I/O completes.
> */
> void
> xfs_buf_attach_iodone(
> @@ -977,10 +966,6 @@ xfs_buf_attach_iodone(
>
> lip->li_cb = cb;
> list_add_tail(&lip->li_bio_list, &bp->b_li_list);
> -
> - ASSERT(bp->b_iodone == NULL ||
> - bp->b_iodone == xfs_buf_iodone_callbacks);
> - bp->b_iodone = xfs_buf_iodone_callbacks;
> }
>
> /*
> @@ -1096,7 +1081,6 @@ xfs_buf_iodone_callback_error(
> goto out_stale;
>
> trace_xfs_buf_item_iodone_async(bp, _RET_IP_);
> - ASSERT(bp->b_iodone != NULL);
>
> cfg = xfs_error_get_cfg(mp, XFS_ERR_METADATA, bp->b_error);
>
> @@ -1182,28 +1166,24 @@ xfs_buf_run_callbacks(
> xfs_buf_do_callbacks(bp);
> bp->b_log_item = NULL;
> list_del_init(&bp->b_li_list);
> - bp->b_iodone = NULL;
> }
>
> /*
> - * This is the iodone() function for buffers which have had callbacks attached
> - * to them by xfs_buf_attach_iodone(). We need to iterate the items on the
> - * callback list, mark the buffer as having no more callbacks and then push the
> - * buffer through IO completion processing.
> + * Inode buffer iodone callback function.
> */
> void
> -xfs_buf_iodone_callbacks(
> +xfs_buf_inode_iodone(
> struct xfs_buf *bp)
> {
> xfs_buf_run_callbacks(bp);
> - xfs_buf_ioend(bp);
> + xfs_buf_ioend_finish(bp);
> }
>
> /*
> - * Inode buffer iodone callback function.
> + * Dquot buffer iodone callback function.
> */
> void
> -xfs_buf_inode_iodone(
> +xfs_buf_dquot_iodone(
> struct xfs_buf *bp)
> {
> xfs_buf_run_callbacks(bp);
> @@ -1211,10 +1191,10 @@ xfs_buf_inode_iodone(
> }
>
> /*
> - * Dquot buffer iodone callback function.
> + * Dirty buffer iodone callback function.
> */
> void
> -xfs_buf_dquot_iodone(
> +xfs_buf_dirty_iodone(
git diff --patience would make the relatively little change going on
here much clearer:
@@ -1182,21 +1166,6 @@ xfs_buf_run_callbacks(
xfs_buf_do_callbacks(bp);
bp->b_log_item = NULL;
list_del_init(&bp->b_li_list);
- bp->b_iodone = NULL;
-}
-
-/*
- * This is the iodone() function for buffers which have had callbacks attached
- * to them by xfs_buf_attach_iodone(). We need to iterate the items on the
- * callback list, mark the buffer as having no more callbacks and then push the
- * buffer through IO completion processing.
- */
-void
-xfs_buf_iodone_callbacks(
- struct xfs_buf *bp)
-{
- xfs_buf_run_callbacks(bp);
- xfs_buf_ioend(bp);
}
/*
@@ -1221,6 +1190,17 @@ xfs_buf_dquot_iodone(
xfs_buf_ioend_finish(bp);
}
+/*
+ * Dirty buffer iodone callback function.
+ */
+void
+xfs_buf_dirty_iodone(
+ struct xfs_buf *bp)
+{
+ xfs_buf_run_callbacks(bp);
+ xfs_buf_ioend_finish(bp);
+}
+
/*
* This is the iodone() function for buffers which have been
* logged. It is called when they are eventually flushed out.
OTOH all the searching I had to do to figure out what was really going
on here persuaded me to go look at what this looked like at the end, so
I guess I'm not that worried.
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
--D
> struct xfs_buf *bp)
> {
> xfs_buf_run_callbacks(bp);
> diff --git a/fs/xfs/xfs_buf_item.h b/fs/xfs/xfs_buf_item.h
> index 27d13d29b5bbb..96f994ec90915 100644
> --- a/fs/xfs/xfs_buf_item.h
> +++ b/fs/xfs/xfs_buf_item.h
> @@ -57,10 +57,10 @@ bool xfs_buf_item_dirty_format(struct xfs_buf_log_item *);
> void xfs_buf_attach_iodone(struct xfs_buf *,
> void(*)(struct xfs_buf *, struct xfs_log_item *),
> struct xfs_log_item *);
> -void xfs_buf_iodone_callbacks(struct xfs_buf *);
> void xfs_buf_iodone(struct xfs_buf *, struct xfs_log_item *);
> void xfs_buf_inode_iodone(struct xfs_buf *);
> void xfs_buf_dquot_iodone(struct xfs_buf *);
> +void xfs_buf_dirty_iodone(struct xfs_buf *);
> bool xfs_buf_log_check_iovec(struct xfs_log_iovec *iovec);
>
> extern kmem_zone_t *xfs_buf_item_zone;
> diff --git a/fs/xfs/xfs_trans_buf.c b/fs/xfs/xfs_trans_buf.c
> index 93d62cb864c15..69e0ebe94a915 100644
> --- a/fs/xfs/xfs_trans_buf.c
> +++ b/fs/xfs/xfs_trans_buf.c
> @@ -465,23 +465,16 @@ xfs_trans_dirty_buf(
>
> ASSERT(bp->b_transp == tp);
> ASSERT(bip != NULL);
> - ASSERT(bp->b_iodone == NULL ||
> - bp->b_iodone == xfs_buf_iodone_callbacks);
>
> /*
> * Mark the buffer as needing to be written out eventually,
> * and set its iodone function to remove the buffer's buf log
> * item from the AIL and free it when the buffer is flushed
> - * to disk. See xfs_buf_attach_iodone() for more details
> - * on li_cb and xfs_buf_iodone_callbacks().
> - * If we end up aborting this transaction, we trap this buffer
> - * inside the b_bdstrat callback so that this won't get written to
> - * disk.
> + * to disk.
> */
> bp->b_flags |= XBF_DONE;
>
> ASSERT(atomic_read(&bip->bli_refcount) > 0);
> - bp->b_iodone = xfs_buf_iodone_callbacks;
> bip->bli_item.li_cb = xfs_buf_iodone;
>
> /*
> --
> 2.26.2.761.g0e0b3e54be
>
next prev parent reply other threads:[~2020-05-22 21:53 UTC|newest]
Thread overview: 91+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-05-22 3:50 [PATCH 00/24] xfs: rework inode flushing to make inode reclaim fully asynchronous Dave Chinner
2020-05-22 3:50 ` [PATCH 01/24] xfs: remove logged flag from inode log item Dave Chinner
2020-05-22 7:25 ` Christoph Hellwig
2020-05-22 21:13 ` Darrick J. Wong
2020-05-22 3:50 ` [PATCH 02/24] xfs: add an inode item lock Dave Chinner
2020-05-22 6:45 ` Amir Goldstein
2020-05-22 21:24 ` Darrick J. Wong
2020-05-23 8:45 ` Christoph Hellwig
2020-05-22 3:50 ` [PATCH 03/24] xfs: mark inode buffers in cache Dave Chinner
2020-05-22 7:45 ` Amir Goldstein
2020-05-22 21:35 ` Darrick J. Wong
2020-05-24 23:41 ` Dave Chinner
2020-05-23 8:48 ` Christoph Hellwig
2020-05-25 0:06 ` Dave Chinner
2020-05-22 3:50 ` [PATCH 04/24] xfs: mark dquot " Dave Chinner
2020-05-22 7:46 ` Amir Goldstein
2020-05-22 21:38 ` Darrick J. Wong
2020-05-22 3:50 ` [PATCH 05/24] xfs: mark log recovery buffers for completion Dave Chinner
2020-05-22 7:41 ` Amir Goldstein
2020-05-24 23:54 ` Dave Chinner
2020-05-22 21:41 ` Darrick J. Wong
2020-05-22 3:50 ` [PATCH 06/24] xfs: call xfs_buf_iodone directly Dave Chinner
2020-05-22 7:56 ` Amir Goldstein
2020-05-22 21:53 ` Darrick J. Wong [this message]
2020-05-22 3:50 ` [PATCH 07/24] xfs: clean up whacky buffer log item list reinit Dave Chinner
2020-05-22 22:01 ` Darrick J. Wong
2020-05-23 8:50 ` Christoph Hellwig
2020-05-22 3:50 ` [PATCH 08/24] xfs: fold xfs_istale_done into xfs_iflush_done Dave Chinner
2020-05-22 22:10 ` Darrick J. Wong
2020-05-23 9:12 ` Christoph Hellwig
2020-05-22 3:50 ` [PATCH 09/24] xfs: use direct calls for dquot IO completion Dave Chinner
2020-05-22 22:13 ` Darrick J. Wong
2020-05-23 9:16 ` Christoph Hellwig
2020-05-22 3:50 ` [PATCH 10/24] xfs: clean up the buffer iodone callback functions Dave Chinner
2020-05-22 22:26 ` Darrick J. Wong
2020-05-25 0:37 ` Dave Chinner
2020-05-23 9:19 ` Christoph Hellwig
2020-05-22 3:50 ` [PATCH 11/24] xfs: get rid of log item callbacks Dave Chinner
2020-05-22 22:27 ` Darrick J. Wong
2020-05-23 9:19 ` Christoph Hellwig
2020-05-22 3:50 ` [PATCH 12/24] xfs: pin inode backing buffer to the inode log item Dave Chinner
2020-05-22 22:39 ` Darrick J. Wong
2020-05-23 9:34 ` Christoph Hellwig
2020-05-23 21:43 ` Dave Chinner
2020-05-24 5:31 ` Christoph Hellwig
2020-05-24 23:13 ` Dave Chinner
2020-05-22 3:50 ` [PATCH 13/24] xfs: make inode reclaim almost non-blocking Dave Chinner
2020-05-22 12:19 ` Amir Goldstein
2020-05-22 22:48 ` Darrick J. Wong
2020-05-23 22:29 ` Dave Chinner
2020-05-22 3:50 ` [PATCH 14/24] xfs: remove IO submission from xfs_reclaim_inode() Dave Chinner
2020-05-22 23:06 ` Darrick J. Wong
2020-05-25 3:49 ` Dave Chinner
2020-05-23 9:40 ` Christoph Hellwig
2020-05-23 22:35 ` Dave Chinner
2020-05-22 3:50 ` [PATCH 15/24] xfs: allow multiple reclaimers per AG Dave Chinner
2020-05-22 23:10 ` Darrick J. Wong
2020-05-23 22:35 ` Dave Chinner
2020-05-22 3:50 ` [PATCH 16/24] xfs: don't block inode reclaim on the ILOCK Dave Chinner
2020-05-22 23:11 ` Darrick J. Wong
2020-05-22 3:50 ` [PATCH 17/24] xfs: remove SYNC_TRYLOCK from inode reclaim Dave Chinner
2020-05-22 23:14 ` Darrick J. Wong
2020-05-23 22:42 ` Dave Chinner
2020-05-22 3:50 ` [PATCH 18/24] xfs: clean up inode reclaim comments Dave Chinner
2020-05-22 23:17 ` Darrick J. Wong
2020-05-22 3:50 ` [PATCH 19/24] xfs: attach inodes to the cluster buffer when dirtied Dave Chinner
2020-05-22 23:48 ` Darrick J. Wong
2020-05-23 22:59 ` Dave Chinner
2020-05-22 3:50 ` [PATCH 20/24] xfs: xfs_iflush() is no longer necessary Dave Chinner
2020-05-22 23:54 ` Darrick J. Wong
2020-05-22 3:50 ` [PATCH 21/24] xfs: rename xfs_iflush_int() Dave Chinner
2020-05-22 12:33 ` Amir Goldstein
2020-05-22 23:57 ` Darrick J. Wong
2020-05-22 3:50 ` [PATCH 22/24] xfs: rework xfs_iflush_cluster() dirty inode iteration Dave Chinner
2020-05-23 0:13 ` Darrick J. Wong
2020-05-23 23:14 ` Dave Chinner
2020-05-23 11:31 ` Christoph Hellwig
2020-05-23 23:23 ` Dave Chinner
2020-05-24 5:32 ` Christoph Hellwig
2020-05-23 11:39 ` Christoph Hellwig
2020-05-22 3:50 ` [PATCH 23/24] xfs: factor xfs_iflush_done Dave Chinner
2020-05-23 0:20 ` Darrick J. Wong
2020-05-23 11:35 ` Christoph Hellwig
2020-05-22 3:50 ` [PATCH 24/24] xfs: remove xfs_inobp_check() Dave Chinner
2020-05-23 0:16 ` Darrick J. Wong
2020-05-23 11:36 ` Christoph Hellwig
2020-05-22 4:04 ` [PATCH 00/24] xfs: rework inode flushing to make inode reclaim fully asynchronous Dave Chinner
2020-05-23 16:18 ` Darrick J. Wong
2020-05-23 21:22 ` Dave Chinner
2020-05-22 6:18 ` Amir Goldstein
2020-05-22 12:01 ` Amir Goldstein
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=20200522215336.GJ8230@magnolia \
--to=darrick.wong@oracle.com \
--cc=david@fromorbit.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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.