lkml.org 
[lkml]   [2010]   [Feb]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] writeback: Fix broken sync writeback


On Wed, 17 Feb 2010, Jan Kara wrote:
>
> The IO size actually does matter for performance because if you switch
> after 4 MB (current value of MAX_WRITEBACK_PAGES) to writing another inode,

No.

Dammit, read the code.

That's my whole _point_. Look at the for-loop.

We DO NOT SWITCH to another inode, because we just continue in the
for-loop.

This is why I think your patch is crap. You clearly haven't even read the
code, the patch makes no sense, and there must be something else going on
than what you _claim_ is going on.

> So I'll post a new patch with a better changelog shortly.

Not "shortly". Because you clearly haven't looked at the code. Look here:

/*
* If we consumed everything, see if we have more
*/
if (wbc.nr_to_write <= 0)
continue;

and here:

/*
* Did we write something? Try for more
*/
if (wbc.nr_to_write < MAX_WRITEBACK_PAGES)
continue;

the point is, the way the code is written, it is very much _meant_ to
write out one file in one go, except it is supposed to chunk it up so that
you never see _too_ many dirty pages in flight at any time. Because tons
of dirty pages in the queues makes for bad latency.

Now, I admit that since your patch makes a difference, there is a bug
somewhere. That's never what I've argued against. What I argue against is
your patch itself, and your explanation for it. They don't make sense.

I suspect that there is something else going on. In particular, I wonder
if multiple calls to "writeback_inodes_wb()" somehow mess up the wbc
state, and it starts writing the same inode from the beginning, or it
simply has some bug that means that it moves the inode to the head of the
queue, or something.

For example, it might be that the logic in writeback_inodes_wb() moves an
inode back (the "redirty_tail()" cases) in bad ways when it shouldn't.

See what I'm trying to say? I think your patch probably just hides the
_real_ bug. Because it really shouldn't matter if we do things in 4MB
chunks or whatever, because the for-loop should happily continue.

Linus


\
 
 \ /
  Last update: 2010-02-17 00:37    [W:2.038 / U:0.560 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site