Fix mapping->writeback_index to point to the last written page

PATCH In review Jun'ichi Nomura 3 participants 7 messages Feb 25, 2011 – Mar 4, 2011

Subsystems (3)

Memory management VFS Page cache

lore

Share & tools

lore

Jun'ichi Nomura

author

2011-02-25 07:55 UTC #0 lore raw

PATCH 1/1 Fix mapping->writeback_index to point to the last written page

Hi,

For range-cyclic writeback (e.g. kupdate), the writeback code sets a continuation point of the next writeback to mapping->writeback_index.

Current code sets the page next to the last written page.
I think it's intended for sequential writer.
However, in many cases, sequential writer is writing in the middle of the page and it just redirties the last written page by continuing from that.

So the next writeback should try to continue from the last written page, not the next one.
(If it's clean because the writer was on the page boundary,
pagevec_lookup_tag just skips it. So no problem.)

Otherwise, the last written page was left dirty until the writeback wraps around.

I.e. if the sequential dirtier has written on pagecache as '*'s below:

|*******|*******|****---|-------|-------| ( |---| is a page )

then, writeback happens:

|-------|-------|-------|-------|-------|

and the dirtier continues:

   |-------|-------|----***|*******|*****--|
                   A       B

Next writeback should start from page A, not B.

As an exceptional case, when I/O error happens, set done_index to the next page as the comment in the code suggests.

Signed-off-by Jun'ichi Nomura <[email protected]>

mm/page-writeback.c +2 -1

index 2cb01f6..beae3ed 100644
@@ -927,7 +927,7 @@ retry:
 				break;
 			}
 
-			done_index = page->index + 1;
+			done_index = page->index;
 
 			lock_page(page);

@@ -977,6 +977,7 @@ continue_unlock:
 					 * not be suitable for data integrity
 					 * writeout).
 					 */
+					done_index = page->index + 1;
 					done = 1;
 					break;
 				}

Jan Kara

maintainer

2011-03-02 22:18 UTC #1 lore raw

Hello,

Quoted 5 lines · Jun'ichi Nomura

For range-cyclic writeback (e.g. kupdate), the writeback code sets a continuation point of the next writeback to mapping->writeback_index.
Current code sets the page next to the last written page. I think it's intended for sequential writer.

Not exactly. The code is meant so that background writeback gets to
writing the end of a file which gets continuously dirtied (if we always started at the beginning, nr_to_write could always get to 0 before we reach end of the file).

Quoted 17 lines

However, in many cases, sequential writer is writing in the middle of the page and it just redirties the last written page by continuing from that.
So the next writeback should try to continue from the last written page, not the next one.
(If it's clean because the writer was on the page boundary,
pagevec_lookup_tag just skips it. So no problem.)
Otherwise, the last written page was left dirty until the writeback wraps around.
I.e. if the sequential dirtier has written on pagecache as '*'s below:
   |*******|*******|****---|-------|-------|     ( |---| is a page )
then, writeback happens:

Quoted 8 lines

   |-------|-------|-------|-------|-------|
and the dirtier continues:
   |-------|-------|----***|*******|*****--|
                   A       B
Next writeback should start from page A, not B.

Yes, this is downside of our current scheme. Have you actually observed
it in practice or is it mostly a theoretic concern?

But as I'm thinking about it, it wouldn't harm our original aim to do what you propose and it can help this relatively common case. So I think it's a good idea. Fengguang, what do you think?

Honza

Signature · 2 lines

Jan Kara <[email protected]>
SUSE Labs, CR

Jun'ichi Nomura

author

2011-03-03 02:26 UTC #2 lore raw

Hi Jan,

thank you for the comments.

Quoted 6 lines · Jan Kara

Quoted 1 line · Jun'ichi Nomura
I think it's intended for sequential writer.
Not exactly. The code is meant so that background writeback gets to
writing the end of a file which gets continuously dirtied (if we always started at the beginning, nr_to_write could always get to 0 before we reach end of the file).

