| Message ID | 20191009032124.10541-8-david@fromorbit.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | mm, xfs: non-blocking inode reclaim | expand |
> if (need_ail) { > - bool mlip_changed = false; > + xfs_lsn_t tail_lsn = 0; > > /* this is an opencoded batch version of xfs_trans_ail_delete */ > spin_lock(&ailp->ail_lock); > list_for_each_entry(blip, &tmp, li_bio_list) { > if (INODE_ITEM(blip)->ili_logged && > + blip->li_lsn == INODE_ITEM(blip)->ili_flush_lsn) { > + /* > + * xfs_ail_update_finish() only cares about the > + * lsn of the first tail item removed, any others This adds an > 80 chars line. Maybe just do the xfs_clear_li_failed in the if and continue to make the code a little easier to read. Otherwise this looks good: Reviewed-by: Christoph Hellwig <hch@lst.de>
On Wed, Oct 09, 2019 at 02:21:05PM +1100, Dave Chinner wrote: > From: Dave Chinner <dchinner@redhat.com> > > We currently wake anything waiting on the log tail to move whenever > the log item at the tail of the log is removed. Historically this > was fine behaviour because there were very few items at any given > LSN. But with delayed logging, there may be thousands of items at > any given LSN, and we can't move the tail until they are all gone. > > Hence if we are removing them in near tail-first order, we might be > waking up processes waiting on the tail LSN to change (e.g. log > space waiters) repeatedly without them being able to make progress. > This also occurs with the new sync push waiters, and can result in > thousands of spurious wakeups every second when under heavy direct > reclaim pressure. > > To fix this, check that the tail LSN has actually changed on the > AIL before triggering wakeups. This will reduce the number of > spurious wakeups when doing bulk AIL removal and make this code much > more efficient. > > Signed-off-by: Dave Chinner <dchinner@redhat.com> > --- > fs/xfs/xfs_inode_item.c | 18 ++++++++++---- > fs/xfs/xfs_trans_ail.c | 52 ++++++++++++++++++++++++++++------------- > fs/xfs/xfs_trans_priv.h | 4 ++-- > 3 files changed, 51 insertions(+), 23 deletions(-) > ... > diff --git a/fs/xfs/xfs_trans_ail.c b/fs/xfs/xfs_trans_ail.c > index 656819523bbd..685a21cd24c0 100644 > --- a/fs/xfs/xfs_trans_ail.c > +++ b/fs/xfs/xfs_trans_ail.c ... > @@ -745,9 +754,10 @@ xfs_trans_ail_update_bulk( > continue; > > trace_xfs_ail_move(lip, lip->li_lsn, lsn); > + if (mlip == lip && !tail_lsn) > + tail_lsn = lip->li_lsn; > + > xfs_ail_delete(ailp, lip); > - if (mlip == lip) > - mlip_changed = 1; FWIW, I think this code could be simplified to just assign tail_lsn directly via the new helper and pass that value into xfs_ail_update_finish() below. I doubt filtering out that extra lsn comparison with the mlip check makes much difference when we're already doing a bulk insertion and referencing every lip along the way. That's just a nit though. Otherwise looks fine to me: Reviewed-by: Brian Foster <bfoster@redhat.com> > } else { > trace_xfs_ail_insert(lip, 0, lsn); > } > @@ -758,15 +768,23 @@ xfs_trans_ail_update_bulk( > if (!list_empty(&tmp)) > xfs_ail_splice(ailp, cur, &tmp, lsn); > > - xfs_ail_update_finish(ailp, mlip_changed); > + xfs_ail_update_finish(ailp, tail_lsn); > } > > -bool > +/* > + * Delete one log item from the AIL. > + * > + * If this item was at the tail of the AIL, return the LSN of the log item so > + * that we can use it to check if the LSN of the tail of the log has moved > + * when finishing up the AIL delete process in xfs_ail_update_finish(). > + */ > +xfs_lsn_t > xfs_ail_delete_one( > struct xfs_ail *ailp, > struct xfs_log_item *lip) > { > struct xfs_log_item *mlip = xfs_ail_min(ailp); > + xfs_lsn_t lsn = lip->li_lsn; > > trace_xfs_ail_delete(lip, mlip->li_lsn, lip->li_lsn); > xfs_ail_delete(ailp, lip); > @@ -774,7 +792,9 @@ xfs_ail_delete_one( > clear_bit(XFS_LI_IN_AIL, &lip->li_flags); > lip->li_lsn = 0; > > - return mlip == lip; > + if (mlip == lip) > + return lsn; > + return 0; > } > > /** > @@ -805,7 +825,7 @@ xfs_trans_ail_delete( > int shutdown_type) > { > struct xfs_mount *mp = ailp->ail_mount; > - bool need_update; > + xfs_lsn_t tail_lsn; > > if (!test_bit(XFS_LI_IN_AIL, &lip->li_flags)) { > spin_unlock(&ailp->ail_lock); > @@ -818,8 +838,8 @@ xfs_trans_ail_delete( > return; > } > > - need_update = xfs_ail_delete_one(ailp, lip); > - xfs_ail_update_finish(ailp, need_update); > + tail_lsn = xfs_ail_delete_one(ailp, lip); > + xfs_ail_update_finish(ailp, tail_lsn); > } > > int > diff --git a/fs/xfs/xfs_trans_priv.h b/fs/xfs/xfs_trans_priv.h > index 64ffa746730e..35655eac01a6 100644 > --- a/fs/xfs/xfs_trans_priv.h > +++ b/fs/xfs/xfs_trans_priv.h > @@ -91,8 +91,8 @@ xfs_trans_ail_update( > xfs_trans_ail_update_bulk(ailp, NULL, &lip, 1, lsn); > } > > -bool xfs_ail_delete_one(struct xfs_ail *ailp, struct xfs_log_item *lip); > -void xfs_ail_update_finish(struct xfs_ail *ailp, bool do_tail_update) > +xfs_lsn_t xfs_ail_delete_one(struct xfs_ail *ailp, struct xfs_log_item *lip); > +void xfs_ail_update_finish(struct xfs_ail *ailp, xfs_lsn_t old_lsn) > __releases(ailp->ail_lock); > void xfs_trans_ail_delete(struct xfs_ail *ailp, struct xfs_log_item *lip, > int shutdown_type); > -- > 2.23.0.rc1 >
diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c index ab12e526540a..0d5eee456b0c 100644 --- a/fs/xfs/xfs_inode_item.c +++ b/fs/xfs/xfs_inode_item.c @@ -731,19 +731,27 @@ xfs_iflush_done( * holding the lock before removing the inode from the AIL. */ if (need_ail) { - bool mlip_changed = false; + xfs_lsn_t tail_lsn = 0; /* this is an opencoded batch version of xfs_trans_ail_delete */ spin_lock(&ailp->ail_lock); list_for_each_entry(blip, &tmp, li_bio_list) { if (INODE_ITEM(blip)->ili_logged && - blip->li_lsn == INODE_ITEM(blip)->ili_flush_lsn) - mlip_changed |= xfs_ail_delete_one(ailp, blip); - else { + blip->li_lsn == INODE_ITEM(blip)->ili_flush_lsn) { + /* + * xfs_ail_update_finish() only cares about the + * lsn of the first tail item removed, any others + * will be at the same or higher lsn so we just + * ignore them. + */ + xfs_lsn_t lsn = xfs_ail_delete_one(ailp, blip); + if (!tail_lsn && lsn) + tail_lsn = lsn; + } else { xfs_clear_li_failed(blip); } } - xfs_ail_update_finish(ailp, mlip_changed); + xfs_ail_update_finish(ailp, tail_lsn); } /* diff --git a/fs/xfs/xfs_trans_ail.c b/fs/xfs/xfs_trans_ail.c index 656819523bbd..685a21cd24c0 100644 --- a/fs/xfs/xfs_trans_ail.c +++ b/fs/xfs/xfs_trans_ail.c @@ -108,17 +108,25 @@ xfs_ail_next( * We need the AIL lock in order to get a coherent read of the lsn of the last * item in the AIL. */ +static xfs_lsn_t +__xfs_ail_min_lsn( + struct xfs_ail *ailp) +{ + struct xfs_log_item *lip = xfs_ail_min(ailp); + + if (lip) + return lip->li_lsn; + return 0; +} + xfs_lsn_t xfs_ail_min_lsn( struct xfs_ail *ailp) { - xfs_lsn_t lsn = 0; - struct xfs_log_item *lip; + xfs_lsn_t lsn; spin_lock(&ailp->ail_lock); - lip = xfs_ail_min(ailp); - if (lip) - lsn = lip->li_lsn; + lsn = __xfs_ail_min_lsn(ailp); spin_unlock(&ailp->ail_lock); return lsn; @@ -681,11 +689,12 @@ xfs_ail_push_all_sync( void xfs_ail_update_finish( struct xfs_ail *ailp, - bool do_tail_update) __releases(ailp->ail_lock) + xfs_lsn_t old_lsn) __releases(ailp->ail_lock) { struct xfs_mount *mp = ailp->ail_mount; - if (!do_tail_update) { + /* if the tail lsn hasn't changed, don't do updates or wakeups. */ + if (!old_lsn || old_lsn == __xfs_ail_min_lsn(ailp)) { spin_unlock(&ailp->ail_lock); return; } @@ -730,7 +739,7 @@ xfs_trans_ail_update_bulk( xfs_lsn_t lsn) __releases(ailp->ail_lock) { struct xfs_log_item *mlip; - int mlip_changed = 0; + xfs_lsn_t tail_lsn = 0; int i; LIST_HEAD(tmp); @@ -745,9 +754,10 @@ xfs_trans_ail_update_bulk( continue; trace_xfs_ail_move(lip, lip->li_lsn, lsn); + if (mlip == lip && !tail_lsn) + tail_lsn = lip->li_lsn; + xfs_ail_delete(ailp, lip); - if (mlip == lip) - mlip_changed = 1; } else { trace_xfs_ail_insert(lip, 0, lsn); } @@ -758,15 +768,23 @@ xfs_trans_ail_update_bulk( if (!list_empty(&tmp)) xfs_ail_splice(ailp, cur, &tmp, lsn); - xfs_ail_update_finish(ailp, mlip_changed); + xfs_ail_update_finish(ailp, tail_lsn); } -bool +/* + * Delete one log item from the AIL. + * + * If this item was at the tail of the AIL, return the LSN of the log item so + * that we can use it to check if the LSN of the tail of the log has moved + * when finishing up the AIL delete process in xfs_ail_update_finish(). + */ +xfs_lsn_t xfs_ail_delete_one( struct xfs_ail *ailp, struct xfs_log_item *lip) { struct xfs_log_item *mlip = xfs_ail_min(ailp); + xfs_lsn_t lsn = lip->li_lsn; trace_xfs_ail_delete(lip, mlip->li_lsn, lip->li_lsn); xfs_ail_delete(ailp, lip); @@ -774,7 +792,9 @@ xfs_ail_delete_one( clear_bit(XFS_LI_IN_AIL, &lip->li_flags); lip->li_lsn = 0; - return mlip == lip; + if (mlip == lip) + return lsn; + return 0; } /** @@ -805,7 +825,7 @@ xfs_trans_ail_delete( int shutdown_type) { struct xfs_mount *mp = ailp->ail_mount; - bool need_update; + xfs_lsn_t tail_lsn; if (!test_bit(XFS_LI_IN_AIL, &lip->li_flags)) { spin_unlock(&ailp->ail_lock); @@ -818,8 +838,8 @@ xfs_trans_ail_delete( return; } - need_update = xfs_ail_delete_one(ailp, lip); - xfs_ail_update_finish(ailp, need_update); + tail_lsn = xfs_ail_delete_one(ailp, lip); + xfs_ail_update_finish(ailp, tail_lsn); } int diff --git a/fs/xfs/xfs_trans_priv.h b/fs/xfs/xfs_trans_priv.h index 64ffa746730e..35655eac01a6 100644 --- a/fs/xfs/xfs_trans_priv.h +++ b/fs/xfs/xfs_trans_priv.h @@ -91,8 +91,8 @@ xfs_trans_ail_update( xfs_trans_ail_update_bulk(ailp, NULL, &lip, 1, lsn); } -bool xfs_ail_delete_one(struct xfs_ail *ailp, struct xfs_log_item *lip); -void xfs_ail_update_finish(struct xfs_ail *ailp, bool do_tail_update) +xfs_lsn_t xfs_ail_delete_one(struct xfs_ail *ailp, struct xfs_log_item *lip); +void xfs_ail_update_finish(struct xfs_ail *ailp, xfs_lsn_t old_lsn) __releases(ailp->ail_lock); void xfs_trans_ail_delete(struct xfs_ail *ailp, struct xfs_log_item *lip, int shutdown_type);