[4/8] xfs: call xlog_state_release_iclog with l_icloglock held
diff mbox series

Message ID 20191009142748.18005-5-hch@lst.de
State New
Headers show
Series
  • [1/8] xfs: pass the correct flag to xlog_write_iclog
Related show

Commit Message

Christoph Hellwig Oct. 9, 2019, 2:27 p.m. UTC
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>
---
 fs/xfs/xfs_log.c | 188 +++++++++++++++++++++++------------------------
 1 file changed, 90 insertions(+), 98 deletions(-)

Comments

Darrick J. Wong Oct. 14, 2019, 6:12 p.m. UTC | #1
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>
> ---
>  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;	

Extra whitespace here                          ^

I kinda wish the relocation and refactoring of xlog_state_release_iclog
had happened as separate patches so that the redistribution of locking
responsibilities in this patch would be more straightforward, but oh
well, I got through it.

With the whitespace nit fixed,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

>  
> -			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
>
Brian Foster Oct. 15, 2019, 5:07 p.m. UTC | #2
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
>

Patch
diff mbox series

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) ||