Improve buffered streaming write ordering

The patch below changes write_cache_pages to only use writeback_index when current_is_pdflush(). The basic idea is that pdflush is the only one who has concurrency control against the bdi, so it is the only one who can safely use and update writeback_index.

Quoted 4 lines · On Wed, 01 Oct 2008 14:40:51 -0400 Chris Mason <[email protected]> wrote:

The patch below changes write_cache_pages to only use writeback_index when current_is_pdflush(). The basic idea is that pdflush is the only one who has concurrency control against the bdi, so it is the only one who can safely use and update writeback_index.

Another approach would be to only update mapping->writeback_index if nobody else altered it meanwhile.

That being said, I don't really see why we get lots of seekiness when two threads start their writing the file from the same offset.

Quoted 7 lines · Andrew Morton

Quoted 4 lines · On Wed, 01 Oct 2008 14:40:51 -0400 Chris Mason <[email protected]> wrote:

The patch below changes write_cache_pages to only use writeback_index when current_is_pdflush(). The basic idea is that pdflush is the only one who has concurrency control against the bdi, so it is the only one who can safely use and update writeback_index.

Quoted 3 lines

Another approach would be to only update mapping->writeback_index if nobody else altered it meanwhile.

Ok, I can give that a short.

Quoted 10 lines · Andrew Morton

Quoted 4 lines · On Wed, 01 Oct 2008 14:40:51 -0400 Chris Mason <[email protected]> wrote:

The patch below changes write_cache_pages to only use writeback_index when current_is_pdflush(). The basic idea is that pdflush is the only one who has concurrency control against the bdi, so it is the only one who can safely use and update writeback_index.

Another approach would be to only update mapping->writeback_index if nobody else altered it meanwhile.

Ok, I can give that a short.

Quoted 2 lines

That being said, I don't really see why we get lots of seekiness when two threads start their writing the file from the same offset.

For metadata, it makes sense. Pages get dirtied in strange order, and if writeback_index is jumping around, we'll get the seeky metadata writeback.

Data makes less sense, especially the very high extent count from ext4. An extra printk shows that ext4 is calling redirty_page_for_writepage quite a bit in ext4_da_writepage. This should be enough to make us jump around in the file.

For a 4.5GB streaming buffered write, this printk inside ext4_da_writepage shows up 37,2429 times in /var/log/messages.

