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 <[email protected]>
----
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;
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 <[email protected]>
>
> ----
>
> 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
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2009-12-17 at 09:40 -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 <[email protected]>
> >
> > ----
> >
> > 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 ...
>
>
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
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 <[email protected]>
> >
> > ----
> >
> > 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 ...
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 <[email protected]>
-aneesh
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 <[email protected]>
Added to the ext4 patch queue, thanks.
- Ted