[patch 24/51] ext3: wait on all pending commits in ext3_sync_fs

[patch 24/51] ext3: wait on all pending commits in ext3_sync_fs

[akpm]

From: Arthur Jones <ajones@riverbed.com>

In ext3_sync_fs, we only wait for a commit to finish if we started it, but
there may be one already in progress which will not be synced.

In the case of a data=ordered umount with pending long symlinks which are
delayed due to a long list of other I/O on the backing block device, this
causes the buffer associated with the long symlinks to not be moved to the
inode dirty list in the second phase of fsync_super.  Then, before they
can be dirtied again, kjournald exits, seeing the UMOUNT flag and the
dirty pages are never written to the backing block device, causing long
symlink corruption and exposing new or previously freed block data to
userspace.

This can be reproduced with a script created
by Eric Sandeen <sandeen@redhat.com>:

	#!/bin/bash

	umount /mnt/test2
	mount /dev/sdb4 /mnt/test2
	rm -f /mnt/test2/*
	dd if=/dev/zero of=/mnt/test2/bigfile bs=1M count=512
	touch
	/mnt/test2/thisisveryveryveryveryveryveryveryveryveryveryveryveryveryveryveryverylongfilename
	ln -s
	/mnt/test2/thisisveryveryveryveryveryveryveryveryveryveryveryveryveryveryveryverylongfilename
	/mnt/test2/link
	umount /mnt/test2
	mount /dev/sdb4 /mnt/test2
	ls /mnt/test2/
	umount /mnt/test2

To ensure all commits are synced, we flush all journal commits now when
sync_fs'ing ext3.

Signed-off-by: Arthur Jones <ajones@riverbed.com>
Cc: Eric Sandeen <sandeen@redhat.com>
Cc: Theodore Ts'o <tytso@mit.edu>
Cc: <linux-ext4@vger.kernel.org>
Cc: <stable@kernel.org>		[2.6.everything]
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 fs/ext3/super.c |   11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff -puN fs/ext3/super.c~ext3-wait-on-all-pending-commits-in-ext3_sync_fs fs/ext3/super.c
--- a/fs/ext3/super.c~ext3-wait-on-all-pending-commits-in-ext3_sync_fs
+++ a/fs/ext3/super.c
@@ -2390,13 +2390,12 @@ static void ext3_write_super (struct sup
 
 static int ext3_sync_fs(struct super_block *sb, int wait)
 {
-	tid_t target;

Re: [patch 24/51] ext3: wait on all pending commits in ext3_sync_fs

[Theodore Tso]

My version of this patch also cleaned up the following comment, which
has been wrong since 2.5.70 or thereabouts...

This isn't urgent, so could you just queue this up for the next merge
window in the -mm tree?

						- Ted

ext3: Clean up outdated and incorrect comment for ext3_write_super()

Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: <linux-ext4@vger.kernel.org>
---
diff --git a/fs/ext3/super.c b/fs/ext3/super.c
index e5717a4..296c044 100644
--- a/fs/ext3/super.c
+++ b/fs/ext3/super.c
@@ -2375,12 +2375,9 @@ int ext3_force_commit(struct super_block *sb)
 /*
  * Ext3 always journals updates to the superblock itself, so we don't
  * have to propagate any other updates to the superblock on disk at this
- * point.  Just start an async writeback to get the buffers on their way
- * to the disk.
- *
- * This implicitly triggers the writebehind on sync().
+ * point.  (We can probably nuke this function altogether, and remove
+ * any mention to sb->s_dirt in all of fs/ext3; eventual cleanup...)
  */

Re: [patch 24/51] ext3: wait on all pending commits in ext3_sync_fs

[Andrew Morton]

On Thu, 6 Nov 2008 16:44:31 -0500
Theodore Tso <tytso@mit.edu> wrote:

> My version of this patch also cleaned up the following comment, which
> has been wrong since 2.5.70 or thereabouts...

oh, I didn't spot that.

I looked at the version in linux-next and saw that it was propagating
the error value back to the VFS as well.  Or maybe that was done in a
separate patch, dunno.  But while that's a good change, I felt that we
should separate it from this bugfix.  I meant to mention it but I
forgot, sorry.
 
> This isn't urgent, so could you just queue this up for the next merge
> window in the -mm tree?
> 
> 						- Ted
> 
> ext3: Clean up outdated and incorrect comment for ext3_write_super()
> 
> Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: <linux-ext4@vger.kernel.org>
> ---
> diff --git a/fs/ext3/super.c b/fs/ext3/super.c
> index e5717a4..296c044 100644
> --- a/fs/ext3/super.c
> +++ b/fs/ext3/super.c
> @@ -2375,12 +2375,9 @@ int ext3_force_commit(struct super_block *sb)
>  /*
>   * Ext3 always journals updates to the superblock itself, so we don't
>   * have to propagate any other updates to the superblock on disk at this
> - * point.  Just start an async writeback to get the buffers on their way
> - * to the disk.
> - *
> - * This implicitly triggers the writebehind on sync().
> + * point.  (We can probably nuke this function altogether, and remove
> + * any mention to sb->s_dirt in all of fs/ext3; eventual cleanup...)
>   */
> -
>  static void ext3_write_super (struct super_block * sb)
>  {
>  	if (mutex_trylock(&sb->s_lock) != 0)

Sure.

Re: [patch 24/51] ext3: wait on all pending commits in ext3_sync_fs

[Theodore Tso]

On Thu, Nov 06, 2008 at 02:20:20PM -0800, Andrew Morton wrote:
> On Thu, 6 Nov 2008 16:44:31 -0500
> Theodore Tso <tytso@mit.edu> wrote:
> 
> > My version of this patch also cleaned up the following comment, which
> > has been wrong since 2.5.70 or thereabouts...
> 
> oh, I didn't spot that.
> 
> I looked at the version in linux-next and saw that it was propagating
> the error value back to the VFS as well.  Or maybe that was done in a
> separate patch, dunno.  But while that's a good change, I felt that we
> should separate it from this bugfix.  I meant to mention it but I
> forgot, sorry.

Fair enough.  Propagating the return value to the VFS is also a
cleanup, although given that the VFS drops return value on the floor,
I considered it a risk-free change and included it in the ext4 version
of the patch.  But yeah, I can see why separating could be argued to
be the right thing.

So that means we'll need two cleanup patches for ext3; one to fix the
comment, and another to propagate the error code back to the VFS.

	     	     		      	    	 - Ted

Re: [patch 24/51] ext3: wait on all pending commits in ext3_sync_fs

[Arthur Jones]

Hi Ted, ...

On Thu, Nov 06, 2008 at 02:42:02PM -0800, Theodore Tso wrote:
> [...]
> So that means we'll need two cleanup patches for ext3; one to fix the
> comment, and another to propagate the error code back to the VFS.

A trivial patch for this is in the linux-ext4
mailing list:

http://marc.info/?l=linux-kernel&m=122574905024321&w=2

Arthur