diff mbox series

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

Message ID 20190905084717.30308-8-david@fromorbit.com (mailing list archive)
State Accepted
Headers show
Series [1/8] xfs: push the AIL in xlog_grant_head_wake | expand

Commit Message

Dave Chinner Sept. 5, 2019, 8:47 a.m. UTC 
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(-)

Comments

Darrick J. Wong Sept. 5, 2019, 3:48 p.m. UTC | #1 
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
>
Dave Chinner Sept. 5, 2019, 10:28 p.m. UTC | #2 
On Thu, Sep 05, 2019 at 08:48:53AM -0700, Darrick J. Wong wrote:
> 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.

Fixed - "just finished IO completion processing"...

-Dave.
diff mbox series

Patch

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
+ * 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);