Ah, ok. Thanks.
My patch doesn't break the above goal.

Quoted 11 lines

Quoted 11 lines
Otherwise, the last written page was left dirty until the writeback wraps around.
I.e. if the sequential dirtier has written on pagecache as '*'s below:
   |*******|*******|****---|-------|-------|     ( |---| is a page )
then, writeback happens:
   |-------|-------|-------|-------|-------|

Quoted 8 lines

Quoted 6 lines
and the dirtier continues:
   |-------|-------|----***|*******|*****--|
                   A       B
Next writeback should start from page A, not B.
Yes, this is downside of our current scheme. Have you actually observed
it in practice or is it mostly a theoretic concern?

Half practical, half theoretic.
It has been observed with a real application, which uses a big ring file.

I took a trace with a test program for example:

[1st writeback session]

       ...
       flush-8:0-2743  4571: block_bio_queue: 8,0 W 94898514 + 8
       flush-8:0-2743  4571: block_bio_queue: 8,0 W 94898522 + 8
       flush-8:0-2743  4571: block_bio_queue: 8,0 W 94898530 + 8
       flush-8:0-2743  4571: block_bio_queue: 8,0 W 94898538 + 8
       flush-8:0-2743  4571: block_bio_queue: 8,0 W 94898546 + 8
     kworker/0:1-11    4571: block_rq_issue: 8,0 W 0 () 94898514 + 40

Quoted 2 lines

    flush-8:0-2743  4571: block_bio_queue: 8,0 W 94898554 + 8
    flush-8:0-2743  4571: block_rq_issue: 8,0 W 0 () 94898554 + 8

[2nd writeback session after 35sec]

       flush-8:0-2743  4606: block_bio_queue: 8,0 W 94898562 + 8
       flush-8:0-2743  4606: block_bio_queue: 8,0 W 94898570 + 8
       flush-8:0-2743  4606: block_bio_queue: 8,0 W 94898578 + 8
       ...
     kworker/0:1-11    4606: block_rq_issue: 8,0 W 0 () 94898562 + 640
     kworker/0:1-11    4606: block_rq_issue: 8,0 W 0 () 94899202 + 72
       ...
       flush-8:0-2743  4606: block_bio_queue: 8,0 W 94899962 + 8
       flush-8:0-2743  4606: block_bio_queue: 8,0 W 94899970 + 8
       flush-8:0-2743  4606: block_bio_queue: 8,0 W 94899978 + 8
       flush-8:0-2743  4606: block_bio_queue: 8,0 W 94899986 + 8
       flush-8:0-2743  4606: block_bio_queue: 8,0 W 94899994 + 8
     kworker/0:1-11    4606: block_rq_issue: 8,0 W 0 () 94899962 + 40

Quoted 2 lines

    flush-8:0-2743  4606: block_bio_queue: 8,0 W 94898554 + 8
    flush-8:0-2743  4606: block_rq_issue: 8,0 W 0 () 94898554 + 8

The 1st writeback ended at block 94898562. (94898554+8) The 2nd writeback started there.
However, since the last page at the 1st writeback was just redirtied, the 2nd writeback looped back to block 94898554 after sequentially submitting blocks from 94898562 to 94900001.

1 extra seek which could be avoided.
I haven't seen fatal problem with the latest kernel, though.

With older kernels (before 2.6.29, without commit 31a12666), kupdate leaves the dirty pages like spots until the application wraps around the ring. (It could take hours to days.)
That led me to this code.

Quoted 3 lines

But as I'm thinking about it, it wouldn't harm our original aim to do what you propose and it can help this relatively common case. So I think it's a good idea. Fengguang, what do you think?

--
Jun'ichi Nomura, NEC Corporation

Wu Fengguang

Tested-by

2011-03-03 13:31 UTC #3 lore raw

Quoted 6 lines · Jun'ichi Nomura

Hi Jan,
thank you for the comments.
Quoted 1 line · Jan Kara
On Fri 25-02-11 16:55:19, Jun'ichi Nomura wrote:

