xfstests-bld: exclude ext4 defrag tests from unsupported configurations

Online defrag is not currently implemented for journaled data and bigalloc ext4 file systems. The xfstests that exercise online defrag do not verify that the test file system supports that feature before executing, resulting in spurious test failures. (In the case of ext4/307, the test reports success but actually fails because the e4compact test component is ignoring failures.)

For the time being, simply exclude these tests to eliminate the test failure noise.

Signed-off-by Eric Whitney <[email protected]>

Quoted 11 lines

Online defrag is not currently implemented for journaled data and bigalloc ext4 file systems. The xfstests that exercise online defrag do not verify that the test file system supports that feature before executing, resulting in spurious test failures. (In the case of ext4/307, the test reports success but actually fails because the e4compact test component is ignoring failures.)

For the time being, simply exclude these tests to eliminate the test failure noise.

Signed-off-by Eric Whitney <[email protected]>

Thanks, applied. BTW, I had since removed data_journal.exclude because generic/068 should be passing with data journalling enabled.

- Ted

I haven't tried to run generic/068 in quite a while, but do see it's passing now. I'll close the related bugzillas. Do you know which patch fixed this?

Thanks,
Eric

* Theodore Ts'o <[email protected]>:

Quoted 17 lines · Eric Whitney

Quoted 11 lines

Online defrag is not currently implemented for journaled data and bigalloc ext4 file systems. The xfstests that exercise online defrag do not verify that the test file system supports that feature before executing, resulting in spurious test failures. (In the case of ext4/307, the test reports success but actually fails because the e4compact test component is ignoring failures.)

For the time being, simply exclude these tests to eliminate the test failure noise.

Signed-off-by Eric Whitney <[email protected]>

Thanks, applied. BTW, I had since removed data_journal.exclude because generic/068 should be passing with data journalling enabled.

- Ted

Spoke too soon - hit what looks like the old problem on ARM running 4.1-rc7 on the ninth trial run. Various components of the test are left in the 'D' state after a familiar oops - kernel BUG at fs/jbd2/transaction.c:2150 while truncating.

I'll see if I can still get the problem to appear on x86_64.

The commit cf108bca465d: "ext4: Invert the locking order of page_lock and transaction start" caused __ext4_journalled_writepage() to drop the page lock before the page was written back, as part of changing the locking order to jbd2_journal_start -> page_lock. However, this introduced a potential race if there was a truncate racing with the data=journalled writeback mode.

Fix this by grabbing the page lock after starting the journal handle, and then checking to see if page had gotten truncated out from under us.

This fixes a number of different crashes or BUG_ON's when running xfstests generic/086 in data=journalled mode, including:

jbd2_journal_dirty_metadata: vdc-8: bad jh for block 84434: transaction (ec90434 ransaction ( (null), 0), jh->b_next_transaction ( (null), 0), jlist 0

- and -

kernel BUG at /usr/projects/linux/ext4/fs/jbd2/transaction.c:2200!

---
 fs/ext4/inode.c | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)

fs/ext4/inode.c +19 -4

index 0554b0b..263a46c 100644
@@ -1701,19 +1701,32 @@ static int __ext4_journalled_writepage(struct page *page,
 		ext4_walk_page_buffers(handle, page_bufs, 0, len,
 				       NULL, bget_one);
 	}
-	/* As soon as we unlock the page, it can go away, but we have
-	 * references to buffers so we are safe */
+	/*
+	 * We need to release the page lock before we start the
+	 * journal, so grab a reference so the page won't disappear
+	 * out from under us.
+	 */
+	get_page(page);
 	unlock_page(page);
 
 	handle = ext4_journal_start(inode, EXT4_HT_WRITE_PAGE,
 				    ext4_writepage_trans_blocks(inode));
 	if (IS_ERR(handle)) {
 		ret = PTR_ERR(handle);
-		goto out;
+		put_page(page);
+		goto out_no_pagelock;
 	}
-
 	BUG_ON(!ext4_handle_valid(handle));
 
+	lock_page(page);
+	put_page(page);
+	if (page->mapping != mapping) {
+		/* The page got truncated from under us */
+		ext4_journal_stop(handle);
+		ret = 0;
+		goto out;
+	}
+
 	if (inline_data) {
 		BUFFER_TRACE(inode_bh, "get write access");
 		ret = ext4_journal_get_write_access(handle, inode_bh);

@@ -1739,6 +1752,8 @@ static int __ext4_journalled_writepage(struct page *page,
 				       NULL, bput_one);
 	ext4_set_inode_state(inode, EXT4_STATE_JDATA);
 out:
+	unlock_page(page);
+out_no_pagelock:
 	brelse(inode_bh);
 	return ret;
 }

