Re: [PATCH 13/14] xfs: remove xlog_state_want_sync

[Brian Foster <bfoster@redhat.com>]

On Mon, Mar 16, 2020 at 03:42:32PM +0100, Christoph Hellwig wrote:
> Open code the xlog_state_want_sync logic in its two callers given that
> this function is a trivial wrapper around xlog_state_switch_iclogs.
> 
> Move the lockdep assert into xlog_state_switch_iclogs to not lose this
> debugging aid, and improve the comment that documents
> xlog_state_switch_iclogs as well.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  fs/xfs/xfs_log.c | 50 +++++++++++++++++-------------------------------
>  1 file changed, 18 insertions(+), 32 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index 865dd1e08679..761b138d97ec 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -62,11 +62,6 @@ xlog_state_switch_iclogs(
>  	struct xlog_in_core	*iclog,
>  	int			eventual_size);
>  STATIC void
> -xlog_state_want_sync(
> -	struct xlog		*log,
> -	struct xlog_in_core	*iclog);
> -
> -STATIC void
>  xlog_grant_push_ail(
>  	struct xlog		*log,
>  	int			need_bytes);
> @@ -938,7 +933,11 @@ xfs_log_write_unmount_record(
>  	spin_lock(&log->l_icloglock);
>  	iclog = log->l_iclog;
>  	atomic_inc(&iclog->ic_refcnt);
> -	xlog_state_want_sync(log, iclog);
> +	if (iclog->ic_state == XLOG_STATE_ACTIVE)
> +		xlog_state_switch_iclogs(log, iclog, 0);
> +	else
> +		ASSERT(iclog->ic_state == XLOG_STATE_WANT_SYNC ||
> +		       iclog->ic_state == XLOG_STATE_IOERROR);
>  	error = xlog_state_release_iclog(log, iclog);
>  	xlog_wait_on_iclog(iclog);
>  
> @@ -2293,7 +2292,11 @@ xlog_write_copy_finish(
>  		*record_cnt = 0;
>  		*data_cnt = 0;
>  
> -		xlog_state_want_sync(log, iclog);
> +		if (iclog->ic_state == XLOG_STATE_ACTIVE)
> +			xlog_state_switch_iclogs(log, iclog, 0);
> +		else
> +			ASSERT(iclog->ic_state == XLOG_STATE_WANT_SYNC ||
> +			       iclog->ic_state == XLOG_STATE_IOERROR);
>  		if (!commit_iclog)
>  			goto release_iclog;
>  		spin_unlock(&log->l_icloglock);
> @@ -3069,11 +3072,12 @@ xlog_ungrant_log_space(
>  }
>  
>  /*
> - * 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.
> - * When this routine is called from xlog_state_get_iclog_space(), the
> - * exact size of the iclog has not yet been determined.  All we know is
> - * that every data block.  We have run out of space in this log record.
> + * Mark the current iclog in the ring as WANT_SYNC and move the current iclog
> + * pointer to the next iclog in the ring.
> + *
> + * When called from xlog_state_get_iclog_space(), the exact size of the iclog
> + * has not yet been determined, all we know is that we have run out of space in
> + * the current iclog.
>   */
>  STATIC void
>  xlog_state_switch_iclogs(
> @@ -3082,6 +3086,8 @@ xlog_state_switch_iclogs(
>  	int			eventual_size)
>  {
>  	ASSERT(iclog->ic_state == XLOG_STATE_ACTIVE);
> +	assert_spin_locked(&log->l_icloglock);
> +
>  	if (!eventual_size)
>  		eventual_size = iclog->ic_offset;
>  	iclog->ic_state = XLOG_STATE_WANT_SYNC;
> @@ -3323,26 +3329,6 @@ xfs_log_force_lsn(
>  	return ret;
>  }
>  
> -/*
> - * Called when we want to mark the current iclog as being ready to sync to
> - * disk.
> - */
> -STATIC void
> -xlog_state_want_sync(
> -	struct xlog		*log,
> -	struct xlog_in_core	*iclog)
> -{
> -	assert_spin_locked(&log->l_icloglock);
> -
> -	if (iclog->ic_state == XLOG_STATE_ACTIVE) {
> -		xlog_state_switch_iclogs(log, iclog, 0);
> -	} else {
> -		ASSERT(iclog->ic_state == XLOG_STATE_WANT_SYNC ||
> -		       iclog->ic_state == XLOG_STATE_IOERROR);
> -	}
> -}
> -
> -
>  /*****************************************************************************
>   *
>   *		TICKET functions
> -- 
> 2.24.1
>