Quoted 8 lines

Quoted 5 lines
Quoted 1 line
I think it's intended for sequential writer.
Not exactly. The code is meant so that background writeback gets to
writing the end of a file which gets continuously dirtied (if we always started at the beginning, nr_to_write could always get to 0 before we reach end of the file).
Ah, ok. Thanks.
My patch doesn't break the above goal.

Yeah.

Quoted 36 lines

Quoted 11 lines
Quoted 11 lines
Otherwise, the last written page was left dirty until the writeback wraps around.
I.e. if the sequential dirtier has written on pagecache as '*'s below:
   |*******|*******|****---|-------|-------|     ( |---| is a page )
then, writeback happens:
   |-------|-------|-------|-------|-------|
Quoted 8 lines
Quoted 6 lines
and the dirtier continues:
   |-------|-------|----***|*******|*****--|
                   A       B
Next writeback should start from page A, not B.
Yes, this is downside of our current scheme. Have you actually observed
it in practice or is it mostly a theoretic concern?
Half practical, half theoretic.
It has been observed with a real application, which uses a big ring file.
I took a trace with a test program for example:
[1st writeback session]
       ...
       flush-8:0-2743  4571: block_bio_queue: 8,0 W 94898514 + 8
       flush-8:0-2743  4571: block_bio_queue: 8,0 W 94898522 + 8
       flush-8:0-2743  4571: block_bio_queue: 8,0 W 94898530 + 8
       flush-8:0-2743  4571: block_bio_queue: 8,0 W 94898538 + 8
       flush-8:0-2743  4571: block_bio_queue: 8,0 W 94898546 + 8
     kworker/0:1-11    4571: block_rq_issue: 8,0 W 0 () 94898514 + 40
Quoted 2 lines
Quoted 2 lines
    flush-8:0-2743  4571: block_bio_queue: 8,0 W 94898554 + 8
    flush-8:0-2743  4571: block_rq_issue: 8,0 W 0 () 94898554 + 8

Quoted 8 lines

[2nd writeback session after 35sec]

       flush-8:0-2743  4606: block_bio_queue: 8,0 W 94898562 + 8
       flush-8:0-2743  4606: block_bio_queue: 8,0 W 94898570 + 8
       flush-8:0-2743  4606: block_bio_queue: 8,0 W 94898578 + 8
       ...
     kworker/0:1-11    4606: block_rq_issue: 8,0 W 0 () 94898562 + 640
     kworker/0:1-11    4606: block_rq_issue: 8,0 W 0 () 94899202 + 72
       ...

        >        flush-8:0-2743  4606: block_bio_queue: 8,0 W 94899962 + 8
        >        flush-8:0-2743  4606: block_bio_queue: 8,0 W 94899970 + 8
        >        flush-8:0-2743  4606: block_bio_queue: 8,0 W 94899978 + 8
        >        flush-8:0-2743  4606: block_bio_queue: 8,0 W 94899986 + 8
        >        flush-8:0-2743  4606: block_bio_queue: 8,0 W 94899994 + 8
==>     >      kworker/0:1-11    4606: block_rq_issue: 8,0 W 0 () 94899962 + 40
        > >>     flush-8:0-2743  4606: block_bio_queue: 8,0 W 94898554 + 8
==>     > >>     flush-8:0-2743  4606: block_rq_issue: 8,0 W 0 () 94898554 + 8

I'd expect the wrapped around 94898554+8 to be merged with 94899962+8. Why kworker/0:1-11 is submitting the request early? And the second request is submitted by flush-8:0-2743.

Quoted 6 lines

The 1st writeback ended at block 94898562. (94898554+8) The 2nd writeback started there.
However, since the last page at the 1st writeback was just redirtied, the 2nd writeback looped back to block 94898554 after sequentially submitting blocks from 94898562 to 94900001.

Quoted 11 lines