Signature · 1 line

2.3.0

Yeah, that's nasty. Thanks for debugging this! However I think your fix
reintroduces the original deadlock issues. do_journal_get_write_access() can end up blocking waiting for jbd2 thread to finish a commit while jbd2 thread may be blocked waiting for the page to be unlocked.

After some thought I don't think the deadlock is real since do_journal_get_write_access() will currently only block if a buffer is under writeout to the journal and at that point we don't wait for page locks anymore. Also ext4_write_begin() does the same in data=journal mode and we haven't observed deadlocks so far. But still things look really fragile here.

Quoted 11 lines · Jan Kara

Yeah, that's nasty. Thanks for debugging this! However I think your fix
reintroduces the original deadlock issues. do_journal_get_write_access() can end up blocking waiting for jbd2 thread to finish a commit while jbd2 thread may be blocked waiting for the page to be unlocked.

After some thought I don't think the deadlock is real since do_journal_get_write_access() will currently only block if a buffer is under writeout to the journal and at that point we don't wait for page locks anymore. Also ext4_write_begin() does the same in data=journal mode and we haven't observed deadlocks so far. But still things look really fragile here.

The reason why there are no deadlocks is the writeback in the commit thread happens when the inode gets written back --- but that only happens for data=ordered inodes, not data=journalled mode. I was a little worried about what might happen when after the 'j' chattr attribute gets set on an inode, and the inode was still on the ordered flush list.

Hmm... I think we could also maybe fix this by having ext4_change_inode_journal_flag() force a journal commit before setting the JOURNAL_DATA flag. If we did that, we could just avoid dropping the page_lock in __ext4_journalled_writepage() altogether.

What do you think?

I think that fully switching lock ordering for data=journal mode back to
page lock -> transaction start (which is what you effectively do when you never drop page lock in ->writepage) is rather error prone. We'd have to be careful to avoid lock inversion also for places like ->write_begin, ->releasepage, ->invalidatepage etc. For example ext4_write_begin() will currently call lock_page() with transaction started which could deadlock against journalled writepage you suggest. So effectively we'd have to completely separate aops for data=journal mode. Doable but I'm not sure it's worth it.

Quoted 11 lines · Jan Kara

Yeah, that's nasty. Thanks for debugging this! However I think your fix
reintroduces the original deadlock issues. do_journal_get_write_access() can end up blocking waiting for jbd2 thread to finish a commit while jbd2 thread may be blocked waiting for the page to be unlocked.

After some thought I don't think the deadlock is real since do_journal_get_write_access() will currently only block if a buffer is under writeout to the journal and at that point we don't wait for page locks anymore. Also ext4_write_begin() does the same in data=journal mode and we haven't observed deadlocks so far. But still things look really fragile here.

The reason why there are no deadlocks is the writeback in the commit thread happens when the inode gets written back --- but that only happens for data=ordered inodes, not data=journalled mode. I was a little worried about what might happen when after the 'j' chattr attribute gets set on an inode, and the inode was still on the ordered flush list.

Hmm... I think we could also maybe fix this by having ext4_change_inode_journal_flag() force a journal commit before setting the JOURNAL_DATA flag. If we did that, we could just avoid dropping the page_lock in __ext4_journalled_writepage() altogether.

Quoted 21 lines · Theodore Ts'o

Quoted 11 lines · Jan Kara

Yeah, that's nasty. Thanks for debugging this! However I think your fix
reintroduces the original deadlock issues. do_journal_get_write_access() can end up blocking waiting for jbd2 thread to finish a commit while jbd2 thread may be blocked waiting for the page to be unlocked.

After some thought I don't think the deadlock is real since do_journal_get_write_access() will currently only block if a buffer is under writeout to the journal and at that point we don't wait for page locks anymore. Also ext4_write_begin() does the same in data=journal mode and we haven't observed deadlocks so far. But still things look really fragile here.

The reason why there are no deadlocks is the writeback in the commit thread happens when the inode gets written back --- but that only happens for data=ordered inodes, not data=journalled mode. I was a little worried about what might happen when after the 'j' chattr attribute gets set on an inode, and the inode was still on the ordered flush list.

