[RFC, PATCH 0/7] fsfreeze: miscellaneous fixes and cleanups

[RFC, PATCH 0/7] fsfreeze: miscellaneous fixes and cleanups

[Fernando Luis Vázquez Cao]

This patch set (probable the first of a series) is to address long standing
issues in the filesytem freeze code and to fill some functionality gaps in the
API. Some minor code rearrangements are included too.

The first 5 patches are a rehash of a previous patch set from Dave Chinner which
I ported to the latest -rc kernel (3.5.0-rc5 at the time of writing) and
updated to fix bugs and reflect previous feedback from Christoph Hellwig and
Al Viro.

Dave, I took the liberty of adding your Signed-off-by as the author of the
original patches. If you do not agree with my modifications or want me to
acknowledge you differently please let me know.

You can find the original discussion here:
http://www.spinics.net/lists/linux-fsdevel/msg32794.html

The last 2 patches add a bdev level check ioctl and sb level check ioctl.

More details follow below:

---
[1/7] fsfreeze: Prevent emergency thaw from looping infinitely
[2/7] fsfreeze: emergency thaw will deadlock on s_umount

This two patches fix the emergency thaw infinite loop reported by Jeff
Merkey and the deadlock on sb->s_umount that the infinite loop hid. These may
be stable kernel candidates.

[3/7] fsfreeze: freeze_super and thaw_bdev don't play well together
[4/7] fsfreeze: switch to using super methods where possible
[5/7] fsfreeze: move emergency thaw code to fs/super.c

These patches address the bdev/sb mismatch and the fact that sb level freezing
does not nest correctly. For all the places that the block device interfaces
are used, we need a superblock anyway so we may as well make freeze/thaw work
only at the superblock level. There is one caveat (pointed out by Christoph in
the discussion linked above) though: we still need to support the "feature"
that we can freeze a block device that does not have a filesystem mounted yet
(this can be used to prevent a filesystem mount during a dm snapshot for
example).

This series moves part of the nesting code to the superblock from the block
device level but retains the freeze_bdev/thaw_bdev so that we can keep the
"feature" mentioned above for those who may need it, namely dm and external
users (yes, block device level API is exported for external use). All the other
users of the block device API are converted to work at the superblock level.

[6/7] fsfreeze: add vfs ioctl to check freeze state
[7/7] fsfreeze: add block device ioctl to check freeze state

These ioctls can be used to by HA and monitoring software to check the freeze
state of block device and filesystems.
---

In a follow-up patch I would like to tackle (second try) a big outstanding
problem: a frozen filesystem can be unmounted but with the current API one
needs to open a file in the target superblock, which is no longer accessible
because it has been unmounted.

Possible approaches to fix this issue include:

1- Add a block device level API to unfreeze the filesystem.

The problem with this approach is that it does not play well with filesystems
sb/bdev association is 1:n (i.e. btrfs and that ilk).

2- Disallow umount on frozen filesystems.

I think this is the more reasonable solution but it may require a refactoring
of the lazy unmount code; a lazy unmount should only be allowed to succeed if
both the target filesystem and all its child mounts are unfrozen (a failure to
guarantee this can lead to a situation where the user cannot unfreeze a
filesystem in the hierarchy because it is not visible).

3- Automatically thaw the filesystem on unmount.

Probably not a good idea.

Other ideas welcome.

Thanks,
Fernando

[PATCH 1/7] fsfreeze: Prevent emergency thaw from looping infinitely

[Fernando Luis Vázquez Cao]

