[PATCH] fs: fix filesystem_sync vs write race on rw=>ro remount

Dmitry Monakhov
2010-01-24T11:41:20Z

[-- Attachment #1: Type: text/plain, Size: 1738 bytes --]

Currently on rw=>ro remount we have following race
| mount /mnt -oremount,ro | write-task |
|-------------------------+------------|
| | open(RDWR) |
| shrink_dcache_sb(sb); | |
| sync_filesystem(sb); | |
| | write() |
| | close() |
| fs_may_remount_ro(sb) | |
| sb->s_flags = new_flags | |
Later writeback or sync() will result in error due to MS_RDONLY flag
In case of ext4 this result in jbd2_start failure on writeback
ext4_da_writepages: jbd2_start: 1024 pages, ino 1431; err -30
In fact all others are affected by this error but it is not visible
because the skip s_flags check on writeback. For example ext3 check
(s_flags & MS_RDONLY) only if page has no buffers during journal start.

In order to prevent the race we have to block new writers before
fs_may_remount_ro() and sync_filesystem(). Let's introduce new
sb->s_flags MS_RO_REMOUNT flag for this purpose. But suddenly we have
no available space in MS_XXX bits, let's share this bit with MS_REMOUNT.
This is possible because MS_REMOUNT used only for passing arguments
from flags to sys_mount() and never used in sb->s_flags.

##TESTCASE_BEGIN:
#! /bin/bash -x
DEV=/dev/sdb5
FSTYPE=ext4
BINDIR=/home/dmon
MNTOPT="data=ordered"
umount /mnt
mkfs.${FSTYPE} ${DEV} || exit 1
mount ${DEV} /mnt -o${MNTOPT} || exit 1
${BINDIR}/fsstress -p1 -l999999999 -n9999999999 -d /mnt/test &
sleep 15
mount /mnt -oremount,ro,${MNTOPT}
sleep 1
killall -9 fsstress
sync
# after this you may get following message in dmesg
# "ext4_da_writepages: jbd2_start: 1024 pages, ino 1431; err -30"
##TESTCASE_END

Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
--

[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 2529 bytes --]

diff --git a/fs/namespace.c b/fs/namespace.c
index c768f73..a216fb3 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -194,7 +194,7 @@ int __mnt_is_readonly(struct vfsmount *mnt)
{
if (mnt->mnt_flags & MNT_READONLY)
return 1;
- if (mnt->mnt_sb->s_flags & MS_RDONLY)
+ if (mnt->mnt_sb->s_flags & (MS_RDONLY| MS_RO_REMOUNT))
return 1;
return 0;
}
diff --git a/fs/super.c b/fs/super.c
index aff046b..756fe88 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -569,42 +569,51 @@ int do_remount_sb(struct super_block *sb, int flags, void *data, int force)
{
int retval;
int remount_rw;
+ int remount_ro;

if (sb->s_frozen != SB_UNFROZEN)
return -EBUSY;
-
+ remount_ro = (flags & MS_RDONLY) && !(sb->s_flags & MS_RDONLY);
#ifdef CONFIG_BLOCK
if (!(flags & MS_RDONLY) && bdev_read_only(sb->s_bdev))
return -EACCES;
#endif
-
if (flags & MS_RDONLY)
acct_auto_close(sb);
- shrink_dcache_sb(sb);
- sync_filesystem(sb);

/* 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)) {
+ retval = -EBUSY;
+ if (remount_ro) {
+ /* Prevent new writers before check */
+ sb->s_flags |= MS_RO_REMOUNT;
if (force)
mark_files_ro(sb);
else if (!fs_may_remount_ro(sb))
- return -EBUSY;
+ goto out;
+ }
+ shrink_dcache_sb(sb);
+ sync_filesystem(sb);
+
+ if (remount_ro) {
retval = vfs_dq_off(sb, 1);
if (retval < 0 && retval != -ENOSYS)
- return -EBUSY;
+ goto out;
}
remount_rw = !(flags & MS_RDONLY) && (sb->s_flags & MS_RDONLY);

if (sb->s_op->remount_fs) {
retval = sb->s_op->remount_fs(sb, &flags, data);
if (retval)
- return retval;
+ goto out;
}
sb->s_flags = (sb->s_flags & ~MS_RMT_MASK) | (flags & MS_RMT_MASK);
if (remount_rw)
vfs_dq_quota_on_remount(sb);
- return 0;
+out:
+ if (remount_ro)
+ sb->s_flags = (sb->s_flags & ~MS_RO_REMOUNT);
+ return retval;
}

static void do_emergency_remount(struct work_struct *work)
diff --git a/include/linux/fs.h b/include/linux/fs.h
index b1bcb27..a613875 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -208,6 +208,9 @@ struct inodes_stat_t {
#define MS_STRICTATIME (1<<24) /* Always perform atime updates */
#define MS_ACTIVE (1<<30)
#define MS_NOUSER (1<<31)
+#define MS_RO_REMOUNT MS_REMOUNT /* Alter flags from rw=>ro of mounted FS.
+ Not conflicting with MS_REMOUNT because
+ it never stored in sb->s_flags */

/*
* Superblock flags that can be altered by MS_REMOUNT