From: Brian Foster <bfoster@redhat.com>
To: Dave Chinner <david@fromorbit.com>
Cc: linux-xfs@vger.kernel.org, linux-mm@kvack.org,
linux-fsdevel@vger.kernel.org
Subject: Re: [PATCH 14/24] xfs: tail updates only need to occur when LSN changes
Date: Tue, 6 Aug 2019 08:53:21 -0400 [thread overview]
Message-ID: <20190806125321.GC2979@bfoster> (raw)
In-Reply-To: <20190806053338.GD7777@dread.disaster.area>
On Tue, Aug 06, 2019 at 03:33:38PM +1000, Dave Chinner wrote:
> On Tue, Aug 06, 2019 at 09:28:26AM +1000, Dave Chinner wrote:
> > On Mon, Aug 05, 2019 at 01:53:26PM -0400, Brian Foster wrote:
> > > On Thu, Aug 01, 2019 at 12:17:42PM +1000, 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.
> > > >
> > > > XXX: occasionally get a temporary hang in xfs_ail_push_sync() with
> > > > this change - log force from log worker gets things moving again.
> > > > Only happens under extreme memory pressure - possibly push racing
> > > > with a tail update on an empty log. Needs further investigation.
> > > >
> > > > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > > > ---
> > >
> > > Ok, this addresses the wakeup granularity issue mentioned in the
> > > previous patch. Note that I was kind of wondering why we wouldn't base
> > > this on the l_tail_lsn update in xlog_assign_tail_lsn_locked() as
> > > opposed to the current approach.
> >
> > Because I didn't think of it? :)
> >
> > There's so much other stuff in this patch set I didn't spend a
> > lot of time thinking about other alternatives. this was a simple
> > code transformation that did what I wanted, and I went on to burning
> > brain cells on other more complex issues that needs to be solved...
> >
> > > For example, xlog_assign_tail_lsn_locked() could simply check the
> > > current min item against the current l_tail_lsn before it does the
> > > assignment and use that to trigger tail change events. If we wanted to
> > > also filter out the other wakeups (as this patch does) then we could
> > > just pass a bool pointer or something that returns whether the tail
> > > actually changed.
> >
> > Yeah, I'll have a look at this - I might rework it as additional
> > patches now the code is looking at decisions based on LSN rather
> > than if the tail log item changed...
>
> Ok, this is not worth the complexity. The wakeup code has to be able
> to tell the difference between a changed tail lsn and an empty AIL
> so that wakeups can be issued when the AIL is finally emptied.
> Unmount (xfs_ail_push_all_sync()) relies on this, and
> xlog_assign_tail_lsn_locked() hides the empty AIL from the caller
> by returning log->l_last_sync_lsn to the caller.
>
Wouldn't either case just be a wakeup from xlog_assign_tail_lsn_locked()
(which should probably be renamed if we took that approach)? It's called
when we've removed the min item from the AIL and so potentially need to
update the tail lsn.
> Hence the wakeup code still has to check for an empty AIL if the
> tail has changed if we use the return value of
> xlog_assign_tail_lsn_locked() as the tail LSN. At which point, the
> logic becomes somewhat convoluted, and it's far simpler to use
> __xfs_ail_min_lsn as it returns when the log is empty.
>
> So, nice idea, but it doesn't make the code simpler or easier to
> understand....
>
It's not that big of a deal either way. BTW on another quick look, I
think something like xfs_ail_update_tail(ailp, old_tail) is a bit more
self-documenting that xfs_ail_delete_finish(ailp, old_lsn).
Brian
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@fromorbit.com
next prev parent reply other threads:[~2019-08-06 12:53 UTC|newest]
Thread overview: 87+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-08-01 2:17 [RFC] [PATCH 00/24] mm, xfs: non-blocking inode reclaim Dave Chinner
2019-08-01 2:17 ` [PATCH 01/24] mm: directed shrinker work deferral Dave Chinner
2019-08-02 15:27 ` Brian Foster
2019-08-04 1:49 ` Dave Chinner
2019-08-05 17:42 ` Brian Foster
2019-08-05 23:43 ` Dave Chinner
2019-08-06 12:27 ` Brian Foster
2019-08-06 22:22 ` Dave Chinner
2019-08-07 11:13 ` Brian Foster
2019-08-01 2:17 ` [PATCH 02/24] shrinkers: use will_defer for GFP_NOFS sensitive shrinkers Dave Chinner
2019-08-02 15:27 ` Brian Foster
2019-08-04 1:50 ` Dave Chinner
2019-08-01 2:17 ` [PATCH 03/24] mm: factor shrinker work calculations Dave Chinner
2019-08-02 15:08 ` Nikolay Borisov
2019-08-04 2:05 ` Dave Chinner
2019-08-02 15:31 ` Brian Foster
2019-08-01 2:17 ` [PATCH 04/24] shrinker: defer work only to kswapd Dave Chinner
2019-08-02 15:34 ` Brian Foster
2019-08-04 16:48 ` Nikolay Borisov
2019-08-04 21:37 ` Dave Chinner
2019-08-07 16:12 ` kbuild test robot
2019-08-07 18:00 ` kbuild test robot
2019-08-01 2:17 ` [PATCH 05/24] shrinker: clean up variable types and tracepoints Dave Chinner
2019-08-01 2:17 ` [PATCH 06/24] mm: reclaim_state records pages reclaimed, not slabs Dave Chinner
2019-08-01 2:17 ` [PATCH 07/24] mm: back off direct reclaim on excessive shrinker deferral Dave Chinner
2019-08-01 2:17 ` [PATCH 08/24] mm: kswapd backoff for shrinkers Dave Chinner
2019-08-01 2:17 ` [PATCH 09/24] xfs: don't allow log IO to be throttled Dave Chinner
2019-08-01 13:39 ` Chris Mason
2019-08-01 23:58 ` Dave Chinner
2019-08-02 8:12 ` Christoph Hellwig
2019-08-02 14:11 ` Chris Mason
2019-08-02 18:34 ` Matthew Wilcox
2019-08-02 23:28 ` Dave Chinner
2019-08-05 18:32 ` Chris Mason
2019-08-05 23:09 ` Dave Chinner
2019-08-01 2:17 ` [PATCH 10/24] xfs: fix missed wakeup on l_flush_wait Dave Chinner
2019-08-01 2:17 ` [PATCH 11/24] xfs:: account for memory freed from metadata buffers Dave Chinner
2019-08-01 8:16 ` Christoph Hellwig
2019-08-01 9:21 ` Dave Chinner
2019-08-06 5:51 ` Christoph Hellwig
2019-08-01 2:17 ` [PATCH 12/24] xfs: correctly acount for reclaimable slabs Dave Chinner
2019-08-06 5:52 ` Christoph Hellwig
2019-08-06 21:05 ` Dave Chinner
2019-08-01 2:17 ` [PATCH 13/24] xfs: synchronous AIL pushing Dave Chinner
2019-08-05 17:51 ` Brian Foster
2019-08-05 23:21 ` Dave Chinner
2019-08-06 12:29 ` Brian Foster
2019-08-01 2:17 ` [PATCH 14/24] xfs: tail updates only need to occur when LSN changes Dave Chinner
2019-08-05 17:53 ` Brian Foster
2019-08-05 23:28 ` Dave Chinner
2019-08-06 5:33 ` Dave Chinner
2019-08-06 12:53 ` Brian Foster [this message]
2019-08-06 21:11 ` Dave Chinner
2019-08-01 2:17 ` [PATCH 15/24] xfs: eagerly free shadow buffers to reduce CIL footprint Dave Chinner
2019-08-05 18:03 ` Brian Foster
2019-08-05 23:33 ` Dave Chinner
2019-08-06 12:57 ` Brian Foster
2019-08-06 21:21 ` Dave Chinner
2019-08-01 2:17 ` [PATCH 16/24] xfs: Lower CIL flush limit for large logs Dave Chinner
2019-08-04 17:12 ` Nikolay Borisov
2019-08-01 2:17 ` [PATCH 17/24] xfs: don't block kswapd in inode reclaim Dave Chinner
2019-08-06 18:21 ` Brian Foster
2019-08-06 21:27 ` Dave Chinner
2019-08-07 11:14 ` Brian Foster
2019-08-01 2:17 ` [PATCH 18/24] xfs: reduce kswapd blocking on inode locking Dave Chinner
2019-08-06 18:22 ` Brian Foster
2019-08-06 21:33 ` Dave Chinner
2019-08-07 11:30 ` Brian Foster
2019-08-07 23:16 ` Dave Chinner
2019-08-01 2:17 ` [PATCH 19/24] xfs: kill background reclaim work Dave Chinner
2019-08-01 2:17 ` [PATCH 20/24] xfs: use AIL pushing for inode reclaim IO Dave Chinner
2019-08-07 18:09 ` Brian Foster
2019-08-07 23:10 ` Dave Chinner
2019-08-08 16:20 ` Brian Foster
2019-08-01 2:17 ` [PATCH 21/24] xfs: remove mode from xfs_reclaim_inodes() Dave Chinner
2019-08-01 2:17 ` [PATCH 22/24] xfs: track reclaimable inodes using a LRU list Dave Chinner
2019-08-08 16:36 ` Brian Foster
2019-08-09 0:10 ` Dave Chinner
2019-08-01 2:17 ` [PATCH 23/24] xfs: reclaim inodes from the LRU Dave Chinner
2019-08-08 16:39 ` Brian Foster
2019-08-09 1:20 ` Dave Chinner
2019-08-09 12:36 ` Brian Foster
2019-08-11 2:17 ` Dave Chinner
2019-08-11 12:46 ` Brian Foster
2019-08-01 2:17 ` [PATCH 24/24] xfs: remove unusued old inode reclaim code Dave Chinner
2019-08-06 5:57 ` [RFC] [PATCH 00/24] mm, xfs: non-blocking inode reclaim Christoph Hellwig
2019-08-06 21:37 ` Dave Chinner
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190806125321.GC2979@bfoster \
--to=bfoster@redhat.com \
--cc=david@fromorbit.com \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=linux-xfs@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).