From: Brian Foster <bfoster@redhat.com>
To: Dave Chinner <david@fromorbit.com>
Cc: linux-xfs@vger.kernel.org
Subject: Re: [PATCH 09/30] xfs: make inode IO completion buffer centric
Date: Wed, 3 Jun 2020 10:58:26 -0400 [thread overview]
Message-ID: <20200603145826.GC12332@bfoster> (raw)
In-Reply-To: <20200601214251.4167140-10-david@fromorbit.com>
On Tue, Jun 02, 2020 at 07:42:30AM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> Having different io completion callbacks for different inode states
> makes things complex. We can detect if the inode is stale via the
> XFS_ISTALE flag in IO completion, so we don't need a special
> callback just for this.
>
> This means inodes only have a single iodone callback, and inode IO
> completion is entirely buffer centric at this point. Hence we no
> longer need to use a log item callback at all as we can just call
> xfs_iflush_done() directly from the buffer completions and walk the
> buffer log item list to complete the all inodes under IO.
>
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
Probably not worth changing now, but I think this would have been
cleaner if the elimination of xfs_istale_done() was factored into a
separate patch. Otherwise LGTM:
Reviewed-by: Brian Foster <bfoster@redhat.com>
> fs/xfs/xfs_buf_item.c | 35 ++++++++++++++++++----
> fs/xfs/xfs_inode.c | 6 ++--
> fs/xfs/xfs_inode_item.c | 65 ++++++++++++++---------------------------
> fs/xfs/xfs_inode_item.h | 5 ++--
> 4 files changed, 56 insertions(+), 55 deletions(-)
>
> diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
> index 5b3cd5e90947c..a4e416af5c614 100644
> --- a/fs/xfs/xfs_buf_item.c
> +++ b/fs/xfs/xfs_buf_item.c
> @@ -13,6 +13,8 @@
> #include "xfs_mount.h"
> #include "xfs_trans.h"
> #include "xfs_buf_item.h"
> +#include "xfs_inode.h"
> +#include "xfs_inode_item.h"
> #include "xfs_trans_priv.h"
> #include "xfs_trace.h"
> #include "xfs_log.h"
> @@ -457,7 +459,8 @@ xfs_buf_item_unpin(
> * the AIL lock.
> */
> if (bip->bli_flags & XFS_BLI_STALE_INODE) {
> - xfs_buf_do_callbacks(bp);
> + lip->li_cb(bp, lip);
> + xfs_iflush_done(bp);
> bp->b_log_item = NULL;
> } else {
> xfs_trans_ail_delete(lip, SHUTDOWN_LOG_IO_ERROR);
> @@ -1141,8 +1144,8 @@ xfs_buf_iodone_callback_error(
> return false;
> }
>
> -static void
> -xfs_buf_run_callbacks(
> +static inline bool
> +xfs_buf_had_callback_errors(
> struct xfs_buf *bp)
> {
>
> @@ -1152,7 +1155,7 @@ xfs_buf_run_callbacks(
> * appropriate action.
> */
> if (bp->b_error && xfs_buf_iodone_callback_error(bp))
> - return;
> + return true;
>
> /*
> * Successful IO or permanent error. Either way, we can clear the
> @@ -1161,7 +1164,16 @@ xfs_buf_run_callbacks(
> bp->b_last_error = 0;
> bp->b_retries = 0;
> bp->b_first_retry_time = 0;
> + return false;
> +}
>
> +static void
> +xfs_buf_run_callbacks(
> + struct xfs_buf *bp)
> +{
> +
> + if (xfs_buf_had_callback_errors(bp))
> + return;
> xfs_buf_do_callbacks(bp);
> bp->b_log_item = NULL;
> }
> @@ -1173,7 +1185,20 @@ void
> xfs_buf_inode_iodone(
> struct xfs_buf *bp)
> {
> - xfs_buf_run_callbacks(bp);
> + struct xfs_buf_log_item *blip = bp->b_log_item;
> + struct xfs_log_item *lip;
> +
> + if (xfs_buf_had_callback_errors(bp))
> + return;
> +
> + /* If there is a buf_log_item attached, run its callback */
> + if (blip) {
> + lip = &blip->bli_item;
> + lip->li_cb(bp, lip);
> + bp->b_log_item = NULL;
> + }
> +
> + xfs_iflush_done(bp);
> xfs_buf_ioend_finish(bp);
> }
>
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index d5dee57f914a9..1b4e8e0bb0cf0 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -2677,7 +2677,6 @@ xfs_ifree_cluster(
> list_for_each_entry(lip, &bp->b_li_list, li_bio_list) {
> if (lip->li_type == XFS_LI_INODE) {
> iip = (struct xfs_inode_log_item *)lip;
> - lip->li_cb = xfs_istale_done;
> xfs_trans_ail_copy_lsn(mp->m_ail,
> &iip->ili_flush_lsn,
> &iip->ili_item.li_lsn);
> @@ -2710,8 +2709,7 @@ xfs_ifree_cluster(
> xfs_trans_ail_copy_lsn(mp->m_ail, &iip->ili_flush_lsn,
> &iip->ili_item.li_lsn);
>
> - xfs_buf_attach_iodone(bp, xfs_istale_done,
> - &iip->ili_item);
> + xfs_buf_attach_iodone(bp, NULL, &iip->ili_item);
>
> if (ip != free_ip)
> xfs_iunlock(ip, XFS_ILOCK_EXCL);
> @@ -3861,7 +3859,7 @@ xfs_iflush_int(
> * the flush lock.
> */
> bp->b_flags |= _XBF_INODES;
> - xfs_buf_attach_iodone(bp, xfs_iflush_done, &iip->ili_item);
> + xfs_buf_attach_iodone(bp, NULL, &iip->ili_item);
>
> /* generate the checksum. */
> xfs_dinode_calc_crc(mp, dip);
> diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
> index 6ef9cbcfc94a7..7049f2ae8d186 100644
> --- a/fs/xfs/xfs_inode_item.c
> +++ b/fs/xfs/xfs_inode_item.c
> @@ -668,40 +668,34 @@ xfs_inode_item_destroy(
> */
> void
> xfs_iflush_done(
> - struct xfs_buf *bp,
> - struct xfs_log_item *lip)
> + struct xfs_buf *bp)
> {
> struct xfs_inode_log_item *iip;
> - struct xfs_log_item *blip, *n;
> - struct xfs_ail *ailp = lip->li_ailp;
> + struct xfs_log_item *lip, *n;
> + struct xfs_ail *ailp = bp->b_mount->m_ail;
> int need_ail = 0;
> LIST_HEAD(tmp);
>
> /*
> - * Scan the buffer IO completions for other inodes being completed and
> - * attach them to the current inode log item.
> + * Pull the attached inodes from the buffer one at a time and take the
> + * appropriate action on them.
> */
> -
> - list_add_tail(&lip->li_bio_list, &tmp);
> -
> - list_for_each_entry_safe(blip, n, &bp->b_li_list, li_bio_list) {
> - if (lip->li_cb != xfs_iflush_done)
> + list_for_each_entry_safe(lip, n, &bp->b_li_list, li_bio_list) {
> + iip = INODE_ITEM(lip);
> + if (xfs_iflags_test(iip->ili_inode, XFS_ISTALE)) {
> + list_del_init(&lip->li_bio_list);
> + xfs_iflush_abort(iip->ili_inode);
> continue;
> + }
>
> - list_move_tail(&blip->li_bio_list, &tmp);
> + list_move_tail(&lip->li_bio_list, &tmp);
>
> /* Do an unlocked check for needing the AIL lock. */
> - iip = INODE_ITEM(blip);
> - if (blip->li_lsn == iip->ili_flush_lsn ||
> - test_bit(XFS_LI_FAILED, &blip->li_flags))
> + if (lip->li_lsn == iip->ili_flush_lsn ||
> + test_bit(XFS_LI_FAILED, &lip->li_flags))
> need_ail++;
> }
> -
> - /* make sure we capture the state of the initial inode. */
> - iip = INODE_ITEM(lip);
> - if (lip->li_lsn == iip->ili_flush_lsn ||
> - test_bit(XFS_LI_FAILED, &lip->li_flags))
> - need_ail++;
> + ASSERT(list_empty(&bp->b_li_list));
>
> /*
> * We only want to pull the item from the AIL if it is actually there
> @@ -713,19 +707,13 @@ xfs_iflush_done(
>
> /* this is an opencoded batch version of xfs_trans_ail_delete */
> spin_lock(&ailp->ail_lock);
> - list_for_each_entry(blip, &tmp, li_bio_list) {
> - if (blip->li_lsn == INODE_ITEM(blip)->ili_flush_lsn) {
> - /*
> - * xfs_ail_update_finish() only cares about the
> - * lsn of the first tail item removed, any
> - * others will be at the same or higher lsn so
> - * we just ignore them.
> - */
> - xfs_lsn_t lsn = xfs_ail_delete_one(ailp, blip);
> + list_for_each_entry(lip, &tmp, li_bio_list) {
> + if (lip->li_lsn == INODE_ITEM(lip)->ili_flush_lsn) {
> + xfs_lsn_t lsn = xfs_ail_delete_one(ailp, lip);
> if (!tail_lsn && lsn)
> tail_lsn = lsn;
> } else {
> - xfs_clear_li_failed(blip);
> + xfs_clear_li_failed(lip);
> }
> }
> xfs_ail_update_finish(ailp, tail_lsn);
> @@ -736,9 +724,9 @@ xfs_iflush_done(
> * ili_last_fields bits now that we know that the data corresponding to
> * them is safely on disk.
> */
> - list_for_each_entry_safe(blip, n, &tmp, li_bio_list) {
> - list_del_init(&blip->li_bio_list);
> - iip = INODE_ITEM(blip);
> + list_for_each_entry_safe(lip, n, &tmp, li_bio_list) {
> + list_del_init(&lip->li_bio_list);
> + iip = INODE_ITEM(lip);
>
> spin_lock(&iip->ili_lock);
> iip->ili_last_fields = 0;
> @@ -746,7 +734,6 @@ xfs_iflush_done(
>
> xfs_ifunlock(iip->ili_inode);
> }
> - list_del(&tmp);
> }
>
> /*
> @@ -779,14 +766,6 @@ xfs_iflush_abort(
> xfs_ifunlock(ip);
> }
>
> -void
> -xfs_istale_done(
> - struct xfs_buf *bp,
> - struct xfs_log_item *lip)
> -{
> - xfs_iflush_abort(INODE_ITEM(lip)->ili_inode);
> -}
> -
> /*
> * convert an xfs_inode_log_format struct from the old 32 bit version
> * (which can have different field alignments) to the native 64 bit version
> diff --git a/fs/xfs/xfs_inode_item.h b/fs/xfs/xfs_inode_item.h
> index 44c47c08b0b59..1545fccad4eeb 100644
> --- a/fs/xfs/xfs_inode_item.h
> +++ b/fs/xfs/xfs_inode_item.h
> @@ -36,15 +36,14 @@ struct xfs_inode_log_item {
> xfs_lsn_t ili_last_lsn; /* lsn at last transaction */
> };
>
> -static inline int xfs_inode_clean(xfs_inode_t *ip)
> +static inline int xfs_inode_clean(struct xfs_inode *ip)
> {
> return !ip->i_itemp || !(ip->i_itemp->ili_fields & XFS_ILOG_ALL);
> }
>
> extern void xfs_inode_item_init(struct xfs_inode *, struct xfs_mount *);
> extern void xfs_inode_item_destroy(struct xfs_inode *);
> -extern void xfs_iflush_done(struct xfs_buf *, struct xfs_log_item *);
> -extern void xfs_istale_done(struct xfs_buf *, struct xfs_log_item *);
> +extern void xfs_iflush_done(struct xfs_buf *);
> extern void xfs_iflush_abort(struct xfs_inode *);
> extern int xfs_inode_item_format_convert(xfs_log_iovec_t *,
> struct xfs_inode_log_format *);
> --
> 2.26.2.761.g0e0b3e54be
>
next prev parent reply other threads:[~2020-06-03 14:58 UTC|newest]
Thread overview: 77+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-06-01 21:42 [PATCH 00/30] xfs: rework inode flushing to make inode reclaim fully asynchronous Dave Chinner
2020-06-01 21:42 ` [PATCH 01/30] xfs: Don't allow logging of XFS_ISTALE inodes Dave Chinner
2020-06-02 4:30 ` Darrick J. Wong
2020-06-02 7:06 ` Dave Chinner
2020-06-02 16:32 ` Brian Foster
2020-06-01 21:42 ` [PATCH 02/30] xfs: remove logged flag from inode log item Dave Chinner
2020-06-02 16:32 ` Brian Foster
2020-06-01 21:42 ` [PATCH 03/30] xfs: add an inode item lock Dave Chinner
2020-06-02 16:34 ` Brian Foster
2020-06-04 1:54 ` Dave Chinner
2020-06-04 14:03 ` Brian Foster
2020-06-01 21:42 ` [PATCH 04/30] xfs: mark inode buffers in cache Dave Chinner
2020-06-02 16:45 ` Brian Foster
2020-06-02 19:22 ` Darrick J. Wong
2020-06-02 21:29 ` Dave Chinner
2020-06-03 14:57 ` Brian Foster
2020-06-03 21:21 ` Dave Chinner
2020-06-01 21:42 ` [PATCH 05/30] xfs: mark dquot " Dave Chinner
2020-06-02 16:45 ` Brian Foster
2020-06-02 19:00 ` Darrick J. Wong
2020-06-01 21:42 ` [PATCH 06/30] xfs: mark log recovery buffers for completion Dave Chinner
2020-06-02 16:45 ` Brian Foster
2020-06-02 19:24 ` Darrick J. Wong
2020-06-01 21:42 ` [PATCH 07/30] xfs: call xfs_buf_iodone directly Dave Chinner
2020-06-02 16:47 ` Brian Foster
2020-06-02 21:38 ` Dave Chinner
2020-06-03 14:58 ` Brian Foster
2020-06-01 21:42 ` [PATCH 08/30] xfs: clean up whacky buffer log item list reinit Dave Chinner
2020-06-02 16:47 ` Brian Foster
2020-06-01 21:42 ` [PATCH 09/30] xfs: make inode IO completion buffer centric Dave Chinner
2020-06-03 14:58 ` Brian Foster [this message]
2020-06-01 21:42 ` [PATCH 10/30] xfs: use direct calls for dquot IO completion Dave Chinner
2020-06-02 19:25 ` Darrick J. Wong
2020-06-03 14:58 ` Brian Foster
2020-06-01 21:42 ` [PATCH 11/30] xfs: clean up the buffer iodone callback functions Dave Chinner
2020-06-03 14:58 ` Brian Foster
2020-06-01 21:42 ` [PATCH 12/30] xfs: get rid of log item callbacks Dave Chinner
2020-06-03 14:58 ` Brian Foster
2020-06-01 21:42 ` [PATCH 13/30] xfs: handle buffer log item IO errors directly Dave Chinner
2020-06-02 20:39 ` Darrick J. Wong
2020-06-02 22:17 ` Dave Chinner
2020-06-03 15:02 ` Brian Foster
2020-06-03 21:34 ` Dave Chinner
2020-06-01 21:42 ` [PATCH 14/30] xfs: unwind log item error flagging Dave Chinner
2020-06-02 20:45 ` Darrick J. Wong
2020-06-03 15:02 ` Brian Foster
2020-06-01 21:42 ` [PATCH 15/30] xfs: move xfs_clear_li_failed out of xfs_ail_delete_one() Dave Chinner
2020-06-02 20:47 ` Darrick J. Wong
2020-06-03 15:02 ` Brian Foster
2020-06-01 21:42 ` [PATCH 16/30] xfs: pin inode backing buffer to the inode log item Dave Chinner
2020-06-02 22:30 ` Darrick J. Wong
2020-06-02 22:53 ` Dave Chinner
2020-06-03 18:58 ` Brian Foster
2020-06-03 22:15 ` Dave Chinner
2020-06-04 14:03 ` Brian Foster
2020-06-01 21:42 ` [PATCH 17/30] xfs: make inode reclaim almost non-blocking Dave Chinner
2020-06-01 21:42 ` [PATCH 18/30] xfs: remove IO submission from xfs_reclaim_inode() Dave Chinner
2020-06-02 22:36 ` Darrick J. Wong
2020-06-01 21:42 ` [PATCH 19/30] xfs: allow multiple reclaimers per AG Dave Chinner
2020-06-01 21:42 ` [PATCH 20/30] xfs: don't block inode reclaim on the ILOCK Dave Chinner
2020-06-01 21:42 ` [PATCH 21/30] xfs: remove SYNC_TRYLOCK from inode reclaim Dave Chinner
2020-06-01 21:42 ` [PATCH 22/30] xfs: remove SYNC_WAIT from xfs_reclaim_inodes() Dave Chinner
2020-06-02 22:43 ` Darrick J. Wong
2020-06-01 21:42 ` [PATCH 23/30] xfs: clean up inode reclaim comments Dave Chinner
2020-06-02 22:45 ` Darrick J. Wong
2020-06-01 21:42 ` [PATCH 24/30] xfs: rework stale inodes in xfs_ifree_cluster Dave Chinner
2020-06-02 23:01 ` Darrick J. Wong
2020-06-01 21:42 ` [PATCH 25/30] xfs: attach inodes to the cluster buffer when dirtied Dave Chinner
2020-06-02 23:03 ` Darrick J. Wong
2020-06-01 21:42 ` [PATCH 26/30] xfs: xfs_iflush() is no longer necessary Dave Chinner
2020-06-01 21:42 ` [PATCH 27/30] xfs: rename xfs_iflush_int() Dave Chinner
2020-06-01 21:42 ` [PATCH 28/30] xfs: rework xfs_iflush_cluster() dirty inode iteration Dave Chinner
2020-06-02 23:23 ` Darrick J. Wong
2020-06-01 21:42 ` [PATCH 29/30] xfs: factor xfs_iflush_done Dave Chinner
2020-06-01 21:42 ` [PATCH 30/30] xfs: remove xfs_inobp_check() Dave Chinner
2020-06-04 7:45 [PATCH 00/30] xfs: rework inode flushing to make inode reclaim fully asynchronous Dave Chinner
2020-06-04 7:45 ` [PATCH 09/30] xfs: make inode IO completion buffer centric Dave Chinner
2020-06-22 8:15 [PATCH 00/30] xfs: rework inode flushing to make inode reclaim fully asynchronous Dave Chinner
2020-06-22 8:15 ` [PATCH 09/30] xfs: make inode IO completion buffer centric Dave Chinner
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=20200603145826.GC12332@bfoster \
--to=bfoster@redhat.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.