xfs
[Top] [All Lists]

Re: [PATCH 08/27] xfs: improve sync behaviour in the fact of aggressive

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH 08/27] xfs: improve sync behaviour in the fact of aggressive dirtying
From: Alex Elder <aelder@xxxxxxx>
Date: Wed, 6 Jul 2011 09:59:49 -0500
Cc: <xfs@xxxxxxxxxxx>
In-reply-to: <20110706081511.GB5361@xxxxxxxxxxxxx>
References: <20110701094321.936534538@xxxxxxxxxxxxxxxxxxxxxx> <20110701094603.789209280@xxxxxxxxxxxxxxxxxxxxxx> <1309905378.1950.50.camel@doink> <20110706081511.GB5361@xxxxxxxxxxxxx>
Reply-to: <aelder@xxxxxxx>
On Wed, 2011-07-06 at 04:15 -0400, Christoph Hellwig wrote:
> On Tue, Jul 05, 2011 at 05:36:18PM -0500, Alex Elder wrote:
> > > A large part of the issue is that XFS writes data out itself two times
> > > in the ->sync_fs method, overriding the lifelock protection in the core
> > > writeback code, and another issue is the lock-less xfs_ioend_wait call,
> > > which doesn't prevent new ioend from beeing queue up while waiting for
> > > the count to reach zero.
> > 
> > The change affects only the first thing you mention here, not
> > the second.
> 
> It does.  We're also removing the xfs_ioend_wait done from
> xfs_sync_data/xfs_sync_inode_data.  We still have another one in
> ->write_inode, though.

OK, now I see what you're talking about.  I guess the way it was
stated I expected that the code would now *prevent* new ioends
from being queued while waiting.

> 
> > The i_iocount wait is not affected by your patch.
> 
> We're only removing one of the two we're doing per inode now.
> 
> > I'm OK with the change, but really prefer to have
> > the description not include stuff that just isn't
> > there.  If you want me to commit this as-is, just
> > say so and I will.  Otherwise, post an update and
> > I'll use that.  In any case, you can consider this
> > reviewed by me.
> 
> If you have an idea how to reword the description send it my way.

Here's an attempt.  (It also gives you a chance to correct
my understanding...)

A large part of the issue is that XFS writes data out itself
two times in the ->sync_fs method, overriding the livelock
protection in the core writeback code.  This patch removes
these XFS-internal sync calls and relies on the VFS to do it's
work just like all other filesystems do.

Another issue is the lock-less xfs_ioend_wait() call,
which doesn't prevent new ioends from being queued up
while waiting for the count to reach zero.  Removing
the second SYNC_WAIT call to xfs_sync_data() eliminates
one place this is used unnecessarily by avoiding the
wait request at the end of xfs_sync_inode_data().  In
most cases there is no need to wait for ongoing writes
to make it to disk, as long as those queued at the time
of a sync request get flushed out.

We still wait like this in ->write_inode, and we should
remove that as well, but that's material for a separate
commit.

                                        -Alex

<Prev in Thread] Current Thread [Next in Thread>