if (page_has_buffers(page)) {
page_bufs = page_buffers(page);
if (walk_page_buffers(NULL, page_bufs, 0, len, NULL,
ext4_bh_unmapped_or_delay)) {
/*
* We don't want to do block allocation
* So redirty the page and return
* We may reach here when we do a journal commit
* via journal_submit_inode_data_buffers.
* If we don't have mapping block we just ignore
* them. We can also reach here via shrink_page_list */
redirty_page_for_writepage(wbc, page);

printk("redirty page %Lu\n", page_offset(page));

			unlock_page(page);
			return 0;
		}
	} else {

-chris

Quoted 4 lines

For a 4.5GB streaming buffered write, this printk inside ext4_da_writepage shows up 37,2429 times in /var/log/messages.

Part of that can happen due to shrink_page_list -> pageout -> writepagee call back with lots of unallocated buffer_heads(blocks).

Quoted 4 lines · Chris Mason

On Wed, 2008-10-01 at 21:52 -0700, Andrew Morton wrote:
For a 4.5GB streaming buffered write, this printk inside ext4_da_writepage shows up 37,2429 times in /var/log/messages.

Part of that can happen due to shrink_page_list -> pageout -> writepagee call back with lots of unallocated buffer_heads(blocks).

Quoted 6 lines · Aneesh Kumar K.V

Quoted 4 lines · Chris Mason

On Wed, 2008-10-01 at 21:52 -0700, Andrew Morton wrote:
For a 4.5GB streaming buffered write, this printk inside ext4_da_writepage shows up 37,2429 times in /var/log/messages.

Quoted 2 lines

Part of that can happen due to shrink_page_list -> pageout -> writepagee call back with lots of unallocated buffer_heads(blocks).

Quite frankly, a simple streaming buffered write should *never* trigger writeback from the LRU in memory reclaim.

Quoted 10 lines · Dave Chinner

Quoted 8 lines · Aneesh Kumar K.V

Quoted 4 lines · Chris Mason

On Wed, 2008-10-01 at 21:52 -0700, Andrew Morton wrote:
For a 4.5GB streaming buffered write, this printk inside ext4_da_writepage shows up 37,2429 times in /var/log/messages.

Part of that can happen due to shrink_page_list -> pageout -> writepagee call back with lots of unallocated buffer_heads(blocks).

Quoted 2 lines

Quite frankly, a simple streaming buffered write should *never* trigger writeback from the LRU in memory reclaim.

The blktrace runs on ext4 didn't show kswapd doing any IO. It isn't clear if this is because ext4 did the redirty trick or if kswapd didn't call writepage.

-chris

Quoted 14 lines · Chris Mason

Quoted 12 lines · Dave Chinner

Quoted 8 lines · Aneesh Kumar K.V

Quoted 4 lines · Chris Mason

On Wed, 2008-10-01 at 21:52 -0700, Andrew Morton wrote:
For a 4.5GB streaming buffered write, this printk inside ext4_da_writepage shows up 37,2429 times in /var/log/messages.

Part of that can happen due to shrink_page_list -> pageout -> writepagee call back with lots of unallocated buffer_heads(blocks).

Quite frankly, a simple streaming buffered write should *never* trigger writeback from the LRU in memory reclaim.

Quoted 5 lines

The blktrace runs on ext4 didn't show kswapd doing any IO. It isn't clear if this is because ext4 did the redirty trick or if kswapd didn't call writepage.

-chris

This patch actually reduced the number of extents for the below test from 564 to 171.

Quoted 15 lines · Aneesh Kumar K.V

Quoted 14 lines · Chris Mason

Quoted 12 lines · Dave Chinner

Quoted 8 lines · Aneesh Kumar K.V

Quoted 4 lines · Chris Mason

On Wed, 2008-10-01 at 21:52 -0700, Andrew Morton wrote:
For a 4.5GB streaming buffered write, this printk inside ext4_da_writepage shows up 37,2429 times in /var/log/messages.

Part of that can happen due to shrink_page_list -> pageout -> writepagee call back with lots of unallocated buffer_heads(blocks).

Quite frankly, a simple streaming buffered write should *never* trigger writeback from the LRU in memory reclaim.

Quoted 9 lines

Quoted 5 lines

The blktrace runs on ext4 didn't show kswapd doing any IO. It isn't clear if this is because ext4 did the redirty trick or if kswapd didn't call writepage.

-chris

This patch actually reduced the number of extents for the below test from 564 to 171.

For my array, this patch brings the number of ext4 extents down from over 4000 to 27. The throughput reported by dd goes up from ~80MB/s to 330MB/s, which means buffered IO is going as fast as O_DIRECT.

Here's the graph:

http://oss.oracle.com/~mason/bugs/writeback_ordering/ext4-aneesh.png

The strange metadata writeback for the uninit block groups is gone.

Looking at the patch, I think the ext4_writepages code should just make its own write_cache_pages. It's pretty hard to follow the code that is there for ext4 vs the code that is there to make write_cache_pages do what ext4 expects it to.

fs/ext4/inode.c static int __mpage_da_writepage(struct page *page, · line 2105

Quoted 4 lines · Aneesh Kumar K.V

fs/ext4/inode.c +4 -0

+static int ext4_write_cache_pages(struct address_space *mapping,
+		      struct writeback_control *wbc, writepage_t writepage,
+		      void *data)
+{

Looking at this functions the only difference is killing the writeback_index and range_start updates. If they are bad why would we only remove them from ext4?

Quoted 9 lines · Christoph Hellwig

fs/ext4/inode.c static int __mpage_da_writepage(struct page *page, · line 2105

Quoted 4 lines · Aneesh Kumar K.V

fs/ext4/inode.c +4 -0

+static int ext4_write_cache_pages(struct address_space *mapping,
+		      struct writeback_control *wbc, writepage_t writepage,
+		      void *data)
+{

Looking at this functions the only difference is killing the writeback_index and range_start updates. If they are bad why would we only remove them from ext4?

I am also not updating wbc->nr_to_write.

I don't think other filesystem have this requirement.

That's true, but there is a lot of code duplication, which means that bugs or changes in write_cache_pages() would need to be fixed in ext4_write_cache_pages(). So another approach that might be better from a long-term code maintenance point of view is to add a flag in struct writeback_control that tells write_cache_pages() not to update those fields, and avoid duplicating approximately 95 lines of code. It means a change in a core mm function, though, so if folks thinks its too ugly, we can make our own copy in fs/ext4.

Opinions? Andrew, as someone who often weighs in on fs and mm issues, what do you think? My preference would be to make the change to mm/page-writeback.c, controlled by a flag which ext4 would set be set by fs/ext4 before it calls write_cache_pages().

Quoted 8 lines · Theodore Tso

That's true, but there is a lot of code duplication, which means that bugs or changes in write_cache_pages() would need to be fixed in ext4_write_cache_pages(). So another approach that might be better from a long-term code maintenance point of view is to add a flag in struct writeback_control that tells write_cache_pages() not to update those fields, and avoid duplicating approximately 95 lines of code. It means a change in a core mm function, though, so if folks thinks its too ugly, we can make our own copy in fs/ext4.

That's true, but there is a lot of code duplication, which means that bugs or changes in write_cache_pages() would need to be fixed in ext4_write_cache_pages(). So another approach that might be better from a long-term code maintenance point of view is to add a flag in struct writeback_control that tells write_cache_pages() not to update those fields, and avoid duplicating approximately 95 lines of code. It means a change in a core mm function, though, so if folks thinks its too ugly, we can make our own copy in fs/ext4.

Opinions? Andrew, as someone who often weighs in on fs and mm issues, what do you think? My preference would be to make the change to mm/page-writeback.c, controlled by a flag which ext4 would set be set by fs/ext4 before it calls write_cache_pages().

Quoted 11 lines · Christoph Hellwig

fs/ext4/inode.c static int __mpage_da_writepage(struct page *page, · line 2105

Quoted 5 lines · Aneesh Kumar K.V

fs/ext4/inode.c +4 -0

+static int ext4_write_cache_pages(struct address_space *mapping,
+		      struct writeback_control *wbc, writepage_t writepage,
+		      void *data)
+{

Looking at this functions the only difference is killing the writeback_index and range_start updates. If they are bad why would we only remove them from ext4?

I am also not updating wbc->nr_to_write.

ext4 delayed allocation writeback is bit tricky. It does

a) Look at the dirty pages and build an in memory extent of contiguous logical file blocks. If we use writecache_pages to do that it will update nr_to_write, writeback_index etc during this stage.

b) Request the block allocator for 'x' blocks. We get the value x from step a.

c) block allocator may return less than 'x' contiguous block. That would mean the variables updated by write_cache_pages need to corrected. The old code was doing that. Chris Mason suggested it would make it easy to use a write_cache_pages which doesn't update the variable for ext4.

I don't think other filesystem have this requirement.

Quoted 30 lines · Aneesh Kumar K.V

Quoted 11 lines · Christoph Hellwig

fs/ext4/inode.c static int __mpage_da_writepage(struct page *page, · line 2105

Quoted 5 lines · Aneesh Kumar K.V

fs/ext4/inode.c +4 -0

+static int ext4_write_cache_pages(struct address_space *mapping,
+		      struct writeback_control *wbc, writepage_t writepage,
+		      void *data)
+{

Looking at this functions the only difference is killing the writeback_index and range_start updates. If they are bad why would we only remove them from ext4?

I am also not updating wbc->nr_to_write.

ext4 delayed allocation writeback is bit tricky. It does

a) Look at the dirty pages and build an in memory extent of contiguous logical file blocks. If we use writecache_pages to do that it will update nr_to_write, writeback_index etc during this stage.

b) Request the block allocator for 'x' blocks. We get the value x from step a.

c) block allocator may return less than 'x' contiguous block. That would mean the variables updated by write_cache_pages need to corrected. The old code was doing that. Chris Mason suggested it would make it easy to use a write_cache_pages which doesn't update the variable for ext4.

Quoted 1 line

I don't think other filesystem have this requirement.

The NFS client can benefit from only writing pages in strictly ascending offset order. The benefit comes from helping the server to do better allocations by not sending file data to the server in random order.

There is also an NFS server in the market which requires data to be sent in strict ascending offset order. This sort of support would make interoperating with that server much easier.

Quoted 8 lines · Aneesh Kumar K.V

Quoted 4 lines · Chris Mason

On Wed, 2008-10-01 at 21:52 -0700, Andrew Morton wrote:
For a 4.5GB streaming buffered write, this printk inside ext4_da_writepage shows up 37,2429 times in /var/log/messages.

Part of that can happen due to shrink_page_list -> pageout -> writepagee call back with lots of unallocated buffer_heads(blocks).

Quite frankly, a simple streaming buffered write should *never* trigger writeback from the LRU in memory reclaim. That indicates that some feedback loop has broken down and we are not cleaning pages fast enough or perhaps in the correct order. Page reclaim in this case should be reclaiming clean pages (those that have already been written back), not writing back random dirty pages.

Quoted 10 lines · Dave Chinner

Quoted 8 lines · Aneesh Kumar K.V

Quoted 4 lines · Chris Mason

On Wed, 2008-10-01 at 21:52 -0700, Andrew Morton wrote:
For a 4.5GB streaming buffered write, this printk inside ext4_da_writepage shows up 37,2429 times in /var/log/messages.

Part of that can happen due to shrink_page_list -> pageout -> writepagee call back with lots of unallocated buffer_heads(blocks).

Quoted 6 lines

Quite frankly, a simple streaming buffered write should *never* trigger writeback from the LRU in memory reclaim. That indicates that some feedback loop has broken down and we are not cleaning pages fast enough or perhaps in the correct order. Page reclaim in this case should be reclaiming clean pages (those that have already been written back), not writing back random dirty pages.

Here are some go faster stripes for the XFS buffered writeback. This patch has a lot of debatable features to it, but the idea is to show which knobs are slowing us down today.

The first change is to avoid calling balance_dirty_pages_ratelimited on every page. When we know we're doing a largeish write it makes more sense to balance things less often. This might just mean our ratelimit_pages magic value is too small.

The second change makes xfs bump wbc->nr_to_write (suggested by Christoph), which probably makes delalloc go in bigger chunks.

Quoted 17 lines · Chris Mason

Quoted 10 lines · Andrew Morton

Quoted 4 lines · On Wed, 01 Oct 2008 14:40:51 -0400 Chris Mason <[email protected]> wrote:

The patch below changes write_cache_pages to only use writeback_index when current_is_pdflush(). The basic idea is that pdflush is the only one who has concurrency control against the bdi, so it is the only one who can safely use and update writeback_index.

Another approach would be to only update mapping->writeback_index if nobody else altered it meanwhile.

Ok, I can give that a short.

Quoted 2 lines

That being said, I don't really see why we get lots of seekiness when two threads start their writing the file from the same offset.

Quoted 8 lines

For metadata, it makes sense. Pages get dirtied in strange order, and if writeback_index is jumping around, we'll get the seeky metadata writeback.

Data makes less sense, especially the very high extent count from ext4. An extra printk shows that ext4 is calling redirty_page_for_writepage quite a bit in ext4_da_writepage. This should be enough to make us jump around in the file.

We need to do start the journal before locking the page with jbd2. That prevent us from doing any block allocation in writepage() call back. So with ext4/jbd2 we do block allocation only in writepages() call back where we start the journal with credit needed to write a single extent. Then we look for contiguous unallocated logical block and request the block allocator for 'x' blocks. If we get less than that. The rest of the pages which we iterated in writepages are redirtied so that we try to allocate them again. We loop inside ext4_da_writepages itself looking at wbc->pages_skipped

2481 if (wbc->range_cont && (pages_skipped != wbc->pages_skipped)) {

2482                 /* We skipped pages in this loop */
2483                 wbc->range_start = range_start;
2484                 wbc->nr_to_write = to_write +

Quoted 4 lines

For a 4.5GB streaming buffered write, this printk inside ext4_da_writepage shows up 37,2429 times in /var/log/messages.

Part of that can happen due to shrink_page_list -> pageout -> writepagee call back with lots of unallocated buffer_heads(blocks). Also a journal commit with jbd2 looks at the inode and all the dirty pages, rather than the buffer_heads (journal_submit_data_buffers). We don't force commit pages that doesn't have blocks allocated with the ext4. The consistency is only with i_size and data.

Quoted 3 lines · Aneesh Kumar K.V

Quoted 2 lines · Chris Mason

Quoted 1 line · Andrew Morton

On Wed, 01 Oct 2008 14:40:51 -0400 Chris Mason

Quoted 42 lines

Quoted 14 lines

Quoted 8 lines

Quoted 5 lines

The patch below changes write_cache_pages to only use writeback_index when current_is_pdflush(). The basic idea is that pdflush is the only one who has concurrency control against the bdi, so it is the only one who can safely use and update writeback_index.

Another approach would be to only update mapping->writeback_index if nobody else altered it meanwhile.

Ok, I can give that a short.

Quoted 2 lines

That being said, I don't really see why we get lots of seekiness when two threads start their writing the file from the same offset.

Quoted 8 lines

For metadata, it makes sense. Pages get dirtied in strange order, and if writeback_index is jumping around, we'll get the seeky metadata writeback.

Data makes less sense, especially the very high extent count from ext4. An extra printk shows that ext4 is calling redirty_page_for_writepage quite a bit in ext4_da_writepage. This should be enough to make us jump around in the file.

We need to do start the journal before locking the page with jbd2. That prevent us from doing any block allocation in writepage() call back. So with ext4/jbd2 we do block allocation only in writepages() call back where we start the journal with credit needed to write a single extent. Then we look for contiguous unallocated logical block and request the block allocator for 'x' blocks. If we get less than that. The rest of the pages which we iterated in writepages are redirtied so that we try to allocate them again. We loop inside ext4_da_writepages itself looking at wbc->pages_skipped

2481 if (wbc->range_cont && (pages_skipped !=
wbc->pages_skipped)) { 2482 /* We skipped pages in this
loop */
2483 wbc->range_start = range_start;
2484 wbc->nr_to_write = to_write +

Quoted 2 lines

For a 4.5GB streaming buffered write, this printk inside ext4_da_writepage shows up 37,2429 times in /var/log/messages.

Quoted 6 lines

Part of that can happen due to shrink_page_list -> pageout -> writepagee call back with lots of unallocated buffer_heads(blocks). Also a journal commit with jbd2 looks at the inode and all the dirty pages, rather than the buffer_heads (journal_submit_data_buffers). We don't force commit pages that doesn't have blocks allocated with the ext4. The consistency is only with i_size and data.

In general, I don't think pdflush or the VM expect redirty_pages_for_writepage to be used this aggressively.

Quoted 9 lines · Chris Mason

Quoted 6 lines

Part of that can happen due to shrink_page_list -> pageout -> writepagee call back with lots of unallocated buffer_heads(blocks). Also a journal commit with jbd2 looks at the inode and all the dirty pages, rather than the buffer_heads (journal_submit_data_buffers). We don't force commit pages that doesn't have blocks allocated with the ext4. The consistency is only with i_size and data.

In general, I don't think pdflush or the VM expect redirty_pages_for_writepage to be used this aggressively.

BTW. redirty_page_for_writepage and the whole model of cleaning the page's dirty bit *before* calling into the filesystem is really nasty IMO. For one thing it opens races that mean a filesystem can't keep metadata about the pagecache properly in synch with the page's dirty bit.

I have a patch in my fsblock series that fixes this and has the writepage() function itself clear the page's dirty bit. This basically makes redirty_page_for_writepages go away completely (at least the uses I looked at, I didn't look at ext4 though).

Shall I break it out and submit it?

Hello everyone,

write_cache_pages can use the address space writeback_index field to try and pick up where it left off between calls. pdflush and balance_dirty_pages both enable this mode in hopes of having writeback evenly walk down the file instead of just servicing pages at the start of the address space.

But, there is no locking around this field, and concurrent callers of write_cache_pages on the same inode can get some very strange results. pdflush uses writeback_acquire function to make sure that only one pdflush process is servicing a given backing device, but balance_dirty_pages does not.

When there are a small number of dirty inodes in the system, balance_dirty_pages is likely to run in parallel with pdflush on one or two of them, leading to somewhat random updates of the writeback_index field in struct address space.

The end result is very seeky writeback during streaming IO. A 4 drive hardware raid0 array here can do 317MB/s streaming O_DIRECT writes on ext4. This is creating a new file, so O_DIRECT is really just a way to bypass write_cache_pages.

If I do buffered writes instead, XFS does 205MB/s, and ext4 clocks in at 81.7MB/s. Looking at the buffered IO traces for each one, we can see a lot of seeks.

http://oss.oracle.com/~mason/bugs/writeback_ordering/ext4-nopatch.png

http://oss.oracle.com/~mason/bugs/writeback_ordering/xfs-nopatch.png

The patch below changes write_cache_pages to only use writeback_index when current_is_pdflush(). The basic idea is that pdflush is the only one who has concurrency control against the bdi, so it is the only one who can safely use and update writeback_index.

The performance changes quite a bit:

        patched        unpatched
XFS     247MB/s        205MB/s
Ext4    246MB/s        81.7MB/s

The graphs after the patch:

http://oss.oracle.com/~mason/bugs/writeback_ordering/ext4-patched.png

http://oss.oracle.com/~mason/bugs/writeback_ordering/xfs-patched.png

The ext4 graph really does look strange. What's happening there is the lazy inode table init has dirtied a whole bunch of pages on the block device inode. I don't have much of an answer for why my patch makes all of this writeback happen up front, other then writeback_index is no longer bouncing all over the address space.

It is also worth noting that before the patch, filefrag shows ext4 using about 4000 extents on the file. After the patch it is around 400. XFS uses 2 extents both patched and unpatched.