data corrupting bug in 2.4.20 ext3, data=journal

In 2.4.20-pre5 an optimisation was made to the ext3 fsync function which can very easily cause file data corruption at unmount time. This was first reported by Nick Piggin on November 29th (one day after 2.4.20 was released, and three months after the bug was merged. Unfortunate timing)

This only affects filesystems which were mounted with the data=journal' option. Or files which are operating under chattr -j'. So most people are unaffected. The problem is not present in 2.5 kernels.

The symptoms are that any file data which was written within the thirty
seconds prior to the unmount may not make it to disk. A workaround is
to run `sync' before unmounting.

The optimisation was intended to avoid writing out and waiting on the inode's buffers when the subsequent commit would do that anyway. This optimisation was applied to both data=journal and data=ordered modes. But it is only valid for data=ordered mode.

In data=journal mode the data is left dirty in memory and the unmount will silently discard it.

The fix is to only apply the optimisation to inodes which are operating under data=ordered.

@@ -63,10 +63,12 @@ int ext3_sync_file(struct file * file, s
 	 */
 	ret = fsync_inode_buffers(inode);
 
-	/* In writeback mode, we need to force out data buffers too.  In
-	 * the other modes, ext3_force_commit takes care of forcing out
-	 * just the right data blocks. */
-	if (test_opt(inode->i_sb, DATA_FLAGS) == EXT3_MOUNT_WRITEBACK_DATA)
+	/*
+	 * If the inode is under ordered-data writeback it is not necessary to
+	 * sync its data buffers here - commit will do that, with potentially
+	 * better IO merging
+	 */
+	if (!ext3_should_order_data(inode))
 		ret |= fsync_inode_data_buffers(inode);
 
 	ext3_force_commit(inode->i_sb);

_

That "fix" didn't fix it. Sorry about that.

Please avoid ext3/data=journal until it is sorted out.

In fact it was reported on lkml on 18th July IIRC before 2.4.19 was released if that is any help to you. 2.4.19 and 2.4.20 are affected and I haven't tested previous releases. I was going to re-report it sometime, but Alan brought it to light just the other day.

Nick

Are you sure? I can't make it happen on 2.4.19. And disabling the new BH_Freed logic (which went into 2.4.20-pre5) makes it go away.

@@ -695,7 +695,7 @@ skip_commit: /* The journal should be un
 		 * use in a different page. */
 		if (__buffer_state(bh, Freed)) {
 			clear_bit(BH_Freed, &bh->b_state);
-			clear_bit(BH_JBDDirty, &bh->b_state);
+//			clear_bit(BH_JBDDirty, &bh->b_state);
 		}
 			
 		if (buffer_jdirty(bh)) {

_

But we've already called fsync_super() at this point. If that completes synchronously, then the buffers will already be out of the journal and queued for writeback when we get here, and clearing BH_JBDDirty won't have any dire consequences.

Indeed, loading the ext3 module with "do_sync_supers=1" cures the symptoms.

However, doing every superblock write synchronously is a non-starter, as that requires a journal commit in the ext3 universe, and so this would essentially force bdflush to serialise all of its filesystem flushes (which is a real problem once you've got a significant number of filesystems mounted.) But the VFS simply doesn't have any clean way of telling foo_write_super() whether the call needs to be completed synchronously or asynchronously.

There *is* a totally unclean way, though. kill_super() sets sb->s_root to NULL before calling its final fsync_super(), and ext3 can easily check that in ext3_write_super(). It's a nasty, messy dependency, but should work for 2.4 at least. For 2.5 we probably want to look at passing an async flag down into the write_super.

The attached patch seems to fix things for me.

Cheers,
Stephen

@@ -1640,7 +1640,12 @@
 	sb->s_dirt = 0;
 	target = log_start_commit(EXT3_SB(sb)->s_journal, NULL);
 
-	if (do_sync_supers) {
+	/*
+	 * Tricky --- if we are unmounting, the write really does need
+	 * to be synchronous.  We can detect that by looking for NULL in
+	 * sb->s_root.
+	 */
+	if (do_sync_supers || !sb->s_root) {
 		unlock_super(sb);
 		log_wait_commit(EXT3_SB(sb)->s_journal, target);
 		lock_super(sb);

That's OK, because there shouldn't be any dirty data attached to the inodes at that time. But there is, because the commit which write_super() started hasn't finished yet:

static int do_sync_supers = 0;
...

        target = log_start_commit(EXT3_SB(sb)->s_journal, NULL);

        if (do_sync_supers) {
                unlock_super(sb);
                log_wait_commit(EXT3_SB(sb)->s_journal, target);

We need to do a full sync at unmount.

I assume that in other journalling modes the buffers are dirty anyway, so sync_buffers() gets them.

And indeed enabling do_sync_supers fixes it up in both 2.4 and 2.5 (2.5 doesn't have sync_buffers(), but sync_inodes_sb() gets everything).

But are we sure that ->write_super() will always be called?

int fsync_super(struct super_block *sb)
{
...

        if (sb->s_dirt && sb->s_op && sb->s_op->write_super)
                sb->s_op->write_super(sb);

I suspect that if s_dirt is not set, and we have dirty inodes, write_super is not called and nothing will force a commit anywhere in the unmount process. Which could explain the similar failure in 2.4.19-rc1 which Nick reports.

We need to be able to distinguish between a periodic flush of the superblock and a real sync. The write_super overload has always been uncomfortable.

Google for "write_super is not for syncing" ;) I think Chris's patch is the way to fix all this up.