The thawing of a filesystem through sysrq-j loops infinitely as it
incorrectly detects a thawed filesytsem as frozen and tries to
unfreeze repeatedly. This is a regression caused by
4504230a71566785a05d3e6b53fa1ee071b864eb ("freeze_bdev: grab active
reference to frozen superblocks") in that it no longer returned
-EINVAL for superblocks that were not frozen.

Return -EINVAL when the filesystem is already unfrozen to avoid this
problem.

Cc: Christoph Hellwig <hch@infradead.org>
Signed-off-by: Dave Chinner <dchinner@redhat.com>
Signed-off-by: Fernando Luis Vazquez Cao <fernando@oss.ntt.co.jp>
---

diff -urNp linux-3.5-rc4-orig/fs/block_dev.c linux-3.5-rc4/fs/block_dev.c
--- linux-3.5-rc4-orig/fs/block_dev.c	2012-06-27 12:00:47.694616000 +0900
+++ linux-3.5-rc4/fs/block_dev.c	2012-06-29 13:38:23.841352532 +0900
@@ -319,22 +319,20 @@ int thaw_bdev(struct block_device *bdev,
 	if (!bdev->bd_fsfreeze_count)
 		goto out;
 
-	error = 0;
-	if (--bdev->bd_fsfreeze_count > 0)
+	if (--bdev->bd_fsfreeze_count > 0) {
+		error = 0;
 		goto out;
+	}
 
 	if (!sb)
 		goto out;
 
 	error = thaw_super(sb);
-	if (error) {
+	if (error)
 		bdev->bd_fsfreeze_count++;
-		mutex_unlock(&bdev->bd_fsfreeze_mutex);
-		return error;
-	}
 out:
 	mutex_unlock(&bdev->bd_fsfreeze_mutex);
-	return 0;
+	return error;
 }
 EXPORT_SYMBOL(thaw_bdev);
 

[PATCH 2/7] fsfreeze: emergency thaw will deadlock on s_umount

[Fernando Luis Vázquez Cao]

The emergency thaw process uses iterate_super() which holds the
sb->s_umount lock in read mode. The current thaw_super() code takes
the sb->s_umount lock in write mode, hence leading to an instant
deadlock.

Pass the emergency state into the thaw_bdev/thaw_super code to avoid
taking the s_umount lock in this case. We are running under the bdev
freeze mutex, so this is still serialised against freeze despite
only having a read lock on the sb->s_umount. Hence it should be safe
to execute in this manner, especially given that emergency thaw is a
rarely executed "get-out-of-jail" feature.

Cc: Christoph Hellwig <hch@infradead.org>
Signed-off-by: Dave Chinner <dchinner@redhat.com>
Signed-off-by: Fernando Luis Vazquez Cao <fernando@oss.ntt.co.jp>
---

diff -urNp linux-3.5-rc4-orig/fs/block_dev.c linux-3.5-rc4/fs/block_dev.c
--- linux-3.5-rc4-orig/fs/block_dev.c	2012-06-29 14:03:40.732008807 +0900
+++ linux-3.5-rc4/fs/block_dev.c	2012-06-29 14:20:29.480002824 +0900
@@ -305,13 +305,14 @@ struct super_block *freeze_bdev(struct b
 EXPORT_SYMBOL(freeze_bdev);
 
 /**
- * thaw_bdev  -- unlock filesystem
+ * __thaw_bdev  -- unlock filesystem
  * @bdev:	blockdevice to unlock
  * @sb:		associated superblock
+ * @emergency:	emergency thaw
  *
  * Unlocks the filesystem and marks it writeable again after freeze_bdev().
  */
-int thaw_bdev(struct block_device *bdev, struct super_block *sb)
+static int __thaw_bdev(struct block_device *bdev, struct super_block *sb, int emergency)
 {
 	int error = -EINVAL;
 
@@ -327,15 +328,35 @@ int thaw_bdev(struct block_device *bdev,
 	if (!sb)
 		goto out;
 
-	error = thaw_super(sb);
+	if (emergency)
+		error = thaw_super_emergency(sb);
+	else
+		error = thaw_super(sb);
 	if (error)
 		bdev->bd_fsfreeze_count++;
 out:
 	mutex_unlock(&bdev->bd_fsfreeze_mutex);
 	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);
+}
 EXPORT_SYMBOL(thaw_bdev);
 
+int thaw_bdev_emergency(struct block_device *bdev, struct super_block *sb)
+{
+	return __thaw_bdev(bdev, sb, 1);
+}
+
 static int blkdev_writepage(struct page *page, struct writeback_control *wbc)
 {
 	return block_write_full_page(page, blkdev_get_block, wbc);
diff -urNp linux-3.5-rc4-orig/fs/buffer.c linux-3.5-rc4/fs/buffer.c
--- linux-3.5-rc4-orig/fs/buffer.c	2012-06-27 12:00:47.762616000 +0900
+++ linux-3.5-rc4/fs/buffer.c	2012-06-29 14:21:41.948002825 +0900
@@ -514,7 +514,7 @@ repeat:
 static void do_thaw_one(struct super_block *sb, void *unused)
 {
 	char b[BDEVNAME_SIZE];
-	while (sb->s_bdev && !thaw_bdev(sb->s_bdev, sb))
+	while (sb->s_bdev && !thaw_bdev_emergency(sb->s_bdev, sb))
 		printk(KERN_WARNING "Emergency Thaw on %s\n",
 		       bdevname(sb->s_bdev, b));
 }
diff -urNp linux-3.5-rc4-orig/fs/super.c linux-3.5-rc4/fs/super.c
--- linux-3.5-rc4-orig/fs/super.c	2012-05-21 07:29:13.000000000 +0900
+++ linux-3.5-rc4/fs/super.c	2012-06-29 14:49:57.480003483 +0900
@@ -1221,19 +1221,24 @@ int freeze_super(struct super_block *sb)
 EXPORT_SYMBOL(freeze_super);
 
 /**
- * thaw_super -- unlock filesystem
+ * __thaw_super -- unlock filesystem
  * @sb: the super to thaw
+ * @emergency:	emergency thaw
  *
  * 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.
  */
-int thaw_super(struct super_block *sb)
+static int __thaw_super(struct super_block *sb, int emergency)
 {
-	int error;
+	int error = 0;
+
+	if (!emergency)
+		down_write(&sb->s_umount);
 
-	down_write(&sb->s_umount);
 	if (sb->s_frozen == SB_UNFROZEN) {
-		up_write(&sb->s_umount);
-		return -EINVAL;
+		error = -EINVAL;
+		goto out_unlock;
 	}
 
 	if (sb->s_flags & MS_RDONLY)
@@ -1245,8 +1250,7 @@ int thaw_super(struct super_block *sb)
 			printk(KERN_ERR
 				"VFS:Filesystem thaw failed\n");
 			sb->s_frozen = SB_FREEZE_TRANS;
-			up_write(&sb->s_umount);
-			return error;
+			goto out_unlock;
 		}
 	}
 
@@ -1254,8 +1258,44 @@ out:
 	sb->s_frozen = SB_UNFROZEN;
 	smp_wmb();
 	wake_up(&sb->s_wait_unfrozen);
-	deactivate_locked_super(sb);
+
+	/*
+	 * When called from emergency scope, we cannot grab the s_umount lock
+	 * so we cannot deactivate the superblock. This may leave unbalanced
+	 * superblock references which could prevent unmount, but given this is
+	 * an emergency operation....
+	 */
+	if (!emergency)
+		deactivate_locked_super(sb);
 
 	return 0;
+
+out_unlock:
+	if (!emergency)
+		up_write(&sb->s_umount);
+	return error;
+}
+
+/**
+ * thaw_super -- unlock filesystem
+ * @sb: the super to thaw
+ *
+ * Unlocks the filesystem and marks it writeable again after freeze_super().
+ */
+int thaw_super(struct super_block *sb)
+{
+	return __thaw_super(sb, 0);
 }
 EXPORT_SYMBOL(thaw_super);
+
+/**
+ * thaw_super_emergency  -- unlock filesystem
+ * @sb: the super to thaw
+ *
+ * Unlocks the filesystem and marks it writeable again after freeze_super().
+ * This avoids taking the s_umount lock if it is already held.
+ */
+int thaw_super_emergency(struct super_block *sb)
+{
+	return __thaw_super(sb, 1);
+}
diff -urNp linux-3.5-rc4-orig/include/linux/fs.h linux-3.5-rc4/include/linux/fs.h
--- linux-3.5-rc4-orig/include/linux/fs.h	2012-06-27 12:00:48.894616001 +0900
+++ linux-3.5-rc4/include/linux/fs.h	2012-06-29 14:43:51.096010758 +0900
@@ -1941,6 +1941,7 @@ extern int fd_statfs(int, struct kstatfs
 extern int vfs_ustat(dev_t, struct kstatfs *);
 extern int freeze_super(struct super_block *super);
 extern int thaw_super(struct super_block *super);
+extern int thaw_super_emergency(struct super_block *super);
 extern bool our_mnt(struct vfsmount *mnt);
 
 extern int current_umask(void);
@@ -2096,6 +2097,8 @@ extern void kill_bdev(struct block_devic
 extern struct super_block *freeze_bdev(struct block_device *);
 extern void emergency_thaw_all(void);
 extern int thaw_bdev(struct block_device *bdev, struct super_block *sb);
+extern int thaw_bdev_emergency(struct block_device *bdev,
+			       struct super_block *sb);
 extern int fsync_bdev(struct block_device *);
 #else
 static inline void bd_forget(struct inode *inode) {}
@@ -2112,6 +2115,12 @@ static inline int thaw_bdev(struct block
 {
 	return 0;
 }
+
+static inline int thaw_bdev_emergency(struct block_device *bdev,
+				      struct super_block *sb)
+{
+	return 0;
+}
 #endif
 extern int sync_filesystem(struct super_block *);
 extern const struct file_operations def_blk_fops;

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

[Fernando Luis Vázquez Cao]

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@infradead.org>
Signed-off-by: Dave Chinner <dchinner@redhat.com>
Signed-off-by: Fernando Luis Vazquez Cao <fernando@oss.ntt.co.jp>
---

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

[PATCH 4/7] fsfreeze: switch to using super methods where possible

[Fernando Luis Vázquez Cao]

Changes from Dave Chinner's version:
- Retain the freeze_bdev/thaw_bdev API so that we can keep the feature that
  we can freeze a block device that does not have a filesystem mounted yet.

---

In some places we are using freeze/thaw_bdev despite the fact the kernel
is operating at the sb level there. Convert these users of the bdev
interfaces to use the superblock interfaces instead.

Cc: Christoph Hellwig <hch@infradead.org>
Signed-off-by: Dave Chinner <dchinner@redhat.com>
Signed-off-by: Fernando Luis Vazquez Cao <fernando@oss.ntt.co.jp>
---

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-04 20:28:43.523571557 +0900
+++ linux-3.5-rc5/fs/block_dev.c	2012-07-04 23:36:25.748850800 +0900
@@ -297,14 +297,14 @@ out:
 EXPORT_SYMBOL(freeze_bdev);
 
 /**
- * __thaw_bdev  -- unlock a block device
+ * thaw_bdev  -- unlock a block device
  * @bdev:	blockdevice to unlock
  * @sb:		associated superblock
  * @emergency:	emergency thaw
  *
  * 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 thaw_bdev(struct block_device *bdev, struct super_block *sb)
 {
 	int error = -EINVAL;
 
@@ -318,28 +318,15 @@ static int __thaw_bdev(struct block_devi
 	if (!sb)
 		goto out;
 
-	if (emergency)
-		error = thaw_super_emergency(sb);
-	else
-		error = thaw_super(sb);
+	error = thaw_super(sb);
 	if (error)
 		bdev->bd_fsfreeze_count++;
 out:
 	mutex_unlock(&bdev->bd_fsfreeze_mutex);
 	return error;
 }
-
-int thaw_bdev(struct block_device *bdev, struct super_block *sb)
-{
-	return __thaw_bdev(bdev, sb, 0);
-}
 EXPORT_SYMBOL(thaw_bdev);
 
-int thaw_bdev_emergency(struct block_device *bdev, struct super_block *sb)
-{
-	return __thaw_bdev(bdev, sb, 1);
-}
-
 static int blkdev_writepage(struct page *page, struct writeback_control *wbc)
 {
 	return block_write_full_page(page, blkdev_get_block, wbc);
diff -urNp linux-3.5-rc5-orig/fs/buffer.c linux-3.5-rc5/fs/buffer.c
--- linux-3.5-rc5-orig/fs/buffer.c	2012-07-04 08:25:45.861457009 +0900
+++ linux-3.5-rc5/fs/buffer.c	2012-07-04 23:37:35.665197491 +0900
@@ -514,7 +514,7 @@ repeat:
 static void do_thaw_one(struct super_block *sb, void *unused)
 {
 	char b[BDEVNAME_SIZE];
-	while (sb->s_bdev && !thaw_bdev_emergency(sb->s_bdev, sb))
+	while (sb->s_bdev && !thaw_super_emergency(sb))
 		printk(KERN_WARNING "Emergency Thaw on %s\n",
 		       bdevname(sb->s_bdev, b));
 }
diff -urNp linux-3.5-rc5-orig/fs/xfs/xfs_fsops.c linux-3.5-rc5/fs/xfs/xfs_fsops.c
--- linux-3.5-rc5-orig/fs/xfs/xfs_fsops.c	2012-07-04 08:20:45.243966324 +0900
+++ linux-3.5-rc5/fs/xfs/xfs_fsops.c	2012-07-04 23:41:22.602322798 +0900
@@ -664,16 +664,12 @@ xfs_fs_goingdown(
 	__uint32_t	inflags)
 {
 	switch (inflags) {
-	case XFS_FSOP_GOING_FLAGS_DEFAULT: {
-		struct super_block *sb = freeze_bdev(mp->m_super->s_bdev);
-
-		if (sb && !IS_ERR(sb)) {
+	case XFS_FSOP_GOING_FLAGS_DEFAULT:
+		if (!freeze_super(mp->m_super)) {
 			xfs_force_shutdown(mp, SHUTDOWN_FORCE_UMOUNT);
-			thaw_bdev(sb->s_bdev, sb);
+			thaw_super(mp->m_super);
 		}
-
 		break;
-	}
 	case XFS_FSOP_GOING_FLAGS_LOGFLUSH:
 		xfs_force_shutdown(mp, SHUTDOWN_FORCE_UMOUNT);
 		break;
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-04 20:28:43.535571618 +0900
+++ linux-3.5-rc5/include/linux/fs.h	2012-07-04 23:42:35.058682005 +0900
@@ -2100,8 +2100,6 @@ extern void kill_bdev(struct block_devic
 extern struct super_block *freeze_bdev(struct block_device *);
 extern void emergency_thaw_all(void);
 extern int thaw_bdev(struct block_device *bdev, struct super_block *sb);
-extern int thaw_bdev_emergency(struct block_device *bdev,
-			       struct super_block *sb);
 extern int fsync_bdev(struct block_device *);
 #else
 static inline void bd_forget(struct inode *inode) {}
@@ -2118,12 +2116,6 @@ static inline int thaw_bdev(struct block
 {
 	return 0;
 }
-
-static inline int thaw_bdev_emergency(struct block_device *bdev,
-				      struct super_block *sb)
-{
-	return 0;
-}
 #endif
 extern int sync_filesystem(struct super_block *);
 extern const struct file_operations def_blk_fops;

[PATCH 5/7] fsfreeze: move emergency thaw code to fs/super.c

[Fernando Luis Vázquez Cao]

From: Fernando Luis Vazquez Cao <fernando@oss.ntt.co.jp>

It makes no sense having the emergency thaw code in fs/buffer.c when all of
it's operations are one superblocks and the code it executes is all in
fs/super.c. Move the code there and clean it up.

Cc: Christoph Hellwig <hch@infradead.org>
Signed-off-by: Dave Chinner <dchinner@redhat.com>
Signed-off-by: Fernando Luis Vazquez Cao <fernando@oss.ntt.co.jp>
---

diff -urNp linux-3.5-rc5-orig/fs/buffer.c linux-3.5-rc5/fs/buffer.c
--- linux-3.5-rc5-orig/fs/buffer.c	2012-07-06 15:19:57.122131765 +0900
+++ linux-3.5-rc5/fs/buffer.c	2012-07-06 15:20:08.129986675 +0900
@@ -511,37 +511,6 @@ repeat:
 	return err;
 }
 
-static void do_thaw_one(struct super_block *sb, void *unused)
-{
-	char b[BDEVNAME_SIZE];
-	while (sb->s_bdev && !thaw_super_emergency(sb))
-		printk(KERN_WARNING "Emergency Thaw on %s\n",
-		       bdevname(sb->s_bdev, b));
-}
-
-static void do_thaw_all(struct work_struct *work)
-{
-	iterate_supers(do_thaw_one, NULL);
-	kfree(work);
-	printk(KERN_WARNING "Emergency Thaw complete\n");
-}
-
-/**
- * emergency_thaw_all -- forcibly thaw every frozen filesystem
- *
- * Used for emergency unfreeze of all filesystems via SysRq
- */
-void emergency_thaw_all(void)
-{
-	struct work_struct *work;
-
-	work = kmalloc(sizeof(*work), GFP_ATOMIC);
-	if (work) {
-		INIT_WORK(work, do_thaw_all);
-		schedule_work(work);
-	}
-}
-
 /**
  * sync_mapping_buffers - write out & wait upon a mapping's "associated" buffers
  * @mapping: the mapping which wants those buffers written
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 15:18:06.157987473 +0900
+++ linux-3.5-rc5/fs/super.c	2012-07-06 15:20:08.129986675 +0900
@@ -1313,9 +1313,38 @@ EXPORT_SYMBOL(thaw_super);
  * @sb: the super to thaw
  *
  * Unlocks the filesystem and marks it writeable again after freeze_super().
- * This avoids taking the s_umount lock if it is already held.
+ * This avoids taking the s_umount lock because it is already held.
  */
-int thaw_super_emergency(struct super_block *sb)
+static void thaw_super_emergency(struct super_block *sb, void *unused)
 {
-	return __thaw_super(sb, 1);
+
+	if (sb->s_bdev) {
+		char b[BDEVNAME_SIZE];
+		printk(KERN_WARNING "Emergency Thaw on %s.\n",
+				    bdevname(sb->s_bdev, b));
+	}
+	while (!__thaw_super(sb, 1));
+}
+
+static void do_thaw_all(struct work_struct *work)
+{
+	iterate_supers(thaw_super_emergency, NULL);
+	kfree(work);
+	printk(KERN_WARNING "Emergency Thaw complete\n");
+}
+
+/**
+ * emergency_thaw_all -- forcibly thaw every frozen filesystem
+ *
+ * Used for emergency unfreeze of all filesystems via SysRq
+ */
+void emergency_thaw_all(void)
+{
+	struct work_struct *work;
+
+	work = kmalloc(sizeof(*work), GFP_ATOMIC);
+	if (work) {
+		INIT_WORK(work, do_thaw_all);
+		schedule_work(work);
+	}
 }
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 15:19:57.122131765 +0900
+++ linux-3.5-rc5/include/linux/fs.h	2012-07-06 15:20:08.129986675 +0900
@@ -1944,7 +1944,6 @@ extern int fd_statfs(int, struct kstatfs
 extern int vfs_ustat(dev_t, struct kstatfs *);
 extern int freeze_super(struct super_block *super);
 extern int thaw_super(struct super_block *super);
-extern int thaw_super_emergency(struct super_block *super);
 extern bool our_mnt(struct vfsmount *mnt);
 
 extern int current_umask(void);

[PATCH 6/7] fsfreeze: add vfs ioctl to check freeze state

[Fernando Luis Vázquez Cao]

The FIISFROZEN ioctl can be use by HA and monitoring software to check
the freeze state of a mounted filesystem.

Cc: Christoph Hellwig <hch@infradead.org>
Cc: Dave Chinner <dchinner@redhat.com>
Signed-off-by: Fernando Luis Vazquez Cao <fernando@oss.ntt.co.jp>
---

diff -urNp linux-3.5-rc5-orig/fs/compat_ioctl.c linux-3.5-rc5/fs/compat_ioctl.c
--- linux-3.5-rc5-orig/fs/compat_ioctl.c	2012-05-21 07:29:13.000000000 +0900
+++ linux-3.5-rc5/fs/compat_ioctl.c	2012-07-06 15:26:52.333999406 +0900
@@ -885,6 +885,7 @@ COMPATIBLE_IOCTL(FIGETBSZ)
 /* 'X' - originally XFS but some now in the VFS */
 COMPATIBLE_IOCTL(FIFREEZE)
 COMPATIBLE_IOCTL(FITHAW)
+COMPATIBLE_IOCTL(FIISFROZEN)
 COMPATIBLE_IOCTL(KDGETKEYCODE)
 COMPATIBLE_IOCTL(KDSETKEYCODE)
 COMPATIBLE_IOCTL(KDGKBTYPE)
diff -urNp linux-3.5-rc5-orig/fs/ioctl.c linux-3.5-rc5/fs/ioctl.c
--- linux-3.5-rc5-orig/fs/ioctl.c	2012-05-21 07:29:13.000000000 +0900
+++ linux-3.5-rc5/fs/ioctl.c	2012-07-06 15:26:52.333999406 +0900
@@ -536,6 +536,16 @@ static int ioctl_fsthaw(struct file *fil
 	return thaw_super(sb);
 }
 
+static int ioctl_fs_isfrozen(struct file *filp)
+{
+	struct super_block *sb = filp->f_path.dentry->d_inode->i_sb;
+
+	if (!capable(CAP_SYS_ADMIN))
+		return -EPERM;
+
+	return isfrozen_super(sb);
+}
+
 /*
  * When you add any new common ioctls to the switches above and below
  * please update compat_sys_ioctl() too.
@@ -585,6 +595,12 @@ int do_vfs_ioctl(struct file *filp, unsi
 		error = ioctl_fsthaw(filp);
 		break;
 
+	case FIISFROZEN:
+		error = ioctl_fs_isfrozen(filp);
+		if (error >= 0)
+			return put_user(error, (int __user *)arg);
+		break;
+
 	case FS_IOC_FIEMAP:
 		return ioctl_fiemap(filp, arg);
 
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 15:24:38.149984676 +0900
+++ linux-3.5-rc5/fs/super.c	2012-07-06 15:26:52.333999406 +0900
@@ -1348,3 +1348,8 @@ void emergency_thaw_all(void)
 		schedule_work(work);
 	}
 }
+
+int isfrozen_super(struct super_block *sb)
+{
+	return sb->s_frozen > SB_UNFROZEN ? 1 : 0;
+}
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 15:24:38.157985208 +0900
+++ linux-3.5-rc5/include/linux/fs.h	2012-07-06 15:26:52.333999406 +0900
@@ -340,6 +340,7 @@ struct inodes_stat_t {
 #define FIFREEZE	_IOWR('X', 119, int)	/* Freeze */
 #define FITHAW		_IOWR('X', 120, int)	/* Thaw */
 #define FITRIM		_IOWR('X', 121, struct fstrim_range)	/* Trim */
+#define FIISFROZEN	_IOR('X', 122, int)	/* get sb freeze state */
 
 #define	FS_IOC_GETFLAGS			_IOR('f', 1, long)
 #define	FS_IOC_SETFLAGS			_IOW('f', 2, long)
@@ -1944,6 +1945,7 @@ extern int fd_statfs(int, struct kstatfs
 extern int vfs_ustat(dev_t, struct kstatfs *);
 extern int freeze_super(struct super_block *super);
 extern int thaw_super(struct super_block *super);
+extern int isfrozen_super(struct super_block *sb);
 extern bool our_mnt(struct vfsmount *mnt);
 
 extern int current_umask(void);

[PATCH 7/7] fsfreeze: add block device ioctl to check freeze state

[Fernando Luis Vázquez Cao]

The BLKISFROZEN ioctl can be use by HA and monitoring software to check
the freeze state of the filesystem sitting on top of a block device.
This will not work for filesystems that can handle multiple devices,
i.e. btrfs.

Cc: Christoph Hellwig <hch@infradead.org>
Cc: Dave Chinner <dchinner@redhat.com>
Signed-off-by: Fernando Luis Vazquez Cao <fernando@oss.ntt.co.jp>
---

diff -urNp linux-3.5-rc5-orig/block/compat_ioctl.c linux-3.5-rc5/block/compat_ioctl.c
--- linux-3.5-rc5-orig/block/compat_ioctl.c	2012-05-21 07:29:13.000000000 +0900
+++ linux-3.5-rc5/block/compat_ioctl.c	2012-07-06 14:12:06.641994024 +0900
@@ -746,6 +746,13 @@ long compat_blkdev_ioctl(struct file *fi
 	case BLKTRACETEARDOWN: /* compatible */
 		ret = blk_trace_ioctl(bdev, cmd, compat_ptr(arg));
 		return ret;
+	case BLKISFROZEN:
+		if (!capable(CAP_SYS_ADMIN))
+			return -EPERM;
+		ret = isfrozen_bdev(bdev);
+		if (ret >= 0)
+			return compat_put_int(arg, ret);
+		return ret;
 	default:
 		if (disk->fops->compat_ioctl)
 			ret = disk->fops->compat_ioctl(bdev, mode, cmd, arg);
diff -urNp linux-3.5-rc5-orig/block/ioctl.c linux-3.5-rc5/block/ioctl.c
--- linux-3.5-rc5-orig/block/ioctl.c	2012-05-21 07:29:13.000000000 +0900
+++ linux-3.5-rc5/block/ioctl.c	2012-07-06 14:12:06.641994024 +0900
@@ -343,6 +343,13 @@ int blkdev_ioctl(struct block_device *bd
 	case BLKTRACETEARDOWN:
 		ret = blk_trace_ioctl(bdev, cmd, (char __user *) arg);
 		break;
+	case BLKISFROZEN:
+		if (!capable(CAP_SYS_ADMIN))
+			return -EPERM;
+		ret = isfrozen_bdev(bdev);
+		if (ret >= 0)
+			return put_int(arg, ret);
+		return ret;
 	default:
 		ret = __blkdev_driver_ioctl(bdev, mode, cmd, arg);
 	}
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 12:27:27.082025564 +0900
+++ linux-3.5-rc5/fs/block_dev.c	2012-07-06 14:20:04.593990972 +0900
@@ -327,6 +327,11 @@ out:
 }
 EXPORT_SYMBOL(thaw_bdev);
 
+int isfrozen_bdev(struct block_device *bdev)
+{
+	return bdev->bd_fsfreeze_count > 0;
+}
+
 static int blkdev_writepage(struct page *page, struct writeback_control *wbc)
 {
 	return block_write_full_page(page, blkdev_get_block, wbc);
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:09:55.841988821 +0900
+++ linux-3.5-rc5/include/linux/fs.h	2012-07-06 14:12:06.641994024 +0900
@@ -333,6 +333,7 @@ struct inodes_stat_t {
 #define BLKDISCARDZEROES _IO(0x12,124)
 #define BLKSECDISCARD _IO(0x12,125)
 #define BLKROTATIONAL _IO(0x12,126)
+#define BLKISFROZEN _IOR(0x12,127, int)	/* get file system freeze state */
 
 #define BMAP_IOCTL 1		/* obsolete - kept for compatibility */
 #define FIBMAP	   _IO(0x00,1)	/* bmap access */
@@ -2101,6 +2102,7 @@ extern void kill_bdev(struct block_devic
 extern struct super_block *freeze_bdev(struct block_device *);
 extern void emergency_thaw_all(void);
 extern int thaw_bdev(struct block_device *bdev, struct super_block *sb);
+extern int isfrozen_bdev(struct block_device *bdev);
 extern int fsync_bdev(struct block_device *);
 #else
 static inline void bd_forget(struct inode *inode) {}
@@ -2117,6 +2119,11 @@ static inline int thaw_bdev(struct block
 {
 	return 0;
 }
+
+static int isfrozen_bdev(struct block_device *bdev)
+{
+	return 0;
+}
 #endif
 extern int sync_filesystem(struct super_block *);
 extern const struct file_operations def_blk_fops;

Re: [PATCH 2/7] fsfreeze: emergency thaw will deadlock on s_umount

[Jan Kara]

On Tue 10-07-12 17:23:35, Fernando Luis Vázquez Cao wrote:
> The emergency thaw process uses iterate_super() which holds the
> sb->s_umount lock in read mode. The current thaw_super() code takes
> the sb->s_umount lock in write mode, hence leading to an instant
> deadlock.
> 
> Pass the emergency state into the thaw_bdev/thaw_super code to avoid
> taking the s_umount lock in this case. We are running under the bdev
> freeze mutex, so this is still serialised against freeze despite
> only having a read lock on the sb->s_umount. Hence it should be safe
> to execute in this manner, especially given that emergency thaw is a
> rarely executed "get-out-of-jail" feature.
  Umm, I find it ugly to have conditional locking in thaw_super(). IMHO
it would be cleaner to provide a __thaw_super() function that would expect
all the locking to be done already.

Also looking into the code, it seems emergency thaw won't be able to thaw
filesystems frozen with FIFREEZE ioctl (bd_fsfreeze_count will be zero)?
Calling thaw_super() directly would solve this but then we'd leave
bd_fzfreeze_count inconsistent... It's a mess with these two types of
freezing.

Finally this patch collides with my patches fixing deadlocks in filesystem
freezing code (http://www.spinics.net/lists/kernel/msg1355763.html), which
are waiting for Al to merge them. But it should be relatively easy to
resolve the conflict.

								Honza

> Cc: Christoph Hellwig <hch@infradead.org>
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> Signed-off-by: Fernando Luis Vazquez Cao <fernando@oss.ntt.co.jp>
> ---
> 
> diff -urNp linux-3.5-rc4-orig/fs/block_dev.c linux-3.5-rc4/fs/block_dev.c
> --- linux-3.5-rc4-orig/fs/block_dev.c	2012-06-29 14:03:40.732008807 +0900
> +++ linux-3.5-rc4/fs/block_dev.c	2012-06-29 14:20:29.480002824 +0900
> @@ -305,13 +305,14 @@ struct super_block *freeze_bdev(struct b
>  EXPORT_SYMBOL(freeze_bdev);
>  
>  /**
> - * thaw_bdev  -- unlock filesystem
> + * __thaw_bdev  -- unlock filesystem
>   * @bdev:	blockdevice to unlock
>   * @sb:		associated superblock
> + * @emergency:	emergency thaw
>   *
>   * Unlocks the filesystem and marks it writeable again after freeze_bdev().
>   */
> -int thaw_bdev(struct block_device *bdev, struct super_block *sb)
> +static int __thaw_bdev(struct block_device *bdev, struct super_block *sb, int emergency)
>  {
>  	int error = -EINVAL;
>  
> @@ -327,15 +328,35 @@ int thaw_bdev(struct block_device *bdev,
>  	if (!sb)
>  		goto out;
>  
> -	error = thaw_super(sb);
> +	if (emergency)
> +		error = thaw_super_emergency(sb);
> +	else
> +		error = thaw_super(sb);
>  	if (error)
>  		bdev->bd_fsfreeze_count++;
>  out:
>  	mutex_unlock(&bdev->bd_fsfreeze_mutex);
>  	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);
> +}
>  EXPORT_SYMBOL(thaw_bdev);
>  
> +int thaw_bdev_emergency(struct block_device *bdev, struct super_block *sb)
> +{
> +	return __thaw_bdev(bdev, sb, 1);
> +}
> +
>  static int blkdev_writepage(struct page *page, struct writeback_control *wbc)
>  {
>  	return block_write_full_page(page, blkdev_get_block, wbc);
> diff -urNp linux-3.5-rc4-orig/fs/buffer.c linux-3.5-rc4/fs/buffer.c
> --- linux-3.5-rc4-orig/fs/buffer.c	2012-06-27 12:00:47.762616000 +0900
> +++ linux-3.5-rc4/fs/buffer.c	2012-06-29 14:21:41.948002825 +0900
> @@ -514,7 +514,7 @@ repeat:
>  static void do_thaw_one(struct super_block *sb, void *unused)
>  {
>  	char b[BDEVNAME_SIZE];
> -	while (sb->s_bdev && !thaw_bdev(sb->s_bdev, sb))
> +	while (sb->s_bdev && !thaw_bdev_emergency(sb->s_bdev, sb))
>  		printk(KERN_WARNING "Emergency Thaw on %s\n",
>  		       bdevname(sb->s_bdev, b));
>  }
> diff -urNp linux-3.5-rc4-orig/fs/super.c linux-3.5-rc4/fs/super.c
> --- linux-3.5-rc4-orig/fs/super.c	2012-05-21 07:29:13.000000000 +0900
> +++ linux-3.5-rc4/fs/super.c	2012-06-29 14:49:57.480003483 +0900
> @@ -1221,19 +1221,24 @@ int freeze_super(struct super_block *sb)
>  EXPORT_SYMBOL(freeze_super);
>  
>  /**
> - * thaw_super -- unlock filesystem
> + * __thaw_super -- unlock filesystem
>   * @sb: the super to thaw
> + * @emergency:	emergency thaw
>   *
>   * 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.
>   */
> -int thaw_super(struct super_block *sb)
> +static int __thaw_super(struct super_block *sb, int emergency)
>  {
> -	int error;
> +	int error = 0;
> +
> +	if (!emergency)
> +		down_write(&sb->s_umount);
>  
> -	down_write(&sb->s_umount);
>  	if (sb->s_frozen == SB_UNFROZEN) {
> -		up_write(&sb->s_umount);
> -		return -EINVAL;
> +		error = -EINVAL;
> +		goto out_unlock;
>  	}
>  
>  	if (sb->s_flags & MS_RDONLY)
> @@ -1245,8 +1250,7 @@ int thaw_super(struct super_block *sb)
>  			printk(KERN_ERR
>  				"VFS:Filesystem thaw failed\n");
>  			sb->s_frozen = SB_FREEZE_TRANS;
> -			up_write(&sb->s_umount);
> -			return error;
> +			goto out_unlock;
>  		}
>  	}
>  
> @@ -1254,8 +1258,44 @@ out:
>  	sb->s_frozen = SB_UNFROZEN;
>  	smp_wmb();
>  	wake_up(&sb->s_wait_unfrozen);
> -	deactivate_locked_super(sb);
> +
> +	/*
> +	 * When called from emergency scope, we cannot grab the s_umount lock
> +	 * so we cannot deactivate the superblock. This may leave unbalanced
> +	 * superblock references which could prevent unmount, but given this is
> +	 * an emergency operation....
> +	 */
> +	if (!emergency)
> +		deactivate_locked_super(sb);
>  
>  	return 0;
> +
> +out_unlock:
> +	if (!emergency)
> +		up_write(&sb->s_umount);
> +	return error;
> +}
> +
> +/**
> + * thaw_super -- unlock filesystem
> + * @sb: the super to thaw
> + *
> + * Unlocks the filesystem and marks it writeable again after freeze_super().
> + */
> +int thaw_super(struct super_block *sb)
> +{
> +	return __thaw_super(sb, 0);
>  }
>  EXPORT_SYMBOL(thaw_super);
> +
> +/**
> + * thaw_super_emergency  -- unlock filesystem
> + * @sb: the super to thaw
> + *
> + * Unlocks the filesystem and marks it writeable again after freeze_super().
> + * This avoids taking the s_umount lock if it is already held.
> + */
> +int thaw_super_emergency(struct super_block *sb)
> +{
> +	return __thaw_super(sb, 1);
> +}
> diff -urNp linux-3.5-rc4-orig/include/linux/fs.h linux-3.5-rc4/include/linux/fs.h
> --- linux-3.5-rc4-orig/include/linux/fs.h	2012-06-27 12:00:48.894616001 +0900
> +++ linux-3.5-rc4/include/linux/fs.h	2012-06-29 14:43:51.096010758 +0900
> @@ -1941,6 +1941,7 @@ extern int fd_statfs(int, struct kstatfs
>  extern int vfs_ustat(dev_t, struct kstatfs *);
>  extern int freeze_super(struct super_block *super);
>  extern int thaw_super(struct super_block *super);
> +extern int thaw_super_emergency(struct super_block *super);
>  extern bool our_mnt(struct vfsmount *mnt);
>  
>  extern int current_umask(void);
> @@ -2096,6 +2097,8 @@ extern void kill_bdev(struct block_devic
>  extern struct super_block *freeze_bdev(struct block_device *);
>  extern void emergency_thaw_all(void);
>  extern int thaw_bdev(struct block_device *bdev, struct super_block *sb);
> +extern int thaw_bdev_emergency(struct block_device *bdev,
> +			       struct super_block *sb);
>  extern int fsync_bdev(struct block_device *);
>  #else
>  static inline void bd_forget(struct inode *inode) {}
> @@ -2112,6 +2115,12 @@ static inline int thaw_bdev(struct block
>  {
>  	return 0;
>  }
> +
> +static inline int thaw_bdev_emergency(struct block_device *bdev,
> +				      struct super_block *sb)
> +{
> +	return 0;
> +}
>  #endif
>  extern int sync_filesystem(struct super_block *);
>  extern const struct file_operations def_blk_fops;
> 
> 
> --
> 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
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR
--
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

Re: [PATCH 2/7] fsfreeze: emergency thaw will deadlock on s_umount

[Jan Kara]

On Tue 10-07-12 11:09:18, Jan Kara wrote:
...
> Also looking into the code, it seems emergency thaw won't be able to thaw
> filesystems frozen with FIFREEZE ioctl (bd_fsfreeze_count will be zero)?
> Calling thaw_super() directly would solve this but then we'd leave
> bd_fzfreeze_count inconsistent... It's a mess with these two types of
> freezing.
  Ah, I see you try to address this in the next patch. So scratch this
comment.

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: [PATCH 2/7] fsfreeze: emergency thaw will deadlock on s_umount

[Fernando Luis Vazquez Cao]

On 2012/07/10 18:13, Jan Kara wrote:

> On Tue 10-07-12 11:09:18, Jan Kara wrote:
> ...
>> Also looking into the code, it seems emergency thaw won't be able to thaw
>> filesystems frozen with FIFREEZE ioctl (bd_fsfreeze_count will be zero)?
>> Calling thaw_super() directly would solve this but then we'd leave
>> bd_fzfreeze_count inconsistent... It's a mess with these two types of
>> freezing.
>    Ah, I see you try to address this in the next patch. So scratch this
> comment.

Ok. I would appreciate if you could take a look at all the patches.

By the way, do you want me to rebase on top of your tree?

Thanks,
Fernando

Re: [PATCH 1/7] fsfreeze: Prevent emergency thaw from looping infinitely

[Jan Kara]

On Tue 10-07-12 17:20:57, Fernando Luis Vázquez Cao wrote:
> The thawing of a filesystem through sysrq-j loops infinitely as it
> incorrectly detects a thawed filesytsem as frozen and tries to
> unfreeze repeatedly. This is a regression caused by
> 4504230a71566785a05d3e6b53fa1ee071b864eb ("freeze_bdev: grab active
> reference to frozen superblocks") in that it no longer returned
> -EINVAL for superblocks that were not frozen.
  Umm, I don't think above mentioned commit is really guilty. Also I think
your patch breaks thawing of a block device without mounted filesystem -
you end up returning EINVAL for that...

								Honza
> 
> Return -EINVAL when the filesystem is already unfrozen to avoid this
> problem.
> 
> Cc: Christoph Hellwig <hch@infradead.org>
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> Signed-off-by: Fernando Luis Vazquez Cao <fernando@oss.ntt.co.jp>
> ---
> 
> diff -urNp linux-3.5-rc4-orig/fs/block_dev.c linux-3.5-rc4/fs/block_dev.c
> --- linux-3.5-rc4-orig/fs/block_dev.c	2012-06-27 12:00:47.694616000 +0900
> +++ linux-3.5-rc4/fs/block_dev.c	2012-06-29 13:38:23.841352532 +0900
> @@ -319,22 +319,20 @@ int thaw_bdev(struct block_device *bdev,
>  	if (!bdev->bd_fsfreeze_count)
>  		goto out;
>  
> -	error = 0;
> -	if (--bdev->bd_fsfreeze_count > 0)
> +	if (--bdev->bd_fsfreeze_count > 0) {
> +		error = 0;
>  		goto out;
> +	}
>  
>  	if (!sb)
>  		goto out;
>  
>  	error = thaw_super(sb);
> -	if (error) {
> +	if (error)
>  		bdev->bd_fsfreeze_count++;
> -		mutex_unlock(&bdev->bd_fsfreeze_mutex);
> -		return error;
> -	}
>  out:
>  	mutex_unlock(&bdev->bd_fsfreeze_mutex);
> -	return 0;
> +	return error;
>  }
>  EXPORT_SYMBOL(thaw_bdev);
>  
> 
> 
> --
> 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
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR
--
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

Re: [PATCH 2/7] fsfreeze: emergency thaw will deadlock on s_umount

[Jan Kara]

On Tue 10-07-12 18:30:27, Fernando Luis Vazquez Cao wrote:
> On 2012/07/10 18:13, Jan Kara wrote:
> 
> >On Tue 10-07-12 11:09:18, Jan Kara wrote:
> >...
> >>Also looking into the code, it seems emergency thaw won't be able to thaw
> >>filesystems frozen with FIFREEZE ioctl (bd_fsfreeze_count will be zero)?
> >>Calling thaw_super() directly would solve this but then we'd leave
> >>bd_fzfreeze_count inconsistent... It's a mess with these two types of
> >>freezing.
> >   Ah, I see you try to address this in the next patch. So scratch this
> >comment.
> 
> Ok. I would appreciate if you could take a look at all the patches.
  Except for patches 1 and 2 I'm basically OK with the series. Maybe I'd
move patches 3 and 4 to the beginning of the series to make fixes of
emergency thawing easier (you don't have to change thaw_bdev() as well).
But that's a minor thing.

> By the way, do you want me to rebase on top of your tree?
  It depends on how Al plans to handle my freezing fixes. I still don't
see them in his tree. Al?

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: [PATCH 1/7] fsfreeze: Prevent emergency thaw from looping infinitely

[Fernando Luis Vazquez Cao]

On 2012/07/10 21:08, Jan Kara wrote:

> On Tue 10-07-12 17:20:57, Fernando Luis Vázquez Cao wrote:
>> The thawing of a filesystem through sysrq-j loops infinitely as it
>> incorrectly detects a thawed filesytsem as frozen and tries to
>> unfreeze repeatedly. This is a regression caused by
>> 4504230a71566785a05d3e6b53fa1ee071b864eb ("freeze_bdev: grab active
>> reference to frozen superblocks") in that it no longer returned
>> -EINVAL for superblocks that were not frozen.
>    Umm, I don't think above mentioned commit is really guilty.

Well, it is after that commit that

-       if (!bdev->bd_fsfreeze_count) {
-               mutex_unlock(&bdev->bd_fsfreeze_mutex);
-               return -EINVAL;
-       }

became

+       if (!bdev->bd_fsfreeze_count)
+               goto out_unlock;
[...]
+out_unlock:
         mutex_unlock(&bdev->bd_fsfreeze_mutex);
         return 0;

which breaks emergency thaw.


> Also I think your patch breaks thawing of a block device without mounted
> filesystem - you end up returning EINVAL for that...

It returns EINVAL only after the last thaw and the block device
is still properly thawed (i.e. bd_fsfreeze_count becomes 0). Things
work because the only place where the kernel checks the return
value of thaw_bdev is emergency thaw (the freeze ioctls use the sb
level API). That said for the sake of readability I could change the
code to return 0 when sb == NULL (either as a follow-up patch
or as part of a rebase to your tree or Al's).

Thanks,
Fernando
--
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

Re: [PATCH 2/7] fsfreeze: emergency thaw will deadlock on s_umount

[Fernando Luis Vazquez Cao]

Hi Jan,

On 2012/07/10 21:15, Jan Kara wrote:

> On Tue 10-07-12 18:30:27, Fernando Luis Vazquez Cao wrote:
>> Ok. I would appreciate if you could take a look at all the patches.
>    Except for patches 1 and 2 I'm basically OK with the series.

Thank you for reviewing the patches.

Hopefully my reply to your comments on patch 1 address your
concerns. By the way, is there anything specific in patch 2
(apart from the conditional locking ugliness) that you want
to me to change? Maybe resolve the conflicts with your patch
set?

>> By the way, do you want me to rebase on top of your tree?
>    It depends on how Al plans to handle my freezing fixes. I still don't
> see them in his tree. Al?

Ok. Could you get your Acked-by or Reviewed-by for the changes
you agree with?

Regards,
Fernando

Re: [PATCH 1/7] fsfreeze: Prevent emergency thaw from looping infinitely

[Jan Kara]

On Wed 11-07-12 11:25:56, Fernando Luis Vazquez Cao wrote:
> On 2012/07/10 21:08, Jan Kara wrote:
> 
> >On Tue 10-07-12 17:20:57, Fernando Luis Vázquez Cao wrote:
> >>The thawing of a filesystem through sysrq-j loops infinitely as it
> >>incorrectly detects a thawed filesytsem as frozen and tries to
> >>unfreeze repeatedly. This is a regression caused by
> >>4504230a71566785a05d3e6b53fa1ee071b864eb ("freeze_bdev: grab active
> >>reference to frozen superblocks") in that it no longer returned
> >>-EINVAL for superblocks that were not frozen.
> >   Umm, I don't think above mentioned commit is really guilty.
> 
> Well, it is after that commit that
> 
> -       if (!bdev->bd_fsfreeze_count) {
> -               mutex_unlock(&bdev->bd_fsfreeze_mutex);
> -               return -EINVAL;
> -       }
> 
> became
> 
> +       if (!bdev->bd_fsfreeze_count)
> +               goto out_unlock;
> [...]
> +out_unlock:
>         mutex_unlock(&bdev->bd_fsfreeze_mutex);
>         return 0;
> 
> which breaks emergency thaw.
  Ah, OK. I misread the code... Sorry for the noise.

> >Also I think your patch breaks thawing of a block device without mounted
> >filesystem - you end up returning EINVAL for that...
> 
> It returns EINVAL only after the last thaw and the block device
> is still properly thawed (i.e. bd_fsfreeze_count becomes 0).
  Yes, after your patch thaw_bdev() returns EINVAL although the device is
thawed. That is just wrong regardless whether any caller cares or not. And
before your patch this was working correctly. So please fix your patch.

								Honza

-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR
--
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

Re: [PATCH 2/7] fsfreeze: emergency thaw will deadlock on s_umount

[Jan Kara]

  Hello,

On Wed 11-07-12 11:38:21, Fernando Luis Vazquez Cao wrote:
> On 2012/07/10 21:15, Jan Kara wrote:
> 
> >On Tue 10-07-12 18:30:27, Fernando Luis Vazquez Cao wrote:
> >>Ok. I would appreciate if you could take a look at all the patches.
> >   Except for patches 1 and 2 I'm basically OK with the series.
> 
> Thank you for reviewing the patches.
  You are welcome.

> Hopefully my reply to your comments on patch 1 address your
> concerns.
  See my reply to patch 1...

> By the way, is there anything specific in patch 2
> (apart from the conditional locking ugliness) that you want
> to me to change? Maybe resolve the conflicts with your patch
> set?
  I would be happier if you based your patches on my patch set (less work
for me ;). OTOH your patches are simpler so Al might decide to merge them
first. So all in all my opinion doesn't matter much here. It is Al who
decides...

> >>By the way, do you want me to rebase on top of your tree?
> >   It depends on how Al plans to handle my freezing fixes. I still don't
> >see them in his tree. Al?
> 
> Ok. Could you get your Acked-by or Reviewed-by for the changes
> you agree with?
  Please fix the patch 1 and the conditional locking in patch 2 (that will
also require changes to other patches in the series - they might be smaller
if you reorder patches as I suggested). Then I'll check the whole series
and can give you my Reviewed-by.

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: [PATCH 1/7] fsfreeze: Prevent emergency thaw from looping infinitely

[Fernando Luis Vazquez Cao]

On 2012/07/11 18:02, Jan Kara wrote:

>    Yes, after your patch thaw_bdev() returns EINVAL although the device is
> thawed. That is just wrong regardless whether any caller cares or not. And
> before your patch this was working correctly. So please fix your patch.

I will fix it for the next iteration of the patch set.

Thanks,
Fernando

Re: [PATCH 2/7] fsfreeze: emergency thaw will deadlock on s_umount

[Fernando Luis Vazquez Cao]

Hi Jan,

On 2012/07/11 18:06, Jan Kara wrote:

>> By the way, is there anything specific in patch 2
>> (apart from the conditional locking ugliness) that you want
>> to me to change? Maybe resolve the conflicts with your patch
>> set?
>    I would be happier if you based your patches on my patch set (less work
> for me ;). OTOH your patches are simpler so Al might decide to merge them
> first. So all in all my opinion doesn't matter much here. It is Al who
> decides...

I will rebase against Al's vfs tree. If your patches get merged first
I do not mind resolving the merge conflicts you mentioned.

>> Ok. Could you get your Acked-by or Reviewed-by for the changes
>> you agree with?
>    Please fix the patch 1 and the conditional locking in patch 2 (that will
> also require changes to other patches in the series - they might be smaller
> if you reorder patches as I suggested). Then I'll check the whole series
> and can give you my Reviewed-by.

Ok, I will fix patch 1 and 2 (probably without reordering) and repost.

Thank you again for your comments!

Regards,
Fernando