Re: [patch] fs: add a remount,ro coherency point

[Nick Piggin <npiggin@suse.de>]

On Mon, Mar 30, 2009 at 08:55:06PM +0800, Wu Fengguang wrote:
> Hi Nick,
> 
> On Mon, Mar 30, 2009 at 01:33:54PM +0200, Nick Piggin wrote:
> > 
> > fs: invalidate sb->s_bdev on remount,ro
> > 
> > Fixes a problem reported by "Jorge Boncompte [DTI2]" <jorge@dti2.net>
> > who is trying to snapshot a minix filesystem image.
> > 
> > Signed-off-by: Nick Piggin <npiggin@suse.de>
> > 
> > ---
> >  fs/super.c |    8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > Index: linux-2.6/fs/super.c
> > ===================================================================
> > --- linux-2.6.orig/fs/super.c
> > +++ linux-2.6/fs/super.c
> > @@ -647,6 +647,14 @@ int do_remount_sb(struct super_block *sb
> >  		acct_auto_close(sb);
> >  	shrink_dcache_sb(sb);
> 
> Not a question to this patch: can we avoid shrink_dcache_sb() on
> ro=>rw remounts? They will happen at system boot time, and eliminating
> the shrink_dcache_sb() could help reduce the boot time.

I don't see why not. Yes it might reduce dcache misses a little
bit.


> >  	fsync_super(sb);
> > +	 /* Some filesystems modify their metadata via some other path
> > +	    than the bdev buffer cache (eg. use a private mapping, or
> > +	    directories in pagecache, etc). Also file data modifications
> > +	    go via their own mappings. So If we try to mount readonly
> > +	    then copy the filesystem from bdev, we could get stale data,
> > +	    so invalidate it to give a best effort at coherency. */
> > +	if (flags & MS_RDONLY && sb->s_bdev)
> > +		invalidate_bdev(sb->s_bdev);
> 
> Or move the above lines to...
> 
> >  	/* If we are remounting RDONLY and current sb is read/write,
> >  	   make sure there are no rw files opened */
>         if ((flags & MS_RDONLY) && !(sb->s_flags & MS_RDONLY)) {
>                 if (force)
>                         mark_files_ro(sb);
>                 else if (!fs_may_remount_ro(sb))
>                         return -EBUSY;
>                 retval = vfs_dq_off(sb, 1);
>                 if (retval < 0 && retval != -ENOSYS)
>                         return -EBUSY;
>         }
> 
> ...here?

Oh... hmm, actually it probably makes even more sensse after
we perform the remount (there might be further changes to the
filesystem in the meantime, or the remount may fail).

How about this instead?
--
fs: invalidate sb->s_bdev on remount,ro

Fixes a problem reported by "Jorge Boncompte [DTI2]" <jorge@dti2.net>
who is trying to snapshot a minix filesystem image.

Signed-off-by: Nick Piggin <npiggin@suse.de>

---
 fs/super.c |   16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

Index: linux-2.6/fs/super.c
===================================================================
--- linux-2.6.orig/fs/super.c
+++ linux-2.6/fs/super.c
@@ -637,7 +637,7 @@ retry:
 int do_remount_sb(struct super_block *sb, int flags, void *data, int force)
 {
 	int retval;
-	int remount_rw;
+	int remount_rw, remount_ro;
 	
 #ifdef CONFIG_BLOCK
 	if (!(flags & MS_RDONLY) && bdev_read_only(sb->s_bdev))
@@ -648,9 +648,12 @@ int do_remount_sb(struct super_block *sb
 	shrink_dcache_sb(sb);
 	fsync_super(sb);
 
+	remount_ro = (flags & MS_RDONLY) && !(sb->s_flags & MS_RDONLY);
+	remount_rw = !(flags & MS_RDONLY) && (sb->s_flags & MS_RDONLY);
+
 	/* If we are remounting RDONLY and current sb is read/write,
 	   make sure there are no rw files opened */
-	if ((flags & MS_RDONLY) && !(sb->s_flags & MS_RDONLY)) {
+	if (remount_ro) {
 		if (force)
 			mark_files_ro(sb);
 		else if (!fs_may_remount_ro(sb))
@@ -659,7 +662,6 @@ int do_remount_sb(struct super_block *sb
 		if (retval < 0 && retval != -ENOSYS)
 			return -EBUSY;
 	}
-	remount_rw = !(flags & MS_RDONLY) && (sb->s_flags & MS_RDONLY);
 
 	if (sb->s_op->remount_fs) {
 		lock_super(sb);
@@ -671,6 +673,14 @@ int do_remount_sb(struct super_block *sb
 	sb->s_flags = (sb->s_flags & ~MS_RMT_MASK) | (flags & MS_RMT_MASK);
 	if (remount_rw)
 		DQUOT_ON_REMOUNT(sb);
+	 /* Some filesystems modify their metadata via some other path
+	    than the bdev buffer cache (eg. use a private mapping, or
+	    directories in pagecache, etc). Also file data modifications
+	    go via their own mappings. So If we try to mount readonly
+	    then copy the filesystem from bdev, we could get stale data,
+	    so invalidate it to give a best effort at coherency. */
+	if (remount_ro && sb->s_bdev)
+		invalidate_bdev(sb->s_bdev);
 	return 0;
 }