writeback fixes and cleanups for 2.6.40 (v2)

sync(2) is performed in two stages: the WB_SYNC_NONE sync and the WB_SYNC_ALL sync. Tag the first stage with wbc.tagged_sync and do livelock prevention for it, too.

Note that writeback_inodes_sb() is called by not only sync(), they are treated the same because the other callers also need livelock prevention.

Impact: It changes the order in which pages/inodes are synced to disk. Now in the WB_SYNC_NONE stage, it won't proceed to write the next inode until finished with the current inode.

Quoted 10 lines · Wu Fengguang

sync(2) is performed in two stages: the WB_SYNC_NONE sync and the WB_SYNC_ALL sync. Tag the first stage with wbc.tagged_sync and do livelock prevention for it, too.

Note that writeback_inodes_sb() is called by not only sync(), they are treated the same because the other callers also need livelock prevention.

Impact: It changes the order in which pages/inodes are synced to disk. Now in the WB_SYNC_NONE stage, it won't proceed to write the next inode until finished with the current inode.

What about all the filesystems that implement their own .writepages()/write_cache_pages() functions or have
have special code that checks WB_SYNC_ALL in .writepages (e.g. gfs2, ext4, btrfs and perhaps others). Don't they all need to be aware of this tagged_sync field?

Quoted 5 lines

What about all the filesystems that implement their own .writepages()/write_cache_pages() functions or have
have special code that checks WB_SYNC_ALL in .writepages (e.g. gfs2, ext4, btrfs and perhaps others). Don't they all need to be aware of this tagged_sync field?

Right, good point. Currently only ext4 is updated. The other filesystems --- afs, btrfs, cifs, gfs2 --- do not even use PAGECACHE_TAG_TOWRITE for livelock prevention. My plan was to add PAGECACHE_TAG_TOWRITE and tagged_sync code to them as the next step, when tagged_sync is accepted and proved to work fine.

Quoted 12 lines · Dave Chinner

Quoted 10 lines · Wu Fengguang

sync(2) is performed in two stages: the WB_SYNC_NONE sync and the WB_SYNC_ALL sync. Tag the first stage with wbc.tagged_sync and do livelock prevention for it, too.

Note that writeback_inodes_sb() is called by not only sync(), they are treated the same because the other callers also need livelock prevention.

Impact: It changes the order in which pages/inodes are synced to disk. Now in the WB_SYNC_NONE stage, it won't proceed to write the next inode until finished with the current inode.

Quoted 5 lines

What about all the filesystems that implement their own .writepages()/write_cache_pages() functions or have
have special code that checks WB_SYNC_ALL in .writepages (e.g. gfs2, ext4, btrfs and perhaps others). Don't they all need to be aware of this tagged_sync field?

Right, good point. Currently only ext4 is updated. The other filesystems --- afs, btrfs, cifs, gfs2 --- do not even use PAGECACHE_TAG_TOWRITE for livelock prevention. My plan was to add PAGECACHE_TAG_TOWRITE and tagged_sync code to them as the next step, when tagged_sync is accepted and proved to work fine.

Quoted 19 lines · Wu Fengguang

Quoted 12 lines · Dave Chinner

Quoted 10 lines · Wu Fengguang

sync(2) is performed in two stages: the WB_SYNC_NONE sync and the WB_SYNC_ALL sync. Tag the first stage with wbc.tagged_sync and do livelock prevention for it, too.

Note that writeback_inodes_sb() is called by not only sync(), they are treated the same because the other callers also need livelock prevention.

Impact: It changes the order in which pages/inodes are synced to disk. Now in the WB_SYNC_NONE stage, it won't proceed to write the next inode until finished with the current inode.

Quoted 5 lines

What about all the filesystems that implement their own .writepages()/write_cache_pages() functions or have
have special code that checks WB_SYNC_ALL in .writepages (e.g. gfs2, ext4, btrfs and perhaps others). Don't they all need to be aware of this tagged_sync field?

Quoted 5 lines

Right, good point. Currently only ext4 is updated. The other filesystems --- afs, btrfs, cifs, gfs2 --- do not even use PAGECACHE_TAG_TOWRITE for livelock prevention. My plan was to add PAGECACHE_TAG_TOWRITE and tagged_sync code to them as the next step, when tagged_sync is accepted and proved to work fine.

Where "proved to work fine" can mean "caused regressions for certain filesystems"? I mean, for btrfs it means that the bio is submitted with WRITE rather than WRITE_SYNC, which causes subtle changes of behaviour in the elevator. that could cause strange regressions that are very hard to isolate.

Hence regardless of whether filesystems use PAGECACHE_TAG_TOWRITE or not, filesystems are checking for synchronous writeback for a reason. If we now have two different ways of signalling sync writeback they need to know about them.

Which just raised the question in my mind - why did you add a new field rather than a new sync_mode definition? After all, this is a new sync control, and it seems clumsy to me to have two separate control fields for defining sync behaviour...

shall we rename .tagged_sync to .tagged_write?

Explicitly update .dirtied_when on synced inodes, so that they are no longer considered for writeback in the next round.

We'll do more aggressive "keep writeback as long as we wrote something" logic in wb_writeback(). The "use LONG_MAX .nr_to_write" trick in commit b9543dac5bbc ("writeback: avoid livelocking WB_SYNC_ALL writeback") will no longer be enough to stop sync livelock.

Reviewed-by Jan Kara <[email protected]>

Signed-off-by Wu Fengguang <[email protected]>

---
 fs/fs-writeback.c |    9 +++++++++
 1 file changed, 9 insertions(+)

ext3/ext4 are working fine now, however tests show that XFS may still livelock inside the XFS routines:

[ 3581.181253] sync            D ffff8800b6ca15d8  4560  4403   4392 0x00000000
[ 3581.181734]  ffff88006f775bc8 0000000000000046 ffff8800b6ca12b8 00000001b6ca1938
[ 3581.182411]  ffff88006f774000 00000000001d2e40 00000000001d2e40 ffff8800b6ca1280
[ 3581.183088]  00000000001d2e40 ffff88006f775fd8 00000340af111ef2 00000000001d2e40
[ 3581.183765] Call Trace:
[ 3581.184008]  [<ffffffff8109be73>] ? lock_release_holdtime+0xa3/0xab
[ 3581.184392]  [<ffffffff8108cc0d>] ? prepare_to_wait+0x6c/0x79
[ 3581.184756]  [<ffffffff8108cc0d>] ? prepare_to_wait+0x6c/0x79
[ 3581.185120]  [<ffffffff812ed520>] xfs_ioend_wait+0x87/0x9f
[ 3581.185474]  [<ffffffff8108c97a>] ? wake_up_bit+0x2a/0x2a
[ 3581.185827]  [<ffffffff812f742a>] xfs_sync_inode_data+0x92/0x9d
[ 3581.186198]  [<ffffffff812f76e2>] xfs_inode_ag_walk+0x1a5/0x287
[ 3581.186569]  [<ffffffff812f779b>] ? xfs_inode_ag_walk+0x25e/0x287
[ 3581.186946]  [<ffffffff812f7398>] ? xfs_sync_worker+0x69/0x69
[ 3581.187311]  [<ffffffff812e2354>] ? xfs_perag_get+0x68/0xd0
[ 3581.187669]  [<ffffffff81092175>] ? local_clock+0x41/0x5a
[ 3581.188020]  [<ffffffff8109be73>] ? lock_release_holdtime+0xa3/0xab
[ 3581.188403]  [<ffffffff812e22ec>] ? xfs_check_sizes+0x160/0x160
[ 3581.188773]  [<ffffffff812e2354>] ? xfs_perag_get+0x68/0xd0
[ 3581.189130]  [<ffffffff812e236c>] ? xfs_perag_get+0x80/0xd0
[ 3581.189488]  [<ffffffff812e22ec>] ? xfs_check_sizes+0x160/0x160
[ 3581.189858]  [<ffffffff812f7831>] ? xfs_inode_ag_iterator+0x6d/0x8f
[ 3581.190241]  [<ffffffff812f7398>] ? xfs_sync_worker+0x69/0x69
[ 3581.190606]  [<ffffffff812f780b>] xfs_inode_ag_iterator+0x47/0x8f
[ 3581.190982]  [<ffffffff811611f5>] ? __sync_filesystem+0x7a/0x7a
[ 3581.191352]  [<ffffffff812f7877>] xfs_sync_data+0x24/0x43
[ 3581.191703]  [<ffffffff812f7911>] xfs_quiesce_data+0x2c/0x88
[ 3581.192065]  [<ffffffff812f5556>] xfs_fs_sync_fs+0x21/0x48
[ 3581.192419]  [<ffffffff811611e1>] __sync_filesystem+0x66/0x7a
[ 3581.192783]  [<ffffffff8116120b>] sync_one_sb+0x16/0x18
[ 3581.193128]  [<ffffffff8113e3e3>] iterate_supers+0x72/0xce
[ 3581.193482]  [<ffffffff81161140>] sync_filesystems+0x20/0x22
[ 3581.193842]  [<ffffffff8116127e>] sys_sync+0x21/0x33
[ 3581.194177]  [<ffffffff819016c2>] system_call_fastpath+0x16/0x1b

fs/fs-writeback.c +0 -0

