From: "Darrick J. Wong" <darrick.wong@oracle.com>
To: Dave Chinner <david@fromorbit.com>
Cc: linux-xfs@vger.kernel.org
Subject: Re: [PATCH 6/8] xfs: factor common AIL item deletion code
Date: Wed, 25 Mar 2020 22:10:01 -0700 [thread overview]
Message-ID: <20200326051001.GB29339@magnolia> (raw)
In-Reply-To: <20200325014205.11843-7-david@fromorbit.com>
On Wed, Mar 25, 2020 at 12:42:03PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> Factor the common AIL deletion code that does all the wakeups into a
> helper so we only have one copy of this somewhat tricky code to
> interface with all the wakeups necessary when the LSN of the log
> tail changes.
>
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
This call site didn't have a wake_up_all and now it does; is that going
to make a difference? I /think/ the answer is that this function
usually puts things on the AIL so we won't trigger the ail_empty wakeup;
and if the AIL was previously empty and we didn't match any log items
(such that it's still empty) then it's fine to wake up anyone who was
waiting for the ail to clear out?
If so,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
--D
> ---
> fs/xfs/xfs_inode_item.c | 12 +----------
> fs/xfs/xfs_trans_ail.c | 48 ++++++++++++++++++++++-------------------
> fs/xfs/xfs_trans_priv.h | 4 +++-
> 3 files changed, 30 insertions(+), 34 deletions(-)
>
> diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
> index 4a3d13d4a0228..bd8c368098707 100644
> --- a/fs/xfs/xfs_inode_item.c
> +++ b/fs/xfs/xfs_inode_item.c
> @@ -742,17 +742,7 @@ xfs_iflush_done(
> xfs_clear_li_failed(blip);
> }
> }
> -
> - if (mlip_changed) {
> - if (!XFS_FORCED_SHUTDOWN(ailp->ail_mount))
> - xlog_assign_tail_lsn_locked(ailp->ail_mount);
> - if (list_empty(&ailp->ail_head))
> - wake_up_all(&ailp->ail_empty);
> - }
> - spin_unlock(&ailp->ail_lock);
> -
> - if (mlip_changed)
> - xfs_log_space_wake(ailp->ail_mount);
> + xfs_ail_update_finish(ailp, mlip_changed);
> }
>
> /*
> diff --git a/fs/xfs/xfs_trans_ail.c b/fs/xfs/xfs_trans_ail.c
> index 2ef0dfbfb303d..26d2e7928121c 100644
> --- a/fs/xfs/xfs_trans_ail.c
> +++ b/fs/xfs/xfs_trans_ail.c
> @@ -681,6 +681,27 @@ xfs_ail_push_all_sync(
> finish_wait(&ailp->ail_empty, &wait);
> }
>
> +void
> +xfs_ail_update_finish(
> + struct xfs_ail *ailp,
> + bool do_tail_update) __releases(ailp->ail_lock)
> +{
> + struct xfs_mount *mp = ailp->ail_mount;
> +
> + if (!do_tail_update) {
> + spin_unlock(&ailp->ail_lock);
> + return;
> + }
> +
> + if (!XFS_FORCED_SHUTDOWN(mp))
> + xlog_assign_tail_lsn_locked(mp);
> +
> + if (list_empty(&ailp->ail_head))
> + wake_up_all(&ailp->ail_empty);
> + spin_unlock(&ailp->ail_lock);
> + xfs_log_space_wake(mp);
> +}
> +
> /*
> * xfs_trans_ail_update - bulk AIL insertion operation.
> *
> @@ -740,15 +761,7 @@ xfs_trans_ail_update_bulk(
> if (!list_empty(&tmp))
> xfs_ail_splice(ailp, cur, &tmp, lsn);
>
> - if (mlip_changed) {
> - if (!XFS_FORCED_SHUTDOWN(ailp->ail_mount))
> - xlog_assign_tail_lsn_locked(ailp->ail_mount);
> - spin_unlock(&ailp->ail_lock);
> -
> - xfs_log_space_wake(ailp->ail_mount);
> - } else {
> - spin_unlock(&ailp->ail_lock);
> - }
> + xfs_ail_update_finish(ailp, mlip_changed);
> }
>
> bool
> @@ -792,10 +805,10 @@ void
> xfs_trans_ail_delete(
> struct xfs_ail *ailp,
> struct xfs_log_item *lip,
> - int shutdown_type) __releases(ailp->ail_lock)
> + int shutdown_type)
> {
> struct xfs_mount *mp = ailp->ail_mount;
> - bool mlip_changed;
> + bool need_update;
>
> if (!test_bit(XFS_LI_IN_AIL, &lip->li_flags)) {
> spin_unlock(&ailp->ail_lock);
> @@ -808,17 +821,8 @@ xfs_trans_ail_delete(
> return;
> }
>
> - mlip_changed = xfs_ail_delete_one(ailp, lip);
> - if (mlip_changed) {
> - if (!XFS_FORCED_SHUTDOWN(mp))
> - xlog_assign_tail_lsn_locked(mp);
> - if (list_empty(&ailp->ail_head))
> - wake_up_all(&ailp->ail_empty);
> - }
> -
> - spin_unlock(&ailp->ail_lock);
> - if (mlip_changed)
> - xfs_log_space_wake(ailp->ail_mount);
> + need_update = xfs_ail_delete_one(ailp, lip);
> + xfs_ail_update_finish(ailp, need_update);
> }
>
> int
> diff --git a/fs/xfs/xfs_trans_priv.h b/fs/xfs/xfs_trans_priv.h
> index 2e073c1c4614f..64ffa746730e4 100644
> --- a/fs/xfs/xfs_trans_priv.h
> +++ b/fs/xfs/xfs_trans_priv.h
> @@ -92,8 +92,10 @@ xfs_trans_ail_update(
> }
>
> bool xfs_ail_delete_one(struct xfs_ail *ailp, struct xfs_log_item *lip);
> +void xfs_ail_update_finish(struct xfs_ail *ailp, bool do_tail_update)
> + __releases(ailp->ail_lock);
> void xfs_trans_ail_delete(struct xfs_ail *ailp, struct xfs_log_item *lip,
> - int shutdown_type) __releases(ailp->ail_lock);
> + int shutdown_type);
>
> static inline void
> xfs_trans_ail_remove(
> --
> 2.26.0.rc2
>
next prev parent reply other threads:[~2020-03-26 5:10 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-03-25 1:41 [PATCH 0/8] xfs: various fixes and cleanups Dave Chinner
2020-03-25 1:41 ` [PATCH 1/8] xfs: Lower CIL flush limit for large logs Dave Chinner
2020-03-25 4:42 ` Allison Collins
2020-03-25 1:41 ` [PATCH 2/8] xfs: Throttle commits on delayed background CIL push Dave Chinner
2020-03-25 4:42 ` Allison Collins
2020-03-25 5:07 ` Dave Chinner
2020-03-26 5:24 ` Darrick J. Wong
2020-03-26 11:33 ` Brian Foster
2020-03-27 0:40 ` Dave Chinner
2020-03-25 1:42 ` [PATCH 3/8] xfs: don't allow log IO to be throttled Dave Chinner
2020-03-25 4:42 ` Allison Collins
2020-03-25 1:42 ` [PATCH 4/8] xfs: Improve metadata buffer reclaim accountability Dave Chinner
2020-03-25 4:42 ` Allison Collins
2020-03-25 13:30 ` Brian Foster
2020-03-26 5:05 ` Darrick J. Wong
2020-03-25 1:42 ` [PATCH 5/8] xfs: correctly acount for reclaimable slabs Dave Chinner
2020-03-25 4:43 ` Allison Collins
2020-03-25 1:42 ` [PATCH 6/8] xfs: factor common AIL item deletion code Dave Chinner
2020-03-25 4:54 ` Allison Collins
2020-03-25 13:30 ` Brian Foster
2020-03-26 5:10 ` Darrick J. Wong [this message]
2020-03-27 0:50 ` Dave Chinner
2020-03-25 1:42 ` [PATCH 7/8] xfs: tail updates only need to occur when LSN changes Dave Chinner
2020-03-25 5:10 ` Allison Collins
2020-03-26 5:14 ` Darrick J. Wong
2020-03-25 1:42 ` [PATCH 8/8] xfs: factor inode lookup from xfs_ifree_cluster Dave Chinner
2020-03-25 13:30 ` Brian Foster
2020-03-25 1:51 ` [PATCH 0/8] xfs: various fixes and cleanups 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=20200326051001.GB29339@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 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).