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

[Wu Fengguang <fengguang.wu@intel.com>]

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