Google Groups no longer supports new Usenet posts or subscriptions. Historical content remains viewable.
Dismiss

[PATCH] ext4: return correct wbc.nr_to_write in ext4_da_writepages

10 views
Skip to first unread message

Richard Kennedy

unread,
Dec 17, 2009, 10:20:01 AM12/17/09
to
ext4: always re-base nr_to_write in ext4_da_writepages

When ext4_da_writepages increases the nr_to_write in writeback_control
then it must always re-base the return value.

Without this change, when wb_writeback calculates how many pages were
actually written it can get a negative value and loop more times than
necessary. In tests I have seen nearly all the dirty pages pushed out to
writeback due to this issue.

Signed-off-by: Richard Kennedy <ric...@rsk.demon.co.uk>

----

patch against 2.6.32
tested on x86_64

wb_writeback calculates (MAX_WRITE_PAGES - nr_to_write) & cannot know
that the value got changed.

I'm not sure what the test I removed was for.
Perhaps
if (nr_to_writebump)
wbc->nr_to_write -= nr_to_writebump;
was intended?

regards
Richard

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 2c8caa5..52a573c 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -2999,8 +2999,7 @@ retry:
out_writepages:
if (!no_nrwrite_index_update)
wbc->no_nrwrite_index_update = 0;
- if (wbc->nr_to_write > nr_to_writebump)
- wbc->nr_to_write -= nr_to_writebump;
+ wbc->nr_to_write -= nr_to_writebump;
wbc->range_start = range_start;
trace_ext4_da_writepages_result(inode, wbc, ret, pages_written);
return ret;


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Eric Sandeen

unread,
Dec 17, 2009, 10:50:03 AM12/17/09
to
Richard Kennedy wrote:
> ext4: always re-base nr_to_write in ext4_da_writepages
>
> When ext4_da_writepages increases the nr_to_write in writeback_control
> then it must always re-base the return value.
>
> Without this change, when wb_writeback calculates how many pages were
> actually written it can get a negative value and loop more times than
> necessary. In tests I have seen nearly all the dirty pages pushed out to
> writeback due to this issue.
>
> Signed-off-by: Richard Kennedy <ric...@rsk.demon.co.uk>
>
> ----
>
> patch against 2.6.32
> tested on x86_64
>
> wb_writeback calculates (MAX_WRITE_PAGES - nr_to_write) & cannot know
> that the value got changed.
>
> I'm not sure what the test I removed was for.
> Perhaps
> if (nr_to_writebump)
> wbc->nr_to_write -= nr_to_writebump;
> was intended?

Ted's commit 55138e0b added it (just part of the commit):

@@ -2914,7 +2994,8 @@ retry:


out_writepages:
if (!no_nrwrite_index_update)
wbc->no_nrwrite_index_update = 0;

- wbc->nr_to_write -= nr_to_writebump;

+ if (wbc->nr_to_write > nr_to_writebump)


+ wbc->nr_to_write -= nr_to_writebump;
wbc->range_start = range_start;
trace_ext4_da_writepages_result(inode, wbc, ret, pages_written);
return ret;

so it looks like the intent there was to stop ->nr_to_write from
going negative ...


> regards
> Richard
>
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 2c8caa5..52a573c 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -2999,8 +2999,7 @@ retry:
> out_writepages:
> if (!no_nrwrite_index_update)
> wbc->no_nrwrite_index_update = 0;
> - if (wbc->nr_to_write > nr_to_writebump)
> - wbc->nr_to_write -= nr_to_writebump;
> + wbc->nr_to_write -= nr_to_writebump;
> wbc->range_start = range_start;
> trace_ext4_da_writepages_result(inode, wbc, ret, pages_written);
> return ret;
>
>
> --

> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in

Richard Kennedy

unread,
Dec 17, 2009, 11:00:01 AM12/17/09
to
wb_writeback is OK with negative, it just needs to know how many pages
were written. Then it can decide if it's done the work it was asked to
do. balance_dirty_pages uses this throttle a device by asking for
writeback on a small number of pages.
regards
Richard

Aneesh Kumar K.V

unread,
Dec 17, 2009, 12:40:02 PM12/17/09
to

I guess writeback code can handle nr_to_write going negative. If we are
not updating wbc->nr_to_write then i guess writeback code will get a
wrong value for number of pages written and can end up doing wrong things
We had it that way as a part of 22208dedbd7626e5fc4339c417f8d24cc21f79d7
and i guess we didn't had any problems with that

So for the patch

Acked-by: Aneesh Kumar K.V <aneesh...@linux.vnet.ibm.com>

-aneesh

ty...@mit.edu

unread,
Dec 25, 2009, 3:20:01 PM12/25/09
to
On Thu, Dec 17, 2009 at 11:02:32PM +0530, Aneesh Kumar K.V wrote:
> On Thu, Dec 17, 2009 at 09:40:25AM -0600, Eric Sandeen wrote:
> > Richard Kennedy wrote:
> > > ext4: always re-base nr_to_write in ext4_da_writepages
> > >
> > > When ext4_da_writepages increases the nr_to_write in writeback_control
> > > then it must always re-base the return value.
> > >
> > > Without this change, when wb_writeback calculates how many pages were
> > > actually written it can get a negative value and loop more times than
> > > necessary. In tests I have seen nearly all the dirty pages pushed out to
> > > writeback due to this issue.
> > >
> > > Signed-off-by: Richard Kennedy <ric...@rsk.demon.co.uk>

Added to the ext4 patch queue, thanks.

- Ted

0 new messages