@@ -419,6 +419,15 @@ writeback_single_inode(struct inode *ino
 	spin_lock(&inode->i_lock);
 	inode->i_state &= ~I_SYNC;

Quoted 61 lines · Wu Fengguang

Explicitly update .dirtied_when on synced inodes, so that they are no longer considered for writeback in the next round.

We'll do more aggressive "keep writeback as long as we wrote something" logic in wb_writeback(). The "use LONG_MAX .nr_to_write" trick in commit b9543dac5bbc ("writeback: avoid livelocking WB_SYNC_ALL writeback") will no longer be enough to stop sync livelock.

Reviewed-by Jan Kara <[email protected]>

Signed-off-by Wu Fengguang <[email protected]>

---
 fs/fs-writeback.c |    9 +++++++++
 1 file changed, 9 insertions(+)

ext3/ext4 are working fine now, however tests show that XFS may still livelock inside the XFS routines:

[ 3581.181253] sync            D ffff8800b6ca15d8  4560  4403   4392 0x00000000
[ 3581.181734]  ffff88006f775bc8 0000000000000046 ffff8800b6ca12b8 00000001b6ca1938
[ 3581.182411]  ffff88006f774000 00000000001d2e40 00000000001d2e40 ffff8800b6ca1280
[ 3581.183088]  00000000001d2e40 ffff88006f775fd8 00000340af111ef2 00000000001d2e40
[ 3581.183765] Call Trace:
[ 3581.184008]  [<ffffffff8109be73>] ? lock_release_holdtime+0xa3/0xab
[ 3581.184392]  [<ffffffff8108cc0d>] ? prepare_to_wait+0x6c/0x79
[ 3581.184756]  [<ffffffff8108cc0d>] ? prepare_to_wait+0x6c/0x79
[ 3581.185120]  [<ffffffff812ed520>] xfs_ioend_wait+0x87/0x9f
[ 3581.185474]  [<ffffffff8108c97a>] ? wake_up_bit+0x2a/0x2a
[ 3581.185827]  [<ffffffff812f742a>] xfs_sync_inode_data+0x92/0x9d
[ 3581.186198]  [<ffffffff812f76e2>] xfs_inode_ag_walk+0x1a5/0x287
[ 3581.186569]  [<ffffffff812f779b>] ? xfs_inode_ag_walk+0x25e/0x287
[ 3581.186946]  [<ffffffff812f7398>] ? xfs_sync_worker+0x69/0x69
[ 3581.187311]  [<ffffffff812e2354>] ? xfs_perag_get+0x68/0xd0
[ 3581.187669]  [<ffffffff81092175>] ? local_clock+0x41/0x5a
[ 3581.188020]  [<ffffffff8109be73>] ? lock_release_holdtime+0xa3/0xab
[ 3581.188403]  [<ffffffff812e22ec>] ? xfs_check_sizes+0x160/0x160
[ 3581.188773]  [<ffffffff812e2354>] ? xfs_perag_get+0x68/0xd0
[ 3581.189130]  [<ffffffff812e236c>] ? xfs_perag_get+0x80/0xd0
[ 3581.189488]  [<ffffffff812e22ec>] ? xfs_check_sizes+0x160/0x160
[ 3581.189858]  [<ffffffff812f7831>] ? xfs_inode_ag_iterator+0x6d/0x8f
[ 3581.190241]  [<ffffffff812f7398>] ? xfs_sync_worker+0x69/0x69
[ 3581.190606]  [<ffffffff812f780b>] xfs_inode_ag_iterator+0x47/0x8f
[ 3581.190982]  [<ffffffff811611f5>] ? __sync_filesystem+0x7a/0x7a
[ 3581.191352]  [<ffffffff812f7877>] xfs_sync_data+0x24/0x43
[ 3581.191703]  [<ffffffff812f7911>] xfs_quiesce_data+0x2c/0x88
[ 3581.192065]  [<ffffffff812f5556>] xfs_fs_sync_fs+0x21/0x48
[ 3581.192419]  [<ffffffff811611e1>] __sync_filesystem+0x66/0x7a
[ 3581.192783]  [<ffffffff8116120b>] sync_one_sb+0x16/0x18
[ 3581.193128]  [<ffffffff8113e3e3>] iterate_supers+0x72/0xce
[ 3581.193482]  [<ffffffff81161140>] sync_filesystems+0x20/0x22
[ 3581.193842]  [<ffffffff8116127e>] sys_sync+0x21/0x33
[ 3581.194177]  [<ffffffff819016c2>] system_call_fastpath+0x16/0x1b

fs/fs-writeback.c +3 -0

@@ -419,6 +419,15 @@ writeback_single_inode(struct inode *ino
 	spin_lock(&inode->i_lock);
 	inode->i_state &= ~I_SYNC;
 	if (!(inode->i_state & I_FREEING)) {
+		/*
+		 * Sync livelock prevention. Each inode is tagged and synced in
+		 * one shot, so we can unconditionally update its dirty time to

fs/fs-writeback.c writeback_single_inode(struct inode *ino · line 430

Quoted 6 lines

fs/fs-writeback.c +6 -0

+		 * prevent syncing it again. Note that time ordering of b_dirty
+		 * list will be kept because the following code either removes
+		 * the inode from b_dirty or calls redirty_tail().
+		 */
+		if (wbc->sync_mode == WB_SYNC_ALL || wbc->tagged_sync)
+			inode->dirtied_when = jiffies;

Shouldn't this update only ocur if the inode is still dirty?

Quoted 7 lines

Quoted 7 lines

fs/fs-writeback.c +3 -0

@@ -419,6 +419,15 @@ writeback_single_inode(struct inode *ino
 	spin_lock(&inode->i_lock);
 	inode->i_state &= ~I_SYNC;
 	if (!(inode->i_state & I_FREEING)) {
+		/*
+		 * Sync livelock prevention. Each inode is tagged and synced in
+		 * one shot, so we can unconditionally update its dirty time to

Quoted 8 lines

fs/fs-writeback.c writeback_single_inode(struct inode *ino · line 430

Quoted 6 lines

fs/fs-writeback.c +6 -0

+		 * prevent syncing it again. Note that time ordering of b_dirty
+		 * list will be kept because the following code either removes
+		 * the inode from b_dirty or calls redirty_tail().
+		 */
+		if (wbc->sync_mode == WB_SYNC_ALL || wbc->tagged_sync)
+			inode->dirtied_when = jiffies;

Shouldn't this update only ocur if the inode is still dirty?

Yeah, that would be better even though the current form won't lead to errors.

Let's add one more test (inode->i_state & I_DIRTY)?
(I was actually aware of the trade offs and didn't bother to add it..)

Quoted 19 lines

Quoted 7 lines

Quoted 7 lines

fs/fs-writeback.c +3 -0

@@ -419,6 +419,15 @@ writeback_single_inode(struct inode *ino
 	spin_lock(&inode->i_lock);
 	inode->i_state &= ~I_SYNC;
 	if (!(inode->i_state & I_FREEING)) {
+		/*
+		 * Sync livelock prevention. Each inode is tagged and synced in
+		 * one shot, so we can unconditionally update its dirty time to

Quoted 8 lines

fs/fs-writeback.c writeback_single_inode(struct inode *ino · line 430

Quoted 6 lines

fs/fs-writeback.c +6 -0

+		 * prevent syncing it again. Note that time ordering of b_dirty
+		 * list will be kept because the following code either removes
+		 * the inode from b_dirty or calls redirty_tail().
+		 */
+		if (wbc->sync_mode == WB_SYNC_ALL || wbc->tagged_sync)
+			inode->dirtied_when = jiffies;

Shouldn't this update only ocur if the inode is still dirty?

Yeah, that would be better even though the current form won't lead to errors.

Quoted 2 lines

Let's add one more test (inode->i_state & I_DIRTY)?
(I was actually aware of the trade offs and didn't bother to add it..)

Oops, the (inode->i_state & I_DIRTY) test is not enough because when the inode is actively dirtied, I_DIRTY_PAGES won't be set when the flusher is writing out the pages with I_SYNC set...

Well it would look clumsy to add another mapping_tagged(mapping, PAGECACHE_TAG_DIRTY) test. So I tend to revert to the original scheme of updating ->dirtied_when only on newly dirtied pages. It's the simplest form that can avoid unnecessarily polluting ->dirtied_when.

(quoted diff) +9 -0

@@ -432,6 +432,15 @@ writeback_single_inode(struct inode *ino
 				requeue_io(inode);
 			} else {
 				/*
+				 * Sync livelock prevention. Each inode is
+				 * tagged and synced in one shot, so we can
+				 * unconditionally update its dirty time to
+				 * prevent syncing it again.
+				 */
+				if (wbc->sync_mode == WB_SYNC_ALL ||
+				    wbc->tagged_writepages)
+					inode->dirtied_when = jiffies;
+				/*
 				 * Writeback blocked by something other than
 				 * congestion. Delay the inode for some time to
 				 * avoid spinning on the CPU (100% iowait)

Thanks,
Fengguang

Quoted 19 lines · Wu Fengguang

Quoted 19 lines

Quoted 7 lines

Quoted 7 lines

(quoted diff) +3 -0

@@ -419,6 +419,15 @@ writeback_single_inode(struct inode *ino
 	spin_lock(&inode->i_lock);
 	inode->i_state &= ~I_SYNC;
 	if (!(inode->i_state & I_FREEING)) {
+		/*
+		 * Sync livelock prevention. Each inode is tagged and synced in
+		 * one shot, so we can unconditionally update its dirty time to

Quoted 8 lines

Review writeback_single_inode(struct inode *ino · line 430

Quoted 6 lines

(quoted diff) +6 -0

+		 * prevent syncing it again. Note that time ordering of b_dirty
+		 * list will be kept because the following code either removes
+		 * the inode from b_dirty or calls redirty_tail().
+		 */
+		if (wbc->sync_mode == WB_SYNC_ALL || wbc->tagged_sync)
+			inode->dirtied_when = jiffies;

Shouldn't this update only ocur if the inode is still dirty?

Yeah, that would be better even though the current form won't lead to errors.

Quoted 6 lines

Quoted 2 lines

Let's add one more test (inode->i_state & I_DIRTY)?
(I was actually aware of the trade offs and didn't bother to add it..)

Oops, the (inode->i_state & I_DIRTY) test is not enough because when the inode is actively dirtied, I_DIRTY_PAGES won't be set when the flusher is writing out the pages with I_SYNC set...

Well, rather on contrary I_DIRTY_PAGES won't be set when noone dirtied
any page after we cleared I_DIRTY_PAGES.

Quoted 4 lines

Well it would look clumsy to add another mapping_tagged(mapping, PAGECACHE_TAG_DIRTY) test. So I tend to revert to the original scheme of updating ->dirtied_when only on newly dirtied pages. It's the simplest form that can avoid unnecessarily polluting ->dirtied_when.

Hmm, but won't now something like:
while true; do touch f; done
livelock sync? We always manage to write something - 1 inode - and the
inode will never be clean (before the IO completes, the other process manages to dirty the inode again with very high probability).

The flusher works on dirty inodes in batches, and may quit prematurely if the batch of inodes happen to be metadata-only dirtied: in this case wbc->nr_to_write won't be decreased at all, which stands for "no pages written" but also mis-interpreted as "no progress".

So introduce writeback_control.inodes_cleaned to count the inodes get cleaned. A non-zero value means there are some progress on writeback, in which case more writeback can be tried.

Quoted 8 lines · Wu Fengguang

The flusher works on dirty inodes in batches, and may quit prematurely if the batch of inodes happen to be metadata-only dirtied: in this case wbc->nr_to_write won't be decreased at all, which stands for "no pages written" but also mis-interpreted as "no progress".

So introduce writeback_control.inodes_cleaned to count the inodes get cleaned. A non-zero value means there are some progress on writeback, in which case more writeback can be tried.

Why introduce a new field for this?

Just decrement nr_to_write for every write_inode() call made in writeback_single_inode()....

Quoted 11 lines · Dave Chinner

Quoted 8 lines · Wu Fengguang

The flusher works on dirty inodes in batches, and may quit prematurely if the batch of inodes happen to be metadata-only dirtied: in this case wbc->nr_to_write won't be decreased at all, which stands for "no pages written" but also mis-interpreted as "no progress".

So introduce writeback_control.inodes_cleaned to count the inodes get cleaned. A non-zero value means there are some progress on writeback, in which case more writeback can be tried.

Why introduce a new field for this?

Yeah sorry, but this is an intermediate field that will be removed in patch 14.

Quoted 2 lines

Just decrement nr_to_write for every write_inode() call made in writeback_single_inode()....

There are two problems

- nr_to_write has always been "# of pages written" and writeback_sb_inodes() is actually making use of it to do page accounting in work->nr_pages.

- write_inode() does not always succeed, and its return value is not reliable on every filesystem.. (I actually tried this approach in v1 and found sync(1) hang on NFS)

Quoted 2 lines

- nr_to_write has always been "# of pages written" and writeback_sb_inodes() is actually making use of it to do page accounting in work->nr_pages.

Do we really care whether it's inodes or pages that are written? As far as i can tell it doesn't, because writing inodes generally requires more IO and so needs to be limited anyway.

So put the accounting in the post-write code in writeback_single_inode() where we already check if the inode is still dirty or not. Splitting per-inode post-write processing between writeback_single_inode and the higher level code is cludgy - I'd much prefer it done in only one place.

Quoted 6 lines · Dave Chinner

Quoted 2 lines

- nr_to_write has always been "# of pages written" and writeback_sb_inodes() is actually making use of it to do page accounting in work->nr_pages.

Do we really care whether it's inodes or pages that are written? As far as i can tell it doesn't, because writing inodes generally requires more IO and so needs to be limited anyway.

We do care, but the current infrastructure already is bad enough to not make it work. E.g. when calling from balance_dirty_pages we couldn't care less if the inode is written back, we just want pages on stable storage, similar for wakeups from the VM code. Sooner or later there's no way around splitting page and inode writeback completely.

Quoted 5 lines

So put the accounting in the post-write code in writeback_single_inode() where we already check if the inode is still dirty or not. Splitting per-inode post-write processing between writeback_single_inode and the higher level code is cludgy - I'd much prefer it done in only one place.

I'd tend to agree. Especially as cleaner separation was the main goal for getting rid of the writeback_control overload in the beginning.

A background flush work may run for ever. So it's reasonable for it to mimic the kupdate behavior of syncing old/expired inodes first.

At each queue_io() time, first try enqueuing only newly expired inodes. If there are zero expired inodes to work with, then relax the rule and enqueue all dirty inodes.

It at least makes sense from the data integrity point of view.

This may also reduce the number of dirty pages encountered by page reclaim, eg. the pageout() calls. Normally older inodes contain older dirty pages, which are more close to the end of the LRU lists. So syncing older inodes first helps reducing the dirty pages reached by the page reclaim code.

More background: as Mel put it, "it makes sense to write old pages first to reduce the chances page reclaim is initiating IO."

Rik also presented the situation with a graph:

LRU head [*] dirty page
[ * * * * * * * * * * *]

Ideally, most dirty pages should lie close to the LRU tail instead of LRU head. That requires the flusher thread to sync old/expired inodes first (as there are obvious correlations between inode age and page age), and to give fair opportunities to newly expired inodes rather than sticking with some large eldest inodes (as larger inodes have weaker correlations in the inode<=>page ages).

This patch helps the flusher to meet both the above requirements.

Side effects: it might reduce the batch size and hence reduce inode_wb_list_lock hold time, but in turn make the cluster-by-partition logic in the same function less effective on reducing disk seeks.

v2: keep policy changes inside wb_writeback() and keep the wbc.older_than_this visibility as suggested by Dave.

CC Dave Chinner <[email protected]>

Acked-by Jan Kara <[email protected]>

Acked-by Rik van Riel<[email protected]>

Acked-by Mel Gorman <[email protected]>

Signed-off-by Wu Fengguang <[email protected]>

---
 fs/fs-writeback.c |   16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

fs/fs-writeback.c +8 -1

@@ -718,7 +718,7 @@ static long wb_writeback(struct bdi_writ
 		if (work->for_background && !over_bground_thresh())
 			break;
 
-		if (work->for_kupdate) {
+		if (work->for_kupdate || work->for_background) {
 			oldest_jif = jiffies -
 				msecs_to_jiffies(dirty_expire_interval * 10);
 			wbc.older_than_this = &oldest_jif;

@@ -729,6 +729,7 @@ static long wb_writeback(struct bdi_writ
 		wbc.pages_skipped = 0;
 		wbc.inodes_cleaned = 0;
 
+retry:
 		trace_wbc_writeback_start(&wbc, wb->bdi);
 		if (work->sb)
 			__writeback_inodes_sb(work->sb, wb, &wbc);

@@ -752,6 +753,19 @@ static long wb_writeback(struct bdi_writ
 		if (wbc.inodes_cleaned)
 			continue;
 		/*
+		 * background writeback will start with expired inodes, and
+		 * if none is found, fallback to all inodes. This order helps
+		 * reduce the number of dirty pages reaching the end of LRU
+		 * lists and cause trouble to the page reclaim.
+		 */
+		if (work->for_background &&

Quoted 85 lines · Wu Fengguang

A background flush work may run for ever. So it's reasonable for it to mimic the kupdate behavior of syncing old/expired inodes first.

At each queue_io() time, first try enqueuing only newly expired inodes. If there are zero expired inodes to work with, then relax the rule and enqueue all dirty inodes.

It at least makes sense from the data integrity point of view.

This may also reduce the number of dirty pages encountered by page reclaim, eg. the pageout() calls. Normally older inodes contain older dirty pages, which are more close to the end of the LRU lists. So syncing older inodes first helps reducing the dirty pages reached by the page reclaim code.

More background: as Mel put it, "it makes sense to write old pages first to reduce the chances page reclaim is initiating IO."

Rik also presented the situation with a graph:

LRU head [*] dirty page
[ * * * * * * * * * * *]

Ideally, most dirty pages should lie close to the LRU tail instead of LRU head. That requires the flusher thread to sync old/expired inodes first (as there are obvious correlations between inode age and page age), and to give fair opportunities to newly expired inodes rather than sticking with some large eldest inodes (as larger inodes have weaker correlations in the inode<=>page ages).

This patch helps the flusher to meet both the above requirements.

Side effects: it might reduce the batch size and hence reduce inode_wb_list_lock hold time, but in turn make the cluster-by-partition logic in the same function less effective on reducing disk seeks.

v2: keep policy changes inside wb_writeback() and keep the wbc.older_than_this visibility as suggested by Dave.

CC Dave Chinner <[email protected]>

Acked-by Jan Kara <[email protected]>

Acked-by Rik van Riel<[email protected]>

Acked-by Mel Gorman <[email protected]>

Signed-off-by Wu Fengguang <[email protected]>

---
 fs/fs-writeback.c |   16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

fs/fs-writeback.c +15 -1

@@ -718,7 +718,7 @@ static long wb_writeback(struct bdi_writ
 		if (work->for_background && !over_bground_thresh())
 			break;
 
-		if (work->for_kupdate) {
+		if (work->for_kupdate || work->for_background) {
 			oldest_jif = jiffies -
 				msecs_to_jiffies(dirty_expire_interval * 10);
 			wbc.older_than_this = &oldest_jif;

@@ -729,6 +729,7 @@ static long wb_writeback(struct bdi_writ
 		wbc.pages_skipped = 0;
 		wbc.inodes_cleaned = 0;
 
+retry:
 		trace_wbc_writeback_start(&wbc, wb->bdi);
 		if (work->sb)
 			__writeback_inodes_sb(work->sb, wb, &wbc);

@@ -752,6 +753,19 @@ static long wb_writeback(struct bdi_writ
 		if (wbc.inodes_cleaned)
 			continue;
 		/*
+		 * background writeback will start with expired inodes, and
+		 * if none is found, fallback to all inodes. This order helps
+		 * reduce the number of dirty pages reaching the end of LRU
+		 * lists and cause trouble to the page reclaim.
+		 */
+		if (work->for_background &&
+		    wbc.older_than_this &&
+		    list_empty(&wb->b_io) &&
+		    list_empty(&wb->b_more_io)) {
+			wbc.older_than_this = NULL;
+			goto retry;
+		}
+		/*
 		 * No more inodes for IO, bail

fs/fs-writeback.c static long wb_writeback(struct bdi_writ · line 771

Quoted 2 lines

fs/fs-writeback.c +0 -0

 		 */
 		if (!wbc.more_io)

I have to say that I dislike this implicit nested looping structure using a goto. It would seem better to me to make it explicit that we can do multiple writeback calls by using a do/while loop here and moving the logic of setting/resetting wbc.older_than_this to one place inside the nested loop...

Isn't this a fix that should go to Linus ASAP independent of the rest of this series to fix tmpfs?

When wbc.more_io was first introduced, it indicates whether there are at least one superblock whose s_more_io contains more IO work. Now with the per-bdi writeback, it can be replaced with a simple b_more_io test.

Acked-by Jan Kara <[email protected]>

Acked-by Mel Gorman <[email protected]>

CC Minchan Kim <[email protected]>

Signed-off-by Wu Fengguang <[email protected]>

When wbc.more_io was first introduced, it indicates whether there are at least one superblock whose s_more_io contains more IO work. Now with the per-bdi writeback, it can be replaced with a simple b_more_io test.

Acked-by Jan Kara <[email protected]>

Acked-by Mel Gorman <[email protected]>

CC Minchan Kim <[email protected]>

Signed-off-by Wu Fengguang <[email protected]>

---
 fs/fs-writeback.c                |    9 ++-------
 include/linux/writeback.h        |    1 -
 include/trace/events/ext4.h      |    6 ++----
 include/trace/events/writeback.h |    5 +----
 4 files changed, 5 insertions(+), 16 deletions(-)

fs/fs-writeback.c +1 -6

@@ -560,12 +560,8 @@ static int writeback_sb_inodes(struct su
 		iput(inode);
 		cond_resched();
 		spin_lock(&wb->list_lock);
-		if (wbc->nr_to_write <= 0) {
-			wbc->more_io = 1;
+		if (wbc->nr_to_write <= 0)
 			return 1;
-		}
-		if (!list_empty(&wb->b_more_io))
-			wbc->more_io = 1;
 	}
 	/* b_io is empty */
 	return 1;

@@ -707,7 +703,6 @@ static long wb_writeback(struct bdi_writ
 			wbc.older_than_this = &oldest_jif;
 		}
 
-		wbc.more_io = 0;
 		wbc.nr_to_write = write_chunk;
 		wbc.pages_skipped = 0;
 		wbc.inodes_cleaned = 0;

Quoted 45 lines · Wu Fengguang

When wbc.more_io was first introduced, it indicates whether there are at least one superblock whose s_more_io contains more IO work. Now with the per-bdi writeback, it can be replaced with a simple b_more_io test.

Acked-by Jan Kara <[email protected]>

Acked-by Mel Gorman <[email protected]>

CC Minchan Kim <[email protected]>

Signed-off-by Wu Fengguang <[email protected]>

---
 fs/fs-writeback.c                |    9 ++-------
 include/linux/writeback.h        |    1 -
 include/trace/events/ext4.h      |    6 ++----
 include/trace/events/writeback.h |    5 +----
 4 files changed, 5 insertions(+), 16 deletions(-)

fs/fs-writeback.c +2 -7

@@ -560,12 +560,8 @@ static int writeback_sb_inodes(struct su
 		iput(inode);
 		cond_resched();
 		spin_lock(&wb->list_lock);
-		if (wbc->nr_to_write <= 0) {
-			wbc->more_io = 1;
+		if (wbc->nr_to_write <= 0)
 			return 1;
-		}
-		if (!list_empty(&wb->b_more_io))
-			wbc->more_io = 1;
 	}
 	/* b_io is empty */
 	return 1;

@@ -707,7 +703,6 @@ static long wb_writeback(struct bdi_writ
 			wbc.older_than_this = &oldest_jif;
 		}
 
-		wbc.more_io = 0;
 		wbc.nr_to_write = write_chunk;
 		wbc.pages_skipped = 0;
 		wbc.inodes_cleaned = 0;

@@ -755,7 +750,7 @@ retry:
 		/*
 		 * No more inodes for IO, bail
 		 */
-		if (!wbc.more_io)
+		if (list_empty(&wb->b_more_io))

fs/fs-writeback.c retry: · line 754

Quoted 1 line

fs/fs-writeback.c +0 -0

 			break;

We're not holding the wb->list_lock here, so we need to be careful here. I think this is safe given that there shuold only be one flusher thread operating on the list, but when we expand to multiple flusher threads per-bdi, this coul dbe a nasty landmine. A comment is probably in order explaining why this is safe to check unlocked right now...

Quoted 46 lines · Dave Chinner

Quoted 45 lines · Wu Fengguang

When wbc.more_io was first introduced, it indicates whether there are at least one superblock whose s_more_io contains more IO work. Now with the per-bdi writeback, it can be replaced with a simple b_more_io test.

Acked-by Jan Kara <[email protected]>

Acked-by Mel Gorman <[email protected]>

CC Minchan Kim <[email protected]>

Signed-off-by Wu Fengguang <[email protected]>

---
 fs/fs-writeback.c                |    9 ++-------
 include/linux/writeback.h        |    1 -
 include/trace/events/ext4.h      |    6 ++----
 include/trace/events/writeback.h |    5 +----
 4 files changed, 5 insertions(+), 16 deletions(-)

fs/fs-writeback.c +2 -7

@@ -560,12 +560,8 @@ static int writeback_sb_inodes(struct su
 		iput(inode);
 		cond_resched();
 		spin_lock(&wb->list_lock);
-		if (wbc->nr_to_write <= 0) {
-			wbc->more_io = 1;
+		if (wbc->nr_to_write <= 0)
 			return 1;
-		}
-		if (!list_empty(&wb->b_more_io))
-			wbc->more_io = 1;
 	}
 	/* b_io is empty */
 	return 1;

@@ -707,7 +703,6 @@ static long wb_writeback(struct bdi_writ
 			wbc.older_than_this = &oldest_jif;
 		}
 
-		wbc.more_io = 0;
 		wbc.nr_to_write = write_chunk;
 		wbc.pages_skipped = 0;
 		wbc.inodes_cleaned = 0;

@@ -755,7 +750,7 @@ retry:
 		/*
 		 * No more inodes for IO, bail
 		 */
-		if (!wbc.more_io)
+		if (list_empty(&wb->b_more_io))

Quoted 8 lines

fs/fs-writeback.c retry: · line 754

Quoted 1 line

fs/fs-writeback.c +0 -0

 			break;

We're not holding the wb->list_lock here, so we need to be careful here. I think this is safe given that there shuold only be one flusher thread operating on the list, but when we expand to multiple flusher threads per-bdi, this coul dbe a nasty landmine. A comment is probably in order explaining why this is safe to check unlocked right now...

OK, how about this?

                /*
                 * No more inodes for IO, bail. The unlocked check is safe
                 * because each &wb will be worked by only one flusher thread.
                 */
                if (list_empty(&wb->b_more_io))
                        break;

I guess in future multiple flusher threads will be working on different bdi_writeback instances, so it will still be safe.

However for now there are possible interactions from the IO-full balance_dirty_pages(). So it looks better to just do the tests inside the lock:

Pass struct wb_writeback_work all the way down to writeback_sb_inodes(), and initialize the struct writeback_control there.

struct writeback_control is basically designed to control writeback of a single file, but we keep abuse it for writing multiple files in writeback_sb_inodes() and its callers.

It immediately clean things up, e.g. suddenly wbc.nr_to_write vs work->nr_pages starts to make sense, and instead of saving and restoring pages_skipped in writeback_sb_inodes it can always start with a clean zero value.

It also makes a neat IO pattern change: large dirty files are now written in the full 4MB writeback chunk size, rather than whatever remained quota in wbc->nr_to_write.

Proposed-by Christoph Hellwig <[email protected]>

Signed-off-by Wu Fengguang <[email protected]>

---
 fs/btrfs/extent_io.c             |    2 
 fs/fs-writeback.c                |  192 +++++++++++++++--------------
 include/linux/writeback.h        |    6 
 include/trace/events/writeback.h |   39 ++++-
 mm/backing-dev.c                 |    9 -
 mm/page-writeback.c              |   17 --
 6 files changed, 140 insertions(+), 125 deletions(-)

change set:
- move writeback_control from wb_writeback() down to writeback_sb_inodes()
- change return value of writeback_sb_inodes()/__writeback_inodes_wb() to the number of pages and/or inodes written
- move writeback_control.older_than_this to struct wb_writeback_work
- remove writeback_control.inodes_cleaned
- remove wbc_writeback_* trace events for now

fs/fs-writeback.c +105 -87

@@ -30,11 +30,21 @@
 #include "internal.h"
 
 /*
+ * The maximum number of pages to writeout in a single bdi flush/kupdate
+ * operation.  We do this so we don't hold I_SYNC against an inode for
+ * enormous amounts of time, which would block a userspace task which has
+ * been forced to throttle against that inode.  Also, the code reevaluates
+ * the dirty each time it has written this many pages.
+ */
+#define MAX_WRITEBACK_PAGES     1024L
+
+/*
  * Passed into wb_writeback(), essentially a subset of writeback_control
  */
 struct wb_writeback_work {
 	long nr_pages;
 	struct super_block *sb;
+	unsigned long *older_than_this;
 	enum writeback_sync_modes sync_mode;
 	unsigned int tagged_sync:1;
 	unsigned int for_kupdate:1;

@@ -463,7 +473,6 @@ writeback_single_inode(struct inode *ino
 			 * No need to add it back to the LRU.
 			 */
 			list_del_init(&inode->i_wb_list);
-			wbc->inodes_cleaned++;
 		}
 	}
 	inode_sync_complete(inode);

@@ -496,6 +505,31 @@ static bool pin_sb_for_writeback(struct 
 	return false;
 }
 
+static long writeback_chunk_size(struct wb_writeback_work *work)
+{
+	long pages;
+
+	/*
+	 * WB_SYNC_ALL mode does livelock avoidance by syncing dirty
+	 * inodes/pages in one big loop. Setting wbc.nr_to_write=LONG_MAX
+	 * here avoids calling into writeback_inodes_wb() more than once.
+	 *
+	 * The intended call sequence for WB_SYNC_ALL writeback is:
+	 *
+	 *      wb_writeback()
+	 *          writeback_sb_inodes()       <== called only once
+	 *              write_cache_pages()     <== called once for each inode
+	 *                   (quickly) tag currently dirty pages
+	 *                   (maybe slowly) sync all tagged pages
+	 */
+	if (work->sync_mode == WB_SYNC_ALL || work->tagged_sync)
+		pages = LONG_MAX;
+	else
+		pages = min(MAX_WRITEBACK_PAGES, work->nr_pages);
+
+	return pages;
+}
+
 /*
  * Write a portion of b_io inodes which belong to @sb.
  *

@@ -503,18 +537,29 @@ static bool pin_sb_for_writeback(struct 
  * inodes. Otherwise write only ones which go sequentially
  * in reverse order.
  *
- * Return 1, if the caller writeback routine should be
- * interrupted. Otherwise return 0.
+ * Return the number of pages and/or inodes cleaned.
  */
-static int writeback_sb_inodes(struct super_block *sb, struct bdi_writeback *wb,
-		struct writeback_control *wbc, bool only_this_sb)
+static long writeback_sb_inodes(struct super_block *sb,
+				struct bdi_writeback *wb,
+				struct wb_writeback_work *work)
 {
+	struct writeback_control wbc = {
+		.sync_mode		= work->sync_mode,
+		.tagged_sync		= work->tagged_sync,
+		.for_kupdate		= work->for_kupdate,
+		.for_background		= work->for_background,
+		.range_cyclic		= work->range_cyclic,
+		.range_start		= 0,
+		.range_end		= LLONG_MAX,
+	};
+	long write_chunk;
+	long wrote = 0;  /* count both pages and inodes */
+
 	while (!list_empty(&wb->b_io)) {
-		long pages_skipped;
 		struct inode *inode = wb_inode(wb->b_io.prev);
 
 		if (inode->i_sb != sb) {
-			if (only_this_sb) {
+			if (work->sb) {
 				/*
 				 * We only want to write back data for this
 				 * superblock, move all inodes not belonging

@@ -529,7 +574,7 @@ static int writeback_sb_inodes(struct su
 			 * Bounce back to the caller to unpin this and
 			 * pin the next superblock.
 			 */
-			return 0;
+			break;
 		}
 
 		/*

@@ -543,34 +588,44 @@ static int writeback_sb_inodes(struct su
 			requeue_io(inode, wb);
 			continue;
 		}
-
 		__iget(inode);
+		write_chunk = writeback_chunk_size(work);
+		wbc.nr_to_write = write_chunk;
+		wbc.pages_skipped = 0;
+
+		writeback_single_inode(inode, wb, &wbc);
 
-		pages_skipped = wbc->pages_skipped;
-		writeback_single_inode(inode, wb, wbc);
-		if (wbc->pages_skipped != pages_skipped) {
+		work->nr_pages -= write_chunk - wbc.nr_to_write;
+		wrote += write_chunk - wbc.nr_to_write;
+		if (wbc.pages_skipped) {
 			/*
 			 * writeback is not making progress due to locked
 			 * buffers.  Skip this inode for now.
 			 */
 			redirty_tail(inode, wb);
-		}
+		} else if (!(inode->i_state & I_DIRTY))
+			wrote++;
 		spin_unlock(&inode->i_lock);
 		spin_unlock(&wb->list_lock);
 		iput(inode);
 		cond_resched();
 		spin_lock(&wb->list_lock);
-		if (wbc->nr_to_write <= 0)
-			return 1;
+		/*
+		 * bail out to wb_writeback() often enough to check
+		 * background threshold and other termination conditions.
+		 */
+		if (wrote >= MAX_WRITEBACK_PAGES)
+			break;
+		if (work->nr_pages <= 0)
+			break;
 	}
-	/* b_io is empty */
-	return 1;
+	return wrote;
 }
 
-static void __writeback_inodes_wb(struct bdi_writeback *wb,
-				  struct writeback_control *wbc)
+static long __writeback_inodes_wb(struct bdi_writeback *wb,
+				  struct wb_writeback_work *work)
 {
-	int ret = 0;
+	long wrote = 0;
 
 	while (!list_empty(&wb->b_io)) {
 		struct inode *inode = wb_inode(wb->b_io.prev);

@@ -580,33 +635,34 @@ static void __writeback_inodes_wb(struct
 			requeue_io(inode, wb);
 			continue;
 		}
-		ret = writeback_sb_inodes(sb, wb, wbc, false);
+		wrote += writeback_sb_inodes(sb, wb, work);
 		drop_super(sb);
 
-		if (ret)
+		if (wrote >= MAX_WRITEBACK_PAGES)
+			break;
+		if (work->nr_pages <= 0)
 			break;
 	}
 	/* Leave any unwritten inodes on b_io */
+	return wrote;
 }
 
-void writeback_inodes_wb(struct bdi_writeback *wb,
-		struct writeback_control *wbc)
+long writeback_inodes_wb(struct bdi_writeback *wb, long nr_pages)
 {
+	struct wb_writeback_work work = {
+		.nr_pages	= nr_pages,
+		.sync_mode	= WB_SYNC_NONE,
+		.range_cyclic	= 1,
+	};
+
 	spin_lock(&wb->list_lock);
 	if (list_empty(&wb->b_io))
-		queue_io(wb, wbc->older_than_this);
-	__writeback_inodes_wb(wb, wbc);
+		queue_io(wb, NULL);
+	__writeback_inodes_wb(wb, &work);
 	spin_unlock(&wb->list_lock);
-}
 
-/*
- * The maximum number of pages to writeout in a single bdi flush/kupdate
- * operation.  We do this so we don't hold I_SYNC against an inode for
- * enormous amounts of time, which would block a userspace task which has
- * been forced to throttle against that inode.  Also, the code reevaluates
- * the dirty each time it has written this many pages.
- */
-#define MAX_WRITEBACK_PAGES     1024
+	return nr_pages - work.nr_pages;
+}
 
 static inline bool over_bground_thresh(void)
 {

@@ -636,42 +692,13 @@ static inline bool over_bground_thresh(v
 static long wb_writeback(struct bdi_writeback *wb,
 			 struct wb_writeback_work *work)
 {
-	struct writeback_control wbc = {
-		.sync_mode		= work->sync_mode,
-		.tagged_sync		= work->tagged_sync,
-		.older_than_this	= NULL,
-		.for_kupdate		= work->for_kupdate,
-		.for_background		= work->for_background,
-		.range_cyclic		= work->range_cyclic,
-	};
+	long nr_pages = work->nr_pages;
 	unsigned long oldest_jif;
-	long wrote = 0;
-	long write_chunk = MAX_WRITEBACK_PAGES;
 	struct inode *inode;
-
-	if (!wbc.range_cyclic) {
-		wbc.range_start = 0;
-		wbc.range_end = LLONG_MAX;
-	}
-
-	/*
-	 * WB_SYNC_ALL mode does livelock avoidance by syncing dirty
-	 * inodes/pages in one big loop. Setting wbc.nr_to_write=LONG_MAX
-	 * here avoids calling into writeback_inodes_wb() more than once.
-	 *
-	 * The intended call sequence for WB_SYNC_ALL writeback is:
-	 *
-	 *      wb_writeback()
-	 *          writeback_sb_inodes()       <== called only once
-	 *              write_cache_pages()     <== called once for each inode
-	 *                   (quickly) tag currently dirty pages
-	 *                   (maybe slowly) sync all tagged pages
-	 */
-	if (wbc.sync_mode == WB_SYNC_ALL || wbc.tagged_sync)
-		write_chunk = LONG_MAX;
+	long progress;
 
 	oldest_jif = jiffies;
-	wbc.older_than_this = &oldest_jif;
+	work->older_than_this = &oldest_jif;
 
 	for (;;) {
 		/*

@@ -700,27 +727,20 @@ static long wb_writeback(struct bdi_writ
 		if (work->for_kupdate || work->for_background) {
 			oldest_jif = jiffies -
 				msecs_to_jiffies(dirty_expire_interval * 10);
-			wbc.older_than_this = &oldest_jif;
+			work->older_than_this = &oldest_jif;
 		}
 
-		wbc.nr_to_write = write_chunk;
-		wbc.pages_skipped = 0;
-		wbc.inodes_cleaned = 0;
-
 retry:
-		trace_wbc_writeback_start(&wbc, wb->bdi);
+		trace_writeback_start(wb->bdi, work);
 		spin_lock(&wb->list_lock);
 		if (list_empty(&wb->b_io))
-			queue_io(wb, wbc.older_than_this);
+			queue_io(wb, work->older_than_this);
 		if (work->sb)
-			writeback_sb_inodes(work->sb, wb, &wbc, true);
+			progress = writeback_sb_inodes(work->sb, wb, work);
 		else
-			__writeback_inodes_wb(wb, &wbc);
+			progress = __writeback_inodes_wb(wb, work);
 		spin_unlock(&wb->list_lock);
-		trace_wbc_writeback_written(&wbc, wb->bdi);
-
-		work->nr_pages -= write_chunk - wbc.nr_to_write;
-		wrote += write_chunk - wbc.nr_to_write;
+		trace_writeback_written(wb->bdi, work);
 
 		/*
 		 * Did we write something? Try for more

@@ -730,9 +750,7 @@ retry:
 		 * mean the overall work is done. So we keep looping as long
 		 * as made some progress on cleaning pages or inodes.
 		 */
-		if (wbc.nr_to_write < write_chunk)
-			continue;
-		if (wbc.inodes_cleaned)
+		if (progress)
 			continue;
 		/*
 		 * background writeback will start with expired inodes, and

@@ -741,10 +759,10 @@ retry:
 		 * lists and cause trouble to the page reclaim.
 		 */
 		if (work->for_background &&
-		    wbc.older_than_this &&
+		    work->older_than_this &&
 		    list_empty(&wb->b_io) &&
 		    list_empty(&wb->b_more_io)) {
-			wbc.older_than_this = NULL;
+			work->older_than_this = NULL;
 			goto retry;
 		}
 		/*

@@ -759,8 +777,8 @@ retry:
 		 */
 		spin_lock(&wb->list_lock);
 		if (!list_empty(&wb->b_more_io))  {
+			trace_writeback_wait(wb->bdi, work);
 			inode = wb_inode(wb->b_more_io.prev);
-			trace_wbc_writeback_wait(&wbc, wb->bdi);
 			spin_lock(&inode->i_lock);
 			inode_wait_for_writeback(inode, wb);
 			spin_unlock(&inode->i_lock);

@@ -768,7 +786,7 @@ retry:
 		spin_unlock(&wb->list_lock);
 	}
 
-	return wrote;
+	return nr_pages - work->nr_pages;
 }
 
 /*

include/linux/writeback.h +1 -5

@@ -24,12 +24,9 @@ enum writeback_sync_modes {
  */
 struct writeback_control {
 	enum writeback_sync_modes sync_mode;
-	unsigned long *older_than_this;	/* If !NULL, only write back inodes
-					   older than this */
 	long nr_to_write;		/* Write this many pages, and decrement
 					   this for each page written */
 	long pages_skipped;		/* Pages which were not written */
-	long inodes_cleaned;		/* # of inodes cleaned */
 
 	/*
 	 * For a_ops->writepages(): is start or end are non-zero then this is

@@ -58,8 +55,7 @@ void writeback_inodes_sb_nr(struct super
 int writeback_inodes_sb_if_idle(struct super_block *);
 int writeback_inodes_sb_nr_if_idle(struct super_block *, unsigned long nr);
 void sync_inodes_sb(struct super_block *);
-void writeback_inodes_wb(struct bdi_writeback *wb,
-		struct writeback_control *wbc);
+long writeback_inodes_wb(struct bdi_writeback *wb, long nr_pages);
 long wb_do_writeback(struct bdi_writeback *wb, int force_wait);
 void wakeup_flusher_threads(long nr_pages);
 

mm/backing-dev.c +1 -8

@@ -262,14 +262,7 @@ int bdi_has_dirty_io(struct backing_dev_
 
 static void bdi_flush_io(struct backing_dev_info *bdi)
 {
-	struct writeback_control wbc = {
-		.sync_mode		= WB_SYNC_NONE,
-		.older_than_this	= NULL,
-		.range_cyclic		= 1,
-		.nr_to_write		= 1024,
-	};
-
-	writeback_inodes_wb(&bdi->wb, &wbc);
+	writeback_inodes_wb(&bdi->wb, 1024);
 }
 
 /*

mm/page-writeback.c +5 -12

@@ -494,13 +494,6 @@ static void balance_dirty_pages(struct a
 		return;
 
 	for (;;) {
-		struct writeback_control wbc = {
-			.sync_mode	= WB_SYNC_NONE,
-			.older_than_this = NULL,
-			.nr_to_write	= write_chunk,
-			.range_cyclic	= 1,
-		};
-
 		nr_reclaimable = global_page_state(NR_FILE_DIRTY) +
 					global_page_state(NR_UNSTABLE_NFS);
 		nr_writeback = global_page_state(NR_WRITEBACK);

@@ -562,17 +555,17 @@ static void balance_dirty_pages(struct a
 		 * threshold otherwise wait until the disk writes catch
 		 * up.
 		 */
-		trace_wbc_balance_dirty_start(&wbc, bdi);
+		trace_balance_dirty_start(bdi);
 		if (bdi_nr_reclaimable > bdi_thresh) {
-			writeback_inodes_wb(&bdi->wb, &wbc);
-			pages_written += write_chunk - wbc.nr_to_write;
-			trace_wbc_balance_dirty_written(&wbc, bdi);
+			pages_written += writeback_inodes_wb(&bdi->wb,
+							     write_chunk);
+			trace_balance_dirty_written(bdi, pages_written);
 			if (pages_written >= write_chunk)
 				break;		/* We've done our duty */
 		}
-		trace_wbc_balance_dirty_wait(&wbc, bdi);
 		__set_current_state(TASK_UNINTERRUPTIBLE);
 		io_schedule_timeout(pause);
+		trace_balance_dirty_wait(bdi);
 
 		/*
 		 * Increase the delay for each loop, up to our previous

include/trace/events/writeback.h +28 -11

@@ -49,6 +49,9 @@ DEFINE_EVENT(writeback_work_class, name,
 DEFINE_WRITEBACK_WORK_EVENT(writeback_nothread);
 DEFINE_WRITEBACK_WORK_EVENT(writeback_queue);
 DEFINE_WRITEBACK_WORK_EVENT(writeback_exec);
+DEFINE_WRITEBACK_WORK_EVENT(writeback_start);
+DEFINE_WRITEBACK_WORK_EVENT(writeback_written);
+DEFINE_WRITEBACK_WORK_EVENT(writeback_wait);
 
 TRACE_EVENT(writeback_pages_written,
 	TP_PROTO(long pages_written),

@@ -88,6 +91,30 @@ DEFINE_WRITEBACK_EVENT(writeback_bdi_reg
 DEFINE_WRITEBACK_EVENT(writeback_bdi_unregister);
 DEFINE_WRITEBACK_EVENT(writeback_thread_start);
 DEFINE_WRITEBACK_EVENT(writeback_thread_stop);
+DEFINE_WRITEBACK_EVENT(balance_dirty_start);
+DEFINE_WRITEBACK_EVENT(balance_dirty_wait);
+
+TRACE_EVENT(balance_dirty_written,
+
+	TP_PROTO(struct backing_dev_info *bdi, int written),
+
+	TP_ARGS(bdi, written),
+
+	TP_STRUCT__entry(
+		__array(char,	name, 32)
+		__field(int,	written)
+	),
+
+	TP_fast_assign(
+		strncpy(__entry->name, dev_name(bdi->dev), 32);
+		__entry->written = written;
+	),
+
+	TP_printk("bdi %s written %d",
+		  __entry->name,
+		  __entry->written
+	)
+);
 
 DECLARE_EVENT_CLASS(wbc_class,
 	TP_PROTO(struct writeback_control *wbc, struct backing_dev_info *bdi),

@@ -101,7 +128,6 @@ DECLARE_EVENT_CLASS(wbc_class,
 		__field(int, for_background)
 		__field(int, for_reclaim)
 		__field(int, range_cyclic)
-		__field(unsigned long, older_than_this)
 		__field(long, range_start)
 		__field(long, range_end)
 	),

@@ -115,14 +141,12 @@ DECLARE_EVENT_CLASS(wbc_class,
 		__entry->for_background	= wbc->for_background;
 		__entry->for_reclaim	= wbc->for_reclaim;
 		__entry->range_cyclic	= wbc->range_cyclic;
-		__entry->older_than_this = wbc->older_than_this ?
-						*wbc->older_than_this : 0;
 		__entry->range_start	= (long)wbc->range_start;
 		__entry->range_end	= (long)wbc->range_end;
 	),
 
 	TP_printk("bdi %s: towrt=%ld skip=%ld mode=%d kupd=%d "
-		"bgrd=%d reclm=%d cyclic=%d older=0x%lx "
+		"bgrd=%d reclm=%d cyclic=%d "
 		"start=0x%lx end=0x%lx",
 		__entry->name,
 		__entry->nr_to_write,

@@ -132,7 +156,6 @@ DECLARE_EVENT_CLASS(wbc_class,
 		__entry->for_background,
 		__entry->for_reclaim,
 		__entry->range_cyclic,
-		__entry->older_than_this,
 		__entry->range_start,
 		__entry->range_end)
 )

@@ -141,12 +164,6 @@ DECLARE_EVENT_CLASS(wbc_class,
 DEFINE_EVENT(wbc_class, name, \
 	TP_PROTO(struct writeback_control *wbc, struct backing_dev_info *bdi), \
 	TP_ARGS(wbc, bdi))
-DEFINE_WBC_EVENT(wbc_writeback_start);
-DEFINE_WBC_EVENT(wbc_writeback_written);
-DEFINE_WBC_EVENT(wbc_writeback_wait);
-DEFINE_WBC_EVENT(wbc_balance_dirty_start);
-DEFINE_WBC_EVENT(wbc_balance_dirty_written);
-DEFINE_WBC_EVENT(wbc_balance_dirty_wait);
 DEFINE_WBC_EVENT(wbc_writepage);
 
 DECLARE_EVENT_CLASS(writeback_congest_waited_template,

fs/btrfs/extent_io.c +0 -2

@@ -2596,7 +2596,6 @@ int extent_write_full_page(struct extent
 	};
 	struct writeback_control wbc_writepages = {
 		.sync_mode	= wbc->sync_mode,
-		.older_than_this = NULL,
 		.nr_to_write	= 64,
 		.range_start	= page_offset(page) + PAGE_CACHE_SIZE,
 		.range_end	= (loff_t)-1,

@@ -2629,7 +2628,6 @@ int extent_write_locked_range(struct ext
 	};
 	struct writeback_control wbc_writepages = {
 		.sync_mode	= mode,
-		.older_than_this = NULL,
 		.nr_to_write	= nr_pages * 2,
 		.range_start	= start,
 		.range_end	= end + 1,

Pass struct wb_writeback_work all the way down to writeback_sb_inodes(), and initialize the struct writeback_control there.

struct writeback_control is basically designed to control writeback of a single file, but we keep abuse it for writing multiple files in writeback_sb_inodes() and its callers.

It immediately clean things up, e.g. suddenly wbc.nr_to_write vs work->nr_pages starts to make sense, and instead of saving and restoring pages_skipped in writeback_sb_inodes it can always start with a clean zero value.

It also makes a neat IO pattern change: large dirty files are now written in the full 4MB writeback chunk size, rather than whatever remained quota in wbc->nr_to_write.

Proposed-by Christoph Hellwig <[email protected]>

Signed-off-by Wu Fengguang <[email protected]>

---

fs/fs-writeback.c +6 -4

@@ -543,34 +588,44 @@ static int writeback_sb_inodes(struct su
 			requeue_io(inode, wb);
 			continue;
 		}
-
 		__iget(inode);
+		write_chunk = writeback_chunk_size(work);
+		wbc.nr_to_write = write_chunk;
+		wbc.pages_skipped = 0;
+
+		writeback_single_inode(inode, wb, &wbc);
 
-		pages_skipped = wbc->pages_skipped;
-		writeback_single_inode(inode, wb, wbc);
-		if (wbc->pages_skipped != pages_skipped) {
+		work->nr_pages -= write_chunk - wbc.nr_to_write;

fs/fs-writeback.c +6 -5

+		if (work->nr_pages <= 0)
+			break;
 	}
-	/* b_io is empty */
-	return 1;
+	return wrote;
 }
 
-static void __writeback_inodes_wb(struct bdi_writeback *wb,
-				  struct writeback_control *wbc)
+static long __writeback_inodes_wb(struct bdi_writeback *wb,
+				  struct wb_writeback_work *work)
 {
-	int ret = 0;
+	long wrote = 0;
 
 	while (!list_empty(&wb->b_io)) {
 		struct inode *inode = wb_inode(wb->b_io.prev);

@@ -580,33 +635,34 @@ static void __writeback_inodes_wb(struct
 			requeue_io(inode, wb);
 			continue;

fs/fs-writeback.c +10 -5

 	}
 	/* Leave any unwritten inodes on b_io */
+	return wrote;
 }
 
-void writeback_inodes_wb(struct bdi_writeback *wb,
-		struct writeback_control *wbc)
+long writeback_inodes_wb(struct bdi_writeback *wb, long nr_pages)
 {
+	struct wb_writeback_work work = {
+		.nr_pages	= nr_pages,
+		.sync_mode	= WB_SYNC_NONE,
+		.range_cyclic	= 1,
+	};
+
 	spin_lock(&wb->list_lock);
 	if (list_empty(&wb->b_io))
-		queue_io(wb, wbc->older_than_this);
-	__writeback_inodes_wb(wb, wbc);
+		queue_io(wb, NULL);
+	__writeback_inodes_wb(wb, &work);
 	spin_unlock(&wb->list_lock);
-}
 

mm/page-writeback.c +1 -4

@@ -562,17 +555,17 @@ static void balance_dirty_pages(struct a
 		 * threshold otherwise wait until the disk writes catch
 		 * up.
 		 */
-		trace_wbc_balance_dirty_start(&wbc, bdi);
+		trace_balance_dirty_start(bdi);
 		if (bdi_nr_reclaimable > bdi_thresh) {
-			writeback_inodes_wb(&bdi->wb, &wbc);
-			pages_written += write_chunk - wbc.nr_to_write;
-			trace_wbc_balance_dirty_written(&wbc, bdi);

Quoted 19 lines · Wu Fengguang

Pass struct wb_writeback_work all the way down to writeback_sb_inodes(), and initialize the struct writeback_control there.

struct writeback_control is basically designed to control writeback of a single file, but we keep abuse it for writing multiple files in writeback_sb_inodes() and its callers.

It immediately clean things up, e.g. suddenly wbc.nr_to_write vs work->nr_pages starts to make sense, and instead of saving and restoring pages_skipped in writeback_sb_inodes it can always start with a clean zero value.

It also makes a neat IO pattern change: large dirty files are now written in the full 4MB writeback chunk size, rather than whatever remained quota in wbc->nr_to_write.

Proposed-by Christoph Hellwig <[email protected]>

Signed-off-by Wu Fengguang <[email protected]>

---

.....

Quoted 21 lines

fs/fs-writeback.c +8 -4

@@ -543,34 +588,44 @@ static int writeback_sb_inodes(struct su
 			requeue_io(inode, wb);
 			continue;
 		}
-
 		__iget(inode);
+		write_chunk = writeback_chunk_size(work);
+		wbc.nr_to_write = write_chunk;
+		wbc.pages_skipped = 0;
+
+		writeback_single_inode(inode, wb, &wbc);
 
-		pages_skipped = wbc->pages_skipped;
-		writeback_single_inode(inode, wb, wbc);
-		if (wbc->pages_skipped != pages_skipped) {
+		work->nr_pages -= write_chunk - wbc.nr_to_write;
+		wrote += write_chunk - wbc.nr_to_write;
+		if (wbc.pages_skipped) {
 			/*
 			 * writeback is not making progress due to locked
 			 * buffers.  Skip this inode for now.

fs/fs-writeback.c static int writeback_sb_inodes(struct su · line 607

Quoted 5 lines

fs/fs-writeback.c +2 -1

 			 */
 			redirty_tail(inode, wb);
-		}
+		} else if (!(inode->i_state & I_DIRTY))
+			wrote++;

Oh, that's just ugly. Do that accounting via nr_to_write in writeback_single_inode() as I suggested earlier, please.

Quoted 11 lines

fs/fs-writeback.c +4 -2

 		spin_unlock(&inode->i_lock);
 		spin_unlock(&wb->list_lock);
 		iput(inode);
 		cond_resched();
 		spin_lock(&wb->list_lock);
-		if (wbc->nr_to_write <= 0)
-			return 1;
+		/*
+		 * bail out to wb_writeback() often enough to check
+		 * background threshold and other termination conditions.
+		 */

Review static int writeback_sb_inodes(struct su · line 618

Quoted 2 lines

(quoted diff) +2 -0

+		if (wrote >= MAX_WRITEBACK_PAGES)
+			break;

Why do this so often? If you are writing large files, it will be once every writeback_single_inode() call that you bail. Seems rather inefficient to me to go back to the top level loop just to check for more work when we already know we have more work to do because there's still inodes on b_io....

Quoted 21 lines

fs/fs-writeback.c +6 -5

+		if (work->nr_pages <= 0)
+			break;
 	}
-	/* b_io is empty */
-	return 1;
+	return wrote;
 }
 
-static void __writeback_inodes_wb(struct bdi_writeback *wb,
-				  struct writeback_control *wbc)
+static long __writeback_inodes_wb(struct bdi_writeback *wb,
+				  struct wb_writeback_work *work)
 {
-	int ret = 0;
+	long wrote = 0;
 
 	while (!list_empty(&wb->b_io)) {
 		struct inode *inode = wb_inode(wb->b_io.prev);

@@ -580,33 +635,34 @@ static void __writeback_inodes_wb(struct
 			requeue_io(inode, wb);
 			continue;

fs/fs-writeback.c static void __writeback_inodes_wb(struct · line 644

Quoted 10 lines

fs/fs-writeback.c +4 -2

 		}
-		ret = writeback_sb_inodes(sb, wb, wbc, false);
+		wrote += writeback_sb_inodes(sb, wb, work);
 		drop_super(sb);
 
-		if (ret)
+		if (wrote >= MAX_WRITEBACK_PAGES)
+			break;
+		if (work->nr_pages <= 0)
 			break;

Same here.

Quoted 24 lines

fs/fs-writeback.c +10 -5

 	}
 	/* Leave any unwritten inodes on b_io */
+	return wrote;
 }
 
-void writeback_inodes_wb(struct bdi_writeback *wb,
-		struct writeback_control *wbc)
+long writeback_inodes_wb(struct bdi_writeback *wb, long nr_pages)
 {
+	struct wb_writeback_work work = {
+		.nr_pages	= nr_pages,
+		.sync_mode	= WB_SYNC_NONE,
+		.range_cyclic	= 1,
+	};
+
 	spin_lock(&wb->list_lock);
 	if (list_empty(&wb->b_io))
-		queue_io(wb, wbc->older_than_this);
-	__writeback_inodes_wb(wb, wbc);
+		queue_io(wb, NULL);
+	__writeback_inodes_wb(wb, &work);
 	spin_unlock(&wb->list_lock);
-}
 

fs/fs-writeback.c static void __writeback_inodes_wb(struct · line 665

Quoted 10 lines

fs/fs-writeback.c +2 -8

-/*
- * The maximum number of pages to writeout in a single bdi flush/kupdate
- * operation.  We do this so we don't hold I_SYNC against an inode for
- * enormous amounts of time, which would block a userspace task which has
- * been forced to throttle against that inode.  Also, the code reevaluates
- * the dirty each time it has written this many pages.
- */
-#define MAX_WRITEBACK_PAGES     1024
+	return nr_pages - work.nr_pages;
+}

And this change means we'll only ever write 1024 pages maximum per call to writeback_inodes_wb() when large files are present. that means:

....

Quoted 10 lines

mm/page-writeback.c +1 -4

@@ -562,17 +555,17 @@ static void balance_dirty_pages(struct a
 		 * threshold otherwise wait until the disk writes catch
 		 * up.
 		 */
-		trace_wbc_balance_dirty_start(&wbc, bdi);
+		trace_balance_dirty_start(bdi);
 		if (bdi_nr_reclaimable > bdi_thresh) {
-			writeback_inodes_wb(&bdi->wb, &wbc);
-			pages_written += write_chunk - wbc.nr_to_write;
-			trace_wbc_balance_dirty_written(&wbc, bdi);

mm/page-writeback.c static void balance_dirty_pages(struct a · line 568

Quoted 10 lines

mm/page-writeback.c +4 -1

+			pages_written += writeback_inodes_wb(&bdi->wb,
+							     write_chunk);
+			trace_balance_dirty_written(bdi, pages_written);
 			if (pages_written >= write_chunk)
 				break;		/* We've done our duty */
 		}
-		trace_wbc_balance_dirty_wait(&wbc, bdi);
 		__set_current_state(TASK_UNINTERRUPTIBLE);
 		io_schedule_timeout(pause);
+		trace_balance_dirty_wait(bdi);

We're going to get different throttling behaviour dependent on whether there are large dirty files present or not in cache....

Cheers,

Dave.

Signature · 2 lines

Dave Chinner
[email protected]

Quoted 42 lines · Dave Chinner

Quoted 19 lines · Wu Fengguang

Pass struct wb_writeback_work all the way down to writeback_sb_inodes(), and initialize the struct writeback_control there.

struct writeback_control is basically designed to control writeback of a single file, but we keep abuse it for writing multiple files in writeback_sb_inodes() and its callers.

It immediately clean things up, e.g. suddenly wbc.nr_to_write vs work->nr_pages starts to make sense, and instead of saving and restoring pages_skipped in writeback_sb_inodes it can always start with a clean zero value.

It also makes a neat IO pattern change: large dirty files are now written in the full 4MB writeback chunk size, rather than whatever remained quota in wbc->nr_to_write.

Proposed-by Christoph Hellwig <[email protected]>

Signed-off-by Wu Fengguang <[email protected]>

---

.....

Quoted 21 lines

fs/fs-writeback.c +8 -4

@@ -543,34 +588,44 @@ static int writeback_sb_inodes(struct su
 			requeue_io(inode, wb);
 			continue;
 		}
-
 		__iget(inode);
+		write_chunk = writeback_chunk_size(work);
+		wbc.nr_to_write = write_chunk;
+		wbc.pages_skipped = 0;
+
+		writeback_single_inode(inode, wb, &wbc);
 
-		pages_skipped = wbc->pages_skipped;
-		writeback_single_inode(inode, wb, wbc);
-		if (wbc->pages_skipped != pages_skipped) {
+		work->nr_pages -= write_chunk - wbc.nr_to_write;
+		wrote += write_chunk - wbc.nr_to_write;
+		if (wbc.pages_skipped) {
 			/*
 			 * writeback is not making progress due to locked
 			 * buffers.  Skip this inode for now.

Quoted 8 lines

fs/fs-writeback.c static int writeback_sb_inodes(struct su · line 607

Quoted 5 lines

fs/fs-writeback.c +2 -1

 			 */
 			redirty_tail(inode, wb);
-		}
+		} else if (!(inode->i_state & I_DIRTY))
+			wrote++;

Oh, that's just ugly. Do that accounting via nr_to_write in writeback_single_inode() as I suggested earlier, please.

This is the more simple and reliable test "whether the inode is cleaned" that does not rely on the return value of ->write_inode(), as replied in the earlier email.

Quoted 11 lines

Quoted 11 lines

fs/fs-writeback.c +4 -2

 		spin_unlock(&inode->i_lock);
 		spin_unlock(&wb->list_lock);
 		iput(inode);
 		cond_resched();
 		spin_lock(&wb->list_lock);
-		if (wbc->nr_to_write <= 0)
-			return 1;
+		/*
+		 * bail out to wb_writeback() often enough to check
+		 * background threshold and other termination conditions.
+		 */

Quoted 8 lines

Review static int writeback_sb_inodes(struct su · line 618

Quoted 2 lines

(quoted diff) +2 -0

+		if (wrote >= MAX_WRITEBACK_PAGES)
+			break;

Why do this so often? If you are writing large files, it will be once every writeback_single_inode() call that you bail. Seems rather inefficient to me to go back to the top level loop just to check for more work when we already know we have more work to do because there's still inodes on b_io....

(answering the below comments together)

For large files, it's exactly the same behavior as in the old wb_writeback(), which sets .nr_to_write = MAX_WRITEBACK_PAGES.

So it's not "more inefficient" than the original code.

For balance_dirty_pages(), it may change behavior by splitting one 16MB write to four 4MB writes.

However the good side could be less
throttle latency.

The fix is to do IO-less balance_dirty_pages() and do larger write chunk size (around half write bandwidth). Then we get reasonable good bail frequent as well as IO efficiency.

Quoted 20 lines

Quoted 11 lines

Quoted 11 lines

(quoted diff) +4 -2

 		spin_unlock(&inode->i_lock);
 		spin_unlock(&wb->list_lock);
 		iput(inode);
 		cond_resched();
 		spin_lock(&wb->list_lock);
-		if (wbc->nr_to_write <= 0)
-			return 1;
+		/*
+		 * bail out to wb_writeback() often enough to check
+		 * background threshold and other termination conditions.
+		 */

Quoted 8 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+		if (wrote >= MAX_WRITEBACK_PAGES)
+			break;

Why do this so often? If you are writing large files, it will be once every writeback_single_inode() call that you bail. Seems rather inefficient to me to go back to the top level loop just to check for more work when we already know we have more work to do because there's still inodes on b_io....

Quoted 6 lines

(answering the below comments together)

For large files, it's exactly the same behavior as in the old wb_writeback(), which sets .nr_to_write = MAX_WRITEBACK_PAGES.

So it's not "more inefficient" than the original code.

I didn't say that. I said it "seems rather inefficient" as a direct comment on the restructured code. We don't need to check the high level loop until we've finished processing b_io - the existing code did that to get nr_to_write updated, but now we've changed it so we don't refill b_io until it is empty, so any tim ewe loop back to the top, we're just going to start from the same point that we were at deep in the loop itself.

That is the current code does:

	wb_writeback {
		wbc->nr_to_write = MAX_WRITEBACK_PAGES
		writeback_inodes_wb {
			queue_io(expired)
			writeback_inodes {
				writeback_single_inode
			} until (wbc->nr_to_write <= 0)
		}
	}

The new code does:

	wb_writeback {
		writeback_inodes_wb {
			if (b_io empty)
				queue_io(expired)
			writeback_sb_inodes {
				wbc->nr_to_write = MAX_WRITEBACK_PAGES
				wrote = writeback_single_inode
				if (wrote >= MAX_WRITEBACK_PAGES)
					break;
			} until (b_io empty)
		}
	}

Which is a very different inner loop structure because now small inodes that write less than MAX_WRITEBACK_PAGES will not cause the inner loop to exit until b_io empties.

However, one large file will
cause the inner loop to exit, go all the way back up to wb_writeback(), which will immeidately come back down into writeback_inodes() and start working on an _unchanged b_io list_.

My point is that breaking out of the inner loop like this is pointless. Especially if all we have is inodes with >1024 dirty pages because of all the unnecessary extra work breaking out of the inner loop entails.

Quoted 2 lines

For balance_dirty_pages(), it may change behavior by splitting one 16MB write to four 4MB writes.

balance_dirty_pages() typically askes for 1536 pages to be written back, so I'm not sure where your numbers are coming from.

Quoted 6 lines

However the good side could be less
throttle latency.

The fix is to do IO-less balance_dirty_pages() and do larger write chunk size (around half write bandwidth). Then we get reasonable good bail frequent as well as IO efficiency.

We're not getting that with this patch set, though, and so the change as proposed needs to work correctly without them.