From: Dave Chinner <david@fromorbit.com>
To: Brian Foster <bfoster@redhat.com>
Cc: linux-xfs@vger.kernel.org, linux-mm@kvack.org,
linux-fsdevel@vger.kernel.org
Subject: Re: [PATCH 13/24] xfs: synchronous AIL pushing
Date: Tue, 6 Aug 2019 09:21:32 +1000 [thread overview]
Message-ID: <20190805232132.GY7777@dread.disaster.area> (raw)
In-Reply-To: <20190805175153.GC14760@bfoster>
On Mon, Aug 05, 2019 at 01:51:53PM -0400, Brian Foster wrote:
> On Thu, Aug 01, 2019 at 12:17:41PM +1000, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> >
> > Provide an interface to push the AIL to a target LSN and wait for
> > the tail of the log to move past that LSN. This is used to wait for
> > all items older than a specific LSN to either be cleaned (written
> > back) or relogged to a higher LSN in the AIL. The primary use for
> > this is to allow IO free inode reclaim throttling.
> >
> > Factor the common AIL deletion code that does all the wakeups into a
> > helper so we only have one copy of this somewhat tricky code to
> > interface with all the wakeups necessary when the LSN of the log
> > tail changes.
> >
> > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > ---
> > fs/xfs/xfs_inode_item.c | 12 +------
> > fs/xfs/xfs_trans_ail.c | 69 +++++++++++++++++++++++++++++++++--------
> > fs/xfs/xfs_trans_priv.h | 6 +++-
> > 3 files changed, 62 insertions(+), 25 deletions(-)
> >
> ...
> > diff --git a/fs/xfs/xfs_trans_ail.c b/fs/xfs/xfs_trans_ail.c
> > index 6ccfd75d3c24..9e3102179221 100644
> > --- a/fs/xfs/xfs_trans_ail.c
> > +++ b/fs/xfs/xfs_trans_ail.c
> > @@ -654,6 +654,37 @@ xfs_ail_push_all(
> > xfs_ail_push(ailp, threshold_lsn);
> > }
> >
> > +/*
> > + * Push the AIL to a specific lsn and wait for it to complete.
> > + */
> > +void
> > +xfs_ail_push_sync(
> > + struct xfs_ail *ailp,
> > + xfs_lsn_t threshold_lsn)
> > +{
> > + struct xfs_log_item *lip;
> > + DEFINE_WAIT(wait);
> > +
> > + spin_lock(&ailp->ail_lock);
> > + while ((lip = xfs_ail_min(ailp)) != NULL) {
> > + prepare_to_wait(&ailp->ail_push, &wait, TASK_UNINTERRUPTIBLE);
> > + if (XFS_FORCED_SHUTDOWN(ailp->ail_mount) ||
> > + XFS_LSN_CMP(threshold_lsn, lip->li_lsn) <= 0)
> > + break;
> > + /* XXX: cmpxchg? */
> > + while (XFS_LSN_CMP(threshold_lsn, ailp->ail_target) > 0)
> > + xfs_trans_ail_copy_lsn(ailp, &ailp->ail_target, &threshold_lsn);
>
> Why the need to repeatedly copy the ail_target like this? If the push
It's a hack because the other updates are done unlocked and this
doesn't contain the memroy barriers needed to make it correct
and race free.
Hence the comment "XXX: cmpxchg" to ensure that:
a) we only ever move the target forwards;
b) we resolve update races in an obvious, simple manner; and
c) we can get rid of the possibly incorrect memory
barriers around this (unlocked) update.
RFC. WIP. :)
> target only ever moves forward, we should only need to do this once at
> the start of the function. In fact I'm kind of wondering why we can't
> just call xfs_ail_push(). If we check the tail item after grabbing the
> spin lock, we should be able to avoid any races with the waker, no?
I didn't use xfs_ail_push() because of having to prepare to wait
between determining if the AIL is empty and checking if we need
to update the target.
I also didn't want to affect the existing xfs_ail_push() as I was
modifying the xfs_ail_push_sync() code to do what was needed.
Eventually they can probably come back together, but for now I'm not
100% sure that the code is correct and race free.
> > +void
> > +xfs_ail_delete_finish(
> > + struct xfs_ail *ailp,
> > + bool do_tail_update) __releases(ailp->ail_lock)
> > +{
> > + struct xfs_mount *mp = ailp->ail_mount;
> > +
> > + if (!do_tail_update) {
> > + spin_unlock(&ailp->ail_lock);
> > + return;
> > + }
> > +
>
> Hmm.. so while what we really care about here are tail updates, this
> logic is currently driven by removing the min ail log item. That seems
> like a lot of potential churn if we're waking the pusher on every object
> written back covered by a single log record / checkpoint. Perhaps we
> should implement a bit more coarse wakeup logic such as only when the
> tail lsn actually changes, for example?
You mean the next patch?
> FWIW, it also doesn't look like you've handled the case of relogged
> items moving the tail forward anywhere that I can see, so we might be
> missing some wakeups here. See xfs_trans_ail_update_bulk() for
> additional AIL manipulation.
Good catch. That might be the race the next patch exposes :)
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
next prev parent reply other threads:[~2019-08-05 23:22 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 [this message]
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
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=20190805232132.GY7777@dread.disaster.area \
--to=david@fromorbit.com \
--cc=bfoster@redhat.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).