Re: [PATCH 7/8] xfs: push iclog state cleaning into xlog_state_clean_log

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Thu, Sep 05, 2019 at 06:47:16PM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> xlog_state_clean_log() is only called from one place, and it occurs
> when an iclog is transitioning back to ACTIVE. Prior to calling
> xlog_state_clean_log, the iclog we are processing has a hard coded
> state check to DIRTY so that xlog_state_clean_log() processes it
> correctly. We also have a hard coded wakeup after
> xlog_state_clean_log() to enfore log force waiters on that iclog
> are woken correctly.
> 
> Both of these things are operations required to finish processing an
> iclog and return it to the ACTIVE state again, so they make little
> sense to be separated from the rest of the clean state transition
> code.
> 
> Hence push these things inside xlog_state_clean_log(), document the
> behaviour and rename it xlog_state_clean_iclog() to indicate that
> it's being driven by an iclog state change and does the iclog state
> change work itself.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_log.c | 57 ++++++++++++++++++++++++++++--------------------
>  1 file changed, 33 insertions(+), 24 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index 356204ddf865..bef314361bc4 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -2521,21 +2521,35 @@ xlog_write(
>   *****************************************************************************
>   */
>  
> -/* Clean iclogs starting from the head.  This ordering must be
> - * maintained, so an iclog doesn't become ACTIVE beyond one that
> - * is SYNCING.  This is also required to maintain the notion that we use
> - * a ordered wait queue to hold off would be writers to the log when every
> - * iclog is trying to sync to disk.
> +/*
> + * An iclog has just finished it's completion processing, so we need to update

it's -> its, but I can fix that on import.

Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> + * the iclog state and propagate that up into the overall log state. Hence we
> + * prepare the iclog for cleaning, and then clean all the pending dirty iclogs
> + * starting from the head, and then wake up any threads that are waiting for the
> + * iclog to be marked clean.
> + *
> + * The ordering of marking iclogs ACTIVE must be maintained, so an iclog
> + * doesn't become ACTIVE beyond one that is SYNCING.  This is also required to
> + * maintain the notion that we use a ordered wait queue to hold off would be
> + * writers to the log when every iclog is trying to sync to disk.
> + *
> + * Caller must hold the icloglock before calling us.
>   *
> - * State Change: DIRTY -> ACTIVE
> + * State Change: !IOERROR -> DIRTY -> ACTIVE
>   */
>  STATIC void
> -xlog_state_clean_log(
> -	struct xlog *log)
> +xlog_state_clean_iclog(
> +	struct xlog		*log,
> +	struct xlog_in_core	*dirty_iclog)
>  {
> -	xlog_in_core_t	*iclog;
> -	int changed = 0;
> +	struct xlog_in_core	*iclog;
> +	int			changed = 0;
> +
> +	/* Prepare the completed iclog. */
> +	if (!(dirty_iclog->ic_state & XLOG_STATE_IOERROR))
> +		dirty_iclog->ic_state = XLOG_STATE_DIRTY;
>  
> +	/* Walk all the iclogs to update the ordered active state. */
>  	iclog = log->l_iclog;
>  	do {
>  		if (iclog->ic_state == XLOG_STATE_DIRTY) {
> @@ -2573,7 +2587,13 @@ xlog_state_clean_log(
>  		iclog = iclog->ic_next;
>  	} while (iclog != log->l_iclog);
>  
> -	/* log is locked when we are called */
> +
> +	/*
> +	 * Wake up threads waiting in xfs_log_force() for the dirty iclog
> +	 * to be cleaned.
> +	 */
> +	wake_up_all(&dirty_iclog->ic_force_wait);
> +
>  	/*
>  	 * Change state for the dummy log recording.
>  	 * We usually go to NEED. But we go to NEED2 if the changed indicates
> @@ -2607,7 +2627,7 @@ xlog_state_clean_log(
>  			ASSERT(0);
>  		}
>  	}
> -}	/* xlog_state_clean_log */
> +}
>  
>  STATIC xfs_lsn_t
>  xlog_get_lowest_lsn(
> @@ -2839,18 +2859,7 @@ xlog_state_do_callback(
>  			cycled_icloglock = true;
>  			xlog_state_do_iclog_callbacks(log, iclog, aborted);
>  
> -			if (!(iclog->ic_state & XLOG_STATE_IOERROR))
> -				iclog->ic_state = XLOG_STATE_DIRTY;
> -
> -			/*
> -			 * Transition from DIRTY to ACTIVE if applicable.
> -			 * NOP if STATE_IOERROR.
> -			 */
> -			xlog_state_clean_log(log);
> -
> -			/* wake up threads waiting in xfs_log_force() */
> -			wake_up_all(&iclog->ic_force_wait);
> -
> +			xlog_state_clean_iclog(log, iclog);
>  			iclog = iclog->ic_next;
>  		} while (first_iclog != iclog);
>  
> -- 
> 2.23.0.rc1
>