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

Alex Elder
2011-07-06T14:59:53Z

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