[patch 2/8] fs: simple fsync race fix

Previous message: [thread] [date] [author]
Next message: [thread] [date] [author]
From: Nick Piggin
Date: Friday, December 17, 2010 - 6:46 pm

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 a standard pattern used throughout the kernel's writeback caches
(I_SYNC ~= I_WRITEBACK, if that makes it clearer).

And so it is not possible to determine an inode's dirty status just by
checking I_DIRTY bits. Especially not for the purpose of data integrity
syncs.

Missing the check for these bits means that fsync can complete while
writeback to the inode is underway. Inode writeback functions get this
right, so call into them rather than try to shortcut things by testing
dirty state improperly.

Signed-off-by: Nick Piggin <npiggin@kernel.dk>


Index: linux-2.6/fs/libfs.c
===================================================================
--- linux-2.6.orig/fs/libfs.c	2010-11-19 16:44:39.000000000 +1100
+++ linux-2.6/fs/libfs.c	2010-11-19 16:49:42.000000000 +1100
@@ -895,11 +895,6 @@ int generic_file_fsync(struct file *file
 	int ret;
 
 	ret = sync_mapping_buffers(inode->i_mapping);
-	if (!(inode->i_state & I_DIRTY))
-		return ret;
-	if (datasync && !(inode->i_state & I_DIRTY_DATASYNC))
-		return ret;
-
 	err = sync_inode_metadata(inode, 1);
 	if (ret == 0)
 		ret = err;
Index: linux-2.6/fs/exofs/file.c
===================================================================
--- linux-2.6.orig/fs/exofs/file.c	2010-11-19 16:50:00.000000000 +1100
+++ linux-2.6/fs/exofs/file.c	2010-11-19 16:50:07.000000000 +1100
@@ -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);
 
 	/* This is a good place to write the sb */


--
Previous message: [thread] [date] [author]
Next message: [thread] [date] [author]

Messages in current thread:
[patch 0/8] Inode data integrity patches, Nick Piggin, (Fri Dec 17, 6:46 pm)
[patch 1/8] fs: mark_inode_dirty barrier fix, Nick Piggin, (Fri Dec 17, 6:46 pm)
[patch 2/8] fs: simple fsync race fix, Nick Piggin, (Fri Dec 17, 6:46 pm)
[patch 3/8] fs: introduce inode writeback helpers, Nick Piggin, (Fri Dec 17, 6:46 pm)
[patch 5/8] fs: ext2 inode sync fix, Nick Piggin, (Fri Dec 17, 6:46 pm)
[patch 6/8] fs: fsync optimisations, Nick Piggin, (Fri Dec 17, 6:46 pm)
[patch 8/8] fs: add i_op-&gt;sync_inode, Nick Piggin, (Fri Dec 17, 6:46 pm)
Re: [patch 7/8] fs: fix or note I_DIRTY handling bugs in f ..., Christoph Hellwig, (Wed Dec 29, 8:01 am)
Re: [patch 8/8] fs: add i_op-&gt;sync_inode, Christoph Hellwig, (Wed Dec 29, 8:12 am)
Re: [patch 8/8] fs: add i_op-&gt;sync_inode, Nick Piggin, (Mon Jan 3, 11:27 pm)
Re: [patch 8/8] fs: add i_op-&gt;sync_inode, Christoph Hellwig, (Mon Jan 3, 11:57 pm)
Re: [patch 8/8] fs: add i_op-&gt;sync_inode, Nick Piggin, (Tue Jan 4, 1:03 am)
Re: [patch 8/8] fs: add i_op-&gt;sync_inode, Nick Piggin, (Tue Jan 4, 2:49 am)