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

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

[Nick Piggin]

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);
 	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);
 
 	/* If we are remounting RDONLY and current sb is read/write,
 	   make sure there are no rw files opened */

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

[Jorge Boncompte [DTI2]]

Nick Piggin escribió:
> 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>

Tested-by: Jorge Boncompte [DTI2] <jorge@dti2.net>

Nick, Thank you for taking the time to fix this.

-- 
==============================================================
Jorge Boncompte - Ingenieria y Gestion de RED
DTI2 - Desarrollo de la Tecnologia de las Comunicaciones
--------------------------------------------------------------
C/ Abogado Enriquez Barrios, 5   14004 CORDOBA (SPAIN)
Tlf: +34 957 761395 / FAX: +34 957 450380
==============================================================
- Sin pistachos no hay Rock & Roll...
- Without wicker a basket cannot be made.
==============================================================

--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Wu Fengguang]

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.

>  	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?

Thanks,
Fengguang

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

[Nick Piggin]

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;
 }
 

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

[Wu Fengguang]

On Mon, Mar 30, 2009 at 03:09:48PM +0200, Nick Piggin wrote:
> 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.

So I'll send a patch after yours :-)

> 
> > >  	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.
> 

Reviewed-by: Wu Fengguang <fengguang.wu@intel.com> 

> 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);

Wow, the twins are exactly I wanted :)

>  	/* 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
                                          ~ forgot to point you to this

> +	    then copy the filesystem from bdev, we could get stale data,
            ~~~~ this 'if...then' clause will be misleading

> +	    so invalidate it to give a best effort at coherency. */
                             ~~~~~~~~~~~~~~~~~~~~~~~~~~~
                             Sorry I'm not that good at English...

> +	if (remount_ro && sb->s_bdev)
> +		invalidate_bdev(sb->s_bdev);
>  	return 0;
>  }
>  
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html