JBD ordered mode rewrite

Hi,

Below is my rewrite of ordered mode in JBD. Now we don't have a list of
data buffers that need syncing on transaction commit but a list of inodes that need writeout during commit. This brings all sorts of advantages such as possibility to get rid of journal heads and buffer heads for data buffers in ordered mode, better ordering of writes on transaction commit, simplification of some JBD code, no more anonymous pages when truncate of data being committed happens. The patch has survived some light testing but it still has some potential of eating your data so beware :) I've run dbench to see whether we didn't decrease performance by different handling of truncate and the throughput I'm getting on my machine is the same (OK, is lower by 0.5%) if I disable the code in truncate waiting for commit to finish... Also the throughput of dbench is about 2% better with my patch than with current JBD.
Any comments or testing most welcome.

Honza

Quoted 17 lines · Jan Kara

Hi,

Below is my rewrite of ordered mode in JBD. Now we don't have a list of
data buffers that need syncing on transaction commit but a list of inodes that need writeout during commit. This brings all sorts of advantages such as possibility to get rid of journal heads and buffer heads for data buffers in ordered mode, better ordering of writes on transaction commit, simplification of some JBD code, no more anonymous pages when truncate of data being committed happens. The patch has survived some light testing but it still has some potential of eating your data so beware :) I've run dbench to see whether we didn't decrease performance by different handling of truncate and the throughput I'm getting on my machine is the same (OK, is lower by 0.5%) if I disable the code in truncate waiting for commit to finish... Also the throughput of dbench is about 2% better with my patch than with current JBD.
Any comments or testing most welcome.

Quoted 1 line

Honza

Just one nit, doesn't compile properly when jbd/ext3 are modules :). Thanks much,

Below is my rewrite of ordered mode in JBD. Now we don't have a list of
data buffers that need syncing on transaction commit but a list of inodes that need writeout during commit. This brings all sorts of advantages such as possibility to get rid of journal heads and buffer heads for data buffers in ordered mode, better ordering of writes on transaction commit, simplification of some JBD code, no more anonymous pages when truncate of

data being committed happens. The patch has survived some light testing but it still has some potential of eating your data so beware :) I've run dbench to see whether we didn't decrease performance by different handling of truncate and the throughput I'm getting on my machine is the same (OK, is lower by 0.5%) if I disable the code in truncate waiting for commit to finish... Also the throughput of dbench is about 2% better with my patch than with current JBD.
Any comments or testing most welcome.

Quoted 6 lines · On Thu, 6 Mar 2008 18:42:09 +0100 Jan Kara <[email protected]> wrote:

Below is my rewrite of ordered mode in JBD. Now we don't have a list of
data buffers that need syncing on transaction commit but a list of inodes that need writeout during commit. This brings all sorts of advantages such as possibility to get rid of journal heads and buffer heads for data buffers in ordered mode, better ordering of writes on transaction commit, simplification of some JBD code, no more anonymous pages when truncate of

Quoted 8 lines

data being committed happens. The patch has survived some light testing but it still has some potential of eating your data so beware :) I've run dbench to see whether we didn't decrease performance by different handling of truncate and the throughput I'm getting on my machine is the same (OK, is lower by 0.5%) if I disable the code in truncate waiting for commit to finish... Also the throughput of dbench is about 2% better with my patch than with current JBD.
Any comments or testing most welcome.

Thanks for plugging away with this.

Please change your patch preparation tools to always always include a diffstat, OK?

 fs/buffer.c               |    3 
 fs/ext3/ialloc.c          |    1 
 fs/ext3/inode.c           |  118 +++++++++---------
 fs/ext3/super.c           |    2 
 fs/jbd/checkpoint.c       |    1 
 fs/jbd/commit.c           |  257 +++++++++++++----------------------------
 fs/jbd/journal.c          |   45 +++++++
 fs/jbd/transaction.c      |  288 +++++++++++-----------------------------------
 fs/mpage.c                |    5 
 include/linux/ext3_fs.h   |    1 
 include/linux/ext3_fs_i.h |    1 
 include/linux/jbd.h       |   70 +++++++----
 include/linux/writeback.h |    2 
 13 files changed, 326 insertions(+), 468 deletions(-)

Would it make sense to turn this patch into a patch series sometime?

Below is my rewrite of ordered mode in JBD. Now we don't have a list of
data buffers that need syncing on transaction commit but a list of inodes that need writeout during commit. This brings all sorts of advantages such as possibility to get rid of journal heads and buffer heads for data buffers in ordered mode, better ordering of writes on transaction commit, simplification of some JBD code, no more anonymous pages when truncate of data being committed happens.

The patch has survived some light testing but it still has some potential of eating your data so beware :)

I've run dbench to see whether we didn't decrease performance by different handling of truncate and the throughput I'm getting on my machine is the same (OK, is lower by 0.5%) if I disable the code in truncate waiting for commit to finish...
Also the throughput of dbench is about 2% better with my patch than with current JBD.
Any comments or testing most welcome.

fs/ext3/inode.c +1 -9

@@ -1465,15 +1444,11 @@ static int journal_dirty_data_fn(handle_t *handle, struct buffer_head *bh)
  * We don't honour synchronous mounts for writepage().  That would be
  * disastrous.  Any write() or metadata operation will sync the fs for
  * us.