1 extra seek which could be avoided.
I haven't seen fatal problem with the latest kernel, though.
With older kernels (before 2.6.29, without commit 31a12666), kupdate leaves the dirty pages like spots until the application wraps around the ring. (It could take hours to days.)
That led me to this code.
Quoted 3 lines
But as I'm thinking about it, it wouldn't harm our original aim to do what you propose and it can help this relatively common case. So I think it's a good idea. Fengguang, what do you think?

I see no problem too.

Tested-by Wu Fengguang <[email protected]>

I compared the writeback_single_inode trace (https://lkml.org/lkml/2011/3/3/73)
and find no differences other than the 1-offset in the index field.

writeback_single_inode: bdi 8:0: ino=12 state=I_DIRTY_PAGES age=292 wrote=8192 to_write=-1 index=8191
writeback_single_inode: bdi 8:0: ino=12 state=I_DIRTY_PAGES age=2820 wrote=8192 to_write=0 index=16383
writeback_single_inode: bdi 8:0: ino=12 state=I_DIRTY_PAGES age=8644 wrote=8192 to_write=0 index=24575
writeback_single_inode: bdi 8:0: ino=12 state=I_DIRTY_PAGES age=15664 wrote=8192 to_write=0 index=32767
writeback_single_inode: bdi 8:0: ino=12 state=I_DIRTY_PAGES age=21660 wrote=8192 to_write=0 index=40959
writeback_single_inode: bdi 8:0: ino=12 state=I_DIRTY_PAGES age=27592 wrote=8192 to_write=0 index=49151
writeback_single_inode: bdi 8:0: ino=12 state=I_DIRTY_PAGES age=34700 wrote=8192 to_write=0 index=57343
writeback_single_inode: bdi 8:0: ino=12 state=I_DIRTY_PAGES age=40936 wrote=8192 to_write=0 index=65535
writeback_single_inode: bdi 8:0: ino=12 state=I_DIRTY_PAGES age=47556 wrote=8192 to_write=0 index=73727
writeback_single_inode: bdi 8:0: ino=12 state=I_DIRTY_PAGES age=53660 wrote=8192 to_write=0 index=81919
writeback_single_inode: bdi 8:0: ino=12 state=I_DIRTY_PAGES age=60220 wrote=8192 to_write=0 index=90111
writeback_single_inode: bdi 8:0: ino=12 state=I_DIRTY_PAGES age=67156 wrote=8192 to_write=0 index=98303
writeback_single_inode: bdi 8:0: ino=12 state=I_DIRTY_PAGES age=73796 wrote=8192 to_write=0 index=106495
writeback_single_inode: bdi 8:0: ino=12 state=I_DIRTY_PAGES age=79948 wrote=8192 to_write=0 index=114687
writeback_single_inode: bdi 8:0: ino=12 state=I_DIRTY_PAGES age=86176 wrote=8192 to_write=0 index=122879

writeback_single_inode: bdi 8:0: ino=12 state=I_DIRTY_PAGES age=480 wrote=8192 to_write=-1 index=8192
writeback_single_inode: bdi 8:0: ino=12 state=I_DIRTY_PAGES age=536 wrote=8192 to_write=0 index=16384
writeback_single_inode: bdi 8:0: ino=12 state=I_DIRTY_PAGES age=828 wrote=8192 to_write=0 index=24576
writeback_single_inode: bdi 8:0: ino=12 state=I_DIRTY_PAGES age=1540 wrote=8192 to_write=0 index=32768
writeback_single_inode: bdi 8:0: ino=12 state=I_DIRTY_PAGES age=2084 wrote=8192 to_write=0 index=40960
writeback_single_inode: bdi 8:0: ino=12 state=I_DIRTY_PAGES age=2620 wrote=8192 to_write=0 index=49152
writeback_single_inode: bdi 8:0: ino=12 state=I_DIRTY_PAGES age=3156 wrote=8192 to_write=0 index=57344
writeback_single_inode: bdi 8:0: ino=12 state=I_DIRTY_PAGES age=4008 wrote=8192 to_write=0 index=65536
writeback_single_inode: bdi 8:0: ino=12 state=I_DIRTY_PAGES age=4964 wrote=8192 to_write=0 index=73728
writeback_single_inode: bdi 8:0: ino=12 state=I_DIRTY_PAGES age=5560 wrote=8192 to_write=0 index=81920
writeback_single_inode: bdi 8:0: ino=12 state=I_DIRTY_PAGES age=6132 wrote=8192 to_write=0 index=90112
writeback_single_inode: bdi 8:0: ino=12 state=I_DIRTY_PAGES age=7436 wrote=8192 to_write=-180 index=98304
writeback_single_inode: bdi 8:0: ino=12 state=I_DIRTY_PAGES age=8200 wrote=8192 to_write=0 index=106496
writeback_single_inode: bdi 8:0: ino=12 state=I_DIRTY_PAGES age=8652 wrote=8192 to_write=0 index=114688
writeback_single_inode: bdi 8:0: ino=12 state=I_DIRTY_PAGES age=9424 wrote=8192 to_write=0 index=122880

Thanks,
Fengguang

Jan Kara

maintainer

2011-03-03 14:08 UTC #4 lore raw

Quoted 11 lines · Wu Fengguang

On Thu, Mar 03, 2011 at 10:26:19AM +0800, Jun'ichi Nomura wrote:

        >        flush-8:0-2743  4606: block_bio_queue: 8,0 W 94899962 + 8
        >        flush-8:0-2743  4606: block_bio_queue: 8,0 W 94899970 + 8
        >        flush-8:0-2743  4606: block_bio_queue: 8,0 W 94899978 + 8
        >        flush-8:0-2743  4606: block_bio_queue: 8,0 W 94899986 + 8
        >        flush-8:0-2743  4606: block_bio_queue: 8,0 W 94899994 + 8
==>     >      kworker/0:1-11    4606: block_rq_issue: 8,0 W 0 () 94899962 + 40
        > >>     flush-8:0-2743  4606: block_bio_queue: 8,0 W 94898554 + 8
==>     > >>     flush-8:0-2743  4606: block_rq_issue: 8,0 W 0 () 94898554 + 8

I'd expect the wrapped around 94898554+8 to be merged with 94899962+8.

How could they be merged? They are not continguous...

Quoted 2 lines

Why kworker/0:1-11 is submitting the request early? And the second request is submitted by flush-8:0-2743.

I'm not sure about that either - but I think that kworker is submitting
the work when unplug happens while flush ends up doing it when the queue is alredy unplugged.

Quoted 14 lines

Quoted 14 lines
The 1st writeback ended at block 94898562. (94898554+8) The 2nd writeback started there.
However, since the last page at the 1st writeback was just redirtied, the 2nd writeback looped back to block 94898554 after sequentially submitting blocks from 94898562 to 94900001.
1 extra seek which could be avoided.
I haven't seen fatal problem with the latest kernel, though.
With older kernels (before 2.6.29, without commit 31a12666), kupdate leaves the dirty pages like spots until the application wraps around the ring. (It could take hours to days.)
That led me to this code.

Quoted 7 lines

Quoted 3 lines
Quoted 3 lines
But as I'm thinking about it, it wouldn't harm our original aim to do what you propose and it can help this relatively common case. So I think it's a good idea. Fengguang, what do you think?
I see no problem too.
Tested-by Wu Fengguang <[email protected]>

OK, I'll update the changelog to reflect our discussion and post the
patch to Andrew for inclusion.

Honza

Jun'ichi Nomura

author

2011-03-04 01:45 UTC #5 lore raw

Quoted 11 lines · Jan Kara

Quoted 10 lines · Wu Fengguang

On Thu, Mar 03, 2011 at 10:26:19AM +0800, Jun'ichi Nomura wrote:

        >        flush-8:0-2743  4606: block_bio_queue: 8,0 W 94899962 + 8
        >        flush-8:0-2743  4606: block_bio_queue: 8,0 W 94899970 + 8
        >        flush-8:0-2743  4606: block_bio_queue: 8,0 W 94899978 + 8
        >        flush-8:0-2743  4606: block_bio_queue: 8,0 W 94899986 + 8
        >        flush-8:0-2743  4606: block_bio_queue: 8,0 W 94899994 + 8
==>     >      kworker/0:1-11    4606: block_rq_issue: 8,0 W 0 () 94899962 + 40
        > >>     flush-8:0-2743  4606: block_bio_queue: 8,0 W 94898554 + 8
==>     > >>     flush-8:0-2743  4606: block_rq_issue: 8,0 W 0 () 94898554 + 8

Quoted 8 lines

Quoted 1 line
I'd expect the wrapped around 94898554+8 to be merged with 94899962+8.
How could they be merged? They are not continguous...
Quoted 2 lines
Why kworker/0:1-11 is submitting the request early? And the second request is submitted by flush-8:0-2743.
I'm not sure about that either - but I think that kworker is submitting
the work when unplug happens while flush ends up doing it when the queue is alredy unplugged.

Yes. For the above case, it was unplug.

For other cases, the reason was max_sectors.
Before writeback wraps around, the request has already grown to max_sectors size (~512KB) and doesn't accept new merge.

Thanks,

Signature · 1 line

Jun'ichi Nomura, NEC Corporation

Wu Fengguang

2011-03-04 02:20 UTC #6 lore raw

Quoted 21 lines · Jun'ichi Nomura

Quoted 20 lines · Jan Kara

Quoted 1 line · Wu Fengguang

On Thu, Mar 03, 2011 at 10:26:19AM +0800, Jun'ichi Nomura wrote:

[2nd writeback session after 35sec]

       flush-8:0-2743  4606: block_bio_queue: 8,0 W 94898562 + 8
       flush-8:0-2743  4606: block_bio_queue: 8,0 W 94898570 + 8
       flush-8:0-2743  4606: block_bio_queue: 8,0 W 94898578 + 8
       ...
     kworker/0:1-11    4606: block_rq_issue: 8,0 W 0 () 94898562 + 640
     kworker/0:1-11    4606: block_rq_issue: 8,0 W 0 () 94899202 + 72 
       ...

Quoted 9 lines

        >        flush-8:0-2743  4606: block_bio_queue: 8,0 W 94899962 + 8
        >        flush-8:0-2743  4606: block_bio_queue: 8,0 W 94899970 + 8
        >        flush-8:0-2743  4606: block_bio_queue: 8,0 W 94899978 + 8
        >        flush-8:0-2743  4606: block_bio_queue: 8,0 W 94899986 + 8
        >        flush-8:0-2743  4606: block_bio_queue: 8,0 W 94899994 + 8
==>     >      kworker/0:1-11    4606: block_rq_issue: 8,0 W 0 () 94899962 + 40
        > >>     flush-8:0-2743  4606: block_bio_queue: 8,0 W 94898554 + 8
==>     > >>     flush-8:0-2743  4606: block_rq_issue: 8,0 W 0 () 94898554 + 8

Quoted 2 lines

Quoted 2 lines
Quoted 1 line
I'd expect the wrapped around 94898554+8 to be merged with 94899962+8.
How could they be merged? They are not continguous...

Oops, it is. It's 94898562+8 that is continuous with 94898554+8, however the former has been submitted on max_sectors limit. So 94898554+8 lost the chance to get merged.

Quoted 11 lines

Quoted 5 lines
Quoted 2 lines
Why kworker/0:1-11 is submitting the request early? And the second request is submitted by flush-8:0-2743.
I'm not sure about that either - but I think that kworker is submitting
the work when unplug happens while flush ends up doing it when the queue is alredy unplugged.
Yes. For the above case, it was unplug.
For other cases, the reason was max_sectors.
Before writeback wraps around, the request has already grown to max_sectors size (~512KB) and doesn't accept new merge.

Yup, good explanation!

Thanks,
Fengguang