[PATCH 3/7] fsfreeze: freeze_super and thaw_bdev don't play well together

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Changes from Dave Chinner's version:
 - Remove s_frozen check in freeze_super which is not needed now that it is
   re-entrant.
 - Decrement freeze counter if the freeze_fs callback fails.

---

thaw_bdev() has re-entrancy guards to allow freezes to nest
together. That is, it ensures that the filesystem is not thawed
until the last thaw command is issued. This is needed to prevent the
filesystem from being unfrozen while an existing freezer is still
operating on the filesystem in a frozen state (e.g. dm-snapshot).

Currently, freeze_super() and thaw_super() bypasses these guards,
and as a result manual freezing and unfreezing via the ioctl methods
do not nest correctly. hence mixing userspace directed freezes with
block device level freezes result in inconsistency due to premature
thawing of the filesystem.

Move the re-enterency guards to the superblock and into freeze_super
and thaw_super() so that userspace directed freezes nest correctly
again. Caveat: Things work as expected as long as direct calls to
thaw_super are always in response to a previous sb level freeze. In
other words an unpaired call to thaw_super can still thaw a
filesystem frozen using freeze_bdev (this issue could be addressed
in a follow-up patch if deemed necessary).

This patch retains the bdev level mutex and counter to keep the
"feature" that we can freeze a block device that does not have a
filesystem mounted yet.

Cc: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
Signed-off-by: Fernando Luis Vazquez Cao <fernando@xxxxxxxxxxxxx>
---

