Possible data integrity problems in lots of filesystems?

This is a call for review and comments from all filesystem developers. Not just those cced, but everyone who maintains a filesystem[*], because I wasn't able to put in a lot of time to understand the more complex filesystems.

[*] You all read linux-fsdevel anyway, right? If not, please do, it's pretty low volume and high s/n.

So there seem to be several problems with inode data and metadata synchronization. Some of it is bugs in core code, but also a couple of oft repeated bugs in filesystems.

http://marc.info/?l=linux-fsdevel&m=129052164315259&w=2

Basically 2 patterns of problem.

First is checking inode dirty bits (I_DIRTY/I_DIRTY_SYNC/I_DIRTY_DATASYNC) without locking. What happens is that other paths (async writeout or even a concurrent sync or fsync) can clear these bits with the data not being on platter.

* solution: must wait on I_SYNC before testing these things. See my patch in above linked series. I think I covered everyone here, but please double check.

* There is opportunity in some filesystems for clearing inode dirty bits in fsync, and for checking and avoiding fsync work if dirty bits are not sure.

Second is confusing sync and async inode metadata writeout Core code clears I_DIRTY_SYNC and I_DIRTY_DATASYNC before calling ->write_inode *regardless* of whether it is a for-integrity call or not. This means background writeback can clear it, and subsequent sync_inode_metadata or sync(2) call will skip the next ->write_inode completely.

* solution: for fsync, you must ensure that everything that a synchronous ->write_inode call does is also done by an asynchronous ->write_inode call *plus* a subsequent fsync.

So if a synchronous ->write_inode syncs a bh, but an async one just marks it dirty, your .fsync would have to sync the bh.

* solution: for sync(2), you must ensure that everything that a synchronous ->write_inode call does is also done by an asynchronous ->write_inode call plus a subsequent ->sync_fs call, plus __sync_blockdev call on the buffer mapping.

Many simple filesystems just go via buffer mapping, and ->write_inode simply dirties the inode's bh. These guys are fine here (although most are broken wrt fsync).

If you need any more state bits in your inode to work out what is going on, Christoph's recent hfsplus fsync optimisation patches has a good example of how it can be done.

The ext2 fix below is an example of how we can do this nicely, the rest of the filesystems I note when it looks like they went wrong, and band aid fixed it.

Index: linux-2.6/fs/exofs/inode.c
===================================================================

Index: linux-2.6/fs/exofs/file.c
===================================================================

fs/exofs/file.c +0 -0

