diff mbox series

[43/45] xfs: avoid cil push lock if possible

Message ID 20210305051143.182133-44-david@fromorbit.com (mailing list archive)
State New
Headers show
Series xfs: consolidated log and optimisation changes | expand

Commit Message

Dave Chinner March 5, 2021, 5:11 a.m. UTC
From: Dave Chinner <dchinner@redhat.com>

Because now it hurts when the CIL fills up.

  - 37.20% __xfs_trans_commit
      - 35.84% xfs_log_commit_cil
         - 19.34% _raw_spin_lock
            - do_raw_spin_lock
                 19.01% __pv_queued_spin_lock_slowpath
         - 4.20% xfs_log_ticket_ungrant
              0.90% xfs_log_space_wake


Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/xfs_log_cil.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

Comments

Darrick J. Wong March 11, 2021, 1:47 a.m. UTC | #1
On Fri, Mar 05, 2021 at 04:11:41PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> Because now it hurts when the CIL fills up.
> 
>   - 37.20% __xfs_trans_commit
>       - 35.84% xfs_log_commit_cil
>          - 19.34% _raw_spin_lock
>             - do_raw_spin_lock
>                  19.01% __pv_queued_spin_lock_slowpath
>          - 4.20% xfs_log_ticket_ungrant
>               0.90% xfs_log_space_wake
> 
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
>  fs/xfs/xfs_log_cil.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c
> index 6dcc23829bef..d60c72ad391a 100644
> --- a/fs/xfs/xfs_log_cil.c
> +++ b/fs/xfs/xfs_log_cil.c
> @@ -1115,10 +1115,18 @@ xlog_cil_push_background(
>  	ASSERT(!test_bit(XLOG_CIL_EMPTY, &cil->xc_flags));
>  
>  	/*
> -	 * Don't do a background push if we haven't used up all the
> -	 * space available yet.
> +	 * We are done if:
> +	 * - we haven't used up all the space available yet; or
> +	 * - we've already queued up a push; and
> +	 * - we're not over the hard limit; and
> +	 * - nothing has been over the hard limit.

Er... do these last three bullet points correspond to the last three
lines of the if test?  I'm not sure how !waitqueue_active() determines
that nothing has been over the hard limit?  Or for that matter how
comparing push_seq against current_seq tells us if we've queued a
push?

--D

> +	 *
> +	 * If so, we don't need to take the push lock as there's nothing to do.
>  	 */
> -	if (space_used < XLOG_CIL_SPACE_LIMIT(log)) {
> +	if (space_used < XLOG_CIL_SPACE_LIMIT(log) ||
> +	    (cil->xc_push_seq == cil->xc_current_sequence &&
> +	     space_used < XLOG_CIL_BLOCKING_SPACE_LIMIT(log) &&
> +	     !waitqueue_active(&cil->xc_push_wait))) {
>  		up_read(&cil->xc_ctx_lock);
>  		return;
>  	}
> -- 
> 2.28.0
>
Dave Chinner March 12, 2021, 2:36 a.m. UTC | #2
On Wed, Mar 10, 2021 at 05:47:09PM -0800, Darrick J. Wong wrote:
> On Fri, Mar 05, 2021 at 04:11:41PM +1100, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> > 
> > Because now it hurts when the CIL fills up.
> > 
> >   - 37.20% __xfs_trans_commit
> >       - 35.84% xfs_log_commit_cil
> >          - 19.34% _raw_spin_lock
> >             - do_raw_spin_lock
> >                  19.01% __pv_queued_spin_lock_slowpath
> >          - 4.20% xfs_log_ticket_ungrant
> >               0.90% xfs_log_space_wake
> > 
> > 
> > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > ---
> >  fs/xfs/xfs_log_cil.c | 14 +++++++++++---
> >  1 file changed, 11 insertions(+), 3 deletions(-)
> > 
> > diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c
> > index 6dcc23829bef..d60c72ad391a 100644
> > --- a/fs/xfs/xfs_log_cil.c
> > +++ b/fs/xfs/xfs_log_cil.c
> > @@ -1115,10 +1115,18 @@ xlog_cil_push_background(
> >  	ASSERT(!test_bit(XLOG_CIL_EMPTY, &cil->xc_flags));
> >  
> >  	/*
> > -	 * Don't do a background push if we haven't used up all the
> > -	 * space available yet.
> > +	 * We are done if:
> > +	 * - we haven't used up all the space available yet; or
> > +	 * - we've already queued up a push; and
> > +	 * - we're not over the hard limit; and
> > +	 * - nothing has been over the hard limit.
> 
> Er... do these last three bullet points correspond to the last three
> lines of the if test?  I'm not sure how !waitqueue_active() determines
> that nothing has been over the hard limit?

If a commit has made space used go over the hard limit, it will be
throttled and put to sleep on the push wait queue. Another commit
can then return space (inode fork gets smaller) and bring us back
under the hard limit. Hence just checking against the space used does
not tell us if we've hit the hard limit or not, but checking if
there is a throttled process on the wait queue does...

> Or for that matter how
> comparing push_seq against current_seq tells us if we've queued a
> push?

We only set push_seq == current_seq when we queue up a push in
xlog_cil_push_now() or xlog_cil_push_background().  Hence if no push
has been queued, then push_seq will be less than current_seq.

Cheers,

Dave.
diff mbox series

Patch

diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c
index 6dcc23829bef..d60c72ad391a 100644
--- a/fs/xfs/xfs_log_cil.c
+++ b/fs/xfs/xfs_log_cil.c
@@ -1115,10 +1115,18 @@  xlog_cil_push_background(
 	ASSERT(!test_bit(XLOG_CIL_EMPTY, &cil->xc_flags));
 
 	/*
-	 * Don't do a background push if we haven't used up all the
-	 * space available yet.
+	 * We are done if:
+	 * - we haven't used up all the space available yet; or
+	 * - we've already queued up a push; and
+	 * - we're not over the hard limit; and
+	 * - nothing has been over the hard limit.
+	 *
+	 * If so, we don't need to take the push lock as there's nothing to do.
 	 */
-	if (space_used < XLOG_CIL_SPACE_LIMIT(log)) {
+	if (space_used < XLOG_CIL_SPACE_LIMIT(log) ||
+	    (cil->xc_push_seq == cil->xc_current_sequence &&
+	     space_used < XLOG_CIL_BLOCKING_SPACE_LIMIT(log) &&
+	     !waitqueue_active(&cil->xc_push_wait))) {
 		up_read(&cil->xc_ctx_lock);
 		return;
 	}