Patchwork [2/2] ext2: add wait flag support to sync_fs

login
register
mail settings
Submitter stephen hemminger
Date Nov. 17, 2009, 5:46 p.m.
Message ID <20091117174647.402219318@vyatta.com>
Download mbox | patch
Permalink /patch/38667/
State Not Applicable
Headers show

Comments

stephen hemminger - Nov. 17, 2009, 5:46 p.m.
Make ext2 safer against accidental data loss during removal
by adding support for waiting for super block update on sync.
Don't know why this wasn't done originally, all the other file
systems have it.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
Jan Kara - Nov. 19, 2009, 3:34 p.m.
> Make ext2 safer against accidental data loss during removal
> by adding support for waiting for super block update on sync.
> Don't know why this wasn't done originally, all the other file
> systems have it.
> 
> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
> 
> --- a/fs/ext2/super.c	2009-11-17 09:14:12.177002522 -0800
> +++ b/fs/ext2/super.c	2009-11-17 09:14:32.698005421 -0800
> @@ -1147,6 +1147,8 @@ static int ext2_sync_fs(struct super_blo
>  		ext2_sync_super(sb, es);
>  	} else {
>  		ext2_commit_super(sb, es);
> +		if (wait)
> +			sync_dirty_buffer(EXT2_SB(sb)->s_sbh);
>  	}
>  	sb->s_dirt = 0;
>  	unlock_kernel();
  Looking at the code it just looks strange. Part of it is because
of Christoph's conversion of ext2_write_super to ext2_sync_fs
(40f31dd47e7c3d15af1f9845eda0fa0c4c33f32f) but the VALID_FS handling
oddness seems to be even older. IMHO we should just clear the VALID_FS
flag on mount and in write_super() and sync_fs() just update block and
inode counters. wait == 1 case should then really wait for superblock
buffer, wait == 0 should not wait.
  BTW: Christoph, why did you choose to call ext2_sync_fs with wait == 1
from ext2_write_super()? I'd think (and looking into callsites seem to
confirm that) that ->write_super() was meant to be asynchronous...
  I've added this to my todo...

								Honza
Christoph Hellwig - Nov. 20, 2009, 10:36 a.m.
On Thu, Nov 19, 2009 at 04:34:19PM +0100, Jan Kara wrote:
>   BTW: Christoph, why did you choose to call ext2_sync_fs with wait == 1
> from ext2_write_super()? I'd think (and looking into callsites seem to
> confirm that) that ->write_super() was meant to be asynchronous...

No particular reason - the argument wasn't and still isn't used in ext2.
And yes, now that ->sync_fs is mandatory ->write_super should be
asynchronous.

--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
stephen hemminger - Nov. 20, 2009, 5:08 p.m.
On Fri, 20 Nov 2009 05:36:22 -0500
Christoph Hellwig <hch@infradead.org> wrote:

> On Thu, Nov 19, 2009 at 04:34:19PM +0100, Jan Kara wrote:
> >   BTW: Christoph, why did you choose to call ext2_sync_fs with wait == 1
> > from ext2_write_super()? I'd think (and looking into callsites seem to
> > confirm that) that ->write_super() was meant to be asynchronous...
> 
> No particular reason - the argument wasn't and still isn't used in ext2.
> And yes, now that ->sync_fs is mandatory ->write_super should be
> asynchronous.
> 

Shouldn't super block (and all other) updates be synchronous if ext2 
is mounted with SYNC and DIRSYNC?
Jan Kara - Nov. 20, 2009, 9:06 p.m.
On Fri 20-11-09 09:08:58, Stephen Hemminger wrote:
> On Fri, 20 Nov 2009 05:36:22 -0500
> Christoph Hellwig <hch@infradead.org> wrote:
> 
> > On Thu, Nov 19, 2009 at 04:34:19PM +0100, Jan Kara wrote:
> > >   BTW: Christoph, why did you choose to call ext2_sync_fs with wait == 1
> > > from ext2_write_super()? I'd think (and looking into callsites seem to
> > > confirm that) that ->write_super() was meant to be asynchronous...
> > 
> > No particular reason - the argument wasn't and still isn't used in ext2.
> > And yes, now that ->sync_fs is mandatory ->write_super should be
> > asynchronous.
> 
> Shouldn't super block (and all other) updates be synchronous if ext2 
> is mounted with SYNC and DIRSYNC?
  Well, looking at the code, we don't seem to do that ;) Maybe we should
but would it really bring anything? The only thing which will go wrong are
counters of free blocks and inodes and those will be recomputed by fsck
anyway. And note that SYNC does not guarantee you that you don't need fsck
if you just pull the device out without umount. It just limits the
damage...

									Honza

Patch

--- a/fs/ext2/super.c	2009-11-17 09:14:12.177002522 -0800
+++ b/fs/ext2/super.c	2009-11-17 09:14:32.698005421 -0800
@@ -1147,6 +1147,8 @@  static int ext2_sync_fs(struct super_blo
 		ext2_sync_super(sb, es);
 	} else {
 		ext2_commit_super(sb, es);
+		if (wait)
+			sync_dirty_buffer(EXT2_SB(sb)->s_sbh);
 	}
 	sb->s_dirt = 0;
 	unlock_kernel();