From: Brian Foster <bfoster@redhat.com>
To: Christoph Hellwig <hch@lst.de>
Cc: linux-xfs@vger.kernel.org
Subject: Re: [PATCH 4/8] xfs: call xlog_state_release_iclog with l_icloglock held
Date: Tue, 15 Oct 2019 13:07:19 -0400 [thread overview]
Message-ID: <20191015170719.GF36108@bfoster> (raw)
In-Reply-To: <20191009142748.18005-5-hch@lst.de>
On Wed, Oct 09, 2019 at 04:27:44PM +0200, Christoph Hellwig wrote:
> All but one caller of xlog_state_release_iclog hold l_icloglock and need
> to drop and reacquire it to call xlog_state_release_iclog. Switch the
> xlog_state_release_iclog calling conventions to expect the lock to be
> held, and open code the logic (using a shared helper) in the only
> remaining caller that does not have the lock (and where not holding it
> is a nice performance optimization). Also move the refactored code to
> require the least amount of forward declarations.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
Modulo the whitespace thing:
Reviewed-by: Brian Foster <bfoster@redhat.com>
> fs/xfs/xfs_log.c | 188 +++++++++++++++++++++++------------------------
> 1 file changed, 90 insertions(+), 98 deletions(-)
>
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index 860a555772fe..67a767d90ebf 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -57,10 +57,6 @@ xlog_state_get_iclog_space(
> struct xlog_ticket *ticket,
> int *continued_write,
> int *logoffsetp);
> -STATIC int
> -xlog_state_release_iclog(
> - struct xlog *log,
> - struct xlog_in_core *iclog);
> STATIC void
> xlog_state_switch_iclogs(
> struct xlog *log,
> @@ -83,7 +79,10 @@ STATIC void
> xlog_ungrant_log_space(
> struct xlog *log,
> struct xlog_ticket *ticket);
> -
> +STATIC void
> +xlog_sync(
> + struct xlog *log,
> + struct xlog_in_core *iclog);
> #if defined(DEBUG)
> STATIC void
> xlog_verify_dest_ptr(
> @@ -552,16 +551,71 @@ xfs_log_done(
> return lsn;
> }
>
> +static bool
> +__xlog_state_release_iclog(
> + struct xlog *log,
> + struct xlog_in_core *iclog)
> +{
> + lockdep_assert_held(&log->l_icloglock);
> +
> + if (iclog->ic_state == XLOG_STATE_WANT_SYNC) {
> + /* update tail before writing to iclog */
> + xfs_lsn_t tail_lsn = xlog_assign_tail_lsn(log->l_mp);
> +
> + iclog->ic_state = XLOG_STATE_SYNCING;
> + iclog->ic_header.h_tail_lsn = cpu_to_be64(tail_lsn);
> + xlog_verify_tail_lsn(log, iclog, tail_lsn);
> + /* cycle incremented when incrementing curr_block */
> + return true;
> + }
> +
> + ASSERT(iclog->ic_state == XLOG_STATE_ACTIVE);
> + return false;
> +}
> +
> +/*
> + * Flush iclog to disk if this is the last reference to the given iclog and the
> + * it is in the WANT_SYNC state.
> + */
> +static int
> +xlog_state_release_iclog(
> + struct xlog *log,
> + struct xlog_in_core *iclog)
> +{
> + lockdep_assert_held(&log->l_icloglock);
> +
> + if (iclog->ic_state & XLOG_STATE_IOERROR)
> + return -EIO;
> +
> + if (atomic_dec_and_test(&iclog->ic_refcnt) &&
> + __xlog_state_release_iclog(log, iclog)) {
> + spin_unlock(&log->l_icloglock);
> + xlog_sync(log, iclog);
> + spin_lock(&log->l_icloglock);
> + }
> +
> + return 0;
> +}
> +
> int
> xfs_log_release_iclog(
> - struct xfs_mount *mp,
> + struct xfs_mount *mp,
> struct xlog_in_core *iclog)
> {
> - if (xlog_state_release_iclog(mp->m_log, iclog)) {
> + struct xlog *log = mp->m_log;
> + bool sync;
> +
> + if (iclog->ic_state & XLOG_STATE_IOERROR) {
> xfs_force_shutdown(mp, SHUTDOWN_LOG_IO_ERROR);
> return -EIO;
> }
>
> + if (atomic_dec_and_lock(&iclog->ic_refcnt, &log->l_icloglock)) {
> + sync = __xlog_state_release_iclog(log, iclog);
> + spin_unlock(&log->l_icloglock);
> + if (sync)
> + xlog_sync(log, iclog);
> + }
> return 0;
> }
>
> @@ -866,10 +920,7 @@ xfs_log_write_unmount_record(
> iclog = log->l_iclog;
> atomic_inc(&iclog->ic_refcnt);
> xlog_state_want_sync(log, iclog);
> - spin_unlock(&log->l_icloglock);
> error = xlog_state_release_iclog(log, iclog);
> -
> - spin_lock(&log->l_icloglock);
> switch (iclog->ic_state) {
> default:
> if (!XLOG_FORCED_SHUTDOWN(log)) {
> @@ -950,13 +1001,9 @@ xfs_log_unmount_write(xfs_mount_t *mp)
> spin_lock(&log->l_icloglock);
> iclog = log->l_iclog;
> atomic_inc(&iclog->ic_refcnt);
> -
> xlog_state_want_sync(log, iclog);
> - spin_unlock(&log->l_icloglock);
> error = xlog_state_release_iclog(log, iclog);
>
> - spin_lock(&log->l_icloglock);
> -
> if ( ! ( iclog->ic_state == XLOG_STATE_ACTIVE
> || iclog->ic_state == XLOG_STATE_DIRTY
> || iclog->ic_state == XLOG_STATE_IOERROR) ) {
> @@ -2255,6 +2302,8 @@ xlog_write_copy_finish(
> int log_offset,
> struct xlog_in_core **commit_iclog)
> {
> + int error;
> +
> if (*partial_copy) {
> /*
> * This iclog has already been marked WANT_SYNC by
> @@ -2262,10 +2311,9 @@ xlog_write_copy_finish(
> */
> spin_lock(&log->l_icloglock);
> xlog_state_finish_copy(log, iclog, *record_cnt, *data_cnt);
> - spin_unlock(&log->l_icloglock);
> *record_cnt = 0;
> *data_cnt = 0;
> - return xlog_state_release_iclog(log, iclog);
> + goto release_iclog;
> }
>
> *partial_copy = 0;
> @@ -2279,15 +2327,19 @@ xlog_write_copy_finish(
> *data_cnt = 0;
>
> xlog_state_want_sync(log, iclog);
> - spin_unlock(&log->l_icloglock);
> -
> if (!commit_iclog)
> - return xlog_state_release_iclog(log, iclog);
> + goto release_iclog;
> + spin_unlock(&log->l_icloglock);
> ASSERT(flags & XLOG_COMMIT_TRANS);
> *commit_iclog = iclog;
> }
>
> return 0;
> +
> +release_iclog:
> + error = xlog_state_release_iclog(log, iclog);
> + spin_unlock(&log->l_icloglock);
> + return error;
> }
>
> /*
> @@ -2349,7 +2401,7 @@ xlog_write(
> int contwr = 0;
> int record_cnt = 0;
> int data_cnt = 0;
> - int error;
> + int error = 0;
>
> *start_lsn = 0;
>
> @@ -2502,13 +2554,15 @@ xlog_write(
>
> spin_lock(&log->l_icloglock);
> xlog_state_finish_copy(log, iclog, record_cnt, data_cnt);
> + if (commit_iclog) {
> + ASSERT(flags & XLOG_COMMIT_TRANS);
> + *commit_iclog = iclog;
> + } else {
> + error = xlog_state_release_iclog(log, iclog);
> + }
> spin_unlock(&log->l_icloglock);
> - if (!commit_iclog)
> - return xlog_state_release_iclog(log, iclog);
>
> - ASSERT(flags & XLOG_COMMIT_TRANS);
> - *commit_iclog = iclog;
> - return 0;
> + return error;
> }
>
>
> @@ -2979,7 +3033,6 @@ xlog_state_get_iclog_space(
> int log_offset;
> xlog_rec_header_t *head;
> xlog_in_core_t *iclog;
> - int error;
>
> restart:
> spin_lock(&log->l_icloglock);
> @@ -3028,24 +3081,22 @@ xlog_state_get_iclog_space(
> * can fit into remaining data section.
> */
> if (iclog->ic_size - iclog->ic_offset < 2*sizeof(xlog_op_header_t)) {
> + int error = 0;
> +
> xlog_state_switch_iclogs(log, iclog, iclog->ic_size);
>
> /*
> - * If I'm the only one writing to this iclog, sync it to disk.
> - * We need to do an atomic compare and decrement here to avoid
> - * racing with concurrent atomic_dec_and_lock() calls in
> + * If we are the only one writing to this iclog, sync it to
> + * disk. We need to do an atomic compare and decrement here to
> + * avoid racing with concurrent atomic_dec_and_lock() calls in
> * xlog_state_release_iclog() when there is more than one
> * reference to the iclog.
> */
> - if (!atomic_add_unless(&iclog->ic_refcnt, -1, 1)) {
> - /* we are the only one */
> - spin_unlock(&log->l_icloglock);
> + if (!atomic_add_unless(&iclog->ic_refcnt, -1, 1))
> error = xlog_state_release_iclog(log, iclog);
> - if (error)
> - return error;
> - } else {
> - spin_unlock(&log->l_icloglock);
> - }
> + spin_unlock(&log->l_icloglock);
> + if (error)
> + return error;
> goto restart;
> }
>
> @@ -3156,60 +3207,6 @@ xlog_ungrant_log_space(
> xfs_log_space_wake(log->l_mp);
> }
>
> -/*
> - * Flush iclog to disk if this is the last reference to the given iclog and
> - * the WANT_SYNC bit is set.
> - *
> - * When this function is entered, the iclog is not necessarily in the
> - * WANT_SYNC state. It may be sitting around waiting to get filled.
> - *
> - *
> - */
> -STATIC int
> -xlog_state_release_iclog(
> - struct xlog *log,
> - struct xlog_in_core *iclog)
> -{
> - int sync = 0; /* do we sync? */
> -
> - if (iclog->ic_state & XLOG_STATE_IOERROR)
> - return -EIO;
> -
> - ASSERT(atomic_read(&iclog->ic_refcnt) > 0);
> - if (!atomic_dec_and_lock(&iclog->ic_refcnt, &log->l_icloglock))
> - return 0;
> -
> - if (iclog->ic_state & XLOG_STATE_IOERROR) {
> - spin_unlock(&log->l_icloglock);
> - return -EIO;
> - }
> - ASSERT(iclog->ic_state == XLOG_STATE_ACTIVE ||
> - iclog->ic_state == XLOG_STATE_WANT_SYNC);
> -
> - if (iclog->ic_state == XLOG_STATE_WANT_SYNC) {
> - /* update tail before writing to iclog */
> - xfs_lsn_t tail_lsn = xlog_assign_tail_lsn(log->l_mp);
> - sync++;
> - iclog->ic_state = XLOG_STATE_SYNCING;
> - iclog->ic_header.h_tail_lsn = cpu_to_be64(tail_lsn);
> - xlog_verify_tail_lsn(log, iclog, tail_lsn);
> - /* cycle incremented when incrementing curr_block */
> - }
> - spin_unlock(&log->l_icloglock);
> -
> - /*
> - * We let the log lock go, so it's possible that we hit a log I/O
> - * error or some other SHUTDOWN condition that marks the iclog
> - * as XLOG_STATE_IOERROR before the bwrite. However, we know that
> - * this iclog has consistent data, so we ignore IOERROR
> - * flags after this point.
> - */
> - if (sync)
> - xlog_sync(log, iclog);
> - return 0;
> -} /* xlog_state_release_iclog */
> -
> -
> /*
> * This routine will mark the current iclog in the ring as WANT_SYNC
> * and move the current iclog pointer to the next iclog in the ring.
> @@ -3333,12 +3330,9 @@ xfs_log_force(
> atomic_inc(&iclog->ic_refcnt);
> lsn = be64_to_cpu(iclog->ic_header.h_lsn);
> xlog_state_switch_iclogs(log, iclog, 0);
> - spin_unlock(&log->l_icloglock);
> -
> if (xlog_state_release_iclog(log, iclog))
> - return -EIO;
> + goto out_error;
>
> - spin_lock(&log->l_icloglock);
> if (be64_to_cpu(iclog->ic_header.h_lsn) != lsn ||
> iclog->ic_state == XLOG_STATE_DIRTY)
> goto out_unlock;
> @@ -3433,12 +3427,10 @@ __xfs_log_force_lsn(
> }
> atomic_inc(&iclog->ic_refcnt);
> xlog_state_switch_iclogs(log, iclog, 0);
> - spin_unlock(&log->l_icloglock);
> if (xlog_state_release_iclog(log, iclog))
> - return -EIO;
> + goto out_error;
> if (log_flushed)
> *log_flushed = 1;
> - spin_lock(&log->l_icloglock);
> }
>
> if (!(flags & XFS_LOG_SYNC) ||
> --
> 2.20.1
>
next prev parent reply other threads:[~2019-10-15 17:07 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-10-09 14:27 misc iclog state machine cleanups Christoph Hellwig
2019-10-09 14:27 ` [PATCH 1/8] xfs: pass the correct flag to xlog_write_iclog Christoph Hellwig
2019-10-14 17:10 ` Darrick J. Wong
2019-10-15 17:06 ` Brian Foster
2019-10-09 14:27 ` [PATCH 2/8] xfs: remove the unused ic_io_size field from xlog_in_core Christoph Hellwig
2019-10-14 17:12 ` Darrick J. Wong
2019-10-15 17:07 ` Brian Foster
2019-10-09 14:27 ` [PATCH 3/8] xfs: move the locking from xlog_state_finish_copy to the callers Christoph Hellwig
2019-10-14 18:02 ` Darrick J. Wong
2019-10-15 17:07 ` Brian Foster
2019-10-09 14:27 ` [PATCH 4/8] xfs: call xlog_state_release_iclog with l_icloglock held Christoph Hellwig
2019-10-14 18:12 ` Darrick J. Wong
2019-10-15 17:07 ` Brian Foster [this message]
2019-10-09 14:27 ` [PATCH 5/8] xfs: remove dead ifdef XFSERRORDEBUG code Christoph Hellwig
2019-10-14 17:13 ` Darrick J. Wong
2019-10-15 17:09 ` Brian Foster
2019-10-09 14:27 ` [PATCH 6/8] xfs: remove the unused XLOG_STATE_ALL and XLOG_STATE_UNUSED flags Christoph Hellwig
2019-10-12 0:36 ` Darrick J. Wong
2019-10-15 17:09 ` Brian Foster
2019-10-09 14:27 ` [PATCH 7/8] xfs: turn ic_state into an enum Christoph Hellwig
2019-10-14 18:25 ` Darrick J. Wong
2019-10-15 17:09 ` Brian Foster
2019-10-09 14:27 ` [PATCH 8/8] xfs: remove the XLOG_STATE_DO_CALLBACK state Christoph Hellwig
2019-10-12 0:41 ` Darrick J. Wong
2019-10-14 7:22 ` Christoph Hellwig
2019-10-14 19:05 ` Darrick J. Wong
2019-10-15 17:09 ` 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=20191015170719.GF36108@bfoster \
--to=bfoster@redhat.com \
--cc=hch@lst.de \
--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).