Quoted 4 lines

Hmm... I think we could also maybe fix this by having ext4_change_inode_journal_flag() force a journal commit before setting the JOURNAL_DATA flag. If we did that, we could just avoid dropping the page_lock in __ext4_journalled_writepage() altogether.

That is already being done:

int ext4_change_inode_journal_flag(struct inode *inode, int val)

{
        :
        :

        jbd2_journal_lock_updates(journal);

        :
        :

/**
* void jbd2_journal_lock_updates () - establish a transaction barrier.
* @journal: Journal to establish a barrier on.
*
* This locks out any further updates from being started, and blocks
* until all existing updates have completed, returning only once the
* journal is in a quiescent state with no updates running. *
* The journal lock should not be held on entry.
*/

The commit cf108bca465d: "ext4: Invert the locking order of page_lock and transaction start" caused __ext4_journalled_writepage() to drop the page lock before the page was written back, as part of changing the locking order to jbd2_journal_start -> page_lock. However, this introduced a potential race if there was a truncate racing with the data=journalled writeback mode.

Fix this by grabbing the page lock after starting the journal handle, and then checking to see if page had gotten truncated out from under us.

This fixes a number of different crashes or BUG_ON's when running xfstests generic/086 in data=journalled mode, including:

jbd2_journal_dirty_metadata: vdc-8: bad jh for block 84434: transaction (ec90434 ransaction ( (null), 0), jh->b_next_transaction ( (null), 0), jlist 0

- and -

kernel BUG at /usr/projects/linux/ext4/fs/jbd2/transaction.c:2200!

    ...
Call Trace:
 [<c02b2ded>] ? __ext4_journalled_invalidatepage+0x117/0x117
 [<c02b2de5>] __ext4_journalled_invalidatepage+0x10f/0x117
 [<c02b2ded>] ? __ext4_journalled_invalidatepage+0x117/0x117
 [<c027d883>] ? lock_buffer+0x36/0x36
 [<c02b2dfa>] ext4_journalled_invalidatepage+0xd/0x22
 [<c0229139>] do_invalidatepage+0x22/0x26
 [<c0229198>] truncate_inode_page+0x5b/0x85
 [<c022934b>] truncate_inode_pages_range+0x156/0x38c
 [<c0229592>] truncate_inode_pages+0x11/0x15
 [<c022962d>] truncate_pagecache+0x55/0x71
 [<c02b913b>] ext4_setattr+0x4a9/0x560
 [<c01ca542>] ? current_kernel_time+0x10/0x44
 [<c026c4d8>] notify_change+0x1c7/0x2be
 [<c0256a00>] do_truncate+0x65/0x85
 [<c0226f31>] ? file_ra_state_init+0x12/0x29

- and -

WARNING: CPU: 1 PID: 1331 at /usr/projects/linux/ext4/fs/jbd2/transaction.c:1396
irty_metadata+0x14a/0x1ae()
    ...
Call Trace:
 [<c01b879f>] ? console_unlock+0x3a1/0x3ce
 [<c082cbb4>] dump_stack+0x48/0x60
 [<c0178b65>] warn_slowpath_common+0x89/0xa0
 [<c02ef2cf>] ? jbd2_journal_dirty_metadata+0x14a/0x1ae
 [<c0178bef>] warn_slowpath_null+0x14/0x18
 [<c02ef2cf>] jbd2_journal_dirty_metadata+0x14a/0x1ae
 [<c02d8615>] __ext4_handle_dirty_metadata+0xd4/0x19d
 [<c02b2f44>] write_end_fn+0x40/0x53
 [<c02b4a16>] ext4_walk_page_buffers+0x4e/0x6a
 [<c02b59e7>] ext4_writepage+0x354/0x3b8
 [<c02b2f04>] ? mpage_release_unused_pages+0xd4/0xd4
 [<c02b1b21>] ? wait_on_buffer+0x2c/0x2c
 [<c02b5a4b>] ? ext4_writepage+0x3b8/0x3b8
 [<c02b5a5b>] __writepage+0x10/0x2e
 [<c0225956>] write_cache_pages+0x22d/0x32c
 [<c02b5a4b>] ? ext4_writepage+0x3b8/0x3b8
 [<c02b6ee8>] ext4_writepages+0x102/0x607
 [<c019adfe>] ? sched_clock_local+0x10/0x10e
 [<c01a8a7c>] ? __lock_is_held+0x2e/0x44
 [<c01a8ad5>] ? lock_is_held+0x43/0x51
 [<c0226dff>] do_writepages+0x1c/0x29
 [<c0276bed>] __writeback_single_inode+0xc3/0x545
 [<c0277c07>] writeback_sb_inodes+0x21f/0x36d
    ...

Signed-off-by Theodore Ts'o <[email protected]>

Cc [email protected]

---
 fs/ext4/inode.c | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)

fs/ext4/inode.c +19 -4

index 0554b0b..263a46c 100644
@@ -1701,19 +1701,32 @@ static int __ext4_journalled_writepage(struct page *page,
 		ext4_walk_page_buffers(handle, page_bufs, 0, len,
 				       NULL, bget_one);
 	}
-	/* As soon as we unlock the page, it can go away, but we have
-	 * references to buffers so we are safe */
+	/*
+	 * We need to release the page lock before we start the
+	 * journal, so grab a reference so the page won't disappear
+	 * out from under us.
+	 */
+	get_page(page);
 	unlock_page(page);
 
 	handle = ext4_journal_start(inode, EXT4_HT_WRITE_PAGE,
 				    ext4_writepage_trans_blocks(inode));
 	if (IS_ERR(handle)) {
 		ret = PTR_ERR(handle);
-		goto out;
+		put_page(page);
+		goto out_no_pagelock;
 	}
-
 	BUG_ON(!ext4_handle_valid(handle));
 
+	lock_page(page);
+	put_page(page);
+	if (page->mapping != mapping) {
+		/* The page got truncated from under us */
+		ext4_journal_stop(handle);
+		ret = 0;
+		goto out;
+	}
+
 	if (inline_data) {
 		BUFFER_TRACE(inode_bh, "get write access");
 		ret = ext4_journal_get_write_access(handle, inode_bh);

@@ -1739,6 +1752,8 @@ static int __ext4_journalled_writepage(struct page *page,
 				       NULL, bput_one);
 	ext4_set_inode_state(inode, EXT4_STATE_JDATA);
 out:
+	unlock_page(page);
+out_no_pagelock:
 	brelse(inode_bh);
 	return ret;
 }

Signature · 1 line

2.3.0

* Theodore Ts'o <[email protected]>:

Quoted 128 lines

The commit cf108bca465d: "ext4: Invert the locking order of page_lock and transaction start" caused __ext4_journalled_writepage() to drop the page lock before the page was written back, as part of changing the locking order to jbd2_journal_start -> page_lock. However, this introduced a potential race if there was a truncate racing with the data=journalled writeback mode.

Fix this by grabbing the page lock after starting the journal handle, and then checking to see if page had gotten truncated out from under us.

This fixes a number of different crashes or BUG_ON's when running xfstests generic/086 in data=journalled mode, including:

jbd2_journal_dirty_metadata: vdc-8: bad jh for block 84434: transaction (ec90434 ransaction ( (null), 0), jh->b_next_transaction ( (null), 0), jlist 0

- and -

kernel BUG at /usr/projects/linux/ext4/fs/jbd2/transaction.c:2200!

    ...
Call Trace:
 [<c02b2ded>] ? __ext4_journalled_invalidatepage+0x117/0x117
 [<c02b2de5>] __ext4_journalled_invalidatepage+0x10f/0x117
 [<c02b2ded>] ? __ext4_journalled_invalidatepage+0x117/0x117
 [<c027d883>] ? lock_buffer+0x36/0x36
 [<c02b2dfa>] ext4_journalled_invalidatepage+0xd/0x22
 [<c0229139>] do_invalidatepage+0x22/0x26
 [<c0229198>] truncate_inode_page+0x5b/0x85
 [<c022934b>] truncate_inode_pages_range+0x156/0x38c
 [<c0229592>] truncate_inode_pages+0x11/0x15
 [<c022962d>] truncate_pagecache+0x55/0x71
 [<c02b913b>] ext4_setattr+0x4a9/0x560
 [<c01ca542>] ? current_kernel_time+0x10/0x44
 [<c026c4d8>] notify_change+0x1c7/0x2be
 [<c0256a00>] do_truncate+0x65/0x85
 [<c0226f31>] ? file_ra_state_init+0x12/0x29

- and -

WARNING: CPU: 1 PID: 1331 at /usr/projects/linux/ext4/fs/jbd2/transaction.c:1396
irty_metadata+0x14a/0x1ae()
    ...
Call Trace:
 [<c01b879f>] ? console_unlock+0x3a1/0x3ce
 [<c082cbb4>] dump_stack+0x48/0x60
 [<c0178b65>] warn_slowpath_common+0x89/0xa0
 [<c02ef2cf>] ? jbd2_journal_dirty_metadata+0x14a/0x1ae
 [<c0178bef>] warn_slowpath_null+0x14/0x18
 [<c02ef2cf>] jbd2_journal_dirty_metadata+0x14a/0x1ae
 [<c02d8615>] __ext4_handle_dirty_metadata+0xd4/0x19d
 [<c02b2f44>] write_end_fn+0x40/0x53
 [<c02b4a16>] ext4_walk_page_buffers+0x4e/0x6a
 [<c02b59e7>] ext4_writepage+0x354/0x3b8
 [<c02b2f04>] ? mpage_release_unused_pages+0xd4/0xd4
 [<c02b1b21>] ? wait_on_buffer+0x2c/0x2c
 [<c02b5a4b>] ? ext4_writepage+0x3b8/0x3b8
 [<c02b5a5b>] __writepage+0x10/0x2e
 [<c0225956>] write_cache_pages+0x22d/0x32c
 [<c02b5a4b>] ? ext4_writepage+0x3b8/0x3b8
 [<c02b6ee8>] ext4_writepages+0x102/0x607
 [<c019adfe>] ? sched_clock_local+0x10/0x10e
 [<c01a8a7c>] ? __lock_is_held+0x2e/0x44
 [<c01a8ad5>] ? lock_is_held+0x43/0x51
 [<c0226dff>] do_writepages+0x1c/0x29
 [<c0276bed>] __writeback_single_inode+0xc3/0x545
 [<c0277c07>] writeback_sb_inodes+0x21f/0x36d
    ...

Signed-off-by Theodore Ts'o <[email protected]>

Cc [email protected]

---
 fs/ext4/inode.c | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)

fs/ext4/inode.c +19 -4

index 0554b0b..263a46c 100644
@@ -1701,19 +1701,32 @@ static int __ext4_journalled_writepage(struct page *page,
 		ext4_walk_page_buffers(handle, page_bufs, 0, len,
 				       NULL, bget_one);
 	}
-	/* As soon as we unlock the page, it can go away, but we have
-	 * references to buffers so we are safe */
+	/*
+	 * We need to release the page lock before we start the
+	 * journal, so grab a reference so the page won't disappear
+	 * out from under us.
+	 */
+	get_page(page);
 	unlock_page(page);
 
 	handle = ext4_journal_start(inode, EXT4_HT_WRITE_PAGE,
 				    ext4_writepage_trans_blocks(inode));
 	if (IS_ERR(handle)) {
 		ret = PTR_ERR(handle);
-		goto out;
+		put_page(page);
+		goto out_no_pagelock;
 	}
-
 	BUG_ON(!ext4_handle_valid(handle));
 
+	lock_page(page);
+	put_page(page);
+	if (page->mapping != mapping) {
+		/* The page got truncated from under us */
+		ext4_journal_stop(handle);
+		ret = 0;
+		goto out;
+	}
+
 	if (inline_data) {
 		BUFFER_TRACE(inode_bh, "get write access");
 		ret = ext4_journal_get_write_access(handle, inode_bh);

@@ -1739,6 +1752,8 @@ static int __ext4_journalled_writepage(struct page *page,
 				       NULL, bput_one);
 	ext4_set_inode_state(inode, EXT4_STATE_JDATA);
 out:
+	unlock_page(page);
+out_no_pagelock:
 	brelse(inode_bh);
 	return ret;
 }

Signature · 1 line

2.3.0

This patch looks promising. I'm running a 1000 trial stress test on a Pandaboard where I've generally been able to force a couple of manifestations of this bug to appear within 5 to 10 runs. Applied to 4.1-rc7, it's passed 135 trials cleanly. The full series will complete sometime tomorrow.

I was also able to reproduce the problem on x86_64 pretty consistently in four runs or less on 4.1-rc7; I'm planning a stress test there as well once -rc8 regression is complete.