| Message ID | 20210305051143.182133-38-david@fromorbit.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xfs: consolidated log and optimisation changes | expand |
On Fri, Mar 05, 2021 at 04:11:35PM +1100, Dave Chinner wrote: > From: Dave Chinner <dchinner@redhat.com> > > To get it out from under the cil spinlock. > > Signed-off-by: Dave Chinner <dchinner@redhat.com> > --- > fs/xfs/xfs_log_cil.c | 11 ++++++----- > fs/xfs/xfs_log_priv.h | 2 +- > 2 files changed, 7 insertions(+), 6 deletions(-) > > diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c > index 5519d112c1fd..a2f93bd7644b 100644 > --- a/fs/xfs/xfs_log_cil.c > +++ b/fs/xfs/xfs_log_cil.c > @@ -492,6 +492,7 @@ xlog_cil_insert_items( > * based on how close we are to the hard limit. > */ > cilpcp = get_cpu_ptr(cil->xc_pcp); > + cilpcp->space_reserved += ctx_res; > cilpcp->space_used += len; > if (space_used >= XLOG_CIL_SPACE_LIMIT(log) || > cilpcp->space_used > > @@ -502,10 +503,6 @@ xlog_cil_insert_items( > } > put_cpu_ptr(cilpcp); > > - spin_lock(&cil->xc_cil_lock); > - ctx->ticket->t_unit_res += ctx_res; > - ctx->ticket->t_curr_res += ctx_res; > - > /* > * If we've overrun the reservation, dump the tx details before we move > * the log items. Shutdown is imminent... > @@ -527,6 +524,7 @@ xlog_cil_insert_items( > * We do this here so we only need to take the CIL lock once during > * the transaction commit. > */ > + spin_lock(&cil->xc_cil_lock); > list_for_each_entry(lip, &tp->t_items, li_trans) { > > /* Skip items which aren't dirty in this transaction. */ > @@ -798,10 +796,13 @@ xlog_cil_push_work( > > down_write(&cil->xc_ctx_lock); > > - /* Reset the CIL pcp counters */ > + /* Aggregate and reset the CIL pcp counters */ > for_each_online_cpu(cpu) { > cilpcp = per_cpu_ptr(cil->xc_pcp, cpu); > + ctx->ticket->t_curr_res += cilpcp->space_reserved; Why isn't it necessary to update ctx->ticket->t_unit_res any more? (Admittedly I'm struggling to figure out why it matters to keep it updated even in the current code base...) --D > cilpcp->space_used = 0; > + cilpcp->space_reserved = 0; > + > } > > spin_lock(&cil->xc_push_lock); > diff --git a/fs/xfs/xfs_log_priv.h b/fs/xfs/xfs_log_priv.h > index 4eb373357f26..278b9eaea582 100644 > --- a/fs/xfs/xfs_log_priv.h > +++ b/fs/xfs/xfs_log_priv.h > @@ -236,7 +236,7 @@ struct xfs_cil_ctx { > */ > struct xlog_cil_pcp { > uint32_t space_used; > - uint32_t curr_res; > + uint32_t space_reserved; > struct list_head busy_extents; > struct list_head log_items; > }; > -- > 2.28.0 >
On Wed, Mar 10, 2021 at 04:26:10PM -0800, Darrick J. Wong wrote: > On Fri, Mar 05, 2021 at 04:11:35PM +1100, Dave Chinner wrote: > > From: Dave Chinner <dchinner@redhat.com> > > > > To get it out from under the cil spinlock. > > > > Signed-off-by: Dave Chinner <dchinner@redhat.com> > > --- > > fs/xfs/xfs_log_cil.c | 11 ++++++----- > > fs/xfs/xfs_log_priv.h | 2 +- > > 2 files changed, 7 insertions(+), 6 deletions(-) > > > > diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c > > index 5519d112c1fd..a2f93bd7644b 100644 > > --- a/fs/xfs/xfs_log_cil.c > > +++ b/fs/xfs/xfs_log_cil.c > > @@ -492,6 +492,7 @@ xlog_cil_insert_items( > > * based on how close we are to the hard limit. > > */ > > cilpcp = get_cpu_ptr(cil->xc_pcp); > > + cilpcp->space_reserved += ctx_res; > > cilpcp->space_used += len; > > if (space_used >= XLOG_CIL_SPACE_LIMIT(log) || > > cilpcp->space_used > > > @@ -502,10 +503,6 @@ xlog_cil_insert_items( > > } > > put_cpu_ptr(cilpcp); > > > > - spin_lock(&cil->xc_cil_lock); > > - ctx->ticket->t_unit_res += ctx_res; > > - ctx->ticket->t_curr_res += ctx_res; > > - > > /* > > * If we've overrun the reservation, dump the tx details before we move > > * the log items. Shutdown is imminent... > > @@ -527,6 +524,7 @@ xlog_cil_insert_items( > > * We do this here so we only need to take the CIL lock once during > > * the transaction commit. > > */ > > + spin_lock(&cil->xc_cil_lock); > > list_for_each_entry(lip, &tp->t_items, li_trans) { > > > > /* Skip items which aren't dirty in this transaction. */ > > @@ -798,10 +796,13 @@ xlog_cil_push_work( > > > > down_write(&cil->xc_ctx_lock); > > > > - /* Reset the CIL pcp counters */ > > + /* Aggregate and reset the CIL pcp counters */ > > for_each_online_cpu(cpu) { > > cilpcp = per_cpu_ptr(cil->xc_pcp, cpu); > > + ctx->ticket->t_curr_res += cilpcp->space_reserved; > > Why isn't it necessary to update ctx->ticket->t_unit_res any more? Because t_unit_res is never used by the CIL ticket becuse they aren't permanent transaction reservations. The unit res is only for granting new space to a ticket, yet the CIL only ever "steals" granted space from an existing ticket. When the ticket is dropped, we return unused reservations from the CIL ticket, but never touch or look at the unit reservation. I can add it back in here if you want, but it's largely dead code... > (Admittedly I'm struggling to figure out why it matters to keep it > updated even in the current code base...) I think I originally did it a decade ago because I probably wasn't 100% sure on what impact not setting it would have. Getting the rest of the delayed logging code right was far more important than sweating on a tiny, largely insignificant detail like this. Cheers, Dave.
diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c index 5519d112c1fd..a2f93bd7644b 100644 --- a/fs/xfs/xfs_log_cil.c +++ b/fs/xfs/xfs_log_cil.c @@ -492,6 +492,7 @@ xlog_cil_insert_items( * based on how close we are to the hard limit. */ cilpcp = get_cpu_ptr(cil->xc_pcp); + cilpcp->space_reserved += ctx_res; cilpcp->space_used += len; if (space_used >= XLOG_CIL_SPACE_LIMIT(log) || cilpcp->space_used > @@ -502,10 +503,6 @@ xlog_cil_insert_items( } put_cpu_ptr(cilpcp); - spin_lock(&cil->xc_cil_lock); - ctx->ticket->t_unit_res += ctx_res; - ctx->ticket->t_curr_res += ctx_res; - /* * If we've overrun the reservation, dump the tx details before we move * the log items. Shutdown is imminent... @@ -527,6 +524,7 @@ xlog_cil_insert_items( * We do this here so we only need to take the CIL lock once during * the transaction commit. */ + spin_lock(&cil->xc_cil_lock); list_for_each_entry(lip, &tp->t_items, li_trans) { /* Skip items which aren't dirty in this transaction. */ @@ -798,10 +796,13 @@ xlog_cil_push_work( down_write(&cil->xc_ctx_lock); - /* Reset the CIL pcp counters */ + /* Aggregate and reset the CIL pcp counters */ for_each_online_cpu(cpu) { cilpcp = per_cpu_ptr(cil->xc_pcp, cpu); + ctx->ticket->t_curr_res += cilpcp->space_reserved; cilpcp->space_used = 0; + cilpcp->space_reserved = 0; + } spin_lock(&cil->xc_push_lock); diff --git a/fs/xfs/xfs_log_priv.h b/fs/xfs/xfs_log_priv.h index 4eb373357f26..278b9eaea582 100644 --- a/fs/xfs/xfs_log_priv.h +++ b/fs/xfs/xfs_log_priv.h @@ -236,7 +236,7 @@ struct xfs_cil_ctx { */ struct xlog_cil_pcp { uint32_t space_used; - uint32_t curr_res; + uint32_t space_reserved; struct list_head busy_extents; struct list_head log_items; };