writeback data integrity and other fixes (take 3)

Terminate the write_cache_pages loop upon encountering the first page past end, without locking the page. Pages cannot have their index change when we have a reference on them (truncate, eg truncate_inode_pages_range performs the same check without the page lock).

---
Index: linux-2.6/mm/page-writeback.c
===================================================================

mm/page-writeback.c +16 -16

@@ -911,15 +911,24 @@ retry:
 		for (i = 0; i < nr_pages; i++) {
 			struct page *page = pvec.pages[i];
 
-			done_index = page->index + 1;
-
 			/*
-			 * At this point we hold neither mapping->tree_lock nor
-			 * lock on the page itself: the page may be truncated or
-			 * invalidated (changing page->mapping to NULL), or even
-			 * swizzled back from swapper_space to tmpfs file
-			 * mapping
+			 * At this point, the page may be truncated or
+			 * invalidated (changing page->mapping to NULL), or
+			 * even swizzled back from swapper_space to tmpfs file
+			 * mapping. However, page->index will not change
+			 * because we have a reference on the page.
 			 */
+			if (page->index > end) {
+				/*
+				 * can't be range_cyclic (1st pass) because
+				 * end == -1 in that case.
+				 */
+				done = 1;
+				break;
+			}
+
+			done_index = page->index + 1;
+
 			lock_page(page);
 
 			/*

@@ -936,15 +945,6 @@ continue_unlock:
 				continue;
 			}
 
-			if (page->index > end) {
-				/*
-				 * can't be range_cyclic (1st pass) because
-				 * end == -1 in that case.
-				 */
-				done = 1;
-				goto continue_unlock;
-			}
-
 			if (!PageDirty(page)) {
 				/* someone wrote it for us */
 				goto continue_unlock;

Signature · 1 line

-- 

Quoted 5 lines · On Wed, 29 Oct 2008 01:47:22 +1100 [email protected] wrote:

Terminate the write_cache_pages loop upon encountering the first page past end, without locking the page. Pages cannot have their index change when we have a reference on them (truncate, eg truncate_inode_pages_range performs the same check without the page lock).

Traditionally lock_page() is used to stabilise ->index and ->mapping.

Here you introduce a new and very subtle sort-of-locking rule without actually really introducing it at all. OK, there's a little comment buried way down in this function. But there's a contradictory comment over truncate_inode_pages_range() ("When looking at...").

How do we make this new locking rule maintainable? How do we avoid breaking it in the future? How do we prevent accidental breakage from slipping past developers' and reviewers' attention?

Given the additional maintenance burdens, is this change worth doing at all?

Quoted 55 lines

---
Index: linux-2.6/mm/page-writeback.c
===================================================================

mm/page-writeback.c +16 -16

@@ -911,15 +911,24 @@ retry:
 		for (i = 0; i < nr_pages; i++) {
 			struct page *page = pvec.pages[i];
 
-			done_index = page->index + 1;
-
 			/*
-			 * At this point we hold neither mapping->tree_lock nor
-			 * lock on the page itself: the page may be truncated or
-			 * invalidated (changing page->mapping to NULL), or even
-			 * swizzled back from swapper_space to tmpfs file
-			 * mapping
+			 * At this point, the page may be truncated or
+			 * invalidated (changing page->mapping to NULL), or
+			 * even swizzled back from swapper_space to tmpfs file
+			 * mapping. However, page->index will not change
+			 * because we have a reference on the page.
 			 */
+			if (page->index > end) {
+				/*
+				 * can't be range_cyclic (1st pass) because
+				 * end == -1 in that case.
+				 */
+				done = 1;
+				break;
+			}
+
+			done_index = page->index + 1;
+
 			lock_page(page);
 
 			/*

@@ -936,15 +945,6 @@ continue_unlock:
 				continue;
 			}
 
-			if (page->index > end) {
-				/*
-				 * can't be range_cyclic (1st pass) because
-				 * end == -1 in that case.
-				 */
-				done = 1;
-				goto continue_unlock;
-			}
-
 			if (!PageDirty(page)) {
 				/* someone wrote it for us */
 				goto continue_unlock;

Signature · 1 line

-- 

Chris Mason notices do_sync_mapping_range didn't actually ask for data integrity writeout. Unfortunately, it is advertised as being usable for data integrity operations.

This is a data interity bug.

Signed-off-by Nick Piggin <[email protected]>

---
Index: linux-2.6/fs/sync.c
===================================================================

Quoted 14 lines · On Wed, 29 Oct 2008 01:47:24 +1100 [email protected] wrote:

Chris Mason notices do_sync_mapping_range didn't actually ask for data integrity writeout. Unfortunately, it is advertised as being usable for data integrity operations.

This is a data interity bug.

Signed-off-by Nick Piggin <[email protected]>

---
Index: linux-2.6/fs/sync.c
===================================================================

fs/sync.c int do_sync_mapping_range(struct address · line 275

Quoted 8 lines

fs/sync.c +1 -1

@@ -269,7 +269,7 @@ int do_sync_mapping_range(struct address
 
 	if (flags & SYNC_FILE_RANGE_WRITE) {
 		ret = __filemap_fdatawrite_range(mapping, offset, endbyte,
-						WB_SYNC_NONE);
+						WB_SYNC_ALL);
 		if (ret < 0)
 			goto out;
 	}

Really?

Some thought did go into the code which you're "fixing".

If the caller
is using sync_file_range() for integrity then the caller has done a SYNC_FILE_RANGE_WAIT_BEFORE.

So all we need to guarantee here is that __filemap_fdatawrite_range(WB_SYNC_NONE) will start writeout on all dirty pages in the range. Probably that gets broken lower down as part of various hacks^woptimisations have gone in, but which ones, and where? Perhaps _this_ (if it's there) is what should be fixed.

And if we _do_ make the above change, we don't need to run the wait_on_page_writeback_range() if userspace asked for SYNC_FILE_RANGE_WAIT_BEFORE|SYNC_FILE_RANGE_WRITE, yes?

IOW, I don't think enough thought (or at least description of that thought) has gone into this one.

Quoted 6 lines

So all we need to guarantee here is that __filemap_fdatawrite_range(WB_SYNC_NONE) will start writeout on all dirty pages in the range. Probably that gets broken lower down as part of various hacks^woptimisations have gone in, but which ones, and where? Perhaps _this_ (if it's there) is what should be fixed.

WB_SYNC_NONE has never (until this was introduced) been used for data integrity AFAIKS. There is code littered throughout fs/ which assumes WB_SYNC_NONE ~= "efficiency / background writeback". At least definitely the buffer.c "trylock" will cause dirty pages to be skipped.

Quoted 7 lines · On Fri, 31 Oct 2008 10:16:16 +0100 Nick Piggin <[email protected]> wrote:

Quoted 6 lines

So all we need to guarantee here is that __filemap_fdatawrite_range(WB_SYNC_NONE) will start writeout on all dirty pages in the range. Probably that gets broken lower down as part of various hacks^woptimisations have gone in, but which ones, and where? Perhaps _this_ (if it's there) is what should be fixed.

Quoted 5 lines

WB_SYNC_NONE has never (until this was introduced) been used for data integrity AFAIKS. There is code littered throughout fs/ which assumes WB_SYNC_NONE ~= "efficiency / background writeback". At least definitely the buffer.c "trylock" will cause dirty pages to be skipped.

The change seriously wrecks sync_file_range(SYNC_FILE_RANGE_WRITE), the whole point of which is to as-nonblockingly-as-possible shove some data into the queues. This is useful.

Perhaps we could use WB_SYNC_ALL if SYNC_FILE_RANGE_WAIT_BEFORE was specified, or WB_SYNC_NONE if it was not (need to think about SYNC_FILE_RANGE_WAIT_AFTER, too).

That's a bit grubby, because userspace could do

	sync_file_range(SYNC_FILE_RANGE_WAIT_BEFORE);
	sync_file_range(SYNC_FILE_RANGE_WRITE);

expecting it to have the same behaviour as

	sync_file_range(SYNC_FILE_RANGE_WAIT_BEFORE|SYNC_FILE_RANGE_WRITE);

How the hell did that stupid sync_mode thing get into writeback_control? :( We should get rid of WB_SYNC_FOO and migrate to better-defined writeback_control fields.

That's a bit grubby, because userspace could do

	sync_file_range(SYNC_FILE_RANGE_WAIT_BEFORE);
	sync_file_range(SYNC_FILE_RANGE_WRITE);

expecting it to have the same behaviour as

	sync_file_range(SYNC_FILE_RANGE_WAIT_BEFORE|SYNC_FILE_RANGE_WRITE);

Quoted 4 lines · Andrew Morton

Quoted 1 line · On Wed, 29 Oct 2008 01:47:24 +1100 [email protected] wrote:

Chris Mason notices do_sync_mapping_range didn't actually ask for

data

Quoted 1 line

Quoted 1 line

integrity writeout. Unfortunately, it is advertised as being

usable for

Quoted 4 lines

Quoted 4 lines

data integrity operations.

This is a data interity bug.

Quoted 3 lines

If the caller
is using sync_file_range() for integrity then the caller has done a SYNC_FILE_RANGE_WAIT_BEFORE.

No disputes about whether the API works "by design". But I think the implementation has a bug. I'll explain:

UID: 518429

Quoted 12 lines · Nick Piggin

Quoted 4 lines · Andrew Morton

Quoted 1 line · On Wed, 29 Oct 2008 01:47:24 +1100 [email protected] wrote:

Chris Mason notices do_sync_mapping_range didn't actually ask for

data

Quoted 1 line

Quoted 1 line

integrity writeout. Unfortunately, it is advertised as being

usable for

Quoted 4 lines

Quoted 4 lines

data integrity operations.

This is a data interity bug.

[ use WB_SYNC_ALL instead of WB_SYNC_NONE ]

Quoted 6 lines

Quoted 3 lines

If the caller
is using sync_file_range() for integrity then the caller has done a SYNC_FILE_RANGE_WAIT_BEFORE.

No disputes about whether the API works "by design". But I think the implementation has a bug. I'll explain:

I'll definitely agree the current usage is clumsy, and there is a bug in fs/buffer.c. A grep through the rest of the filesystems doesn't turn up many assumptions that WB_SYNC_NONE means it's ok to skip dirty pages.

Greps for WB_SYNC_ALL and WB_SYNC_NONE in the fs code reveal:

fs/buffer.c:__block_write_full_page()

               if (wbc->sync_mode != WB_SYNC_NONE || !wbc->nonblocking) {
                        lock_buffer(bh);
                } else if (!trylock_buffer(bh)) {
                        redirty_page_for_writepage(wbc, page);
                        continue;
                }

Easily fixed s/||/&&/, which is what XFS does. reiser3 has the same bug in fs/reiserfs/inode.c

ntfs and gfs2 each have a check that assumes WB_SYNC_NONE means optional writeback, both seem fixable with one liners.

Quoted 3 lines

ntfs and gfs2 each have a check that assumes WB_SYNC_NONE means optional writeback, both seem fixable with one liners.

I doubt that is the case for GFS2... Its a bit subtle, but in order to ensure that we have the correct lock ordering the code in writepage for jdata (the only place this occurs, and I assume is what you are referring to) relies in the fact that because we have a writepages set for jdata, writepage will only ever be called with WB_SYNC_NONE and can thus be skipped.

Quoted 12 lines · Nick Piggin

Quoted 4 lines · Andrew Morton

Quoted 1 line · On Wed, 29 Oct 2008 01:47:24 +1100 [email protected] wrote:

Chris Mason notices do_sync_mapping_range didn't actually ask for

data

Quoted 1 line

Quoted 1 line

integrity writeout. Unfortunately, it is advertised as being

usable for

Quoted 4 lines

Quoted 4 lines

data integrity operations.

This is a data interity bug.

[ use WB_SYNC_ALL instead of WB_SYNC_NONE ]

Quoted 6 lines

Quoted 3 lines

If the caller
is using sync_file_range() for integrity then the caller has done a SYNC_FILE_RANGE_WAIT_BEFORE.

No disputes about whether the API works "by design". But I think the implementation has a bug. I'll explain:

I'll definitely agree the current usage is clumsy, and there is a bug in fs/buffer.c. A grep through the rest of the filesystems doesn't turn up many assumptions that WB_SYNC_NONE means it's ok to skip dirty pages.

Greps for WB_SYNC_ALL and WB_SYNC_NONE in the fs code reveal:

fs/buffer.c:__block_write_full_page()

               if (wbc->sync_mode != WB_SYNC_NONE || !wbc->nonblocking) {
                        lock_buffer(bh);
                } else if (!trylock_buffer(bh)) {
                        redirty_page_for_writepage(wbc, page);
                        continue;
                }

Easily fixed s/||/&&/, which is what XFS does. reiser3 has the same bug in fs/reiserfs/inode.c

ntfs and gfs2 each have a check that assumes WB_SYNC_NONE means optional writeback, both seem fixable with one liners.

Everywhere we wait on page writeback while we're trying to build nice big bios hurts performance. I'd rather see us switch to something closer to the do_sync_mapping_range expectation of WB_SYNC_NONE than sprinkle WB_SYNC_ALLs everywhere.

OK, I'm happier with this patchset now. Note that I've taken your patch and mangled it a bit at the end of the series.

This one survives and seems to run OK here, but I'm mainly doing dumb stress testing with a handful of filesystems, and data-io error injection testing. There are a lot of combinations of ways this function can operate and interact obviously, so it would be helpful to get more review.

Chris, would you possibly have time to run your btrfs tests that are sensitive to problems in this code? I could provide you a single patch rollup against mainline if it helps.

Quoted 11 lines · On Wed, Oct 29, 2008 at 01:47:15AM +1100, [email protected] wrote:

OK, I'm happier with this patchset now. Note that I've taken your patch and mangled it a bit at the end of the series.

This one survives and seems to run OK here, but I'm mainly doing dumb stress testing with a handful of filesystems, and data-io error injection testing. There are a lot of combinations of ways this function can operate and interact obviously, so it would be helpful to get more review.

Chris, would you possibly have time to run your btrfs tests that are sensitive to problems in this code? I could provide you a single patch rollup against mainline if it helps.

BTW. XFS seems to be doing something interesting with my simple sync test case with IO error injection. I map a file MAP_SHARED into a number of processes, which then each run a loop that dirties the memory then calls msync(MS_SYNC) on the range.

ext2 mostly reports -EIO back to userspace when a failure is injected AFAIKS. ext3 (ordered) doesn't until a lot of errors have been injected, but eventually reports -EIO and shuts down the filesystem. reiserfs seems to report failure more consistently.

I haven't seen any -EIO failures from XFS... maybe I'm just not doing the right thing, or there is a caveat I'm not aware of.

All fault injections I noticed had a trace like this:
FAULT_INJECTION: forcing a failure

Call Trace:
9f9cd758:  [<6019f1de>] random32+0xe/0x20
9f9cd768:  [<601a31b9>] should_fail+0xd9/0x130
9f9cd798:  [<6018d0c4>] generic_make_request+0x304/0x4e0
9f9cd7a8:  [<60062301>] mempool_alloc+0x51/0x130
9f9cd858:  [<6018e6bf>] submit_bio+0x4f/0xe0
9f9cd8a8:  [<60165505>] xfs_submit_ioend_bio+0x25/0x40
9f9cd8c8:  [<6016603c>] xfs_submit_ioend+0xbc/0xf0
9f9cd908:  [<60166bf9>] xfs_page_state_convert+0x3d9/0x6a0
9f9cd928:  [<6005d515>] delayacct_end+0x95/0xb0
9f9cda08:  [<60166ffd>] xfs_vm_writepage+0x6d/0x110
9f9cda18:  [<6006618b>] set_page_dirty+0x4b/0xd0
9f9cda58:  [<60066115>] __writepage+0x15/0x40
9f9cda78:  [<60066775>] write_cache_pages+0x255/0x470
9f9cda90:  [<60066100>] __writepage+0x0/0x40
9f9cdb98:  [<600669b0>] generic_writepages+0x20/0x30
9f9cdba8:  [<60165ba3>] xfs_vm_writepages+0x53/0x70

9f9cdbd8:  [<600669eb>] do_writepages+0x2b/0x40
9f9cdbf8:  [<6006004c>] __filemap_fdatawrite_range+0x5c/0x70
9f9cdc58:  [<6006026a>] filemap_fdatawrite+0x1a/0x20
9f9cdc68:  [<600a7a05>] do_fsync+0x45/0xe0
9f9cdc98:  [<6007794b>] sys_msync+0x14b/0x1d0
9f9cdcf8:  [<60019a70>] handle_syscall+0x50/0x80
9f9cdd18:  [<6002a10f>] userspace+0x44f/0x510
9f9cdfc8:  [<60016792>] fork_handler+0x62/0x70

And the kernel would sometimes say this:
Buffer I/O error on device ram0, logical block 279 lost page write due to I/O error on ram0 Buffer I/O error on device ram0, logical block 379 lost page write due to I/O error on ram0 Buffer I/O error on device ram0, logical block 389 lost page write due to I/O error on ram0

I think I also saw a slab bug when running dbench with fault injection on. Running latest Linus kernel.

bash-3.1# dbench -t10 -c ../client.txt 8
dbench version 3.04 - Copyright Andrew Tridgell 1999-2004

Running for 10 seconds with load '../client.txt' and minimum warmup 2 secs 8 clients started
FAULT_INJECTION: forcing a failure

Call Trace:
9e7bb548:  [<601623ae>] random32+0xe/0x20
9e7bb558:  [<60166389>] should_fail+0xd9/0x130
9e7bb588:  [<60150294>] generic_make_request+0x304/0x4e0
9e7bb598:  [<60062301>] mempool_alloc+0x51/0x130
9e7bb648:  [<6015188f>] submit_bio+0x4f/0xe0
9e7bb698:  [<6012b440>] _xfs_buf_ioapply+0x180/0x2a0
9e7bb6a0:  [<6002f600>] default_wake_function+0x0/0x10
9e7bb6f8:  [<6012bae1>] xfs_buf_iorequest+0x31/0x90
9e7bb718:  [<60112f75>] xlog_bdstrat_cb+0x45/0x50
9e7bb738:  [<60114135>] xlog_sync+0x195/0x440
9e7bb778:  [<60114491>] xlog_state_release_iclog+0xb1/0xc0
9e7bb7a8:  [<60114ca9>] xlog_write+0x539/0x550
9e7bb858:  [<60114e60>] xfs_log_write+0x40/0x60
9e7bb888:  [<6011fbaa>] _xfs_trans_commit+0x19a/0x360
9e7bb8b8:  [<600838e2>] poison_obj+0x42/0x60
9e7bb8d0:  [<60082cb3>] dbg_redzone1+0x13/0x30
9e7bb8e8:  [<60083999>] cache_alloc_debugcheck_after+0x99/0x1c0
9e7bb918:  [<6008517b>] kmem_cache_alloc+0x8b/0x100
9e7bb958:  [<60128084>] kmem_zone_alloc+0x74/0xe0
9e7bb998:  [<60082ad9>] kmem_cache_size+0x9/0x10
9e7bb9a8:  [<60128124>] kmem_zone_zalloc+0x34/0x50
9e7bb9e8:  [<60121e8b>] xfs_dir_ialloc+0x13b/0x2e0
9e7bba58:  [<601f534b>] __down_write+0xb/0x10
9e7bbaa8:  [<60125b9e>] xfs_mkdir+0x37e/0x4b0
9e7bbb38:  [<601f5589>] _spin_unlock+0x9/0x10
9e7bbb78:  [<601301a4>] xfs_vn_mknod+0xf4/0x1a0
9e7bbbd8:  [<6013025e>] xfs_vn_mkdir+0xe/0x10
9e7bbbe8:  [<60091010>] vfs_mkdir+0x90/0xc0
9e7bbc18:  [<600934d6>] sys_mkdirat+0x106/0x120
9e7bbc88:  [<6008629b>] filp_close+0x4b/0x80
9e7bbce8:  [<60093503>] sys_mkdir+0x13/0x20
9e7bbcf8:  [<60019a70>] handle_syscall+0x50/0x80
9e7bbd18:  [<6002a10f>] userspace+0x44f/0x510
9e7bbfc8:  [<60016792>] fork_handler+0x62/0x70

I/O error in filesystem ("ram0") meta-data dev ram0 block 0x8002c ("xlog_i
odone") error 5 buf count 32768
xfs_force_shutdown(ram0,0x2) called from line 1056 of file /home/npiggin/usr/src /linux-2.6/fs/xfs/xfs_log.c. Return address = 0x000000006011370d Filesystem "ram0": Log I/O Error Detected. Shutting down filesystem: ram0 Please umount the filesystem, and rectify the problem(s) xfs_force_shutdown(ram0,0x2) called from line 818 of file /home/npiggin/usr/src/ linux-2.6/fs/xfs/xfs_log.c. Return address = 0x0000000060114e7d slab error in verify_redzone_free(): cache `xfs_log_ticket': double free detecte d

Call Trace:
9e7bb998:  [<6008372f>] __slab_error+0x1f/0x30
9e7bb9a8:  [<60083cae>] cache_free_debugcheck+0x1ee/0x240
9e7bb9b0:  [<60112ef0>] xlog_ticket_put+0x10/0x20
9e7bb9e8:  [<60083f70>] kmem_cache_free+0x50/0xc0
9e7bba18:  [<60112ef0>] xlog_ticket_put+0x10/0x20
9e7bba28:  [<60114dc9>] xfs_log_done+0x59/0xb0
9e7bba68:  [<6011f5de>] xfs_trans_cancel+0x7e/0x140
9e7bbaa8:  [<60125a1e>] xfs_mkdir+0x1fe/0x4b0
9e7bbb38:  [<601f5589>] _spin_unlock+0x9/0x10
9e7bbb78:  [<601301a4>] xfs_vn_mknod+0xf4/0x1a0

9e7bbbd8:  [<6013025e>] xfs_vn_mkdir+0xe/0x10
9e7bbbe8:  [<60091010>] vfs_mkdir+0x90/0xc0
9e7bbc18:  [<600934d6>] sys_mkdirat+0x106/0x120
9e7bbc88:  [<6008629b>] filp_close+0x4b/0x80
9e7bbce8:  [<60093503>] sys_mkdir+0x13/0x20
9e7bbcf8:  [<60019a70>] handle_syscall+0x50/0x80
9e7bbd18:  [<6002a10f>] userspace+0x44f/0x510
9e7bbfc8:  [<60016792>] fork_handler+0x62/0x70

Quoted 5 lines · Nick Piggin

On Wed, Oct 29, 2008 at 01:47:15AM +1100, [email protected] wrote:
I haven't seen any -EIO failures from XFS... maybe I'm just not doing the right thing, or there is a caveat I'm not aware of.

All fault injections I noticed had a trace like this:

XFS reports bio errors through the I/O completion path, not the submission path.

Quoted 7 lines

And the kernel would sometimes say this:
Buffer I/O error on device ram0, logical block 279 lost page write due to I/O error on ram0 Buffer I/O error on device ram0, logical block 379 lost page write due to I/O error on ram0 Buffer I/O error on device ram0, logical block 389 lost page write due to I/O error on ram0

Yes - that's coming from end_buffer_async_write() when an error is reported in bio completion. This does:

 465                 set_bit(AS_EIO, &page->mapping->flags);
 466                 set_buffer_write_io_error(bh);
 467                 clear_buffer_uptodate(bh);
 468                 SetPageError(page);

Hmmmm - do_fsync() calls filemap_fdatawait() which ends up in wait_on_page_writeback_range() which is appears to be checking the mapping flags for errors. I wonder why that error is not being propagated then? AFAICT both XFS and the fsync code are doing the right thing but somewhere the error has gone missing...

Quoted 2 lines

I think I also saw a slab bug when running dbench with fault injection on. Running latest Linus kernel.

Now that is interesting.

We've got a rolling transaction in progress, and the commit of the first part of the transaction has got the I/O error. That frees the transaction structure used during that commit, as well as the ticket.

However, before we committed the initial transaction, we duplicated the transaction structure to allow the transaction to continue to track all the dirty objects in the first commit. That included duplicating the pointer to the ticket.

Then the EIO is returned to mkdir code with the duplicated transaction, which is then cancelled, and that frees the transaction and the ticket it holds. However, we'd already freed the ticket.

Ok, we're only seeing this problem now because I recently modified the ticket allocation to use a slab instead of a roll-your-own free list structure that wouldn't have been poisoned. Nice to know that this change did more than just remove code. ;)

This might take a little while to fix - a lot of code needs auditing - but thanks for reporting the problem.

Quoted 23 lines · Nick Piggin

I haven't seen any -EIO failures from XFS... maybe I'm just not doing the right thing, or there is a caveat I'm not aware of.

All fault injections I noticed had a trace like this:
FAULT_INJECTION: forcing a failure

Call Trace:
9f9cd758:  [<6019f1de>] random32+0xe/0x20
9f9cd768:  [<601a31b9>] should_fail+0xd9/0x130
9f9cd798:  [<6018d0c4>] generic_make_request+0x304/0x4e0
9f9cd7a8:  [<60062301>] mempool_alloc+0x51/0x130
9f9cd858:  [<6018e6bf>] submit_bio+0x4f/0xe0
9f9cd8a8:  [<60165505>] xfs_submit_ioend_bio+0x25/0x40
9f9cd8c8:  [<6016603c>] xfs_submit_ioend+0xbc/0xf0
9f9cd908:  [<60166bf9>] xfs_page_state_convert+0x3d9/0x6a0
9f9cd928:  [<6005d515>] delayacct_end+0x95/0xb0
9f9cda08:  [<60166ffd>] xfs_vm_writepage+0x6d/0x110
9f9cda18:  [<6006618b>] set_page_dirty+0x4b/0xd0
9f9cda58:  [<60066115>] __writepage+0x15/0x40
9f9cda78:  [<60066775>] write_cache_pages+0x255/0x470
9f9cda90:  [<60066100>] __writepage+0x0/0x40
9f9cdb98:  [<600669b0>] generic_writepages+0x20/0x30
9f9cdba8:  [<60165ba3>] xfs_vm_writepages+0x53/0x70

Quoted 8 lines

9f9cdbd8:  [<600669eb>] do_writepages+0x2b/0x40
9f9cdbf8:  [<6006004c>] __filemap_fdatawrite_range+0x5c/0x70
9f9cdc58:  [<6006026a>] filemap_fdatawrite+0x1a/0x20
9f9cdc68:  [<600a7a05>] do_fsync+0x45/0xe0
9f9cdc98:  [<6007794b>] sys_msync+0x14b/0x1d0
9f9cdcf8:  [<60019a70>] handle_syscall+0x50/0x80
9f9cdd18:  [<6002a10f>] userspace+0x44f/0x510
9f9cdfc8:  [<60016792>] fork_handler+0x62/0x70

XFS reports bio errors through the I/O completion path, not the submission path.

Quoted 7 lines

And the kernel would sometimes say this:
Buffer I/O error on device ram0, logical block 279 lost page write due to I/O error on ram0 Buffer I/O error on device ram0, logical block 379 lost page write due to I/O error on ram0 Buffer I/O error on device ram0, logical block 389 lost page write due to I/O error on ram0

Yes - that's coming from end_buffer_async_write() when an error is reported in bio completion. This does:

 465                 set_bit(AS_EIO, &page->mapping->flags);
 466                 set_buffer_write_io_error(bh);
 467                 clear_buffer_uptodate(bh);
 468                 SetPageError(page);

Hmmmm - do_fsync() calls filemap_fdatawait() which ends up in wait_on_page_writeback_range() which is appears to be checking the mapping flags for errors. I wonder why that error is not being propagated then? AFAICT both XFS and the fsync code are doing the right thing but somewhere the error has gone missing...

Quoted 9 lines · Dave Chinner

On Tue, Oct 28, 2008 at 04:39:53PM +0100, Nick Piggin wrote:
Yes - that's coming from end_buffer_async_write() when an error is reported in bio completion. This does:

 465                 set_bit(AS_EIO, &page->mapping->flags);
 466                 set_buffer_write_io_error(bh);
 467                 clear_buffer_uptodate(bh);
 468                 SetPageError(page);

Quoted 5 lines

Hmmmm - do_fsync() calls filemap_fdatawait() which ends up in wait_on_page_writeback_range() which is appears to be checking the mapping flags for errors. I wonder why that error is not being propagated then? AFAICT both XFS and the fsync code are doing the right thing but somewhere the error has gone missing...

This one-liner has it reporting EIO errors like a champion. I don't know if you'll actually need to put this into the linux API layer or not, but anyway the root cause of the problem AFAIKS is this.
--

XFS: fix fsync errors not being propogated back to userspace.
---
Index: linux-2.6/fs/xfs/xfs_vnodeops.c
===================================================================

fs/xfs/xfs_vnodeops.c xfs_fsync( · line 718

Quoted 12 lines · Nick Piggin

XFS: fix fsync errors not being propogated back to userspace.
---
Index: linux-2.6/fs/xfs/xfs_vnodeops.c
===================================================================

fs/xfs/xfs_vnodeops.c +1 -1

@@ -715,7 +715,7 @@ xfs_fsync(
 	/* capture size updates in I/O completion before writing the inode. */
 	error = filemap_fdatawait(VFS_I(ip)->i_mapping);
 	if (error)
-		return XFS_ERROR(error);
+		return XFS_ERROR(-error);

<groan>

Yeah, that'd do it. Good catch. I can't believe I recently fixed a bug that touched these lines of code without noticing the inversion. Sometimes I wonder if we should just conver the entire of XFS to return negative errors - mistakes in handling negative error numbers in the core XFS code happen all the time.

Quoted 16 lines · Dave Chinner

fs/xfs/xfs_vnodeops.c xfs_fsync( · line 718

Quoted 12 lines · Nick Piggin

XFS: fix fsync errors not being propogated back to userspace.
---
Index: linux-2.6/fs/xfs/xfs_vnodeops.c
===================================================================

fs/xfs/xfs_vnodeops.c +1 -1

@@ -715,7 +715,7 @@ xfs_fsync(
 	/* capture size updates in I/O completion before writing the inode. */
 	error = filemap_fdatawait(VFS_I(ip)->i_mapping);
 	if (error)
-		return XFS_ERROR(error);
+		return XFS_ERROR(-error);

<groan>

Quoted 5 lines

Yeah, that'd do it. Good catch. I can't believe I recently fixed a bug that touched these lines of code without noticing the inversion. Sometimes I wonder if we should just conver the entire of XFS to return negative errors - mistakes in handling negative error numbers in the core XFS code happen all the time.

Ok, I was right - these problems happen all the time. The above call should really call xfs_flush_pages() to do the flush and wait. I note that xfs_flush_pages() returns negative errors, and all the callers expect positive errors. I bet the same occurs for xfs_flushinval_pages() and xfs_tosspages() which are the wrappers that core XFS code is supposed to be using for flushing and invalidating file ranges....

Quoted 23 lines · Dave Chinner

Quoted 16 lines · Dave Chinner

fs/xfs/xfs_vnodeops.c xfs_fsync( · line 718

Quoted 12 lines · Nick Piggin

XFS: fix fsync errors not being propogated back to userspace.
---
Index: linux-2.6/fs/xfs/xfs_vnodeops.c
===================================================================

fs/xfs/xfs_vnodeops.c +1 -1

@@ -715,7 +715,7 @@ xfs_fsync(
 	/* capture size updates in I/O completion before writing the inode. */
 	error = filemap_fdatawait(VFS_I(ip)->i_mapping);
 	if (error)
-		return XFS_ERROR(error);
+		return XFS_ERROR(-error);

<groan>

Quoted 5 lines

Yeah, that'd do it. Good catch. I can't believe I recently fixed a bug that touched these lines of code without noticing the inversion. Sometimes I wonder if we should just conver the entire of XFS to return negative errors - mistakes in handling negative error numbers in the core XFS code happen all the time.

Quoted 7 lines

Ok, I was right - these problems happen all the time. The above call should really call xfs_flush_pages() to do the flush and wait. I note that xfs_flush_pages() returns negative errors, and all the callers expect positive errors. I bet the same occurs for xfs_flushinval_pages() and xfs_tosspages() which are the wrappers that core XFS code is supposed to be using for flushing and invalidating file ranges....

Just be careful -- in your xfs_flush_pages, I think after the first filemap_fdatawrite, the mapping may no longer be tagged with PAGECACHE_TAG_DIRTY, so you may not pick up those writeback ones you need to wait on.

Might need a different variant, or we could just bite the bullet and push through the ->fsync conversion so you get full control of the writeout.

BTW. the Linux pagecache APIs should support range operations quite nicely for these. Any reason not to use them (it looks like the wrappers can take ranges)?

Quoted 4 lines · Nick Piggin

Just be careful -- in your xfs_flush_pages, I think after the first filemap_fdatawrite, the mapping may no longer be tagged with PAGECACHE_TAG_DIRTY, so you may not pick up those writeback ones you need to wait on.

Yes, I realised this as soon as I looked at the code. I added xfs_wait_on_pages() to do this wait. ;)

Quoted 3 lines

Might need a different variant, or we could just bite the bullet and push through the ->fsync conversion so you get full control of the writeout.

Not important right now, though....

Quoted 3 lines

BTW. the Linux pagecache APIs should support range operations quite nicely for these. Any reason not to use them (it looks like the wrappers can take ranges)?

Because I haven't got around to modifying these wrappers now that the range primitives are in place - XFS inherited the range operations from Irix and have sat unimplemented since being ported to Linux.

The patch below should fix this entire class of error value screwup in XFS. I've just started running it through XFSQA, so it's not really tested yet.

FWIW, your entire patch series made it through XFSQA without any new regressions, so it looks good from that POV.

Cheers,

Dave.

Signature · 24 lines

Dave Chinner
[email protected]

XFS: fix error inversion problems with data flushing

XFS gets the sign of the error wrong in several places when
gathering the error from generic linux functions. These functions
return negative error values, while the core XFS code returns
positive error values. Hence when XFS inverts the error to be
returned to the VFS, it can incorrectly invert a negative
error and this error will be ignored by the syscall return.

Fix all the problems related to calling filemap_* functions.

Problem initially identified by Nick Piggin in xfs_fsync().

Signed-off-by: Dave Chinner <[email protected]>
---
 fs/xfs/linux-2.6/xfs_fs_subr.c |   23 ++++++++++++++++++++---
 fs/xfs/linux-2.6/xfs_lrw.c     |    2 +-
 fs/xfs/linux-2.6/xfs_super.c   |   13 +++++++++----
 fs/xfs/xfs_vnodeops.c          |    2 +-
 fs/xfs/xfs_vnodeops.h          |    1 +
 5 files changed, 32 insertions(+), 9 deletions(-)

fs/xfs/linux-2.6/xfs_fs_subr.c +20 -3

index 36caa6d..5aeb777 100644
@@ -24,6 +24,10 @@ int  fs_noerr(void) { return 0; }
 int  fs_nosys(void) { return ENOSYS; }
 void fs_noval(void) { return; }
 
+/*
+ * note: all filemap functions return negative error codes. These
+ * need to be inverted before returning to the xfs core functions.
+ */
 void
 xfs_tosspages(
 	xfs_inode_t	*ip,

@@ -53,7 +57,7 @@ xfs_flushinval_pages(
 		if (!ret)
 			truncate_inode_pages(mapping, first);
 	}
-	return ret;
+	return -ret;
 }
 
 int

@@ -72,10 +76,23 @@ xfs_flush_pages(
 		xfs_iflags_clear(ip, XFS_ITRUNCATED);
 		ret = filemap_fdatawrite(mapping);
 		if (flags & XFS_B_ASYNC)
-			return ret;
+			return -ret;
 		ret2 = filemap_fdatawait(mapping);
 		if (!ret)
 			ret = ret2;
 	}
-	return ret;
+	return -ret;
+}
+
+int
+xfs_wait_on_pages(
+	xfs_inode_t	*ip,
+	xfs_off_t	first,
+	xfs_off_t	last)
+{
+	struct address_space *mapping = VFS_I(ip)->i_mapping;
+
+	if (mapping_tagged(mapping, PAGECACHE_TAG_WRITEBACK))
+		return -filemap_fdatawait(mapping);
+	return 0;
 }

fs/xfs/linux-2.6/xfs_lrw.c +1 -1

index 1957e53..4959c87 100644
@@ -243,7 +243,7 @@ xfs_read(
 
 	if (unlikely(ioflags & IO_ISDIRECT)) {
 		if (inode->i_mapping->nrpages)
-			ret = xfs_flushinval_pages(ip, (*offset & PAGE_CACHE_MASK),
+			ret = -xfs_flushinval_pages(ip, (*offset & PAGE_CACHE_MASK),
 						    -1, FI_REMAPF_LOCKED);
 		mutex_unlock(&inode->i_mutex);
 		if (ret) {

fs/xfs/linux-2.6/xfs_super.c +9 -4

index 9dc977d..c5cfc1e 100644
@@ -1012,21 +1012,26 @@ xfs_fs_write_inode(
 	struct inode		*inode,
 	int			sync)
 {
+	struct xfs_inode	*ip = XFS_I(inode);
 	int			error = 0;
 	int			flags = 0;
 
-	xfs_itrace_entry(XFS_I(inode));
+	xfs_itrace_entry(ip);
 	if (sync) {
-		filemap_fdatawait(inode->i_mapping);
+		error = xfs_wait_on_pages(ip, 0, -1);
+		if (error)
+			goto out_error;
 		flags |= FLUSH_SYNC;
 	}
-	error = xfs_inode_flush(XFS_I(inode), flags);
+	error = xfs_inode_flush(ip, flags);
+
+out_error:
 	/*
 	 * if we failed to write out the inode then mark
 	 * it dirty again so we'll try again later.
 	 */
 	if (error)
-		xfs_mark_inode_dirty_sync(XFS_I(inode));
+		xfs_mark_inode_dirty_sync(ip);
 
 	return -error;
 }

fs/xfs/xfs_vnodeops.c +1 -1

index 2e2fbd9..5809c42 100644
@@ -713,7 +713,7 @@ xfs_fsync(
 		return XFS_ERROR(EIO);
 
 	/* capture size updates in I/O completion before writing the inode. */
-	error = filemap_fdatawait(VFS_I(ip)->i_mapping);
+	error = xfs_wait_on_pages(ip, 0, -1);
 	if (error)
 		return XFS_ERROR(error);
 

fs/xfs/xfs_vnodeops.h +1 -0

index e932a96..f9cd376 100644
@@ -78,5 +78,6 @@ int xfs_flushinval_pages(struct xfs_inode *ip, xfs_off_t first,
 		xfs_off_t last, int fiopt);
 int xfs_flush_pages(struct xfs_inode *ip, xfs_off_t first,
 		xfs_off_t last, uint64_t flags, int fiopt);
+int xfs_wait_on_pages(struct xfs_inode *ip, xfs_off_t first, xfs_off_t last);
 
 #endif /* _XFS_VNODEOPS_H */

Ok, I was right - these problems happen all the time. The above call should really call xfs_flush_pages() to do the flush and wait. I note that xfs_flush_pages() returns negative errors, and all the callers expect positive errors. I bet the same occurs for xfs_flushinval_pages() and xfs_tosspages() which are the wrappers that core XFS code is supposed to be using for flushing and invalidating file ranges....

I'll write up a patch that covers all of these.

Quoted 8 lines · Dave Chinner

Ok, I was right - these problems happen all the time. The above call should really call xfs_flush_pages() to do the flush and wait. I note that xfs_flush_pages() returns negative errors, and all the callers expect positive errors. I bet the same occurs for xfs_flushinval_pages() and xfs_tosspages() which are the wrappers that core XFS code is supposed to be using for flushing and invalidating file ranges....

Quoted 1 line

I'll write up a patch that covers all of these.

Can you also merge xfs_fsync into xfs_file_fsync while you're at it? The split newer made any sense as xfs_fsync is as Linux-specific as it gets and shouldn't be in the pseudo OS-independent layer.

I'll do that as a separate patch - it's not really part of a "fix error value inversion" bug fix....

Quoted 2 lines · Dave Chinner

I'll do that as a separate patch - it's not really part of a "fix error value inversion" bug fix....

Well it is kindof. Because we'd call Linux library functions directly from the method we skip to places that need sign conversion and could possibly go wrong. But if you want a separate patch that's fine with me too.

Quoted 29 lines · Nick Piggin

Quoted 9 lines · Dave Chinner

On Tue, Oct 28, 2008 at 04:39:53PM +0100, Nick Piggin wrote:
Yes - that's coming from end_buffer_async_write() when an error is reported in bio completion. This does:

 465                 set_bit(AS_EIO, &page->mapping->flags);
 466                 set_buffer_write_io_error(bh);
 467                 clear_buffer_uptodate(bh);
 468                 SetPageError(page);

Quoted 5 lines

Hmmmm - do_fsync() calls filemap_fdatawait() which ends up in wait_on_page_writeback_range() which is appears to be checking the mapping flags for errors. I wonder why that error is not being propagated then? AFAICT both XFS and the fsync code are doing the right thing but somewhere the error has gone missing...

This one-liner has it reporting EIO errors like a champion. I don't know if you'll actually need to put this into the linux API layer or not, but anyway the root cause of the problem AFAIKS is this.
--

XFS: fix fsync errors not being propogated back to userspace.
---
Index: linux-2.6/fs/xfs/xfs_vnodeops.c
===================================================================

fs/xfs/xfs_vnodeops.c xfs_fsync( · line 718

Quoted 5 lines

fs/xfs/xfs_vnodeops.c +1 -1

@@ -715,7 +715,7 @@ xfs_fsync(
 	/* capture size updates in I/O completion before writing the inode. */
 	error = filemap_fdatawait(VFS_I(ip)->i_mapping);
 	if (error)
-		return XFS_ERROR(error);
+		return XFS_ERROR(-error);

<groan>

Yeah, that'd do it. Good catch. I can't believe I recently fixed a bug that touched these lines of code without noticing the inversion. Sometimes I wonder if we should just conver the entire of XFS to return negative errors - mistakes in handling negative error numbers in the core XFS code happen all the time.

FWIW, the core issue here is that we've got to do the filemap_fdatawait() call in the ->fsync method because ->fsync gets called before we've waited for the data I/O to complete. XFS updates inode state on I/O completion, so we *must* wait for data I/O to complete before logging the inode changes. I think btrfs has the same problem....

Thanks again, Nick.

Quoted 6 lines · Dave Chinner

FWIW, the core issue here is that we've got to do the filemap_fdatawait() call in the ->fsync method because ->fsync gets called before we've waited for the data I/O to complete. XFS updates inode state on I/O completion, so we *must* wait for data I/O to complete before logging the inode changes. I think btrfs has the same problem....

Interesting. Does that mean you can do without the final filemap_fdatawait?

Can you do the first fdatawait without i_mutex held?

There was some talk IIRC about moving all this logic into the filesystem to provide more flexibility here. If there is still enough interest, we should get that moving...

Yeah, that'd do it. Good catch. I can't believe I recently fixed a bug that touched these lines of code without noticing the inversion. Sometimes I wonder if we should just conver the entire of XFS to return negative errors - mistakes in handling negative error numbers in the core XFS code happen all the time.

FWIW, the core issue here is that we've got to do the filemap_fdatawait() call in the ->fsync method because ->fsync gets called before we've waited for the data I/O to complete. XFS updates inode state on I/O completion, so we *must* wait for data I/O to complete before logging the inode changes. I think btrfs has the same problem....

Quoted 6 lines · Dave Chinner

Yeah, that'd do it. Good catch. I can't believe I recently fixed a bug that touched these lines of code without noticing the inversion. Sometimes I wonder if we should just conver the entire of XFS to return negative errors - mistakes in handling negative error numbers in the core XFS code happen all the time.

Quoted 6 lines

FWIW, the core issue here is that we've got to do the filemap_fdatawait() call in the ->fsync method because ->fsync gets called before we've waited for the data I/O to complete. XFS updates inode state on I/O completion, so we *must* wait for data I/O to complete before logging the inode changes. I think btrfs has the same problem....

Yes. I have patches to fix this by changing what ->fsync does and how it's called. I really need to get them out on the list.

Please do.

Quoted 1 line · Nick Piggin

Please do.

Well, there's one stumling block I haven't made progress on yet:

I've changed the prototype of ->fsync to lose the dentry as we should always have a valid file struct. Except that nfsd doesn't on directories. So I either need to fake up a file there, or bail out and add a ->dir_sync export operation that needs just a dentry.

Quoted 9 lines · Christoph Hellwig

Quoted 1 line · Nick Piggin

Please do.

Well, there's one stumling block I haven't made progress on yet:

I've changed the prototype of ->fsync to lose the dentry as we should always have a valid file struct. Except that nfsd doesn't on directories. So I either need to fake up a file there, or bail out and add a ->dir_sync export operation that needs just a dentry.

OK. I don't know much about hthat code, but I would think nfsd should look as close to the syscall layer as possible. I guess there must be something prohibitive (some protocol semantics?).

Is there anything that particularly makes it a file operation as opposed to an inode operation?

Quoted 13 lines · Nick Piggin

Quoted 11 lines · Christoph Hellwig

Quoted 2 lines · Nick Piggin

Please do.

Well, there's one stumling block I haven't made progress on yet:

I've changed the prototype of ->fsync to lose the dentry as we should always have a valid file struct. Except that nfsd doesn't on directories. So I either need to fake up a file there, or bail out and add a ->dir_sync export operation that needs just a dentry.

Quoted 7 lines

OK. I don't know much about hthat code, but I would think nfsd should look as close to the syscall layer as possible. I guess there must be something prohibitive (some protocol semantics?).

Is there anything that particularly makes it a file operation as opposed to an inode operation?

In principle, is fsync() required to flush all dirty data written through any file descriptor ever, or just dirty data written through the file descriptor used for fsync()?

-- Jamie
--

Quoted 8 lines · Jamie Lokier

Quoted 3 lines

Is there anything that particularly makes it a file operation as opposed to an inode operation?

In principle, is fsync() required to flush all dirty data written through any file descriptor ever, or just dirty data written through the file descriptor used for fsync()?

Quoted 3 lines

-- Jamie
--

http://www.opengroup.org/onlinepubs/009695399/functions/fsync.html

Is a pointer to what seems to be the official posix spec for this - it is definitely per file descriptor, not per file system, etc...

Quoted 10 lines · Ric Wheeler

Quoted 8 lines · Jamie Lokier

Quoted 3 lines

Is there anything that particularly makes it a file operation as opposed to an inode operation?

In principle, is fsync() required to flush all dirty data written through any file descriptor ever, or just dirty data written through the file descriptor used for fsync()?

Quoted 8 lines

Quoted 3 lines

-- Jamie
--

http://www.opengroup.org/onlinepubs/009695399/functions/fsync.html

Is a pointer to what seems to be the official posix spec for this - it is definitely per file descriptor, not per file system, etc...

Maybe I'm reading Jamie's question wrong, but I think he's saying:

/* open exactly the same file twice */
fd = open("file");
fd2 = open("file");

write(fd, "stuff")
write(fd2, "more stuff")
fsync(fd);

Does the fsync promise "more stuff" will be on disk? I think the answer should be yes.

Quoted 26 lines · Chris Mason

Quoted 8 lines · Ric Wheeler

Quoted 7 lines · Jamie Lokier

Quoted 3 lines

Is there anything that particularly makes it a file operation as opposed to an inode operation?

In principle, is fsync() required to flush all dirty data written through any file descriptor ever, or just dirty data written through the file descriptor used for fsync()?

Quoted 7 lines

Quoted 3 lines

-- Jamie
--

http://www.opengroup.org/onlinepubs/009695399/functions/fsync.html

Is a pointer to what seems to be the official posix spec for this - it is definitely per file descriptor, not per file system, etc...

Maybe I'm reading Jamie's question wrong, but I think he's saying:

/* open exactly the same file twice */
fd = open("file");
fd2 = open("file");

write(fd, "stuff")
write(fd2, "more stuff")
fsync(fd);

Quoted 2 lines

Does the fsync promise "more stuff" will be on disk? I think the answer should be yes.

I think so. And this is in the context of making ->fsync an inode operation and avoid the NFS NULL-file problem... I don't think there is any fd specific metadata that fsync has to deal with? Any other reasons it has to be a file operation?

Quoted 15 lines · Nick Piggin

Quoted 9 lines · Christoph Hellwig

Quoted 1 line · Nick Piggin

Please do.

Well, there's one stumling block I haven't made progress on yet:

I've changed the prototype of ->fsync to lose the dentry as we should always have a valid file struct. Except that nfsd doesn't on directories. So I either need to fake up a file there, or bail out and add a ->dir_sync export operation that needs just a dentry.

OK. I don't know much about hthat code, but I would think nfsd should look as close to the syscall layer as possible. I guess there must be something prohibitive (some protocol semantics?).

Quoted 2 lines

Is there anything that particularly makes it a file operation as opposed to an inode operation?

In principle, is fsync() required to flush all dirty data written through any file descriptor ever, or just dirty data written through the file descriptor used for fsync()?

Quoted 3 lines · Nick Piggin

On Wed, Oct 29, 2008 at 01:47:15AM +1100, [email protected] wrote:
I think I also saw a slab bug when running dbench with fault injection on. Running latest Linus kernel.

Quoted 8 lines

xfs_force_shutdown(ram0,0x2) called from line 1056 of file /home/npiggin/usr/src /linux-2.6/fs/xfs/xfs_log.c. Return address = 0x000000006011370d Filesystem "ram0": Log I/O Error Detected. Shutting down filesystem: ram0 Please umount the filesystem, and rectify the problem(s) xfs_force_shutdown(ram0,0x2) called from line 818 of file /home/npiggin/usr/src/ linux-2.6/fs/xfs/xfs_log.c. Return address = 0x0000000060114e7d slab error in verify_redzone_free(): cache `xfs_log_ticket': double free detecte d

Now that is interesting.

We've got a rolling transaction in progress, and the commit of the first part of the transaction has got the I/O error. That frees the transaction structure used during that commit, as well as the ticket.

However, before we committed the initial transaction, we duplicated the transaction structure to allow the transaction to continue to track all the dirty objects in the first commit. That included duplicating the pointer to the ticket.

Then the EIO is returned to mkdir code with the duplicated transaction, which is then cancelled, and that frees the transaction and the ticket it holds. However, we'd already freed the ticket.

Ok, we're only seeing this problem now because I recently modified the ticket allocation to use a slab instead of a roll-your-own free list structure that wouldn't have been poisoned. Nice to know that this change did more than just remove code. ;)

This might take a little while to fix - a lot of code needs auditing - but thanks for reporting the problem.

OK, I'm happier with this patchset now. Note that I've taken your patch and mangled it a bit at the end of the series.

This one survives and seems to run OK here, but I'm mainly doing dumb stress testing with a handful of filesystems, and data-io error injection testing. There are a lot of combinations of ways this function can operate and interact obviously, so it would be helpful to get more review.

Chris, would you possibly have time to run your btrfs tests that are sensitive to problems in this code? I could provide you a single patch rollup against mainline if it helps.

Quoted 11 lines · On Wed, Oct 29, 2008 at 01:47:15AM +1100, [email protected] wrote:

OK, I'm happier with this patchset now. Note that I've taken your patch and mangled it a bit at the end of the series.

This one survives and seems to run OK here, but I'm mainly doing dumb stress testing with a handful of filesystems, and data-io error injection testing. There are a lot of combinations of ways this function can operate and interact obviously, so it would be helpful to get more review.

Chris, would you possibly have time to run your btrfs tests that are sensitive to problems in this code? I could provide you a single patch rollup against mainline if it helps.

Nick, after applying the patchset:

CC mm/page-writeback.o
mm/page-writeback.c: In function write_cache_pages:
mm/page-writeback.c:871: warning: wrteback_index may be used uninitialized in this function

Looks harmless, but it probably should be cleaned up. Compiler is 'gcc version 4.3.1 (Debian 4.3.1-9)'

Quoted 19 lines · Dave Chinner

Quoted 11 lines · On Wed, Oct 29, 2008 at 01:47:15AM +1100, [email protected] wrote:

OK, I'm happier with this patchset now. Note that I've taken your patch and mangled it a bit at the end of the series.

This one survives and seems to run OK here, but I'm mainly doing dumb stress testing with a handful of filesystems, and data-io error injection testing. There are a lot of combinations of ways this function can operate and interact obviously, so it would be helpful to get more review.

Chris, would you possibly have time to run your btrfs tests that are sensitive to problems in this code? I could provide you a single patch rollup against mainline if it helps.

Nick, after applying the patchset:

CC mm/page-writeback.o
mm/page-writeback.c: In function write_cache_pages:
mm/page-writeback.c:871: warning: wrteback_index may be used uninitialized in this function

Quoted 2 lines

Looks harmless, but it probably should be cleaned up. Compiler is 'gcc version 4.3.1 (Debian 4.3.1-9)'

Yeah, it's annoying but I couldn't find a way to shut it up nicely (yes AFAIKS it is harmless).

I could just put the old "= 0; /* shut up gcc*/" trick there, but that sucks too ;(

Quoted 7 lines · On Wed, 29 Oct 2008 00:57:55 +0100 Nick Piggin <[email protected]> wrote:

Yeah, it's annoying but I couldn't find a way to shut it up nicely (yes AFAIKS it is harmless).

I could just put the old "= 0; /* shut up gcc*/" trick there, but that sucks too ;(

uninitialized_var()?