diff -urNp linux-3.5-rc5-orig/fs/block_dev.c linux-3.5-rc5/fs/block_dev.c
--- linux-3.5-rc5-orig/fs/block_dev.c	2012-07-06 14:51:53.797991327 +0900
+++ linux-3.5-rc5/fs/block_dev.c	2012-07-06 14:52:31.905977295 +0900
@@ -257,16 +257,18 @@ int fsync_bdev(struct block_device *bdev
 EXPORT_SYMBOL(fsync_bdev);
 
 /**
- * freeze_bdev  --  lock a filesystem and force it into a consistent state
+ * freeze_bdev  --  lock a block device
  * @bdev:	blockdevice to lock
  *
- * If a superblock is found on this device, we take the s_umount semaphore
- * on it to make sure nobody unmounts until the snapshot creation is done.
- * The reference counter (bd_fsfreeze_count) guarantees that only the last
- * unfreeze process can unfreeze the frozen filesystem actually when multiple
- * freeze requests arrive simultaneously. It counts up in freeze_bdev() and
- * count down in thaw_bdev(). When it becomes 0, thaw_bdev() will unfreeze
- * actually.
+ * Locks the block device and, if present, the associated filesystem too.
+ *
+ * The reference counter (bd_fsfreeze_count) is used to implement the feature
+ * that allows one to freeze a block device that does not have a filesystem
+ * mounted yet. For filesystems using mount_bdev the kernel takes care of
+ * things by preventing the mount operation from succeeding if the underlying
+ * block device is frozen. Other filesystems should check this counter or risk
+ * a situation where a freeze_bdev user (e.g. dm snapshot) and mount race,
+ * which may lead to inconsistencies.
  */
 struct super_block *freeze_bdev(struct block_device *bdev)
 {
@@ -274,17 +276,7 @@ struct super_block *freeze_bdev(struct b
 	int error = 0;
 
 	mutex_lock(&bdev->bd_fsfreeze_mutex);
-	if (++bdev->bd_fsfreeze_count > 1) {
-		/*
-		 * We don't even need to grab a reference - the first call
-		 * to freeze_bdev grab an active reference and only the last
-		 * thaw_bdev drops it.
-		 */
-		sb = get_super(bdev);
-		drop_super(sb);
-		mutex_unlock(&bdev->bd_fsfreeze_mutex);
-		return sb;
-	}
+	bdev->bd_fsfreeze_count++;
 
 	sb = get_active_super(bdev);
 	if (!sb)
@@ -297,33 +289,31 @@ struct super_block *freeze_bdev(struct b
 		return ERR_PTR(error);
 	}
 	deactivate_super(sb);
- out:
+out:
 	sync_blockdev(bdev);
 	mutex_unlock(&bdev->bd_fsfreeze_mutex);
-	return sb;	/* thaw_bdev releases s->s_umount */
+	return sb;
 }
 EXPORT_SYMBOL(freeze_bdev);
 
 /**
- * __thaw_bdev  -- unlock filesystem
+ * __thaw_bdev  -- unlock a block device
  * @bdev:	blockdevice to unlock
  * @sb:		associated superblock
  * @emergency:	emergency thaw
  *
- * Unlocks the filesystem and marks it writeable again after freeze_bdev().
+ * Unlocks the block device and, if present, the associated filesystem too.
  */
 static int __thaw_bdev(struct block_device *bdev, struct super_block *sb, int emergency)
 {
 	int error = -EINVAL;
 
 	mutex_lock(&bdev->bd_fsfreeze_mutex);
+
 	if (!bdev->bd_fsfreeze_count)
 		goto out;
 
-	if (--bdev->bd_fsfreeze_count > 0) {
-		error = 0;
-		goto out;
-	}
+	bdev->bd_fsfreeze_count--;
 
 	if (!sb)
 		goto out;
@@ -339,13 +329,6 @@ out:
 	return error;
 }
 
-/**
- * thaw_bdev  -- unlock filesystem
- * @bdev:	blockdevice to unlock
- * @sb:		associated superblock
- *
- * Unlocks the filesystem and marks it writeable again after freeze_bdev().
- */
 int thaw_bdev(struct block_device *bdev, struct super_block *sb)
 {
 	return __thaw_bdev(bdev, sb, 0);
diff -urNp linux-3.5-rc5-orig/fs/gfs2/ops_fstype.c linux-3.5-rc5/fs/gfs2/ops_fstype.c
--- linux-3.5-rc5-orig/fs/gfs2/ops_fstype.c	2012-07-06 14:50:08.845987003 +0900
+++ linux-3.5-rc5/fs/gfs2/ops_fstype.c	2012-07-06 14:52:31.909977446 +0900
@@ -1275,11 +1275,6 @@ static struct dentry *gfs2_mount(struct
 	if (IS_ERR(bdev))
 		return ERR_CAST(bdev);
 
-	/*
-	 * once the super is inserted into the list by sget, s_umount
-	 * will protect the lockfs code from trying to start a snapshot
-	 * while we are mounting
-	 */
 	mutex_lock(&bdev->bd_fsfreeze_mutex);
 	if (bdev->bd_fsfreeze_count > 0) {
 		mutex_unlock(&bdev->bd_fsfreeze_mutex);
diff -urNp linux-3.5-rc5-orig/fs/nilfs2/super.c linux-3.5-rc5/fs/nilfs2/super.c
--- linux-3.5-rc5-orig/fs/nilfs2/super.c	2012-05-21 07:29:13.000000000 +0900
+++ linux-3.5-rc5/fs/nilfs2/super.c	2012-07-06 14:52:31.909977446 +0900
@@ -1277,11 +1277,6 @@ nilfs_mount(struct file_system_type *fs_
 		goto failed;
 	}
 
-	/*
-	 * once the super is inserted into the list by sget, s_umount
-	 * will protect the lockfs code from trying to start a snapshot
-	 * while we are mounting
-	 */
 	mutex_lock(&sd.bdev->bd_fsfreeze_mutex);
 	if (sd.bdev->bd_fsfreeze_count > 0) {
 		mutex_unlock(&sd.bdev->bd_fsfreeze_mutex);
diff -urNp linux-3.5-rc5-orig/fs/super.c linux-3.5-rc5/fs/super.c
--- linux-3.5-rc5-orig/fs/super.c	2012-07-06 14:51:53.813990308 +0900
+++ linux-3.5-rc5/fs/super.c	2012-07-06 14:57:24.993984128 +0900
@@ -185,6 +185,8 @@ static struct super_block *alloc_super(s
 		s->s_shrink.seeks = DEFAULT_SEEKS;
 		s->s_shrink.shrink = prune_super;
 		s->s_shrink.batch = 1024;
+
+		mutex_init(&s->s_freeze_mutex);
 	}
 out:
 	return s;
@@ -981,11 +983,6 @@ struct dentry *mount_bdev(struct file_sy
 	if (IS_ERR(bdev))
 		return ERR_CAST(bdev);
 
-	/*
-	 * once the super is inserted into the list by sget, s_umount
-	 * will protect the lockfs code from trying to start a snapshot
-	 * while we are mounting
-	 */
 	mutex_lock(&bdev->bd_fsfreeze_mutex);
 	if (bdev->bd_fsfreeze_count > 0) {
 		mutex_unlock(&bdev->bd_fsfreeze_mutex);
@@ -1168,30 +1165,29 @@ out:
  * @sb: the super to lock
  *
  * Syncs the super to make sure the filesystem is consistent and calls the fs's
- * freeze_fs.  Subsequent calls to this without first thawing the fs will return
- * -EBUSY.
+ * freeze_fs.  The reference counter (s_freeze_count) guarantees that only the
+ * last unfreeze process can unfreeze the frozen filesystem actually when
+ * multiple freeze requests arrive simultaneously. It counts up in
+ * freeze_super() and count down in thaw_super(). When it becomes 0,
+ * thaw_super() will execute the unfreeze.
  */
 int freeze_super(struct super_block *sb)
 {
-	int ret;
+	int ret = 0;
 
 	atomic_inc(&sb->s_active);
 	down_write(&sb->s_umount);
-	if (sb->s_frozen) {
-		deactivate_locked_super(sb);
-		return -EBUSY;
-	}
+	mutex_lock(&sb->s_freeze_mutex);
+	if (++sb->s_freeze_count > 1)
+		goto out_deactivate;
 
-	if (!(sb->s_flags & MS_BORN)) {
-		up_write(&sb->s_umount);
-		return 0;	/* sic - it's "nothing to do" */
-	}
+	if (!(sb->s_flags & MS_BORN))
+		goto out_active;	/* sic - it's "nothing to do" */
 
 	if (sb->s_flags & MS_RDONLY) {
 		sb->s_frozen = SB_FREEZE_TRANS;
 		smp_wmb();
-		up_write(&sb->s_umount);
-		return 0;
+		goto out_active;
 	}
 
 	sb->s_frozen = SB_FREEZE_WRITE;
@@ -1206,17 +1202,24 @@ int freeze_super(struct super_block *sb)
 	if (sb->s_op->freeze_fs) {
 		ret = sb->s_op->freeze_fs(sb);
 		if (ret) {
+			sb->s_freeze_count--;
 			printk(KERN_ERR
 				"VFS:Filesystem freeze failed\n");
 			sb->s_frozen = SB_UNFROZEN;
 			smp_wmb();
 			wake_up(&sb->s_wait_unfrozen);
-			deactivate_locked_super(sb);
-			return ret;
+			goto out_deactivate;
 		}
 	}
+
+out_active:
 	up_write(&sb->s_umount);
-	return 0;
+out_unlock:
+	mutex_unlock(&sb->s_freeze_mutex);
+	return ret;
+out_deactivate:
+	deactivate_locked_super(sb);
+	goto out_unlock;
 }
 EXPORT_SYMBOL(freeze_super);
 
@@ -1227,22 +1230,36 @@ EXPORT_SYMBOL(freeze_super);
  *
  * Unlocks the filesystem and marks it writeable again after freeze_super().
  * If we are doing an emergency thaw, we don't need to grab the sb->s_umount
- * lock as it is already held.
+ * lock as it is already held.  Return -EINVAL if @sb is not frozen, 0 if the
+ * unfreeze was executed and succeeded or the error if it failed. If the
+ * unfreeze fails, then leave @sb in the frozen state.
  */
 static int __thaw_super(struct super_block *sb, int emergency)
 {
-	int error = 0;
+	int error = -EINVAL;
 
-	if (!emergency)
+	if (!emergency) {
 		down_write(&sb->s_umount);
+		mutex_lock(&sb->s_freeze_mutex);
 
-	if (sb->s_frozen == SB_UNFROZEN) {
-		error = -EINVAL;
-		goto out_unlock;
+		if (!sb->s_freeze_count)
+			goto out_unlock;
+
+		if (--sb->s_freeze_count > 0) {
+			error = 0;
+			goto out_unlock;
+		}
+	}
+	else {
+		/* already under down_read(&sb->s_umount) */
+		mutex_lock(&sb->s_freeze_mutex);
 	}
 
+	if (sb->s_frozen == SB_UNFROZEN)
+		goto out_unlock;
+
 	if (sb->s_flags & MS_RDONLY)
-		goto out;
+		goto out_unfreeze;
 
 	if (sb->s_op->unfreeze_fs) {
 		error = sb->s_op->unfreeze_fs(sb);
@@ -1254,11 +1271,11 @@ static int __thaw_super(struct super_blo
 		}
 	}
 
-out:
+out_unfreeze:
 	sb->s_frozen = SB_UNFROZEN;
 	smp_wmb();
 	wake_up(&sb->s_wait_unfrozen);
-
+	mutex_unlock(&sb->s_freeze_mutex);
 	/*
 	 * When called from emergency scope, we cannot grab the s_umount lock
 	 * so we cannot deactivate the superblock. This may leave unbalanced
@@ -1271,6 +1288,9 @@ out:
 	return 0;
 
 out_unlock:
+	if (error && error != -EINVAL)
+		sb->s_freeze_count++;
+	mutex_unlock(&sb->s_freeze_mutex);
 	if (!emergency)
 		up_write(&sb->s_umount);
 	return error;
diff -urNp linux-3.5-rc5-orig/include/linux/fs.h linux-3.5-rc5/include/linux/fs.h
--- linux-3.5-rc5-orig/include/linux/fs.h	2012-07-06 14:51:53.825989109 +0900
+++ linux-3.5-rc5/include/linux/fs.h	2012-07-06 14:52:31.909977446 +0900
@@ -1542,6 +1542,9 @@ struct super_block {
 
 	/* Being remounted read-only */
 	int s_readonly_remount;
+
+	int			s_freeze_count;	/* nr of nested freezes */
+	struct mutex		s_freeze_mutex;	/* nesting lock */
 };
 
 /* superblock cache pruning functions */


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


[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [Samba]     [Device Mapper]     [CEPH Development]
  Powered by Linux