- *
- * AKPM2: if all the page's buffers are mapped to disk and !data=journal,
- * we don't need to open a transaction here.
  */
 static int ext3_ordered_writepage(struct page *page,
 				struct writeback_control *wbc)
 {
 	struct inode *inode = page->mapping->host;
-	struct buffer_head *page_bufs;
 	handle_t *handle = NULL;
 	int ret = 0;
 	int err;

@@ -1487,46 +1462,49 @@ static int ext3_ordered_writepage(struct page *page,
 	if (ext3_journal_current_handle())
 		goto out_fail;
 
-	handle = ext3_journal_start(inode, ext3_writepage_trans_blocks(inode));
-
-	if (IS_ERR(handle)) {
-		ret = PTR_ERR(handle);
-		goto out_fail;
+	/*

fs/jbd/transaction.c +9 -0

+/*
+ * This function must be called when inode is journaled in ordered mode
+ * before truncation happens. It starts writeout of truncated part in
+ * case it is in the committing transaction so that we stand to ordered
+ * mode consistency guarantees.
+ */
+int journal_begin_ordered_truncate(struct jbd_inode *inode, loff_t new_size)
+{
+	journal_t *journal;

fs/jbd/transaction.c +8 -0

+		ret = __filemap_fdatawrite_range(inode->i_vfs_inode->i_mapping,
+			new_size, LLONG_MAX, WB_SYNC_ALL);
+		if (ret)
+			journal_abort(journal, ret);
+	}
+out:
+	return ret;
+}

fs/mpage.c +4 -1

index 235e4d3..4f66bae 100644
@@ -527,7 +527,10 @@ static int __mpage_writepage(struct page *page, struct writeback_control *wbc,
 
 		map_bh.b_state = 0;
 		map_bh.b_size = 1 << blkbits;
-		if (mpd->get_block(inode, block_in_file, &map_bh, 1))
+		if (mpd->get_block(inode, block_in_file, &map_bh,
+				   !wbc->skip_unmapped))
+			goto confused;
+		if (!buffer_mapped(&map_bh))
 			goto confused;
 		if (buffer_new(&map_bh))
 			unmap_underlying_metadata(map_bh.b_bdev,

include/linux/ext3_fs.h +0 -0

index 36c5403..7aa8327 100644

Quoted 7 lines · Jan Kara

Below is my rewrite of ordered mode in JBD. Now we don't have a list of
data buffers that need syncing on transaction commit but a list of inodes that need writeout during commit. This brings all sorts of advantages such as possibility to get rid of journal heads and buffer heads for data buffers in ordered mode, better ordering of writes on transaction commit, simplification of some JBD code, no more anonymous pages when truncate of data being committed happens.

There's a lot of JBD code which gets removed by this patch - cool :)

Quoted 2 lines

The patch has survived some light testing but it still has some potential of eating your data so beware :)

Looking through the patch, I don't see how you solve the page lock / transaction ordering issues. I see that you avoid starting a handle in ->writepage during transaction commit, but what about another process which starts a handle under page lock and needs to wait for transactions to be written out before continuing?

Quoted 7 lines

I've run dbench to see whether we didn't decrease performance by different handling of truncate and the throughput I'm getting on my machine is the same (OK, is lower by 0.5%) if I disable the code in truncate waiting for commit to finish...
Also the throughput of dbench is about 2% better with my patch than with current JBD.
Any comments or testing most welcome.

My attempt at helpful review follows.

Quoted 32 lines

fs/ext3/inode.c +7 -9

@@ -1465,15 +1444,11 @@ static int journal_dirty_data_fn(handle_t *handle, struct buffer_head *bh)
  * We don't honour synchronous mounts for writepage().  That would be
  * disastrous.  Any write() or metadata operation will sync the fs for
  * us.
- *
- * AKPM2: if all the page's buffers are mapped to disk and !data=journal,
- * we don't need to open a transaction here.
  */
 static int ext3_ordered_writepage(struct page *page,
 				struct writeback_control *wbc)
 {
 	struct inode *inode = page->mapping->host;
-	struct buffer_head *page_bufs;
 	handle_t *handle = NULL;
 	int ret = 0;
 	int err;

@@ -1487,46 +1462,49 @@ static int ext3_ordered_writepage(struct page *page,
 	if (ext3_journal_current_handle())
 		goto out_fail;
 
-	handle = ext3_journal_start(inode, ext3_writepage_trans_blocks(inode));
-
-	if (IS_ERR(handle)) {
-		ret = PTR_ERR(handle);
-		goto out_fail;
+	/*
+	 * Now there are two different reasons why we can be called:
+	 *   1) write out during commit
+	 *   2) fsync / writeout to free memory
+	 *
+	 * In the first case, we just need to write the buffer to disk, in the
+	 * second case we may need to do hole filling and attach the inode to

fs/ext3/inode.c static int ext3_ordered_writepage(struct page *page, · line 1475

Quoted 4 lines

fs/ext3/inode.c +4 -0

+	 * the transaction. Note that even in the first case, we may get an
+	 * unmapped buffer (hole fill with data via mmap) but we don't have to
+	 * write it - actually, we can't because from a transaction commit we
+	 * cannot start a new transaction or we could deadlock.

What happens to the data if we get here under the 1st case with a hole? Do we eventually fill the hole (with correct data) via some mechanism I don't see here?

<snip>

Quoted 18 lines

fs/jbd/transaction.c +18 -0

+/*
+ * This function must be called when inode is journaled in ordered mode
+ * before truncation happens. It starts writeout of truncated part in
+ * case it is in the committing transaction so that we stand to ordered
+ * mode consistency guarantees.
+ */
+int journal_begin_ordered_truncate(struct jbd_inode *inode, loff_t new_size)
+{
+	journal_t *journal;
+	transaction_t *commit_trans;
+	int ret = 0;
+
+	if (!inode->i_transaction && !inode->i_next_transaction)
+		goto out;
+	journal = inode->i_transaction->t_journal;
+	spin_lock(&journal->j_state_lock);
+	commit_trans = journal->j_committing_transaction;
+	spin_unlock(&journal->j_state_lock);

fs/jbd/transaction.c void journal_refile_buffer(journal_t *journal, struct journal_head *jh) · line 1960

Quoted 1 line

fs/jbd/transaction.c +1 -0

+	if (inode->i_transaction == commit_trans) {

AFAICT, this is called in ext3 before a handle is started for truncate. Is it possible for the current running transaction to become the new committing transaction shortly after the spinlock is dropped, but before the truncate transaction starts? Could this lead to ordered data not being written out if the inode is part of the current running transaction but not part of the committing transaction?

Quoted 27 lines

fs/jbd/transaction.c +8 -0

+		ret = __filemap_fdatawrite_range(inode->i_vfs_inode->i_mapping,
+			new_size, LLONG_MAX, WB_SYNC_ALL);
+		if (ret)
+			journal_abort(journal, ret);
+	}
+out:
+	return ret;
+}

fs/mpage.c +4 -1

index 235e4d3..4f66bae 100644
@@ -527,7 +527,10 @@ static int __mpage_writepage(struct page *page, struct writeback_control *wbc,
 
 		map_bh.b_state = 0;
 		map_bh.b_size = 1 << blkbits;
-		if (mpd->get_block(inode, block_in_file, &map_bh, 1))
+		if (mpd->get_block(inode, block_in_file, &map_bh,
+				   !wbc->skip_unmapped))
+			goto confused;
+		if (!buffer_mapped(&map_bh))
 			goto confused;
 		if (buffer_new(&map_bh))
 			unmap_underlying_metadata(map_bh.b_bdev,

include/linux/ext3_fs.h +0 -1

index 36c5403..7aa8327 100644
--- a/include/linux/ext3_fs.h

include/linux/ext3_fs.h int ext3_get_blocks_handle(handle_t *handle, struct inode *inode, · line 829

Quoted 6 lines

+++ b/include/linux/ext3_fs.h

include/linux/ext3_fs.h +1 -0

@@ -826,6 +826,7 @@ int ext3_get_blocks_handle(handle_t *handle, struct inode *inode,
 extern struct inode *ext3_iget(struct super_block *, unsigned long);
 extern int  ext3_write_inode (struct inode *, int);
 extern int  ext3_setattr (struct dentry *, struct iattr *);
+extern void ext3_drop_inode (struct inode *);

Not sure what this is here for...

Hi,

Below is my rewrite of ordered mode in JBD. Now we don't have a list of
data buffers that need syncing on transaction commit but a list of inodes that need writeout during commit. This brings all sorts of advantages such as possibility to get rid of journal heads and buffer heads for data buffers in ordered mode, better ordering of writes on transaction commit,

simplification of some JBD code, no more anonymous pages when truncate of data being committed happens. The patch has survived some light testing but it still has some potential of eating your data so beware :) I've run dbench to see whether we didn't decrease performance by different handling of truncate and the throughput I'm getting on my machine is the same (OK, is lower by 0.5%) if I disable the code in truncate waiting for commit to finish... Also the throughput of dbench is about 2% better with my patch than with current JBD.

Any comments or testing most welcom

/*
* Note that we always start a transaction even if we're not journalling
* data. This is to preserve ordering: any hole instantiation within

fs/ext3/inode.c +2 -9

@@ -1465,15 +1444,11 @@ static int journal_dirty_data_fn(handle_t *handle, struct buffer_head *bh)
  * We don't honour synchronous mounts for writepage().  That would be
  * disastrous.  Any write() or metadata operation will sync the fs for
  * us.
- *
- * AKPM2: if all the page's buffers are mapped to disk and !data=journal,
- * we don't need to open a transaction here.
  */
 static int ext3_ordered_writepage(struct page *page,
 				struct writeback_control *wbc)
 {
 	struct inode *inode = page->mapping->host;
-	struct buffer_head *page_bufs;
 	handle_t *handle = NULL;
 	int ret = 0;
 	int err;

@@ -1487,46 +1462,49 @@ static int ext3_ordered_writepage(struct page *page,
 	if (ext3_journal_current_handle())
 		goto out_fail;
 
-	handle = ext3_journal_start(inode, ext3_writepage_trans_blocks(inode));
-
-	if (IS_ERR(handle)) {
-		ret = PTR_ERR(handle);
-		goto out_fail;
+	/*
+	 * Now there are two different reasons why we can be called:

Quoted 5 lines · Jan Kara

Below is my rewrite of ordered mode in JBD. Now we don't have a list of
data buffers that need syncing on transaction commit but a list of inodes that need writeout during commit. This brings all sorts of advantages such as possibility to get rid of journal heads and buffer heads for data buffers in ordered mode, better ordering of writes on transaction commit,

Quoted 8 lines

simplification of some JBD code, no more anonymous pages when truncate of data being committed happens. The patch has survived some light testing but it still has some potential of eating your data so beware :) I've run dbench to see whether we didn't decrease performance by different handling of truncate and the throughput I'm getting on my machine is the same (OK, is lower by 0.5%) if I disable the code in truncate waiting for commit to finish... Also the throughput of dbench is about 2% better with my patch than with current JBD.

I know ext4 is keep changing that it's a bit hard to create patch against ext4, but I feel features like especially rewrite the default ordered mode should done in ext4/jbd2. I could port to current ext4 and JBD2 if you agrees with this.

Also, would it make sense to create a new ordered mode writepage routines, and keep the old ordered mode code there for a while, to allow easy comparison? This could a good transition for people to start experiment this ordered mode without worrying about put data in danger by default.

Quoted 1 line

Any comments or testing most welcom

[...]

Quoted 30 lines

/*
* Note that we always start a transaction even if we're not journalling
* data. This is to preserve ordering: any hole instantiation within

fs/ext3/inode.c +2 -9

@@ -1465,15 +1444,11 @@ static int journal_dirty_data_fn(handle_t *handle, struct buffer_head *bh)
  * We don't honour synchronous mounts for writepage().  That would be
  * disastrous.  Any write() or metadata operation will sync the fs for
  * us.
- *
- * AKPM2: if all the page's buffers are mapped to disk and !data=journal,
- * we don't need to open a transaction here.
  */
 static int ext3_ordered_writepage(struct page *page,
 				struct writeback_control *wbc)
 {
 	struct inode *inode = page->mapping->host;
-	struct buffer_head *page_bufs;
 	handle_t *handle = NULL;
 	int ret = 0;
 	int err;

@@ -1487,46 +1462,49 @@ static int ext3_ordered_writepage(struct page *page,
 	if (ext3_journal_current_handle())
 		goto out_fail;
 
-	handle = ext3_journal_start(inode, ext3_writepage_trans_blocks(inode));
-
-	if (IS_ERR(handle)) {
-		ret = PTR_ERR(handle);
-		goto out_fail;
+	/*
+	 * Now there are two different reasons why we can be called:

fs/ext3/inode.c static int ext3_ordered_writepage(struct page *page, · line 1476

Quoted 10 lines

fs/ext3/inode.c +10 -0

+	 *   1) write out during commit
+	 *   2) fsync / writeout to free memory
+	 *
+	 * In the first case, we just need to write the buffer to disk, in the
+	 * second case we may need to do hole filling and attach the inode to
+	 * the transaction. Note that even in the first case, we may get an
+	 * unmapped buffer (hole fill with data via mmap) but we don't have to
+	 * write it - actually, we can't because from a transaction commit we
+	 * cannot start a new transaction or we could deadlock.
+	 */

Any thoughts how to handle the unmapped page under case 1)? Right now I see it fails. Your comments here saying that we still have the issue that "can't start a new transaction while commiting", but likely, with delayed allocation, starting a new transaction could to happen a lot to do defered block allocation.

I really hope this new mode could be easy to add delayed allocation support. Any thoughts that we could workaround the locking in the JBD2 layer?

fs/ext3/inode.c static int ext3_ordered_writepage(struct page *page, · line 1487

Quoted 11 lines

fs/ext3/inode.c +9 -0

+	if (!wbc->skip_unmapped) {
+		handle = ext3_journal_start(inode,
+				ext3_writepage_trans_blocks(inode));
+		if (IS_ERR(handle)) {
+			ret = PTR_ERR(handle);
+			goto out_fail;
+		}
 	}
+	else if (!PageMappedToDisk(page))
+		goto out_fail;
 

Below is my rewrite of ordered mode in JBD. Now we don't have a list of
data buffers that need syncing on transaction commit but a list of inodes that need writeout during commit. This brings all sorts of advantages such as possibility to get rid of journal heads and buffer heads for data buffers in ordered mode, better ordering of writes on transaction commit, simplification of some JBD code, no more anonymous pages when truncate of

data being committed happens. The patch has survived some light testing but it still has some potential of eating your data so beware :) I've run dbench to see whether we didn't decrease performance by different handling of truncate and the throughput I'm getting on my machine is the same (OK, is lower by 0.5%) if I disable the code in truncate waiting for commit to finish... Also the throughput of dbench is about 2% better with my patch than with current JBD.
Any comments or testing most welcome.

fs/jbd/commit.c +6 -0

+	spin_lock(&journal->j_list_lock);
+	list_for_each_entry(jinode, &commit_transaction->t_inode_list, i_list) {
+		mapping = jinode->i_vfs_inode->i_mapping;
+		if (!mapping_cap_writeback_dirty(mapping))
+			continue;
+		wbc.nr_to_write = mapping->nrpages * 2;

fs/jbd/transaction.c +14 -0

+/*
+ * This function must be called when inode is journaled in ordered mode
+ * before truncation happens. It starts writeout of truncated part in
+ * case it is in the committing transaction so that we stand to ordered
+ * mode consistency guarantees.
+ */
+int journal_begin_ordered_truncate(struct jbd_inode *inode, loff_t new_size)
+{
+	journal_t *journal;
+	transaction_t *commit_trans;
+	int ret = 0;
+
+	if (!inode->i_transaction && !inode->i_next_transaction)
+		goto out;

Quoted 6 lines · Jan Kara

Below is my rewrite of ordered mode in JBD. Now we don't have a list of
data buffers that need syncing on transaction commit but a list of inodes that need writeout during commit. This brings all sorts of advantages such as possibility to get rid of journal heads and buffer heads for data buffers in ordered mode, better ordering of writes on transaction commit, simplification of some JBD code, no more anonymous pages when truncate of

Quoted 8 lines

data being committed happens. The patch has survived some light testing but it still has some potential of eating your data so beware :) I've run dbench to see whether we didn't decrease performance by different handling of truncate and the throughput I'm getting on my machine is the same (OK, is lower by 0.5%) if I disable the code in truncate waiting for commit to finish... Also the throughput of dbench is about 2% better with my patch than with current JBD.
Any comments or testing most welcome.

Looks like a very good patch - thanks for your effort in moving this beyond the "hand-waving" stage that it's been in for the past few years.

I'm looking at what implications this has for delayed allocation in ext4, because the vast majority of file data will be unmapped in that case and a journal commit in ordered mode will no longer cause the data to be flushed to disk.

I _think_ is OK, because the pdflushd will now be totally in charge of flushing the dirty pages to disk, instead of this previously being done by ordered mode in the journal.

I know there have been some bugs in this
area in the past, but I guess it isn't much different than running in writeback mode. That said, I don't know how many users run in writeback mode unless they are running a database, and the database is doing a lot of explicit fsync of file data so there may still be bugs lurking...

Some care is still needed here because with ext4 delayed allocation the common case will be unmapped buffers, while the ext3 implementation will only have this in very rare cases (unmapped mmap writes), but it looks like the right mechanism is already in place for both.

I'll just put a bunch of comments inline, not necessarily problems, just walking through the code to ensure I understand what is going on...

fs/buffer.c static int __block_write_full_page(struct inode *inode, struct page *page, · line 1679

Quoted 7 lines

fs/buffer.c +2 -1

@@ -1675,7 +1675,8 @@ static int __block_write_full_page(struct inode *inode, struct page *page,
 			 */
 			clear_buffer_dirty(bh);
 			set_buffer_uptodate(bh);
-		} else if (!buffer_mapped(bh) && buffer_dirty(bh)) {
+		} else if (!buffer_mapped(bh) && buffer_dirty(bh)
+			   && !wbc->skip_unmapped) {

(comment) This is skipping writeout of unmapped pages during journal commit - good, otherwise we would deadlock trying to add block mappings...

fs/ext3/inode.c void ext3_delete_inode (struct inode * inode) · line 188

Quoted 6 lines

fs/ext3/inode.c +2 -0

@@ -183,6 +184,8 @@ void ext3_delete_inode (struct inode * inode)
 {
 	handle_t *handle;
 
+	if (ext3_should_order_data(inode))
+		ext3_begin_ordered_truncate(inode, 0);

(comment) This is flushing out pages allocated in the previous transaction to keep consistent semantics in case of a crash loses the delete... There is some possibility for optimization here - comments below at journal_begin_ordered_truncate().

fs/ext3/inode.c static int ext3_ordered_writepage(struct page *page, · line 1469

Quoted 9 lines

fs/ext3/inode.c +7 -0

@@ -1487,46 +1462,49 @@ static int ext3_ordered_writepage(struct page *page,
+	if (!wbc->skip_unmapped) {
+		handle = ext3_journal_start(inode,
+				ext3_writepage_trans_blocks(inode));
+		if (IS_ERR(handle)) {
+			ret = PTR_ERR(handle);
+			goto out_fail;
+		}
 	}

I guess the common case here for delalloc is that if someone cares to fsync the file then that inode would be added to the journal list and the pages would be mapped here in the process context and flushed with the journal commit. This is ideal because it avoids syncing out all of the dirty pages for inodes that were NOT fsync'd and fixes the "one process doing fsync kills performance for streaming writes" problem in ordered mode that was recently discussed on the list.

We in fact win twice because fsync_page_range() will potentially only map and flush the range of pages that the application cares about and does not necessarily have to write out all pages (though that will still happen in ordered mode if the pages had previously been mapped).

fs/ext3/inode.c static int ext3_ordered_writepage(struct page *page, · line 1486

Quoted 2 lines

fs/ext3/inode.c +2 -0

+	else if (!PageMappedToDisk(page))
+		goto out_fail;

(style) } else if (...) {
		goto out_fail;
	}

fs/ext3/inode.c static int ext3_ordered_writepage(struct page *page, · line 1492

Quoted 5 lines

fs/ext3/inode.c +1 -0

+	/* This can go as soon as someone cares about that ;) */
 	if (!page_has_buffers(page)) {
 		create_empty_buffers(page, inode->i_sb->s_blocksize,
 				(1 << BH_Dirty)|(1 << BH_Uptodate));
 	}

Is there any reason to keep this in the non-data-journal case? Adding buffer heads to every page is a non-trivial amount of RAM usage.

fs/ext3/inode.c int ext3_write_inode(struct inode *inode, int wait) · line 2976

Quoted 6 lines

fs/ext3/inode.c +1 -1

@@ -2989,7 +2973,14 @@ int ext3_write_inode(struct inode *inode, int wait)
  * be freed, so we have a strong guarantee that no future commit will
  * leave these blocks visible to the user.)
  *
- * Called with inode->sem down.
+ * Another thing we have to asure is that if we are in ordered mode

s/asure/assure/

fs/ext3/inode.c int ext3_write_inode(struct inode *inode, int wait) · line 2978

Quoted 2 lines

fs/ext3/inode.c +2 -0

+ * and inode is still attached to the committing transaction, we must
+ * we start writeout of all the dirty buffers which are being truncated.

s/buffers/pages/

fs/ext3/inode.c int ext3_setattr(struct dentry *dentry, struct iattr *attr) · line 3028

Quoted 7 lines

fs/ext3/inode.c +3 -0

@@ -3032,6 +3023,13 @@ int ext3_setattr(struct dentry *dentry, struct iattr *attr)
 	    attr->ia_valid & ATTR_SIZE && attr->ia_size < inode->i_size) {
 		handle_t *handle;
 
+		if (ext3_should_order_data(inode)) {
+			error = ext3_begin_ordered_truncate(inode,
+					attr->ia_size);

(style) align "attr->ia_size" with previous '('.

fs/ext3/super.c static void ext3_clear_inode(struct inode *inode) · line 524

Quoted 6 lines

fs/ext3/super.c +2 -0

@@ -520,6 +520,8 @@ static void ext3_clear_inode(struct inode *inode)
 	EXT3_I(inode)->i_block_alloc_info = NULL;
 	if (unlikely(rsv))
 		kfree(rsv);
+	journal_release_jbd_inode(EXT3_SB(inode->i_sb)->s_journal,
+		&EXT3_I(inode)->jinode);

(style) align with '(' on previous line.

fs/jbd/commit.c static void journal_end_buffer_io_sync(struct buffer_head *bh, int uptodate) · line 41

Quoted 5 lines

fs/jbd/commit.c +2 -2

 /*
- * When an ext3-ordered file is truncated, it is possible that many pages are
- * not sucessfully freed, because they are attached to a committing transaction.
+ * When an ext3 file is truncated, it is possible that some pages are not
+ * sucessfully freed, because they are attached to a committing transaction.

s/sucessfully/successfully/

fs/jbd/commit.c nope: · line 93

Quoted 9 lines

fs/jbd/commit.c +0 -9

-static int inverted_lock(journal_t *journal, struct buffer_head *bh)
-{
-	if (!jbd_trylock_bh_state(bh)) {
-		spin_unlock(&journal->j_list_lock);
-		schedule();
-		return 0;
-	}
-	return 1;
-}

Always nice to see unpleasant code like this be removed.

fs/jbd/commit.c static int journal_write_commit_record(journal_t *journal, · line 170

Quoted 9 lines

fs/jbd/commit.c +8 -0

+static int journal_submit_data_buffers(journal_t *journal,
+		transaction_t *commit_transaction)
 {
+	/*
+	 * We are in a committing transaction. Therefore no new inode
+	 * can be added to our inode list. We use JI_COMMIT_RUNNING
+	 * flag to protect inode we currently operate on from being
+	 * released while we write out pages.
+	 */

Should we J_ASSERT() this is true? Probably better to put this comment before the function instead of inside it.

Quoted 6 lines

fs/jbd/commit.c +6 -0

+	spin_lock(&journal->j_list_lock);
+	list_for_each_entry(jinode, &commit_transaction->t_inode_list, i_list) {
+		mapping = jinode->i_vfs_inode->i_mapping;
+		if (!mapping_cap_writeback_dirty(mapping))
+			continue;
+		wbc.nr_to_write = mapping->nrpages * 2;

fs/jbd/commit.c static int journal_write_commit_record(journal_t *journal, · line 186

Quoted 10 lines

fs/jbd/commit.c +9 -0

+		jinode->i_flags |= JI_COMMIT_RUNNING;
+		spin_unlock(&journal->j_list_lock);
+		err = do_writepages(jinode->i_vfs_inode->i_mapping, &wbc);
+		if (!ret)
+			ret = err;
+		spin_lock(&journal->j_list_lock);
+		jinode->i_flags &= ~JI_COMMIT_RUNNING;
+		wake_up_bit(&jinode->i_flags, __JI_COMMIT_RUNNING);
 	}
+	spin_unlock(&journal->j_list_lock);

Hmm, this is one area I'm a bit worried about. In the old code the number of buffers to sync for a transaction are bounded because they can only be added to the transaction while it is open. With this new inode-list code there could be a process busily adding new dirty + mapped pages to the inode(s) in the running transaction, while we are blocked here writing out the pages in kjournald trying to commit the previous transaction.

At some point we will eventually no longer be able to add new pages to the running transaction (because it is full or was closed due to timeout) and we won't be able to start a new transaction because the committing transaction has not yet committed. At that point we would be able to finish the commit of the previous transaction, but not until we had reached the point of blocking all operations in the filesystem waiting for the new transaction to open. This would essentially lead to livelock of the system, with stop-start-stop cycles for the transactions and IO.

What would be needed is some kind of mechanism to separate the dirty pages on the inode into "dirty from current transaction" and "dirty from the previous transaction". That seems non-trivial, however. I guess we could also force any process doing writing to flush out all of the dirty and mapped pages on the inode if it detects the inode is in two transactions.

This would at least parallelize the IO submission and also throttle the addition of new pages until the dirty ones on the committing transaction had been flushed but may essentially mean sync IO performance for streaming writes on large files without delalloc (which would not add new pages that are mapped normally).

fs/jbd/commit.c static int journal_write_commit_record(journal_t *journal,

Quoted 6 lines

fs/jbd/commit.c +5 -0

+static int journal_finish_data_buffers(journal_t *journal,
+		transaction_t *commit_transaction)
 {
+	/* Now refile inode to proper lists */
+	list_for_each_entry_safe(jinode, next_i,
+			&commit_transaction->t_inode_list, i_list) {

(style) align with '(' on previous line

fs/jbd/commit.c static int journal_write_commit_record(journal_t *journal, · line 225

Quoted 9 lines

fs/jbd/commit.c +8 -0

+		list_del(&jinode->i_list);
+		if (jinode->i_next_transaction) {
+			jinode->i_transaction = jinode->i_next_transaction;
+			jinode->i_next_transaction = NULL;
+			list_add(&jinode->i_list,
+				&jinode->i_transaction->t_inode_list);
 		}
+		else
+			jinode->i_transaction = NULL;

(style)		} else {
			jinode->i_transaction = NULL;
		}

fs/jbd/commit.c start_journal_io: · line 568

Quoted 6 lines

fs/jbd/commit.c +1 -1

@@ -655,7 +565,14 @@ start_journal_io:
 	   so we incur less scheduling load.
 	*/
 
-	jbd_debug(3, "JBD: commit phase 4\n");
+	jbd_debug(3, "JBD: commit phase 3\n");

Rather than numbering these phases, which has little meaning and often means they just get renumbered like here, it is probably more useful to give them descriptive names like "JBD: commit wait for data buffers", etc.

fs/jbd/transaction.c __blist_del_buffer(struct journal_head **list, struct journal_head *jh) · line 1332

Quoted 11 lines

fs/jbd/transaction.c +3 -4

@@ -1504,10 +1327,10 @@ __blist_del_buffer(struct journal_head **list, struct journal_head *jh)
  * Remove a buffer from the appropriate transaction list.
  *
  * Note that this function can *change* the value of
- * bh->b_transaction->t_sync_datalist, t_buffers, t_forget,
- * t_iobuf_list, t_shadow_list, t_log_list or t_reserved_list.  If the caller
- * is holding onto a copy of one of thee pointers, it could go bad.
- * Generally the caller needs to re-read the pointer from the transaction_t.
+ * bh->b_transaction->t_buffers, t_forget, t_iobuf_list, t_shadow_list,
+ * t_log_list or t_reserved_list.  If the caller is holding onto a copy of one
+ * of thee pointers, it could go bad.  Generally the caller needs to re-read

s/thee/these/

fs/jbd/transaction.c void journal_refile_buffer(journal_t *journal, struct journal_head *jh) · line 1912

Quoted 10 lines

fs/jbd/transaction.c +10 -0

+/*
+ * File inode in the inode list of the handle's transaction
+ */
+int journal_file_inode(handle_t *handle, struct jbd_inode *jinode)
+{
+	transaction_t *transaction = handle->h_transaction;
+	journal_t *journal = transaction->t_journal;
+
+	if (is_handle_aborted(handle))
+		return 0;

Should we be returning an error back to the caller at this point?

fs/jbd/transaction.c void journal_refile_buffer(journal_t *journal, struct journal_head *jh) · line 1921

Quoted 8 lines

fs/jbd/transaction.c +8 -0

+	jbd_debug(4, "Adding inode %lu, tid:%d\n", jinode->i_vfs_inode->i_ino,
+			transaction->t_tid);
+
+	spin_lock(&journal->j_list_lock);
+
+	if (jinode->i_transaction == transaction ||
+	    jinode->i_next_transaction == transaction)
+		goto done;

Is it possible/safe to do the above check outside of the j_list_lock as an optimization? We will be calling this function for every page that is dirtied and it will likely be quite highly contended. It used to be that we HAD to get this lock because we were almost certainly adding the new buffers to the transaction list, but in this updated code the common case is that the inode will already be on the list...

Quoted 14 lines

fs/jbd/transaction.c +14 -0

+/*
+ * This function must be called when inode is journaled in ordered mode
+ * before truncation happens. It starts writeout of truncated part in
+ * case it is in the committing transaction so that we stand to ordered
+ * mode consistency guarantees.
+ */
+int journal_begin_ordered_truncate(struct jbd_inode *inode, loff_t new_size)
+{
+	journal_t *journal;
+	transaction_t *commit_trans;
+	int ret = 0;
+
+	if (!inode->i_transaction && !inode->i_next_transaction)
+		goto out;

fs/jbd/transaction.c void journal_refile_buffer(journal_t *journal, struct journal_head *jh) · line 1965

Quoted 10 lines

fs/jbd/transaction.c +10 -0

+	journal = inode->i_transaction->t_journal;
+	spin_lock(&journal->j_state_lock);
+	commit_trans = journal->j_committing_transaction;
+	spin_unlock(&journal->j_state_lock);
+	if (inode->i_transaction == commit_trans) {
+		ret = __filemap_fdatawrite_range(inode->i_vfs_inode->i_mapping,
+			new_size, LLONG_MAX, WB_SYNC_ALL);
+		if (ret)
+			journal_abort(journal, ret);
+	}

Is there any way here to entirely avoid writing blocks which were just allocated during the current transaction (e.g. temp files during compile or whatever)? One possibility is to store the transaction number when the inode was created (or first dirtied?) in ext3_inode_info and if that is equal to the committing transaction then we don't need to write anything at all?

Cheers, Andreas
--
Andreas Dilger
Sr. Staff Engineer, Lustre Group
Sun Microsystems of Canada, Inc.

--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

I'm looking at what implications this has for delayed allocation in ext4, because the vast majority of file data will be unmapped in that case and a journal commit in ordered mode will no longer cause the data to be flushed to disk.

Quoted 6 lines · Jan Kara

Below is my rewrite of ordered mode in JBD. Now we don't have a list of
data buffers that need syncing on transaction commit but a list of inodes that need writeout during commit. This brings all sorts of advantages such as possibility to get rid of journal heads and buffer heads for data buffers in ordered mode, better ordering of writes on transaction commit, simplification of some JBD code, no more anonymous pages when truncate of

Quoted 8 lines

data being committed happens. The patch has survived some light testing but it still has some potential of eating your data so beware :) I've run dbench to see whether we didn't decrease performance by different handling of truncate and the throughput I'm getting on my machine is the same (OK, is lower by 0.5%) if I disable the code in truncate waiting for commit to finish... Also the throughput of dbench is about 2% better with my patch than with current JBD.
Any comments or testing most welcome.

Looks like a very good patch - thanks for your effort in moving this beyond the "hand-waving" stage that it's been in for the past few years.

I'm looking at what implications this has for delayed allocation in ext4, because the vast majority of file data will be unmapped in that case and a journal commit in ordered mode will no longer cause the data to be flushed to disk.

I _think_ is OK, because the pdflushd will now be totally in charge of flushing the dirty pages to disk, instead of this previously being done by ordered mode in the journal. I know there have been some bugs in this area in the past, but I guess it isn't much different than running in writeback mode. That said, I don't know how many users run in writeback mode unless they are running a database, and the database is doing a lot of explicit fsync of file data so there may still be bugs lurking...

Some care is still needed here because with ext4 delayed allocation the common case will be unmapped buffers, while the ext3 implementation will only have this in very rare cases (unmapped mmap writes), but it looks like the right mechanism is already in place for both.

I'll just put a bunch of comments inline, not necessarily problems, just walking through the code to ensure I understand what is going on...

fs/buffer.c static int __block_write_full_page(struct inode *inode, struct page *page, · line 1679

Quoted 7 lines

fs/buffer.c +2 -1

@@ -1675,7 +1675,8 @@ static int __block_write_full_page(struct inode *inode, struct page *page,
 			 */
 			clear_buffer_dirty(bh);
 			set_buffer_uptodate(bh);
-		} else if (!buffer_mapped(bh) && buffer_dirty(bh)) {
+		} else if (!buffer_mapped(bh) && buffer_dirty(bh)
+			   && !wbc->skip_unmapped) {

(comment) This is skipping writeout of unmapped pages during journal commit - good, otherwise we would deadlock trying to add block mappings...

fs/ext3/inode.c void ext3_delete_inode (struct inode * inode) · line 188

Quoted 6 lines

fs/ext3/inode.c +2 -0

@@ -183,6 +184,8 @@ void ext3_delete_inode (struct inode * inode)
 {
 	handle_t *handle;
 
+	if (ext3_should_order_data(inode))
+		ext3_begin_ordered_truncate(inode, 0);

(comment) This is flushing out pages allocated in the previous transaction to keep consistent semantics in case of a crash loses the delete... There is some possibility for optimization here - comments below at journal_begin_ordered_truncate().

fs/ext3/inode.c static int ext3_ordered_writepage(struct page *page, · line 1469

Quoted 9 lines

fs/ext3/inode.c +7 -0

@@ -1487,46 +1462,49 @@ static int ext3_ordered_writepage(struct page *page,
+	if (!wbc->skip_unmapped) {
+		handle = ext3_journal_start(inode,
+				ext3_writepage_trans_blocks(inode));
+		if (IS_ERR(handle)) {
+			ret = PTR_ERR(handle);
+			goto out_fail;
+		}
 	}

I guess the common case here for delalloc is that if someone cares to fsync the file then that inode would be added to the journal list and the pages would be mapped here in the process context and flushed with the journal commit. This is ideal because it avoids syncing out all of the dirty pages for inodes that were NOT fsync'd and fixes the "one process doing fsync kills performance for streaming writes" problem in ordered mode that was recently discussed on the list.

We in fact win twice because fsync_page_range() will potentially only map and flush the range of pages that the application cares about and does not necessarily have to write out all pages (though that will still happen in ordered mode if the pages had previously been mapped).

fs/ext3/inode.c static int ext3_ordered_writepage(struct page *page, · line 1486

Quoted 2 lines

fs/ext3/inode.c +2 -0

+	else if (!PageMappedToDisk(page))
+		goto out_fail;

(style) } else if (...) {
		goto out_fail;
	}

fs/ext3/inode.c static int ext3_ordered_writepage(struct page *page, · line 1492

Quoted 5 lines

fs/ext3/inode.c +1 -0

+	/* This can go as soon as someone cares about that ;) */
 	if (!page_has_buffers(page)) {
 		create_empty_buffers(page, inode->i_sb->s_blocksize,
 				(1 << BH_Dirty)|(1 << BH_Uptodate));
 	}

Is there any reason to keep this in the non-data-journal case? Adding buffer heads to every page is a non-trivial amount of RAM usage.

Quoted 4 lines

fs/jbd/commit.c +3 -0

+static int journal_submit_data_buffers(journal_t *journal,
+		transaction_t *commit_transaction)
 {
+	/*

fs/jbd/commit.c static int journal_write_commit_record(journal_t *journal, · line 170

Quoted 5 lines

fs/jbd/commit.c +5 -0

+	 * We are in a committing transaction. Therefore no new inode
+	 * can be added to our inode list. We use JI_COMMIT_RUNNING
+	 * flag to protect inode we currently operate on from being
+	 * released while we write out pages.
+	 */

Should we J_ASSERT() this is true? Probably better to put this comment before the function instead of inside it.

Quoted 15 lines

fs/jbd/commit.c +14 -0

+	spin_lock(&journal->j_list_lock);
+	list_for_each_entry(jinode, &commit_transaction->t_inode_list, i_list) {
+		mapping = jinode->i_vfs_inode->i_mapping;
+		if (!mapping_cap_writeback_dirty(mapping))
+			continue;
+		wbc.nr_to_write = mapping->nrpages * 2;
+		jinode->i_flags |= JI_COMMIT_RUNNING;
+		spin_unlock(&journal->j_list_lock);
+		err = do_writepages(jinode->i_vfs_inode->i_mapping, &wbc);
+		if (!ret)
+			ret = err;
+		spin_lock(&journal->j_list_lock);
+		jinode->i_flags &= ~JI_COMMIT_RUNNING;
+		wake_up_bit(&jinode->i_flags, __JI_COMMIT_RUNNING);
 	}

Review static int ext3_ordered_writepage(struct page *page, · line 1501

Quoted 1 line

(quoted diff) +1 -0

+	spin_unlock(&journal->j_list_lock);

Hmm, this is one area I'm a bit worried about. In the old code the number of buffers to sync for a transaction are bounded because they can only be added to the transaction while it is open. With this new inode-list code there could be a process busily adding new dirty + mapped pages to the inode(s) in the running transaction, while we are blocked here writing out the pages in kjournald trying to commit the previous transaction.

At some point we will eventually no longer be able to add new pages to the running transaction (because it is full or was closed due to timeout) and we won't be able to start a new transaction because the committing transaction has not yet committed. At that point we would be able to finish the commit of the previous transaction, but not until we had reached the point of blocking all operations in the filesystem waiting for the new transaction to open. This would essentially lead to livelock of the system, with stop-start-stop cycles for the transactions and IO.

What would be needed is some kind of mechanism to separate the dirty pages on the inode into "dirty from current transaction" and "dirty from the previous transaction". That seems non-trivial, however. I guess we could also force any process doing writing to flush out all of the dirty and mapped pages on the inode if it detects the inode is in two transactions.

This would at least parallelize the IO submission and also throttle the addition of new pages until the dirty ones on the committing transaction had been flushed but may essentially mean sync IO performance for streaming writes on large files without delalloc (which would not add new pages that are mapped normally).

Quoted 4 lines

fs/jbd/commit.c +0 -0

@@ -655,7 +565,14 @@ start_journal_io:
 	   so we incur less scheduling load.
 	*/
 

fs/jbd/commit.c start_journal_io: · line 568

Quoted 2 lines

fs/jbd/commit.c +1 -1

-	jbd_debug(3, "JBD: commit phase 4\n");
+	jbd_debug(3, "JBD: commit phase 3\n");

Rather than numbering these phases, which has little meaning and often means they just get renumbered like here, it is probably more useful to give them descriptive names like "JBD: commit wait for data buffers", etc.

Quoted 4 lines

fs/jbd/transaction.c +4 -0

+/*
+ * File inode in the inode list of the handle's transaction
+ */
+int journal_file_inode(handle_t *handle, struct jbd_inode *jinode)

fs/jbd/transaction.c void journal_refile_buffer(journal_t *journal, struct journal_head *jh) · line 1912

Quoted 6 lines

fs/jbd/transaction.c +6 -0

+{
+	transaction_t *transaction = handle->h_transaction;
+	journal_t *journal = transaction->t_journal;
+
+	if (is_handle_aborted(handle))
+		return 0;

Should we be returning an error back to the caller at this point?

Quoted 7 lines

fs/jbd/transaction.c +7 -0

+	jbd_debug(4, "Adding inode %lu, tid:%d\n", jinode->i_vfs_inode->i_ino,
+			transaction->t_tid);
+
+	spin_lock(&journal->j_list_lock);
+
+	if (jinode->i_transaction == transaction ||
+	    jinode->i_next_transaction == transaction)

Review

Quoted 1 line

(quoted diff) +1 -0

+		goto done;

Is it possible/safe to do the above check outside of the j_list_lock as an optimization? We will be calling this function for every page that is dirtied and it will likely be quite highly contended. It used to be that we HAD to get this lock because we were almost certainly adding the new buffers to the transaction list, but in this updated code the common case is that the inode will already be on the list...

Quoted 23 lines

fs/jbd/transaction.c +23 -0

+/*
+ * This function must be called when inode is journaled in ordered mode
+ * before truncation happens. It starts writeout of truncated part in
+ * case it is in the committing transaction so that we stand to ordered
+ * mode consistency guarantees.
+ */
+int journal_begin_ordered_truncate(struct jbd_inode *inode, loff_t new_size)
+{
+	journal_t *journal;
+	transaction_t *commit_trans;
+	int ret = 0;
+
+	if (!inode->i_transaction && !inode->i_next_transaction)
+		goto out;
+	journal = inode->i_transaction->t_journal;
+	spin_lock(&journal->j_state_lock);
+	commit_trans = journal->j_committing_transaction;
+	spin_unlock(&journal->j_state_lock);
+	if (inode->i_transaction == commit_trans) {
+		ret = __filemap_fdatawrite_range(inode->i_vfs_inode->i_mapping,
+			new_size, LLONG_MAX, WB_SYNC_ALL);
+		if (ret)
+			journal_abort(journal, ret);

Review

Quoted 1 line

(quoted diff) +1 -0

+	}

Is there any way here to entirely avoid writing blocks which were just allocated during the current transaction (e.g. temp files during compile or whatever)? One possibility is to store the transaction number when the inode was created (or first dirtied?) in ext3_inode_info and if that is equal to the committing transaction then we don't need to write anything at all?

Quoted 5 lines · Andreas Dilger

I'm looking at what implications this has for delayed allocation in ext4, because the vast majority of file data will be unmapped in that case and a journal commit in ordered mode will no longer cause the data to be flushed to disk.

Quoted 7 lines

I _think_ is OK, because the pdflushd will now be totally in charge of flushing the dirty pages to disk, instead of this previously being done by ordered mode in the journal. I know there have been some bugs in this area in the past, but I guess it isn't much different than running in writeback mode. That said, I don't know how many users run in writeback mode unless they are running a database, and the database is doing a lot of explicit fsync of file data so there may still be bugs lurking...

Yes, they basically get guarantees as in writeback mode. As I wrote to
Mingming, I'm not sure how that is handled with current ordered-mode handling because you cannot afford to do block allocation from the commit code anyway. We could possibly do the allocation if we were sure that we have enough credits in the transaction (but that currently isn't the case since we really count the number of buffers dirtied on the account of transaction handle and after the handle is dropped, we give back unused credits to the transaction).

Quoted 6 lines

Hmm, this is one area I'm a bit worried about. In the old code the number of buffers to sync for a transaction are bounded because they can only be added to the transaction while it is open. With this new inode-list code there could be a process busily adding new dirty + mapped pages to the inode(s) in the running transaction, while we are blocked here writing out the pages in kjournald trying to commit the previous transaction.

Correct me if I'm wrong but as far as I look into the write-out code, we
do just one-sweep scan of mapping when writing out dirty pages. So we could possibly write out more than we need but at most the whole file. Oh, ok, someone could be extending the file as well but we could fix that by setting .range_end to the current i_size - that's actually a nice optimization anyway so I've done that.

Quoted 4 lines

Quoted 4 lines

fs/jbd/transaction.c +4 -0

+	spin_lock(&journal->j_list_lock);
+
+	if (jinode->i_transaction == transaction ||
+	    jinode->i_next_transaction == transaction)

Quoted 8 lines

Review

Quoted 1 line

(quoted diff) +1 -0

+		goto done;

Is it possible/safe to do the above check outside of the j_list_lock as an optimization? We will be calling this function for every page that is dirtied and it will likely be quite highly contended. It used to be that we HAD to get this lock because we were almost certainly adding the new buffers to the transaction list, but in this updated code the common case is that the inode will already be on the list...

Probably we could do that - there are two places where we remove inode
from a transaction list (and these are the only ones interesting for races). One is the commit code and the other one is journal_release_jbd_inode(). We are safe from the second one because we obviously hold a reference to the inode. The first one is more subtle but if jinode->i_transaction == transaction, it is filed on the running transaction's list from which we have handle and so commit code cannot touch it. If jinode->i_next_transaction == transaction, then commit code can change inode under us but it will file the inode to the running transaction's list anyway so there's nothing to lose.
Changed the code and added a big comment...

Quoted 18 lines

Quoted 18 lines

fs/jbd/transaction.c +18 -0

+int journal_begin_ordered_truncate(struct jbd_inode *inode, loff_t new_size)
+{
+	journal_t *journal;
+	transaction_t *commit_trans;
+	int ret = 0;
+
+	if (!inode->i_transaction && !inode->i_next_transaction)
+		goto out;
+
+	journal = inode->i_transaction->t_journal;
+	spin_lock(&journal->j_state_lock);
+	commit_trans = journal->j_committing_transaction;
+	spin_unlock(&journal->j_state_lock);
+	if (inode->i_transaction == commit_trans) {
+		ret = __filemap_fdatawrite_range(inode->i_vfs_inode->i_mapping,
+			new_size, LLONG_MAX, WB_SYNC_ALL);
+		if (ret)
+			journal_abort(journal, ret);

Quoted 8 lines

Review

Quoted 1 line

(quoted diff) +1 -0

+	}

Is there any way here to entirely avoid writing blocks which were just allocated during the current transaction (e.g. temp files during compile or whatever)? One possibility is to store the transaction number when the inode was created (or first dirtied?) in ext3_inode_info and if that is equal to the committing transaction then we don't need to write anything at all?

Note that we write out blocks only if we truncate inode that is being
currently committed. I.e., it has been created in the previous transaction and is truncated in the current one. Or did you mean something else?

Hi,

While looking at a bug related to direct IO returns to EIO, after looking at the code, I found there is a window that try_to_free_buffers() from direct IO could race with JBD, which holds the reference to the data buffers before journal_commit_transaction() ensures the data buffers has reached to the disk.

A little more detail: to prepare for direct IO, generic_file_direct_IO() calls invalidate_inode_pages2_range() to invalidate the pages in the cache before performaning direct IO. invalidate_inode_pages2_range() tries to free the buffers via try_to free_buffers(), but sometimes it can't, due to the buffers is possible still on some transaction's t_sync_datalist or t_locked_list waiting for journal_commit_transaction() to process it.

Currently Direct IO simply returns EIO if try_to_free_buffers() finds the buffer is busy, as it has no clue that JBD is referencing it.

Is this a known issue and expected behavior? Any thoughts?

While looking at a bug related to direct IO returns to EIO, after looking at the code, I found there is a window that try_to_free_buffers() from direct IO could race with JBD, which holds the reference to the data buffers before journal_commit_transaction() ensures the data buffers has reached to the disk.

A little more detail: to prepare for direct IO, generic_file_direct_IO() calls invalidate_inode_pages2_range() to invalidate the pages in the cache before performaning direct IO. invalidate_inode_pages2_range() tries to free the buffers via try_to free_buffers(), but sometimes it can't, due to the buffers is possible still on some transaction's t_sync_datalist or t_locked_list waiting for journal_commit_transaction() to process it.

Currently Direct IO simply returns EIO if try_to_free_buffers() finds the buffer is busy, as it has no clue that JBD is referencing it.

Is this a known issue and expected behavior? Any thoughts?

Hi,

Quoted 14 lines · Mingming Cao

While looking at a bug related to direct IO returns to EIO, after looking at the code, I found there is a window that try_to_free_buffers() from direct IO could race with JBD, which holds the reference to the data buffers before journal_commit_transaction() ensures the data buffers has reached to the disk.

A little more detail: to prepare for direct IO, generic_file_direct_IO() calls invalidate_inode_pages2_range() to invalidate the pages in the cache before performaning direct IO. invalidate_inode_pages2_range() tries to free the buffers via try_to free_buffers(), but sometimes it can't, due to the buffers is possible still on some transaction's t_sync_datalist or t_locked_list waiting for journal_commit_transaction() to process it.

Quoted 4 lines

Currently Direct IO simply returns EIO if try_to_free_buffers() finds the buffer is busy, as it has no clue that JBD is referencing it.

Is this a known issue and expected behavior? Any thoughts?

Are you seeing this in data=ordered mode? As Andrew pointed out we do
filemap_write_and_wait() so all the relevant data buffers of the inode should be already on disk. In __journal_try_to_free_buffer() we check whether the buffer is already-written-out data buffer and unfile and free it in that case. It shouldn't happen that a data buffer has b_next_transaction set so really the only idea why try_to_free_buffers() could fail is that somebody manages to write to a page via mmap before invalidate_inode_pages2_range() gets to it. Under which kind of load do you observe the problem? Do you know exactly because of which condition does journal_try_to_free_buffers() fail?

Quoted 21 lines · Jan Kara

Hi,

Quoted 14 lines · Mingming Cao

While looking at a bug related to direct IO returns to EIO, after looking at the code, I found there is a window that try_to_free_buffers() from direct IO could race with JBD, which holds the reference to the data buffers before journal_commit_transaction() ensures the data buffers has reached to the disk.

A little more detail: to prepare for direct IO, generic_file_direct_IO() calls invalidate_inode_pages2_range() to invalidate the pages in the cache before performaning direct IO. invalidate_inode_pages2_range() tries to free the buffers via try_to free_buffers(), but sometimes it can't, due to the buffers is possible still on some transaction's t_sync_datalist or t_locked_list waiting for journal_commit_transaction() to process it.

Quoted 4 lines

Currently Direct IO simply returns EIO if try_to_free_buffers() finds the buffer is busy, as it has no clue that JBD is referencing it.

Is this a known issue and expected behavior? Any thoughts?

Quoted 11 lines

Are you seeing this in data=ordered mode? As Andrew pointed out we do
filemap_write_and_wait() so all the relevant data buffers of the inode should be already on disk. In __journal_try_to_free_buffer() we check whether the buffer is already-written-out data buffer and unfile and free it in that case. It shouldn't happen that a data buffer has b_next_transaction set so really the only idea why try_to_free_buffers() could fail is that somebody manages to write to a page via mmap before invalidate_inode_pages2_range() gets to it. Under which kind of load do you observe the problem? Do you know exactly because of which condition does journal_try_to_free_buffers() fail?

Thank you for your reply.

What we are noticing is invalidate_inode_pages2_range() fails with -EIO (from try_to_free_buffers() since b_count > 0).

I don't think the file is being updated through mmap(). Previous writepage() added these buffers to t_sync_data list (data=ordered). filemap_write_and_wait() waits for pagewrite back to be cleared. So, buffers are no longer dirty, but still on the t_sync_data and kjournald didn't get chance to process them yet :(

Since we have elevated b_count on these buffers, try_to_free_buffers() fails. How can we make filemap_write_and_wait() to wait for kjournald to unfile these buffers ?

Quoted 46 lines · Badari Pulavarty

Quoted 21 lines · Jan Kara

Hi,

Quoted 14 lines · Mingming Cao

While looking at a bug related to direct IO returns to EIO, after looking at the code, I found there is a window that try_to_free_buffers() from direct IO could race with JBD, which holds the reference to the data buffers before journal_commit_transaction() ensures the data buffers has reached to the disk.

A little more detail: to prepare for direct IO, generic_file_direct_IO() calls invalidate_inode_pages2_range() to invalidate the pages in the cache before performaning direct IO. invalidate_inode_pages2_range() tries to free the buffers via try_to free_buffers(), but sometimes it can't, due to the buffers is possible still on some transaction's t_sync_datalist or t_locked_list waiting for journal_commit_transaction() to process it.

Quoted 4 lines

Currently Direct IO simply returns EIO if try_to_free_buffers() finds the buffer is busy, as it has no clue that JBD is referencing it.

Is this a known issue and expected behavior? Any thoughts?

Quoted 11 lines

Are you seeing this in data=ordered mode? As Andrew pointed out we do
filemap_write_and_wait() so all the relevant data buffers of the inode should be already on disk. In __journal_try_to_free_buffer() we check whether the buffer is already-written-out data buffer and unfile and free it in that case. It shouldn't happen that a data buffer has b_next_transaction set so really the only idea why try_to_free_buffers() could fail is that somebody manages to write to a page via mmap before invalidate_inode_pages2_range() gets to it. Under which kind of load do you observe the problem? Do you know exactly because of which condition does journal_try_to_free_buffers() fail?

Thank you for your reply.

What we are noticing is invalidate_inode_pages2_range() fails with -EIO (from try_to_free_buffers() since b_count > 0).

I don't think the file is being updated through mmap(). Previous writepage() added these buffers to t_sync_data list (data=ordered). filemap_write_and_wait() waits for pagewrite back to be cleared. So, buffers are no longer dirty, but still on the t_sync_data and kjournald didn't get chance to process them yet :(

Quoted 3 lines

Since we have elevated b_count on these buffers, try_to_free_buffers() fails. How can we make filemap_write_and_wait() to wait for kjournald to unfile these buffers ?

Hmm, I don't get one thing:
The call chain is invalidate_inode_pages2_range() ->
invalidate_complete_page2() -> try_to_release_page() -> ext3_releasepage() -> journal_try_to_free_buffers() -> __journal_try_to_free_buffer() and this function should remove the buffer from the committing transaction.

So who's
holding the reference to those buffers?

Or is it that
__journal_try_to_free_buffer() fails to remove the buffer from the committing transaction? Why?
Hmm, maybe I have one idea - in theory we could call
__journal_try_to_free_buffer() exactly at the moment commit code inspects the buffer. Then we'd release the buffer from the transaction but try_to_free_buffers() would fail because of elevated b_count exactly as you described. Could you maybe verify this? Not that I'd know how to easily fix this ;)...

Honza

Quoted 47 lines · Jan Kara

Quoted 46 lines · Badari Pulavarty

Quoted 21 lines · Jan Kara

Hi,

Quoted 14 lines · Mingming Cao

While looking at a bug related to direct IO returns to EIO, after looking at the code, I found there is a window that try_to_free_buffers() from direct IO could race with JBD, which holds the reference to the data buffers before journal_commit_transaction() ensures the data buffers has reached to the disk.

A little more detail: to prepare for direct IO, generic_file_direct_IO() calls invalidate_inode_pages2_range() to invalidate the pages in the cache before performaning direct IO. invalidate_inode_pages2_range() tries to free the buffers via try_to free_buffers(), but sometimes it can't, due to the buffers is possible still on some transaction's t_sync_datalist or t_locked_list waiting for journal_commit_transaction() to process it.

Quoted 4 lines

Currently Direct IO simply returns EIO if try_to_free_buffers() finds the buffer is busy, as it has no clue that JBD is referencing it.

Is this a known issue and expected behavior? Any thoughts?

Quoted 11 lines

Are you seeing this in data=ordered mode? As Andrew pointed out we do
filemap_write_and_wait() so all the relevant data buffers of the inode should be already on disk. In __journal_try_to_free_buffer() we check whether the buffer is already-written-out data buffer and unfile and free it in that case. It shouldn't happen that a data buffer has b_next_transaction set so really the only idea why try_to_free_buffers() could fail is that somebody manages to write to a page via mmap before invalidate_inode_pages2_range() gets to it. Under which kind of load do you observe the problem? Do you know exactly because of which condition does journal_try_to_free_buffers() fail?

Thank you for your reply.

What we are noticing is invalidate_inode_pages2_range() fails with -EIO (from try_to_free_buffers() since b_count > 0).

I don't think the file is being updated through mmap(). Previous writepage() added these buffers to t_sync_data list (data=ordered). filemap_write_and_wait() waits for pagewrite back to be cleared. So, buffers are no longer dirty, but still on the t_sync_data and kjournald didn't get chance to process them yet :(

Quoted 8 lines

Quoted 3 lines

Since we have elevated b_count on these buffers, try_to_free_buffers() fails. How can we make filemap_write_and_wait() to wait for kjournald to unfile these buffers ?

Hmm, I don't get one thing:
The call chain is invalidate_inode_pages2_range() ->
invalidate_complete_page2() -> try_to_release_page() -> ext3_releasepage() -> journal_try_to_free_buffers() -> __journal_try_to_free_buffer() and this function should remove the buffer from the committing transaction.

Thanks, yes I noticed that after you pointing this out.

But __journal_try_to_free_buffer() only unfile the buffer from t_sync_datalist or t_locked list, the journal head is not removed in journal_remove_journal_head() there, at that time, journal_remove_journal_head() just check if counter b_jcount is 0. But before calling __journal_try_to_free_buffer(), since journal_try_to_free_buffers() already increase the b_jcount in journal_grab_journal_head(), so the journal head is not removed in __journal_try_to_free_buffer-> journal_remove_journal_head()

Quoted 2 lines

So who's
holding the reference to those buffers?

Looking at the code, it seems the it's the journal_put_journal_head(jh) who remove the journal head and decrease the bh

journal_try_to_free_buffers()
{
...

jh = journal_grab_journal_head(bh);
                if (!jh)
                        continue;

                jbd_lock_bh_state(bh);
                __journal_try_to_free_buffer(journal, bh);
                journal_put_journal_head(jh);
                jbd_unlock_bh_state(bh);

...

}
so when journal_put_journal_head()-> __journal_remove_journal_head(),
now the b_jcount is zero, but is
jh->b_transaction is NULL? So it seems possible that bh ref count is non zero when exit from journal_put_journal_head() if jh_b_transaction is not cleared.

I miss where jh->b_transaction is clear to NULL?

Quoted 6 lines

Hmm, maybe I have one idea - in theory we could call
__journal_try_to_free_buffer() exactly at the moment commit code inspects the buffer. Then we'd release the buffer from the transaction but try_to_free_buffers() would fail because of elevated b_count exactly as you described. Could you maybe verify this? Not that I'd know how to easily fix this ;)...

Quoted 96 lines · Mingming Cao

Quoted 47 lines · Jan Kara

Quoted 46 lines · Badari Pulavarty

Quoted 21 lines · Jan Kara

Hi,

Quoted 14 lines · Mingming Cao

While looking at a bug related to direct IO returns to EIO, after looking at the code, I found there is a window that try_to_free_buffers() from direct IO could race with JBD, which holds the reference to the data buffers before journal_commit_transaction() ensures the data buffers has reached to the disk.

A little more detail: to prepare for direct IO, generic_file_direct_IO() calls invalidate_inode_pages2_range() to invalidate the pages in the cache before performaning direct IO. invalidate_inode_pages2_range() tries to free the buffers via try_to free_buffers(), but sometimes it can't, due to the buffers is possible still on some transaction's t_sync_datalist or t_locked_list waiting for journal_commit_transaction() to process it.

Quoted 4 lines

Currently Direct IO simply returns EIO if try_to_free_buffers() finds the buffer is busy, as it has no clue that JBD is referencing it.

Is this a known issue and expected behavior? Any thoughts?

Quoted 11 lines

Are you seeing this in data=ordered mode? As Andrew pointed out we do
filemap_write_and_wait() so all the relevant data buffers of the inode should be already on disk. In __journal_try_to_free_buffer() we check whether the buffer is already-written-out data buffer and unfile and free it in that case. It shouldn't happen that a data buffer has b_next_transaction set so really the only idea why try_to_free_buffers() could fail is that somebody manages to write to a page via mmap before invalidate_inode_pages2_range() gets to it. Under which kind of load do you observe the problem? Do you know exactly because of which condition does journal_try_to_free_buffers() fail?

Thank you for your reply.

What we are noticing is invalidate_inode_pages2_range() fails with -EIO (from try_to_free_buffers() since b_count > 0).

I don't think the file is being updated through mmap(). Previous writepage() added these buffers to t_sync_data list (data=ordered). filemap_write_and_wait() waits for pagewrite back to be cleared. So, buffers are no longer dirty, but still on the t_sync_data and kjournald didn't get chance to process them yet :(

Quoted 8 lines

Quoted 3 lines

Since we have elevated b_count on these buffers, try_to_free_buffers() fails. How can we make filemap_write_and_wait() to wait for kjournald to unfile these buffers ?

Hmm, I don't get one thing:
The call chain is invalidate_inode_pages2_range() ->
invalidate_complete_page2() -> try_to_release_page() -> ext3_releasepage() -> journal_try_to_free_buffers() -> __journal_try_to_free_buffer() and this function should remove the buffer from the committing transaction.

Thanks, yes I noticed that after you pointing this out.

But __journal_try_to_free_buffer() only unfile the buffer from t_sync_datalist or t_locked list, the journal head is not removed in journal_remove_journal_head() there, at that time, journal_remove_journal_head() just check if counter b_jcount is 0. But before calling __journal_try_to_free_buffer(), since journal_try_to_free_buffers() already increase the b_jcount in journal_grab_journal_head(), so the journal head is not removed in __journal_try_to_free_buffer-> journal_remove_journal_head()

Quoted 2 lines

So who's
holding the reference to those buffers?

Looking at the code, it seems the it's the journal_put_journal_head(jh) who remove the journal head and decrease the bh

journal_try_to_free_buffers()
{
...

jh = journal_grab_journal_head(bh);
                if (!jh)
                        continue;

                jbd_lock_bh_state(bh);
                __journal_try_to_free_buffer(journal, bh);
                journal_put_journal_head(jh);
                jbd_unlock_bh_state(bh);

...

}
so when journal_put_journal_head()-> __journal_remove_journal_head(),
now the b_jcount is zero, but is
jh->b_transaction is NULL? So it seems possible that bh ref count is non zero when exit from journal_put_journal_head() if jh_b_transaction is not cleared.

Quoted 1 line

I miss where jh->b_transaction is clear to NULL?

__journal_unfile_buffer() called from __journal_try_to_free_buffer() sets
jh->b_transaction to NULL. So as soon as journal_put_journal_head() is called, it results in freeing of journal head and releasing buffer reference.

So really the only possible race I see is what I describe below...

Quoted 6 lines

Quoted 6 lines

Hmm, maybe I have one idea - in theory we could call
__journal_try_to_free_buffer() exactly at the moment commit code inspects the buffer. Then we'd release the buffer from the transaction but try_to_free_buffers() would fail because of elevated b_count exactly as you described. Could you maybe verify this? Not that I'd know how to easily fix this ;)...

Honza

Hi Andrew & Jan,

I was able to reproduce the customer problem involving DIO (invalidate_inode_pages2) problem by writing simple testcase to keep writing to a file using buffered writes and DIO writes forever in a loop. I see DIO writes fail with -EIO.

After a long debug, found 2 cases how this could happen. These are race conditions with
and journal_commit_transaction().

1) journal_submit_data_buffers() tries to get bh_state lock. If try lock fails, it drops the j_list_lock and sleeps for bh_state lock, while holding a reference on the buffer.
In the meanwhile, journal_try_to_free_buffers() can clean up the journal head could call try_to_free_buffers(). try_to_free_buffers() would fail due to the reference held by journal_submit_data_buffers()
- which in turn causes failues for DIO (invalidate_inode_pages2()).

2) When the buffer is on t_locked_list waiting for IO to finish, we hold a reference and give up the cpu, if we can't get bh_state lock. This causes try_to_free_buffers() to fail.

Hi Andrew & Jan,

I was able to reproduce the customer problem involving DIO (invalidate_inode_pages2) problem by writing simple testcase to keep writing to a file using buffered writes and DIO writes forever in a loop. I see DIO writes fail with -EIO.

After a long debug, found 2 cases how this could happen. These are race conditions with journal_try_to_free_buffers() and journal_commit_transaction().

1) journal_submit_data_buffers() tries to get bh_state lock. If try lock fails, it drops the j_list_lock and sleeps for bh_state lock, while holding a reference on the buffer.
In the meanwhile, journal_try_to_free_buffers() can clean up the journal head could call try_to_free_buffers(). try_to_free_buffers() would fail due to the reference held by journal_submit_data_buffers()
- which in turn causes failues for DIO (invalidate_inode_pages2()).

2) When the buffer is on t_locked_list waiting for IO to finish, we hold a reference and give up the cpu, if we can't get bh_state lock. This causes try_to_free_buffers() to fail.

Fix is to drop the reference on the buffer if we can't get bh_state lock, give up the cpu and re-try the whole operation - instead of waiting for the vh_state lock.

Does this look like a resonable fix ?

1) journal_submit_data_buffers() tries to get bh_state lock. If try lock fails, it drops the j_list_lock and sleeps for bh_state lock, while holding a reference on the buffer head. In the meanwhile, journal_try_to_free_buffers() can clean up the journal head could call try_to_free_buffers(). try_to_free_buffers() would fail due to the reference held by journal_submit_data_buffers()
- which inturn causes failues for DIO (invalidate_inode_pages2()).

2) When the buffer is on t_locked_list waiting for IO to finish, we hold a reference and give up the cpu, if we can't get bh_state lock. This causes try_to_free_buffers() to fail.

Fix is to drop the reference on the buffer, give up the cpu and re-try the whole operation.

Signed-off-by Badari Pulavarty <[email protected]>

Reviewed-by Mingming Cao <[email protected]>

Quoted 23 lines · Badari Pulavarty

Hi Andrew & Jan,

I was able to reproduce the customer problem involving DIO (invalidate_inode_pages2) problem by writing simple testcase to keep writing to a file using buffered writes and DIO writes forever in a loop. I see DIO writes fail with -EIO.

After a long debug, found 2 cases how this could happen. These are race conditions with journal_try_to_free_buffers() and journal_commit_transaction().

1) journal_submit_data_buffers() tries to get bh_state lock. If try lock fails, it drops the j_list_lock and sleeps for bh_state lock, while holding a reference on the buffer.
In the meanwhile, journal_try_to_free_buffers() can clean up the journal head could call try_to_free_buffers(). try_to_free_buffers() would fail due to the reference held by journal_submit_data_buffers()
- which in turn causes failues for DIO (invalidate_inode_pages2()).

2) When the buffer is on t_locked_list waiting for IO to finish, we hold a reference and give up the cpu, if we can't get bh_state lock. This causes try_to_free_buffers() to fail.

Quoted 5 lines

Fix is to drop the reference on the buffer if we can't get bh_state lock, give up the cpu and re-try the whole operation - instead of waiting for the vh_state lock.

Does this look like a resonable fix ?

As Mingming pointed out there are few other places where we could hold
the bh reference.

Note also that we accumulate references to buffers in the wbuf[] list and we need that for submit_bh() which consumes one bh reference.

Generally, it seems to me as a too fragile and impractical
rule "nobody can hold bh reference when not holding page lock" which is basically what it comes down to if you really want to be sure that journal_try_to_free_buffers() succeeds. And also note that in principle there are other places which hold references to buffers without holding the page lock - for example writepage() in ordered mode (although this one is in practice hardly triggerable). So how we could fix at least the races with commit code is to implement launder_page() callback for ext3/4 which would wait for the previous transaction commit in case the page has buffers that are part of that commit (I don't want this logic in journal_try_to_free_buffers() as that is called also on memory-reclaim path, but journal_launder_page() is fine with me). This would be correct but could considerably slow down O_DIRECT writes in cases they're mixed with buffered writes so I'm not sure if this is acceptable.
OTOH with the ordered mode rewrite patch, the problem with commit code
also goes away since there we don't need extra references to data buffers (we use just filemap_fdatawrite).

Quoted 12 lines

1) journal_submit_data_buffers() tries to get bh_state lock. If try lock fails, it drops the j_list_lock and sleeps for bh_state lock, while holding a reference on the buffer head. In the meanwhile, journal_try_to_free_buffers() can clean up the journal head could call try_to_free_buffers(). try_to_free_buffers() would fail due to the reference held by journal_submit_data_buffers()
- which inturn causes failues for DIO (invalidate_inode_pages2()).

2) When the buffer is on t_locked_list waiting for IO to finish, we hold a reference and give up the cpu, if we can't get bh_state lock. This causes try_to_free_buffers() to fail.

Quoted 5 lines

Fix is to drop the reference on the buffer, give up the cpu and re-try the whole operation.

Signed-off-by Badari Pulavarty <[email protected]>

Reviewed-by Mingming Cao <[email protected]>

Honza

Quoted 23 lines · Badari Pulavarty

Hi Andrew & Jan,

I was able to reproduce the customer problem involving DIO (invalidate_inode_pages2) problem by writing simple testcase to keep writing to a file using buffered writes and DIO writes forever in a loop. I see DIO writes fail with -EIO.

After a long debug, found 2 cases how this could happen. These are race conditions with journal_try_to_free_buffers() and journal_commit_transaction().

1) journal_submit_data_buffers() tries to get bh_state lock. If try lock fails, it drops the j_list_lock and sleeps for bh_state lock, while holding a reference on the buffer.
In the meanwhile, journal_try_to_free_buffers() can clean up the journal head could call try_to_free_buffers(). try_to_free_buffers() would fail due to the reference held by journal_submit_data_buffers()
- which in turn causes failues for DIO (invalidate_inode_pages2()).

2) When the buffer is on t_locked_list waiting for IO to finish, we hold a reference and give up the cpu, if we can't get bh_state lock. This causes try_to_free_buffers() to fail.

Quoted 5 lines

Fix is to drop the reference on the buffer if we can't get bh_state lock, give up the cpu and re-try the whole operation - instead of waiting for the vh_state lock.

Does this look like a resonable fix ?

As Mingming pointed out there are few other places where we could hold
the bh reference. Note also that we accumulate references to buffers in the wbuf[] list and we need that for submit_bh() which consumes one bh reference. Generally, it seems to me as a too fragile and impractical rule "nobody can hold bh reference when not holding page lock" which is basically what it comes down to if you really want to be sure that journal_try_to_free_buffers() succeeds. And also note that in principle there are other places which hold references to buffers without holding the page lock - for example writepage() in ordered mode (although this one is

in practice hardly triggerable). So how we could fix at least the races with commit code is to implement launder_page() callback for ext3/4 which would wait for the previous transaction commit in case the page has buffers that are part of that commit (I don't want this logic in journal_try_to_free_buffers() as that is called also on memory-reclaim path, but journal_launder_page() is fine with me). This would be correct but could considerably slow down O_DIRECT writes in cases they're mixed with buffered writes so I'm not sure if this is acceptable.

Quoted 23 lines · Badari Pulavarty

Hi Andrew & Jan,

I was able to reproduce the customer problem involving DIO (invalidate_inode_pages2) problem by writing simple testcase to keep writing to a file using buffered writes and DIO writes forever in a loop. I see DIO writes fail with -EIO.

After a long debug, found 2 cases how this could happen. These are race conditions with journal_try_to_free_buffers() and journal_commit_transaction().

1) journal_submit_data_buffers() tries to get bh_state lock. If try lock fails, it drops the j_list_lock and sleeps for bh_state lock, while holding a reference on the buffer.
In the meanwhile, journal_try_to_free_buffers() can clean up the journal head could call try_to_free_buffers(). try_to_free_buffers() would fail due to the reference held by journal_submit_data_buffers()
- which in turn causes failues for DIO (invalidate_inode_pages2()).

2) When the buffer is on t_locked_list waiting for IO to finish, we hold a reference and give up the cpu, if we can't get bh_state lock. This causes try_to_free_buffers() to fail.

Quoted 5 lines

Fix is to drop the reference on the buffer if we can't get bh_state lock, give up the cpu and re-try the whole operation - instead of waiting for the vh_state lock.

Does this look like a resonable fix ?

As Mingming pointed out there are few other places where we could hold
the bh reference. Note also that we accumulate references to buffers in the wbuf[] list and we need that for submit_bh() which consumes one bh reference. Generally, it seems to me as a too fragile and impractical rule "nobody can hold bh reference when not holding page lock" which is basically what it comes down to if you really want to be sure that journal_try_to_free_buffers() succeeds. And also note that in principle there are other places which hold references to buffers without holding the page lock - for example writepage() in ordered mode (although this one is in practice hardly triggerable). So how we could fix at least the races with commit code is to implement launder_page() callback for ext3/4 which would wait for the previous transaction commit in case the page has buffers that are part of that commit (I don't want this logic in journal_try_to_free_buffers() as that is called also on memory-reclaim path, but journal_launder_page() is fine with me).

Updating the current transaction's t_state is being protected by j_state_lock. We need to do the same when updating the t_state to T_COMMIT.

Signed-off-by Mingming Cao <[email protected]>

---
 fs/jbd/commit.c |    2 ++
 1 file changed, 2 insertions(+)

Index: linux-2.6.26-rc1/fs/jbd/commit.c
===================================================================

fs/jbd/commit.c +2 -0

@@ -478,7 +478,9 @@ void journal_commit_transaction(journal_
 	 * transaction!  Now comes the tricky part: we need to write out
 	 * metadata.  Loop over the transaction's entire buffer list:
 	 */
+	spin_lock(&journal->j_state_lock);
 	commit_transaction->t_state = T_COMMIT;
+	spin_unlock(&journal->j_state_lock);
 
 	J_ASSERT(commit_transaction->t_nr_buffers <=
 		 commit_transaction->t_outstanding_credits);

Quoted 26 lines

Quoted 21 lines

I was able to reproduce the customer problem involving DIO (invalidate_inode_pages2) problem by writing simple testcase to keep writing to a file using buffered writes and DIO writes forever in a loop. I see DIO writes fail with -EIO.

After a long debug, found 2 cases how this could happen. These are race conditions with journal_try_to_free_buffers() and journal_commit_transaction().

1) journal_submit_data_buffers() tries to get bh_state lock. If try lock fails, it drops the j_list_lock and sleeps for bh_state lock, while holding a reference on the buffer.
In the meanwhile, journal_try_to_free_buffers() can clean up the journal head could call try_to_free_buffers(). try_to_free_buffers() would fail due to the reference held by journal_submit_data_buffers()
- which in turn causes failues for DIO (invalidate_inode_pages2()).

2) When the buffer is on t_locked_list waiting for IO to finish, we hold a reference and give up the cpu, if we can't get bh_state lock. This causes try_to_free_buffers() to fail.

Quoted 5 lines

Fix is to drop the reference on the buffer if we can't get bh_state lock, give up the cpu and re-try the whole operation - instead of waiting for the vh_state lock.

Does this look like a resonable fix ?

Quoted 15 lines

As Mingming pointed out there are few other places where we could hold
the bh reference. Note also that we accumulate references to buffers in the wbuf[] list and we need that for submit_bh() which consumes one bh reference. Generally, it seems to me as a too fragile and impractical rule "nobody can hold bh reference when not holding page lock" which is basically what it comes down to if you really want to be sure that journal_try_to_free_buffers() succeeds. And also note that in principle there are other places which hold references to buffers without holding the page lock - for example writepage() in ordered mode (although this one is in practice hardly triggerable). So how we could fix at least the races with commit code is to implement launder_page() callback for ext3/4 which would wait for the previous transaction commit in case the page has buffers that are part of that commit (I don't want this logic in journal_try_to_free_buffers() as that is called also on memory-reclaim path, but journal_launder_page() is fine with me).

I am not sure how we are going to gurantee that by the time journal_try_to_free_buffers() get called, the page has buffers are not as part of the current transaction commit(which could be different than the one we waited in ext3_launder_page())?

It seems more realistic to fix the races one by one to me.

There is still a window that journal_submit_data_buffers() already removed the jh from the bh (when found the buffers are already being synced), but still keep a reference to the buffer head. journal_try_to_free_buffers() could be called. In that case try_to_free_buffers() will be called since there is no jh related to this buffer, and failed due to journal_submit_data_buffers() hasn't finish the cleanup business yet.

For this new race, we could just grab the j_list_lock when re-try try_to_free_buffers() to force waiting for journal_commit_transaction() to finish it flush work. But not sure if this is acceptable approach?

Patch like this? Comments?

---------------------------------------------------------------------
There are a few cases direct IO could race with kjournal flushing data buffers which could result direct IO return EIO error.

1) journal_submit_data_buffers() tries to get bh_state lock. If try lock fails, it drops the j_list_lock and sleeps for bh_state lock, while holding a reference on the buffer.
In the meanwhile, journal_try_to_free_buffers() can clean up the journal head could call try_to_free_buffers(). try_to_free_buffers() would fail due to the reference held by journal_submit_data_buffers()
- which in turn causes failues for DIO (invalidate_inode_pages2()).

2) When the buffer is on t_locked_list waiting for IO to finish, we hold a reference and give up the cpu, if we can't get bh_state lock. This causes try_to_free_buffers() to fail.

3) when journal_submit_data_buffers() saw the buffer is dirty but failed to lock the buffer bh1, journal_submit_data_buffers() released the j_list_lock and submit other buffers collected from previous check, with the reference to bh1 still hold. During this time journal_try_to_free_buffers() could clean up the journal head of bh1 and remove it from the t_syncdata_list. Then try_to_free_buffers() would fail because the reference held by journal_submit_data_buffers()

4) journal_submit_data_buffers() already removed the jh from the bh (when found the buffers are already being synced), but still keep a reference to the buffer head. journal_try_to_free_buffers() could be called. In that case try_to_free_buffers() will be called since there is no jh related to this buffer, and failed due to journal_submit_data_buffers() hasn't finish the cleanup business yet.

Fix for first three races is to drop the reference on the buffer head
when release the j_list_lock,
give up the cpu and re-try the whole operation.

This patch also fixes the race that data buffers has been flushed to disk and journal head is cleard
by journal_submit_data_buffers() but did not get a chance to release
buffer head reference before the journal_try_to_free_buffers() kicked in.

Signed-off-by Badari Pulavarty <[email protected]>

Signed-off-by Mingming Cao <[email protected]>

---
 fs/jbd/commit.c      |   21 ++++++++++++++++-----
 fs/jbd/transaction.c |   13 +++++++++++++
 2 files changed, 29 insertions(+), 5 deletions(-)

Index: linux-2.6.26-rc1/fs/jbd/commit.c
===================================================================

fs/jbd/commit.c +6 -2

@@ -79,12 +79,16 @@ nope:
 
 /*
  * Try to acquire jbd_lock_bh_state() against the buffer, when j_list_lock is
- * held.  For ranking reasons we must trylock.  If we lose, schedule away and
+ * held.  For ranking reasons we must trylock.  If we lose,  unlock the buffer
+ * if needed, drop the reference on the buffer, schedule away and
  * return 0.  j_list_lock is dropped in this case.
  */
-static int inverted_lock(journal_t *journal, struct buffer_head *bh)
+static int inverted_lock(journal_t *journal, struct buffer_head *bh, int locked)
 {
 	if (!jbd_trylock_bh_state(bh)) {
+		if (locked)
+			unlock_buffer(bh);
+		put_bh(bh);
 		spin_unlock(&journal->j_list_lock);
 		schedule();
 		return 0;

@@ -209,19 +213,24 @@ write_out_data:
 		if (buffer_dirty(bh)) {

fs/jbd/commit.c +0 -2

 			}
 			locked = 1;
 		}
-		/* We have to get bh_state lock. Again out of order, sigh. */
-		if (!inverted_lock(journal, bh)) {

fs/jbd/commit.c +1 -2

 		/* Someone already cleaned up the buffer? */
 		if (!buffer_jbd(bh)

@@ -430,8 +439,7 @@ void journal_commit_transaction(journal_
 				err = -EIO;
 			spin_lock(&journal->j_list_lock);
 		}
-		if (!inverted_lock(journal, bh)) {
-			put_bh(bh);
+		if (!inverted_lock(journal, bh, 0)) {
 			spin_lock(&journal->j_list_lock);
 			continue;
 		}

Index: linux-2.6.26-rc1/fs/jbd/transaction.c
===================================================================

fs/jbd/transaction.c +6 -0

@@ -1714,6 +1714,19 @@ int journal_try_to_free_buffers(journal_
 			goto busy;
 	} while ((bh = bh->b_this_page) != head);
 	ret = try_to_free_buffers(page);
+	if (ret == 0) {
+		/*
+		 * it is possible that journal_submit_data_buffers()
+		 * still holds the bh ref even if clears the jh
+		 * after journal_remove_journal_head,
+		 * which leads to try_to_free_buffers() failed

Hello,

Quoted 42 lines · Mingming Cao

Quoted 41 lines

Quoted 21 lines

I was able to reproduce the customer problem involving DIO (invalidate_inode_pages2) problem by writing simple testcase to keep writing to a file using buffered writes and DIO writes forever in a loop. I see DIO writes fail with -EIO.

After a long debug, found 2 cases how this could happen. These are race conditions with journal_try_to_free_buffers() and journal_commit_transaction().

1) journal_submit_data_buffers() tries to get bh_state lock. If try lock fails, it drops the j_list_lock and sleeps for bh_state lock, while holding a reference on the buffer.
In the meanwhile, journal_try_to_free_buffers() can clean up the journal head could call try_to_free_buffers(). try_to_free_buffers() would fail due to the reference held by journal_submit_data_buffers()
- which in turn causes failues for DIO (invalidate_inode_pages2()).

2) When the buffer is on t_locked_list waiting for IO to finish, we hold a reference and give up the cpu, if we can't get bh_state lock. This causes try_to_free_buffers() to fail.

Quoted 5 lines

Fix is to drop the reference on the buffer if we can't get bh_state lock, give up the cpu and re-try the whole operation - instead of waiting for the vh_state lock.

Does this look like a resonable fix ?

As Mingming pointed out there are few other places where we could hold
the bh reference. Note also that we accumulate references to buffers in the wbuf[] list and we need that for submit_bh() which consumes one bh reference. Generally, it seems to me as a too fragile and impractical rule "nobody can hold bh reference when not holding page lock" which is basically what it comes down to if you really want to be sure that journal_try_to_free_buffers() succeeds. And also note that in principle there are other places which hold references to buffers without holding the page lock - for example writepage() in ordered mode (although this one is in practice hardly triggerable). So how we could fix at least the races with commit code is to implement launder_page() callback for ext3/4 which would wait for the previous transaction commit in case the page has buffers that are part of that commit (I don't want this logic in journal_try_to_free_buffers() as that is called also on memory-reclaim path, but journal_launder_page() is fine with me).

Quoted 4 lines

I am not sure how we are going to gurantee that by the time journal_try_to_free_buffers() get called, the page has buffers are not as part of the current transaction commit(which could be different than the one we waited in ext3_launder_page())?

Hmm, you are right. It is not enough to just wait in ext3_launder_page()
because we don't have a transaction for direct_IO started yet. But if we actually released buffers from the page there, it should be fine.

Quoted 1 line

It seems more realistic to fix the races one by one to me.

Not to me, really. The scheme for buffer references you are trying to
impose is awkward to say the least. First, it is completely counter-intuitive (at least to me ;), second, it is impractical as well.

For example in your scheme, you have no sensible way of locking ordered data mode buffer - you cannot just do: get the reference and do lock_buffer() because that violates your requirements. The only reasonable way you could do that is to lock the page to make sure buffer won't go away from you - but you cannot currently do that in journal commit code because of lock ordering. So the only way I can see which is left is: get some jbd spin lock to serialize with journal_try_to_free_buffers(), get the buffer

reference, try to lock buffer, if it fails, drop everything and restart. And this is IMO no-go...
And BTW even if you fix such races, I think you'll still have races like:
CPU1: CPU2:
filemap_write_and_wait()
dirty a page
msync() (dirties buffers)
invalidate_inode_page2_range() -> -EIO

The code could historically always return EIO when mixing buffered and
unbuffered accesses and the question is, under which circumstances is this acceptable? I agree that the current state when if you do "buffered write, DIO write" in sequence and you'll possibly get EIO is bad and we should fix it. But I'm not sure we should fix the EIO return under all possible circumstances at all costs...

Quoted 92 lines

There is still a window that journal_submit_data_buffers() already removed the jh from the bh (when found the buffers are already being synced), but still keep a reference to the buffer head. journal_try_to_free_buffers() could be called. In that case try_to_free_buffers() will be called since there is no jh related to this buffer, and failed due to journal_submit_data_buffers() hasn't finish the cleanup business yet.

For this new race, we could just grab the j_list_lock when re-try try_to_free_buffers() to force waiting for journal_commit_transaction() to finish it flush work. But not sure if this is acceptable approach?

Patch like this? Comments?

---------------------------------------------------------------------
There are a few cases direct IO could race with kjournal flushing data buffers which could result direct IO return EIO error.

1) journal_submit_data_buffers() tries to get bh_state lock. If try lock fails, it drops the j_list_lock and sleeps for bh_state lock, while holding a reference on the buffer.
In the meanwhile, journal_try_to_free_buffers() can clean up the journal head could call try_to_free_buffers(). try_to_free_buffers() would fail due to the reference held by journal_submit_data_buffers()
- which in turn causes failues for DIO (invalidate_inode_pages2()).

2) When the buffer is on t_locked_list waiting for IO to finish, we hold a reference and give up the cpu, if we can't get bh_state lock. This causes try_to_free_buffers() to fail.

3) when journal_submit_data_buffers() saw the buffer is dirty but failed to lock the buffer bh1, journal_submit_data_buffers() released the j_list_lock and submit other buffers collected from previous check, with the reference to bh1 still hold. During this time journal_try_to_free_buffers() could clean up the journal head of bh1 and remove it from the t_syncdata_list. Then try_to_free_buffers() would fail because the reference held by journal_submit_data_buffers()

4) journal_submit_data_buffers() already removed the jh from the bh (when found the buffers are already being synced), but still keep a reference to the buffer head. journal_try_to_free_buffers() could be called. In that case try_to_free_buffers() will be called since there is no jh related to this buffer, and failed due to journal_submit_data_buffers() hasn't finish the cleanup business yet.

Fix for first three races is to drop the reference on the buffer head
when release the j_list_lock,
give up the cpu and re-try the whole operation.

This patch also fixes the race that data buffers has been flushed to disk and journal head is cleard
by journal_submit_data_buffers() but did not get a chance to release
buffer head reference before the journal_try_to_free_buffers() kicked in.

Signed-off-by Badari Pulavarty <[email protected]>

Signed-off-by Mingming Cao <[email protected]>

---
 fs/jbd/commit.c      |   21 ++++++++++++++++-----
 fs/jbd/transaction.c |   13 +++++++++++++
 2 files changed, 29 insertions(+), 5 deletions(-)

Index: linux-2.6.26-rc1/fs/jbd/commit.c
===================================================================

fs/jbd/commit.c +7 -2

@@ -79,12 +79,16 @@ nope:
 
 /*
  * Try to acquire jbd_lock_bh_state() against the buffer, when j_list_lock is
- * held.  For ranking reasons we must trylock.  If we lose, schedule away and
+ * held.  For ranking reasons we must trylock.  If we lose,  unlock the buffer
+ * if needed, drop the reference on the buffer, schedule away and
  * return 0.  j_list_lock is dropped in this case.
  */
-static int inverted_lock(journal_t *journal, struct buffer_head *bh)
+static int inverted_lock(journal_t *journal, struct buffer_head *bh, int locked)
 {
 	if (!jbd_trylock_bh_state(bh)) {
+		if (locked)
+			unlock_buffer(bh);
+		put_bh(bh);
 		spin_unlock(&journal->j_list_lock);
 		schedule();
 		return 0;

@@ -209,19 +213,24 @@ write_out_data:
 		if (buffer_dirty(bh)) {
 			if (test_set_buffer_locked(bh)) {
 				BUFFER_TRACE(bh, "needs blocking lock");
+				put_bh(bh);
 				spin_unlock(&journal->j_list_lock);
 				/* Write out all data to prevent deadlocks */

fs/jbd/commit.c write_out_data: · line 222

Quoted 5 lines

fs/jbd/commit.c +1 -1

 				journal_do_submit_data(wbuf, bufs);
 				bufs = 0;
-				lock_buffer(bh);
 				spin_lock(&journal->j_list_lock);
+				continue;

^^^ Here you can see what I wrote above. Basically you just busy-loop
wait for buffer lock. You should at least put schedule() there so that you don't lockup the CPU but it's ugly anyway.

Quoted 5 lines

fs/jbd/commit.c +0 -2

 			}
 			locked = 1;
 		}
-		/* We have to get bh_state lock. Again out of order, sigh. */
-		if (!inverted_lock(journal, bh)) {

fs/jbd/commit.c write_out_data: · line 234

Quoted 10 lines

fs/jbd/commit.c +7 -1

-			jbd_lock_bh_state(bh);
+		/*
+		 * We have to get bh_state lock. If the try lock fails,
+		 * release the ref on the buffer, give up cpu and retry the
+		 * whole operation.
+		 */
+		if (!inverted_lock(journal, bh, locked)) {
 			spin_lock(&journal->j_list_lock);
+			continue;
 		}

^^^ And here you add a place where we are not guaranteed to make any
progress... If someone intensively spins on that buffer, commit code could cycle here forever (or at least for quite a long time).

Quoted 26 lines

fs/jbd/commit.c +1 -2

 		/* Someone already cleaned up the buffer? */
 		if (!buffer_jbd(bh)

@@ -430,8 +439,7 @@ void journal_commit_transaction(journal_
 				err = -EIO;
 			spin_lock(&journal->j_list_lock);
 		}
-		if (!inverted_lock(journal, bh)) {
-			put_bh(bh);
+		if (!inverted_lock(journal, bh, 0)) {
 			spin_lock(&journal->j_list_lock);
 			continue;
 		}

Index: linux-2.6.26-rc1/fs/jbd/transaction.c
===================================================================

fs/jbd/transaction.c +6 -0

@@ -1714,6 +1714,19 @@ int journal_try_to_free_buffers(journal_
 			goto busy;
 	} while ((bh = bh->b_this_page) != head);
 	ret = try_to_free_buffers(page);
+	if (ret == 0) {
+		/*
+		 * it is possible that journal_submit_data_buffers()
+		 * still holds the bh ref even if clears the jh
+		 * after journal_remove_journal_head,
+		 * which leads to try_to_free_buffers() failed

fs/jbd/transaction.c int journal_try_to_free_buffers(journal_ · line 1732

Quoted 10 lines

fs/jbd/transaction.c +7 -0

+		 * let's wait for journal_submit_data_buffers()
+		 * to finishing remove the bh from the sync_data_list
+		 */
+		spin_lock(&journal->j_list_lock);
+		ret = try_to_free_buffers(page);
+		spin_unlock(&journal->j_list_lock);
+	}
 busy:
 	return ret;
 }

Honza

Quoted 43 lines · Jan Kara

Quoted 42 lines · Mingming Cao

Quoted 41 lines

Quoted 21 lines

I was able to reproduce the customer problem involving DIO (invalidate_inode_pages2) problem by writing simple testcase to keep writing to a file using buffered writes and DIO writes forever in a loop. I see DIO writes fail with -EIO.

After a long debug, found 2 cases how this could happen. These are race conditions with journal_try_to_free_buffers() and journal_commit_transaction().

1) journal_submit_data_buffers() tries to get bh_state lock. If try lock fails, it drops the j_list_lock and sleeps for bh_state lock, while holding a reference on the buffer.
In the meanwhile, journal_try_to_free_buffers() can clean up the journal head could call try_to_free_buffers(). try_to_free_buffers() would fail due to the reference held by journal_submit_data_buffers()
- which in turn causes failues for DIO (invalidate_inode_pages2()).

2) When the buffer is on t_locked_list waiting for IO to finish, we hold a reference and give up the cpu, if we can't get bh_state lock. This causes try_to_free_buffers() to fail.

Quoted 5 lines

Fix is to drop the reference on the buffer if we can't get bh_state lock, give up the cpu and re-try the whole operation - instead of waiting for the vh_state lock.

Does this look like a resonable fix ?

As Mingming pointed out there are few other places where we could hold
the bh reference. Note also that we accumulate references to buffers in the wbuf[] list and we need that for submit_bh() which consumes one bh reference. Generally, it seems to me as a too fragile and impractical rule "nobody can hold bh reference when not holding page lock" which is basically what it comes down to if you really want to be sure that journal_try_to_free_buffers() succeeds. And also note that in principle there are other places which hold references to buffers without holding the page lock - for example writepage() in ordered mode (although this one is in practice hardly triggerable). So how we could fix at least the races with commit code is to implement launder_page() callback for ext3/4 which would wait for the previous transaction commit in case the page has buffers that are part of that commit (I don't want this logic in journal_try_to_free_buffers() as that is called also on memory-reclaim path, but journal_launder_page() is fine with me).

Quoted 8 lines

Quoted 4 lines

I am not sure how we are going to gurantee that by the time journal_try_to_free_buffers() get called, the page has buffers are not as part of the current transaction commit(which could be different than the one we waited in ext3_launder_page())?

Hmm, you are right. It is not enough to just wait in ext3_launder_page()
because we don't have a transaction for direct_IO started yet. But if we actually released buffers from the page there, it should be fine.

Do you mean calling journal_try_to_free_buffers() inside ext3_launder_page()? I think we still need some lock to serialize launder_page() with kjournald commit code(not sure if is that Okay?), otherwise there is always a window that by the time try_to_free_buffers() get called, the current transaction has be changed...

Quoted 4 lines

Quoted 1 line

It seems more realistic to fix the races one by one to me.

Not to me, really. The scheme for buffer references you are trying to
impose is awkward to say the least. First, it is completely counter-intuitive (at least to me ;), second, it is impractical as well.

Sigh...I am not very happy with the solution either, but I could not see a decent solution that could fix this problem. Currently we constantly hit EIO error only in 10 minutes with the simple parallel buffered IO and direct IO:(...

Quoted 7 lines

For example in your scheme, you have no sensible way of locking ordered data mode buffer - you cannot just do: get the reference and do lock_buffer() because that violates your requirements. The only reasonable way you could do that is to lock the page to make sure buffer won't go away from you - but you cannot currently do that in journal commit code because of lock ordering. So the only way I can see which is left is: get some jbd spin lock to serialize with journal_try_to_free_buffers(), get the buffer

Quoted 9 lines

reference, try to lock buffer, if it fails, drop everything and restart. And this is IMO no-go...
And BTW even if you fix such races, I think you'll still have races like:
CPU1: CPU2:
filemap_write_and_wait()
dirty a page
msync() (dirties buffers)
invalidate_inode_page2_range() -> -EIO

I could see this is possible with mapped IO. But for buffered IO, since direct IO is holding a i_mutex, this case should not happen,right?

Quoted 7 lines

Quoted 7 lines

fs/jbd/commit.c +1 -0

@@ -209,19 +213,24 @@ write_out_data:
 		if (buffer_dirty(bh)) {
 			if (test_set_buffer_locked(bh)) {
 				BUFFER_TRACE(bh, "needs blocking lock");
+				put_bh(bh);
 				spin_unlock(&journal->j_list_lock);
 				/* Write out all data to prevent deadlocks */

Quoted 9 lines

fs/jbd/commit.c write_out_data: · line 222

Quoted 5 lines

fs/jbd/commit.c +1 -1

 				journal_do_submit_data(wbuf, bufs);
 				bufs = 0;
-				lock_buffer(bh);
 				spin_lock(&journal->j_list_lock);
+				continue;

^^^ Here you can see what I wrote above. Basically you just busy-loop
wait for buffer lock. You should at least put schedule() there so that you don't lockup the CPU but it's ugly anyway.

Yup.

The conflict is that if we still held the bh refrence after released the j_list_lock, journal_try_to_free_buffers() could came in returns EIO to direct IO since buffer is busy(); but if we release the bh reference after released the j_list_lock, it made possible that journal_try_to_free_buffers() to free that buffer, so we can't do lock_buffer() here anymore so we have to loop here. This is a trade off ...

On the other hand, the journal_submit_data_bufferes() continue process this buffer after regrab the j_list_lock even if it's has been removed from the t_syncdata_list by __journal_try_to_free_buffers(). IMO this is not the optimized way.

Hello,

Quoted 42 lines · Mingming Cao

Quoted 41 lines

Quoted 21 lines

I was able to reproduce the customer problem involving DIO (invalidate_inode_pages2) problem by writing simple testcase to keep writing to a file using buffered writes and DIO writes forever in a loop. I see DIO writes fail with -EIO.

After a long debug, found 2 cases how this could happen. These are race conditions with journal_try_to_free_buffers() and journal_commit_transaction().

1) journal_submit_data_buffers() tries to get bh_state lock. If try lock fails, it drops the j_list_lock and sleeps for bh_state lock, while holding a reference on the buffer.
In the meanwhile, journal_try_to_free_buffers() can clean up the journal head could call try_to_free_buffers(). try_to_free_buffers() would fail due to the reference held by journal_submit_data_buffers()
- which in turn causes failues for DIO (invalidate_inode_pages2()).

2) When the buffer is on t_locked_list waiting for IO to finish, we hold a reference and give up the cpu, if we can't get bh_state lock. This causes try_to_free_buffers() to fail.

Quoted 5 lines

Fix is to drop the reference on the buffer if we can't get bh_state lock, give up the cpu and re-try the whole operation - instead of waiting for the vh_state lock.

Does this look like a resonable fix ?

As Mingming pointed out there are few other places where we could hold
the bh reference. Note also that we accumulate references to buffers in the wbuf[] list and we need that for submit_bh() which consumes one bh reference. Generally, it seems to me as a too fragile and impractical rule "nobody can hold bh reference when not holding page lock" which is basically what it comes down to if you really want to be sure that journal_try_to_free_buffers() succeeds. And also note that in principle there are other places which hold references to buffers without holding the page lock - for example writepage() in ordered mode (although this one is in practice hardly triggerable). So how we could fix at least the races with commit code is to implement launder_page() callback for ext3/4 which would wait for the previous transaction commit in case the page has buffers that are part of that commit (I don't want this logic in journal_try_to_free_buffers() as that is called also on memory-reclaim path, but journal_launder_page() is fine with me).

Quoted 4 lines

I am not sure how we are going to gurantee that by the time journal_try_to_free_buffers() get called, the page has buffers are not as part of the current transaction commit(which could be different than the one we waited in ext3_launder_page())?

Hmm, you are right. It is not enough to just wait in ext3_launder_page()
because we don't have a transaction for direct_IO started yet. But if we actually released buffers from the page there, it should be fine.

On Mon, 2008-05-12 at 17:54 +0200, Jan Kara wrote:
Does this match what you are thinking? It certainly slow down the DIO path, but the positive side is it doesn't disturb the other code path... thanks for your feedback!

--------------------------------------------

An unexpected EIO error gets returned when writing to a file using buffered writes and DIO writes at the same time.

We found there are a number of places where journal_try_to_free_buffers() could race with journal_commit_transaction(), the later still
helds the reference to the buffers on the t_syncdata_list or t_locked_list , while journal_try_to_free_buffers() tries to free them, which resulting an EIO error returns back to the dio caller.

The logic fix is to retry freeing if journal_try_to_free_buffers() to failed to free those data buffers while journal_commit_transaction() is still reference those buffers.
This is done via implement ext3 launder_page() callback, instead of inside journal_try_to_free_buffers() itself, so that it doesn't affecting other code path calling journal_try_to_free_buffers and only dio path get affected.

Signed-off-by Mingming Cao <[email protected]>

Index: linux-2.6.26-rc1/fs/ext3/inode.c
===================================================================

fs/ext3/inode.c +7 -0

@@ -1766,6 +1766,23 @@ static int ext3_journalled_set_page_dirt
 	return __set_page_dirty_nobuffers(page);
 }
 
+static int ext3_launder_page(struct page *page)
+{
+        int ret;
+	int retry = 5;
+
+	while (retry --) {
+		ret = ext3_releasepage(page, GFP_KERNEL);

fs/ext3/inode.c +2 -0

 static const struct address_space_operations ext3_ordered_aops = {
 	.readpage	= ext3_readpage,
 	.readpages	= ext3_readpages,

@@ -1778,6 +1795,7 @@ static const struct address_space_operat
 	.releasepage	= ext3_releasepage,
 	.direct_IO	= ext3_direct_IO,
 	.migratepage	= buffer_migrate_page,
+	.launder_page	= ext3_launder_page,
 };
 
 static const struct address_space_operations ext3_writeback_aops = {

@@ -1792,6 +1810,7 @@ static const struct address_space_operat
 	.releasepage	= ext3_releasepage,
 	.direct_IO	= ext3_direct_IO,
 	.migratepage	= buffer_migrate_page,
+	.launder_page	= ext3_launder_page,
 };
 
 static const struct address_space_operations ext3_journalled_aops = {

Quoted 15 lines · Mingming Cao

Index: linux-2.6.26-rc1/fs/ext3/inode.c
===================================================================

fs/ext3/inode.c +7 -0

@@ -1766,6 +1766,23 @@ static int ext3_journalled_set_page_dirt
 	return __set_page_dirty_nobuffers(page);
 }
 
+static int ext3_launder_page(struct page *page)
+{
+        int ret;
+	int retry = 5;
+
+	while (retry --) {
+		ret = ext3_releasepage(page, GFP_KERNEL);

fs/ext3/inode.c static int ext3_journalled_set_page_dirt · line 1785

Quoted 10 lines

fs/ext3/inode.c +10 -0

+		if (ret == 1)
+			break;
+		else
+			schedule();
+	}
+
+        return ret;
+}
+
+

Yes, I meant something like this. We could be more clever and do:

	head = bh = page_buffers(page);
	do {
		wait_on_buffer(bh);
		bh = bh->b_this_page;
	} while (bh != head);
	/*
	 * Now commit code should have been able to proceed and release
         * those buffers
	 */
        schedule();

or we could do simple:

	log_wait_commit(...);

That would impose larger perf. penalty but on the other hand you shouldn't hit this path too often.

But maybe the code above would be fine and would
handle most cases. Also please add a big comment to that function to explain why this magic is needed.

Quoted 26 lines

fs/ext3/inode.c +3 -0

 static const struct address_space_operations ext3_ordered_aops = {
 	.readpage	= ext3_readpage,
 	.readpages	= ext3_readpages,

@@ -1778,6 +1795,7 @@ static const struct address_space_operat
 	.releasepage	= ext3_releasepage,
 	.direct_IO	= ext3_direct_IO,
 	.migratepage	= buffer_migrate_page,
+	.launder_page	= ext3_launder_page,
 };
 
 static const struct address_space_operations ext3_writeback_aops = {

@@ -1792,6 +1810,7 @@ static const struct address_space_operat
 	.releasepage	= ext3_releasepage,
 	.direct_IO	= ext3_direct_IO,
 	.migratepage	= buffer_migrate_page,
+	.launder_page	= ext3_launder_page,
 };
 
 static const struct address_space_operations ext3_journalled_aops = {

@@ -1805,6 +1824,7 @@ static const struct address_space_operat
 	.bmap		= ext3_bmap,
 	.invalidatepage	= ext3_invalidatepage,
 	.releasepage	= ext3_releasepage,
+	.launder_page	= ext3_launder_page,
 };
 

fs/ext3/inode.c static const struct address_space_operat · line 1830

Quoted 1 line

fs/ext3/inode.c +0 -0

 void ext3_set_aops(struct inode *inode)

Actually, we need .launder_page callback only in data=order mode.
data=writeback mode doesn't need it at all (journal code doesn't touch data buffers there) and for data=journal mode DIO could have never worked reasonably when mixed with buffered IO and it would have to do a different and much more expensive trickery (like flushing the journal, or at least forcing current transaction to commit).

Quoted 39 lines · Mingming Cao

On Mon, 2008-05-12 at 17:54 +0200, Jan Kara wrote:
Does this match what you are thinking? It certainly slow down the DIO path, but the positive side is it doesn't disturb the other code path... thanks for your feedback!

--------------------------------------------

An unexpected EIO error gets returned when writing to a file using buffered writes and DIO writes at the same time.

We found there are a number of places where journal_try_to_free_buffers() could race with journal_commit_transaction(), the later still
helds the reference to the buffers on the t_syncdata_list or t_locked_list , while journal_try_to_free_buffers() tries to free them, which resulting an EIO error returns back to the dio caller.

The logic fix is to retry freeing if journal_try_to_free_buffers() to failed to free those data buffers while journal_commit_transaction() is still reference those buffers.
This is done via implement ext3 launder_page() callback, instead of inside journal_try_to_free_buffers() itself, so that it doesn't affecting other code path calling journal_try_to_free_buffers and only dio path get affected.

Signed-off-by Mingming Cao <[email protected]>

Index: linux-2.6.26-rc1/fs/ext3/inode.c
===================================================================

fs/ext3/inode.c +7 -0

@@ -1766,6 +1766,23 @@ static int ext3_journalled_set_page_dirt
 	return __set_page_dirty_nobuffers(page);
 }
 
+static int ext3_launder_page(struct page *page)
+{
+        int ret;
+	int retry = 5;
+
+	while (retry --) {
+		ret = ext3_releasepage(page, GFP_KERNEL);

fs/ext3/inode.c static int ext3_journalled_set_page_dirt · line 1785

Quoted 10 lines

fs/ext3/inode.c +10 -0

+		if (ret == 1)
+			break;
+		else
+			schedule();
+	}
+
+        return ret;
+}
+
+

Yes, I meant something like this. We could be more clever and do:

	head = bh = page_buffers(page);
	do {
		wait_on_buffer(bh);
		bh = bh->b_this_page;
	} while (bh != head);
	/*
	 * Now commit code should have been able to proceed and release
         * those buffers
	 */
        schedule();

or we could do simple:

	log_wait_commit(...);

That would impose larger perf. penalty but on the other hand you shouldn't hit this path too often.

Quoted 52 lines · Jan Kara

Quoted 39 lines · Mingming Cao

On Mon, 2008-05-12 at 17:54 +0200, Jan Kara wrote:
Does this match what you are thinking? It certainly slow down the DIO path, but the positive side is it doesn't disturb the other code path... thanks for your feedback!

--------------------------------------------

An unexpected EIO error gets returned when writing to a file using buffered writes and DIO writes at the same time.

We found there are a number of places where journal_try_to_free_buffers() could race with journal_commit_transaction(), the later still
helds the reference to the buffers on the t_syncdata_list or t_locked_list , while journal_try_to_free_buffers() tries to free them, which resulting an EIO error returns back to the dio caller.

The logic fix is to retry freeing if journal_try_to_free_buffers() to failed to free those data buffers while journal_commit_transaction() is still reference those buffers.
This is done via implement ext3 launder_page() callback, instead of inside journal_try_to_free_buffers() itself, so that it doesn't affecting other code path calling journal_try_to_free_buffers and only dio path get affected.

Signed-off-by Mingming Cao <[email protected]>

Index: linux-2.6.26-rc1/fs/ext3/inode.c
===================================================================

fs/ext3/inode.c +7 -0

@@ -1766,6 +1766,23 @@ static int ext3_journalled_set_page_dirt
 	return __set_page_dirty_nobuffers(page);
 }
 
+static int ext3_launder_page(struct page *page)
+{
+        int ret;
+	int retry = 5;
+
+	while (retry --) {
+		ret = ext3_releasepage(page, GFP_KERNEL);

fs/ext3/inode.c static int ext3_journalled_set_page_dirt · line 1785

Quoted 10 lines

fs/ext3/inode.c +10 -0

+		if (ret == 1)
+			break;
+		else
+			schedule();
+	}
+
+        return ret;
+}
+
+

Yes, I meant something like this. We could be more clever and do:

Quoted 11 lines

	head = bh = page_buffers(page);
	do {
		wait_on_buffer(bh);
		bh = bh->b_this_page;
	} while (bh != head);
	/*
	 * Now commit code should have been able to proceed and release
         * those buffers
	 */
        schedule();

Bummer, we can't free buffers in ext3_launder_page() before calling try_to_free_page, as later invalidate_complete_page2()->try_to_free_page() expecting the page buffers are still here, and will return EIO if it launder_page() has already freed those buffers.:(

Doing wait_on_buffer() alone in launder_page() is not enough as it doesn't wait for buffer reference drop to 0.

Quoted 65 lines · Mingming Cao

Quoted 52 lines · Jan Kara

Quoted 39 lines · Mingming Cao

On Mon, 2008-05-12 at 17:54 +0200, Jan Kara wrote:
Does this match what you are thinking? It certainly slow down the DIO path, but the positive side is it doesn't disturb the other code path... thanks for your feedback!

--------------------------------------------

An unexpected EIO error gets returned when writing to a file using buffered writes and DIO writes at the same time.

We found there are a number of places where journal_try_to_free_buffers() could race with journal_commit_transaction(), the later still
helds the reference to the buffers on the t_syncdata_list or t_locked_list , while journal_try_to_free_buffers() tries to free them, which resulting an EIO error returns back to the dio caller.

The logic fix is to retry freeing if journal_try_to_free_buffers() to failed to free those data buffers while journal_commit_transaction() is still reference those buffers.
This is done via implement ext3 launder_page() callback, instead of inside journal_try_to_free_buffers() itself, so that it doesn't affecting other code path calling journal_try_to_free_buffers and only dio path get affected.

Signed-off-by Mingming Cao <[email protected]>

Index: linux-2.6.26-rc1/fs/ext3/inode.c
===================================================================

fs/ext3/inode.c +7 -0

@@ -1766,6 +1766,23 @@ static int ext3_journalled_set_page_dirt
 	return __set_page_dirty_nobuffers(page);
 }
 
+static int ext3_launder_page(struct page *page)
+{
+        int ret;
+	int retry = 5;
+
+	while (retry --) {
+		ret = ext3_releasepage(page, GFP_KERNEL);

fs/ext3/inode.c static int ext3_journalled_set_page_dirt · line 1785

Quoted 10 lines

fs/ext3/inode.c +10 -0

+		if (ret == 1)
+			break;
+		else
+			schedule();
+	}
+
+        return ret;
+}
+
+

Yes, I meant something like this. We could be more clever and do:

Quoted 11 lines

	head = bh = page_buffers(page);
	do {
		wait_on_buffer(bh);
		bh = bh->b_this_page;
	} while (bh != head);
	/*
	 * Now commit code should have been able to proceed and release
         * those buffers
	 */
        schedule();

Quoted 5 lines

Bummer, we can't free buffers in ext3_launder_page() before calling try_to_free_page, as later invalidate_complete_page2()->try_to_free_page() expecting the page buffers are still here, and will return EIO if it launder_page() has already freed those buffers.:(

Are you sure? Because if bufferes are released in ext3_launder_page(),
PagePrivate() has been set to 0 and we should directly fall through to releasing the page without ever calling try_to_release_page()... So I'd want to find out why PagePrivate is still set in invalidate_complete_page2().

Quoted 11 lines

Quoted 5 lines

Bummer, we can't free buffers in ext3_launder_page() before calling try_to_free_page, as later invalidate_complete_page2()->try_to_free_page() expecting the page buffers are still here, and will return EIO if it launder_page() has already freed those buffers.:(

Are you sure? Because if bufferes are released in ext3_launder_page(),
PagePrivate() has been set to 0 and we should directly fall through to releasing the page without ever calling try_to_release_page()... So I'd want to find out why PagePrivate is still set in invalidate_complete_page2().

You are right. PagePrivate() is being set to 0 in drop_buffers().

The problem is do_launder_page() returns successfully if the page is not dirty (our case), so ext3_launder_page() is not even get called. This also explains why the log_wait_commit() approach doesn't work for me:(