@@ -48,11 +48,6 @@ static int exofs_file_fsync(struct file
 	struct inode *inode = filp->f_mapping->host;

Hi Nick.
Thanks for digging into this issue, I bet it's causing pain. Which I totally missed in my tests. I wish I had a better xsync+reboot tests for all this.

So in that previous patch you had:

Quoted 15 lines

Index: linux-2.6/fs/exofs/file.c
===================================================================

fs/exofs/file.c +0 -5

@@ -48,11 +48,6 @@ static int exofs_file_fsync(struct file
 	struct inode *inode = filp->f_mapping->host;
 	struct super_block *sb;
 
-	if (!(inode->i_state & I_DIRTY))
-		return 0;
-	if (datasync && !(inode->i_state & I_DIRTY_DATASYNC))
-		return 0;
-
 	ret = sync_inode_metadata(inode, 1);
 

fs/exofs/file.c static int exofs_file_fsync(struct file · line 53

Quoted 2 lines

fs/exofs/file.c +0 -0

 	/* This is a good place to write the sb */

Is that a good enough fix for the issue in your opinion? Or is there more involved?

In exofs there is nothing special to do other than VFS managment and the final call, by vfs, to .write_inode.

I wish we had a simple_file_fsync() from VFS that does what the VFS expects us to do. So when code evolves it does not need to change all FSs. This is the third time I'm fixing this code trying to second guess the VFS.

Actually the only other thing I need to do in file_fsync today is sb_sync. But this is a stupidity (and a bug) that I'm fixing soon. So that theoretical simple_file_fsync() would be all I need.

Please advise?
BTW: Do you want that I take the changes through my tree?

Quoted 15 lines · Boaz Harrosh

Quoted 15 lines

Index: linux-2.6/fs/exofs/file.c
===================================================================

fs/exofs/file.c +0 -5

@@ -48,11 +48,6 @@ static int exofs_file_fsync(struct file
 	struct inode *inode = filp->f_mapping->host;
 	struct super_block *sb;
 
-	if (!(inode->i_state & I_DIRTY))
-		return 0;
-	if (datasync && !(inode->i_state & I_DIRTY_DATASYNC))
-		return 0;
-
 	ret = sync_inode_metadata(inode, 1);
 

Quoted 5 lines

fs/exofs/file.c static int exofs_file_fsync(struct file · line 53

Quoted 2 lines

fs/exofs/file.c +0 -0

 	/* This is a good place to write the sb */

Is that a good enough fix for the issue in your opinion? Or is there more involved?

For the inode dirty bit race problem, yes it should fix it. sync_inode_metadata basically makes the same checks without races (in a subsequent patch I re-introduced the datasync optimisation).

Well in your fsync, you need to wait for inode writeback that might have been started by an asynchronous write_inode.

Also, with your sync_inode_metadata call, you shouldn't need the sync_inode call by the looks.

Thanks,
Nick

From: Nick Piggin <[email protected]>

It is incorrect to test inode dirty bits without participating in the inode writeback protocol. Inode writeback sets I_SYNC and clears I_DIRTY_?, then writes out the particular bits, then clears I_SYNC when it is done. BTW. it may not completely write all pages out, so I_DIRTY_PAGES would get set again.

This is fine, we'll get it merged before 2.6.37, just give a couple of days for review, I haven't had much confirmation from other vfs people about these problems.

Thanks,
Nick

Quoted 8 lines

From: Nick Piggin <[email protected]>

It is incorrect to test inode dirty bits without participating in the inode writeback protocol. Inode writeback sets I_SYNC and clears I_DIRTY_?, then writes out the particular bits, then clears I_SYNC when it is done. BTW. it may not completely write all pages out, so I_DIRTY_PAGES would get set again.

Quoted 31 lines · Nick Piggin

Quoted 15 lines · Boaz Harrosh

Quoted 15 lines

Index: linux-2.6/fs/exofs/file.c
===================================================================

fs/exofs/file.c +0 -5

@@ -48,11 +48,6 @@ static int exofs_file_fsync(struct file
 	struct inode *inode = filp->f_mapping->host;
 	struct super_block *sb;
 
-	if (!(inode->i_state & I_DIRTY))
-		return 0;
-	if (datasync && !(inode->i_state & I_DIRTY_DATASYNC))
-		return 0;
-
 	ret = sync_inode_metadata(inode, 1);
 

Quoted 5 lines

fs/exofs/file.c static int exofs_file_fsync(struct file · line 53

Quoted 2 lines

fs/exofs/file.c +0 -0

 	/* This is a good place to write the sb */

Is that a good enough fix for the issue in your opinion? Or is there more involved?

For the inode dirty bit race problem, yes it should fix it. sync_inode_metadata basically makes the same checks without races (in a subsequent patch I re-introduced the datasync optimisation).

Quoted 3 lines

Well in your fsync, you need to wait for inode writeback that might have been started by an asynchronous write_inode.

All I'm calling is sync_inode_metadata(,1) which calls sync_inode() which calls writeback_single_inode(sync_mode == WB_SYNC_ALL). It gets a little complicated but from the looks of it, even though the call to .write_inode() is not under any lock the state machine there will do inode_wait_for_writeback() if there was one in motion all ready. ?

And it looks like writeback_single_inode() does all the proper checks in the correct order for these flags above.

So current code in exofs_file_fsync() looks scary to me. I would like to push your above patch for this Kernel. (I'll repost it)

Quoted 3 lines

Also, with your sync_inode_metadata call, you shouldn't need the sync_inode call by the looks.

What? I missed you. You mean I don't need to sync_inode_metadata(,wait==1), or what did you mean?

On Thu, Nov 25, 2010 at 12:51:11PM +0200, Boaz Harrosh wrote:

It does not get it right, because of the situation I described above. Background writeout can come in first, and clear the inode dirty bits, and call your ->write_inode for async writeout.

That means you skip doing the exofs_put_io_state(), and (I presume) this means you aren't waiting for write completion there.

What then happens is that sync_inode_metadata() from your fsync does not call ->write_inode because the inode dirty bits are clear. It's basically a noop. So you need to either make your .write_inode always synchronous, or wait for it in your .fsync and .sync_fs.

Second is confusing sync and async inode metadata writeout Core code clears I_DIRTY_SYNC and I_DIRTY_DATASYNC before calling ->write_inode *regardless* of whether it is a for-integrity call or not. This means background writeback can clear it, and subsequent sync_inode_metadata or sync(2) call will skip the next ->write_inode completely.

Quoted 6 lines · Nick Piggin

Second is confusing sync and async inode metadata writeout Core code clears I_DIRTY_SYNC and I_DIRTY_DATASYNC before calling ->write_inode *regardless* of whether it is a for-integrity call or not. This means background writeback can clear it, and subsequent sync_inode_metadata or sync(2) call will skip the next ->write_inode completely.

Hmm, this also means that write_inode_now(sync=1) is buggy. It needs to in fact call ->fsync -- which is a file operation unfortunately, Christoph didn't you have some patches to move it into an inode operation?