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@xxxxxxxx>
> > > who is trying to snapshot a minix filesystem image.
> > >
> > > Signed-off-by: Nick Piggin <npiggin@xxxxxxx>
> > >
> > > ---
> > > 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@xxxxxxxx>
> who is trying to snapshot a minix filesystem image.
>
Reviewed-by: Wu Fengguang <fengguang.wu@xxxxxxxxx>
> Signed-off-by: Nick Piggin <npiggin@xxxxxxx>
>
> ---
> 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@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
