* [patch] fs: add a remount,ro coherency point @ 2009-03-30 11:33 Nick Piggin 2009-03-30 12:19 ` Jorge Boncompte [DTI2] 2009-03-30 12:55 ` Wu Fengguang 0 siblings, 2 replies; 5+ messages in thread From: Nick Piggin @ 2009-03-30 11:33 UTC (permalink / raw) To: viro; +Cc: Jorge Boncompte [DTI2], linux-fsdevel, Andrew Morton 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 */ ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [patch] fs: add a remount,ro coherency point 2009-03-30 11:33 [patch] fs: add a remount,ro coherency point Nick Piggin @ 2009-03-30 12:19 ` Jorge Boncompte [DTI2] 2009-03-30 12:55 ` Wu Fengguang 1 sibling, 0 replies; 5+ messages in thread From: Jorge Boncompte [DTI2] @ 2009-03-30 12:19 UTC (permalink / raw) To: npiggin; +Cc: viro, linux-fsdevel, Andrew Morton 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [patch] fs: add a remount,ro coherency point 2009-03-30 11:33 [patch] fs: add a remount,ro coherency point Nick Piggin 2009-03-30 12:19 ` Jorge Boncompte [DTI2] @ 2009-03-30 12:55 ` Wu Fengguang 2009-03-30 13:09 ` Nick Piggin 1 sibling, 1 reply; 5+ messages in thread From: Wu Fengguang @ 2009-03-30 12:55 UTC (permalink / raw) To: Nick Piggin; +Cc: viro, Jorge Boncompte [DTI2], linux-fsdevel, Andrew Morton 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [patch] fs: add a remount,ro coherency point 2009-03-30 12:55 ` Wu Fengguang @ 2009-03-30 13:09 ` Nick Piggin 2009-03-30 13:22 ` Wu Fengguang 0 siblings, 1 reply; 5+ messages in thread From: Nick Piggin @ 2009-03-30 13:09 UTC (permalink / raw) To: Wu Fengguang; +Cc: viro, Jorge Boncompte [DTI2], linux-fsdevel, Andrew Morton 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; } ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [patch] fs: add a remount,ro coherency point 2009-03-30 13:09 ` Nick Piggin @ 2009-03-30 13:22 ` Wu Fengguang 0 siblings, 0 replies; 5+ messages in thread From: Wu Fengguang @ 2009-03-30 13:22 UTC (permalink / raw) To: Nick Piggin; +Cc: viro, Jorge Boncompte [DTI2], linux-fsdevel, Andrew Morton 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2009-03-30 13:23 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2009-03-30 11:33 [patch] fs: add a remount,ro coherency point Nick Piggin 2009-03-30 12:19 ` Jorge Boncompte [DTI2] 2009-03-30 12:55 ` Wu Fengguang 2009-03-30 13:09 ` Nick Piggin 2009-03-30 13:22 ` Wu Fengguang
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).