[RFC 0/9 v3] fsfreeze: miscellaneous fixes and cleanups

[RFC 0/9 v3] fsfreeze: miscellaneous fixes and cleanups

[Fernando Luis Vázquez Cao]

This patch set 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 following patches are included:

---
[1/9] vfs: add __iterate_supers() helper
[2/9] fsfreeze: add unlocked version of thaw_super

Preparatory patches to fix s_umount lockup of emergency thaw code.

[3/9] fsfreeze: Prevent emergency thaw from looping infinitely

Fix thaw_bdev so that it propagates the error code properly to the caller.
This bug caused emergency thaw to loop infinitely. This is a forward port of
a previous patch by Dave Chinner.

[4/9] fsfreeze: emergency thaw will deadlock on s_umount

Avoid emergency thaw deadlock on s_umount by using unlocked version of
thaw_super() and __iterate_supers()i (introduced in patches 2 and 1
respectively).

[5/9] xfs: switch to using super methods for fsfreeze
[6/9] fsfreeze: move emergency thaw code to fs/super.c

Two cleanups (based on previous patches by David Chinner): convert xfs to using
superblock methods for fsfreeze and move emergency thaw code to where it
belongs.

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

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

[8/9] fsfreeze: add vfs ioctl to check freeze state
[9/9] 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 devices and filesystems.
---

In a follow-up patch set I will tackle a big outstanding problem: we allow
users to umount a frozen filesystem but we have no bdev level fsfreeze API to
thaw it after that. My plan is to add the bdev level API but this requires
some modifications to btrfs and vfs layer that I am still working on. By
the way, the approach of returning EBUSY on umount was discarded because it
would break lazy umount.

Changes since v2:
  - Rebase on top of Jan Kara's patch set which made it upstream.
  - Do without the horrible "emergency" parameter as suggested by Jan Kara.
  - New approach to fix emergency thaw deadlock on s_umount which does locking
    properly.

Changes since v1:
  - Do not break unfreezing of block devices without a mounted filesystem.
  - Avoid conditional locking in thaw_bdev.
  - Update filesystem locking documentation.

[PATCH 1/9] vfs: add __iterate_supers() helper

[Fernando Luis Vázquez Cao]

iterate_supers() calls a function provided by the caller with the s_umount
semaphore taken in read mode. However, there may be cases where write mode
is preferable, so we add a new helper, __iterate_supers(), which lets one
specify the mode of the lock.

This will be used to fix the emergency thaw (filesystem unfreeze) code, which
iterates over the list of superblocks but needs to hold the s_umount semaphore
in _write_ mode bebore carrying out the actual thaw operation.

Cc: Josef Bacik <jbacik@fusionio.com>
Cc: Eric Sandeen <sandeen@redhat.com>
Cc: Christoph Hellwig <hch@infradead.org>
Cc: Jan Kara <jack@suse.cz>
Cc: Dave Chinner <dchinner@redhat.com>
Signed-off-by: Fernando Luis Vazquez Cao <fernando@oss.ntt.co.jp>
---

diff -urNp linux-3.6-rc5-orig/fs/super.c linux-3.6-rc5/fs/super.c
--- linux-3.6-rc5-orig/fs/super.c	2012-09-12 18:45:13.818046999 +0900
+++ linux-3.6-rc5/fs/super.c	2012-09-12 19:08:58.214034467 +0900
@@ -537,14 +537,22 @@ void drop_super(struct super_block *sb)
 EXPORT_SYMBOL(drop_super);
 
 /**
- *	iterate_supers - call function for all active superblocks
+ *	__iterate_supers - call function for all active superblocks
  *	@f: function to call
  *	@arg: argument to pass to it
+ *	@wlock: mode of superblock lock (false->read lock, true->write lock)
  *
  *	Scans the superblock list and calls given function, passing it
  *	locked superblock and given argument.
+ *
+ *	When the caller asks for the superblock lock (s_umount semaphore) to be
+ *	taken in write mode, the lock is taken but not released because the
+ *	function provided by the caller may deactivate the superblock itself.
+ *	It is that function's job to unlock the superblock as needed in such a
+ *	case.
  */
-void iterate_supers(void (*f)(struct super_block *, void *), void *arg)
+void __iterate_supers(void (*f)(struct super_block *, void *), void *arg,
+		      bool wlock)
 {
 	struct super_block *sb, *p = NULL;
 
@@ -555,10 +563,19 @@ void iterate_supers(void (*f)(struct sup
 		sb->s_count++;
 		spin_unlock(&sb_lock);
 
-		down_read(&sb->s_umount);
+		if (wlock)
+			down_write(&sb->s_umount);
+		else
+			down_read(&sb->s_umount);
+
 		if (sb->s_root && (sb->s_flags & MS_BORN))
 			f(sb, arg);
-		up_read(&sb->s_umount);
+
+		/* When the semaphore was taken in write mode the function
+		 * provided by the caller takes care of unlocking it as
+		 * needed. See explanation above for details. */
+		if (!wlock)
+			up_read(&sb->s_umount);
 
 		spin_lock(&sb_lock);
 		if (p)
@@ -571,6 +588,19 @@ void iterate_supers(void (*f)(struct sup
 }
 
 /**
+ *	iterate_supers - call function for all active superblocks
+ *	@f: function to call
+ *	@arg: argument to pass to it
+ *
+ *	Scans the superblock list and calls given function, passing it
+ *	a superblock locked in _read_ mode and given argument.
+ */
+void iterate_supers(void (*f)(struct super_block *, void *), void *arg)
+{
+	__iterate_supers(f, arg , false);
+}
+
+/**
  *	iterate_supers_type - call function for superblocks of given type
  *	@type: fs type
  *	@f: function to call
diff -urNp linux-3.6-rc5-orig/include/linux/fs.h linux-3.6-rc5/include/linux/fs.h
--- linux-3.6-rc5-orig/include/linux/fs.h	2012-09-12 18:45:14.002047001 +0900
+++ linux-3.6-rc5/include/linux/fs.h	2012-09-12 18:46:39.466082276 +0900
@@ -2684,6 +2684,8 @@ extern struct super_block *get_super(str
 extern struct super_block *get_super_thawed(struct block_device *);
 extern struct super_block *get_active_super(struct block_device *bdev);
 extern void drop_super(struct super_block *sb);
+extern void __iterate_supers(void (*f)(struct super_block *, void *), void *arg,
+		      bool wlock);
 extern void iterate_supers(void (*)(struct super_block *, void *), void *);
 extern void iterate_supers_type(struct file_system_type *,
 			        void (*)(struct super_block *, void *), void *);

[RFC 2/9] fsfreeze: add unlocked version of thaw_super

[Fernando Luis Vázquez Cao]

thaw_super may be called with superblock lock already taken (fsfreeze's
emergency thaw being one example), so we need an unlocked version to avoid
lockups.

Cc: Josef Bacik <jbacik@fusionio.com>
Cc: Eric Sandeen <sandeen@redhat.com>
Cc: Christoph Hellwig <hch@infradead.org>
Cc: Jan Kara <jack@suse.cz>
Cc: Dave Chinner <dchinner@redhat.com>
Signed-off-by: Fernando Luis Vazquez Cao <fernando@oss.ntt.co.jp>
---

diff -urNp linux-3.6-rc5-orig/fs/super.c linux-3.6-rc5/fs/super.c
--- linux-3.6-rc5-orig/fs/super.c	2012-09-12 19:13:00.710047001 +0900
+++ linux-3.6-rc5/fs/super.c	2012-09-12 19:13:12.185924077 +0900
@@ -1420,40 +1420,58 @@ int freeze_super(struct super_block *sb)
 EXPORT_SYMBOL(freeze_super);
 
 /**
- * thaw_super -- unlock filesystem
+ * __thaw_super -- unlock filesystem
  * @sb: the super to thaw
  *
- * Unlocks the filesystem and marks it writeable again after freeze_super().
+ * Unlocks the filesystem and marks it writeable again.
+ *
+ * This is the unlocked version of thaw_super, so it is the caller's job to
+ * to protect the superblock by grabing the s_umount semaphore in write mode.
  */
-int thaw_super(struct super_block *sb)
+int __thaw_super(struct super_block *sb)
 {
-	int error;
+	int error = 0;
 
-	down_write(&sb->s_umount);
 	if (sb->s_writers.frozen == SB_UNFROZEN) {
-		up_write(&sb->s_umount);
-		return -EINVAL;
+		error = -EINVAL;
+		goto out;
 	}
 
 	if (sb->s_flags & MS_RDONLY)
-		goto out;
+		goto out_thaw;
 
 	if (sb->s_op->unfreeze_fs) {
 		error = sb->s_op->unfreeze_fs(sb);
 		if (error) {
 			printk(KERN_ERR
 				"VFS:Filesystem thaw failed\n");
-			up_write(&sb->s_umount);
-			return error;
+			goto out;
 		}
 	}
 
-out:
+out_thaw:
 	sb->s_writers.frozen = SB_UNFROZEN;
 	smp_wmb();
 	wake_up(&sb->s_writers.wait_unfrozen);
-	deactivate_locked_super(sb);
+out:
+	return error;
+}
 
-	return 0;
+/**
+ * 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)
+{
+	int res;
+	down_write(&sb->s_umount);
+	res = __thaw_super(sb);
+	if (!res)
+		deactivate_locked_super(sb);
+	else
+		up_write(&sb->s_umount);
+	return res;
 }
 EXPORT_SYMBOL(thaw_super);
diff -urNp linux-3.6-rc5-orig/include/linux/fs.h linux-3.6-rc5/include/linux/fs.h
--- linux-3.6-rc5-orig/include/linux/fs.h	2012-09-12 19:13:00.830047035 +0900
+++ linux-3.6-rc5/include/linux/fs.h	2012-09-12 19:13:12.197929820 +0900
@@ -2082,6 +2082,7 @@ extern int user_statfs(const char __user
 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(struct super_block *super);
 extern bool our_mnt(struct vfsmount *mnt);
 

[RFC 3/9] 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: Josef Bacik <jbacik@fusionio.com>
Cc: Eric Sandeen <sandeen@redhat.com>
Cc: Christoph Hellwig <hch@infradead.org>
Reviewed-by: Jan Kara <jack@suse.cz>
Signed-off-by: Dave Chinner <dchinner@redhat.com>
Signed-off-by: Fernando Luis Vazquez Cao <fernando@oss.ntt.co.jp>
---

diff -urNp vfs-orig/fs/block_dev.c vfs/fs/block_dev.c
--- vfs-orig/fs/block_dev.c	2012-07-04 18:57:54.000000000 +0900
+++ vfs/fs/block_dev.c	2012-07-12 13:22:38.124815295 +0900
@@ -319,22 +319,17 @@ int thaw_bdev(struct block_device *bdev,
 	if (!bdev->bd_fsfreeze_count)
 		goto out;
 
-	error = 0;
-	if (--bdev->bd_fsfreeze_count > 0)
-		goto out;
-
-	if (!sb)
+	if (--bdev->bd_fsfreeze_count > 0 || !sb) {
+		error = 0;
 		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);
 

[RFC 4/9] 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.

Use the unlocked version of thaw_super() to do the thawing and replace
iterate_supers() with __iterate_supers() so that the unfreeze operation can
be performed with s_umount held as the locking rules for fsfreeze indicate.

Cc: Josef Bacik <jbacik@fusionio.com>
Cc: Eric Sandeen <sandeen@redhat.com>
Cc: Christoph Hellwig <hch@infradead.org>
Cc: Jan Kara <jack@suse.cz>
Cc: Dave Chinner <dchinner@redhat.com>
Signed-off-by: Fernando Luis Vazquez Cao <fernando@oss.ntt.co.jp>
---

diff -urNp linux-3.6-rc5-orig/fs/buffer.c linux-3.6-rc5/fs/buffer.c
--- linux-3.6-rc5-orig/fs/buffer.c	2012-09-12 20:23:59.146047001 +0900
+++ linux-3.6-rc5/fs/buffer.c	2012-09-12 20:33:35.994042452 +0900
@@ -511,17 +511,32 @@ repeat:
 	return err;
 }
 
+static int thaw_super_emergency(struct super_block *sb)
+{
+	int res;
+	/* We were called from __iterate_supers with superblock lock taken
+	 * so we do not need to do it here. */
+	res = __thaw_super(sb);
+	if (!res)
+		deactivate_locked_super(sb);
+	else
+		up_write(&sb->s_umount);
+	return res;
+}
+
 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))
-		printk(KERN_WARNING "Emergency Thaw on %s\n",
+	if (sb->s_bdev) {
+		char b[BDEVNAME_SIZE];
+		printk(KERN_WARNING "Emergency Thaw on %s.\n",
 		       bdevname(sb->s_bdev, b));
+	}
+	while (!thaw_super_emergency(sb));
 }
 
 static void do_thaw_all(struct work_struct *work)
 {
-	iterate_supers(do_thaw_one, NULL);
+	__iterate_supers(do_thaw_one, NULL, true);
 	kfree(work);
 	printk(KERN_WARNING "Emergency Thaw complete\n");
 }

[RFC 5/9] xfs: switch to using super methods for fsfreeze

[Fernando Luis Vázquez Cao]

xfs uses the bdev interfaces despite the fact that the kernel is operating
at the superblock level here. Convert xfs to use the superblock interfaces
instead.

Cc: Josef Bacik <jbacik@fusionio.com>
Cc: Eric Sandeen <sandeen@redhat.com>
Cc: Christoph Hellwig <hch@infradead.org>
Reviewed-by: Jan Kara <jack@suse.cz>
Signed-off-by: Dave Chinner <dchinner@redhat.com>
Signed-off-by: Fernando Luis Vazquez Cao <fernando@oss.ntt.co.jp>
---

diff -urNp linux-3.6-rc5-orig/fs/xfs/xfs_fsops.c linux-3.6-rc5/fs/xfs/xfs_fsops.c
--- linux-3.6-rc5-orig/fs/xfs/xfs_fsops.c	2012-07-22 05:58:29.000000000 +0900
+++ linux-3.6-rc5/fs/xfs/xfs_fsops.c	2012-09-12 19:56:33.174046534 +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;

[RFC 6/9] fsfreeze: move emergency thaw code to fs/super.c

[Fernando Luis Vázquez Cao]

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: Josef Bacik <jbacik@fusionio.com>
Cc: Eric Sandeen <sandeen@redhat.com>
Cc: Christoph Hellwig <hch@infradead.org>
Reviewed-by: Jan Kara <jack@suse.cz>
Signed-off-by: Dave Chinner <dchinner@redhat.com>
Signed-off-by: Fernando Luis Vazquez Cao <fernando@oss.ntt.co.jp>
---

diff -urNp linux-3.6-rc5-orig/fs/buffer.c linux-3.6-rc5/fs/buffer.c
--- linux-3.6-rc5-orig/fs/buffer.c	2012-09-12 20:44:13.226112590 +0900
+++ linux-3.6-rc5/fs/buffer.c	2012-09-12 20:50:25.406058417 +0900
@@ -511,52 +511,6 @@ repeat:
 	return err;
 }
 
-static int thaw_super_emergency(struct super_block *sb)
-{
-	int res;
-	/* We were called from __iterate_supers with superblock lock taken
-	 * so we do not need to do it here. */
-	res = __thaw_super(sb);
-	if (!res)
-		deactivate_locked_super(sb);
-	else
-		up_write(&sb->s_umount);
-	return res;
-}
-
-static void do_thaw_one(struct super_block *sb, void *unused)
-{
-	if (sb->s_bdev) {
-		char b[BDEVNAME_SIZE];
-		printk(KERN_WARNING "Emergency Thaw on %s.\n",
-		       bdevname(sb->s_bdev, b));
-	}
-	while (!thaw_super_emergency(sb));
-}
-
-static void do_thaw_all(struct work_struct *work)
-{
-	__iterate_supers(do_thaw_one, NULL, true);
-	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.6-rc5-orig/fs/super.c linux-3.6-rc5/fs/super.c
--- linux-3.6-rc5-orig/fs/super.c	2012-09-12 20:24:10.474041390 +0900
+++ linux-3.6-rc5/fs/super.c	2012-09-12 20:50:42.546044906 +0900
@@ -1475,3 +1475,49 @@ int thaw_super(struct super_block *sb)
 	return res;
 }
 EXPORT_SYMBOL(thaw_super);
+
+static int thaw_super_emergency(struct super_block *sb)
+{
+	int res;
+	/* We were called from __iterate_supers with superblock lock taken
+	 * so we do not need to do it here. */
+	res = __thaw_super(sb);
+	if (!res)
+		deactivate_locked_super(sb);
+	else
+		up_write(&sb->s_umount);
+	return res;
+}
+
+static void do_thaw_one(struct super_block *sb, void *unused)
+{
+	if (sb->s_bdev) {
+		char b[BDEVNAME_SIZE];
+		printk(KERN_WARNING "Emergency Thaw on %s.\n",
+		       bdevname(sb->s_bdev, b));
+	}
+	while (!thaw_super_emergency(sb));
+}
+
+static void do_thaw_all(struct work_struct *work)
+{
+	__iterate_supers(do_thaw_one, NULL, true);
+	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.6-rc5-orig/include/linux/fs.h linux-3.6-rc5/include/linux/fs.h
--- linux-3.6-rc5-orig/include/linux/fs.h	2012-09-12 20:24:10.474041390 +0900
+++ linux-3.6-rc5/include/linux/fs.h	2012-09-12 20:46:22.454047005 +0900
@@ -2082,7 +2082,6 @@ extern int user_statfs(const char __user
 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(struct super_block *super);
 extern bool our_mnt(struct vfsmount *mnt);
 
@@ -2685,8 +2684,6 @@ extern struct super_block *get_super(str
 extern struct super_block *get_super_thawed(struct block_device *);
 extern struct super_block *get_active_super(struct block_device *bdev);
 extern void drop_super(struct super_block *sb);
-extern void __iterate_supers(void (*f)(struct super_block *, void *), void *arg,
-		      bool wlock);
 extern void iterate_supers(void (*)(struct super_block *, void *), void *);
 extern void iterate_supers_type(struct file_system_type *,
 			        void (*)(struct super_block *, void *), void *);

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

[Fernando Luis Vázquez Cao]

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.

IMPORTANT CAVEAT: This patch restores the nesting feature of the fsfreeze ioctl
which got removed by commit 18e9e5104fcd9a973ffe3eed3816c87f2a1b6cd2
("Introduce freeze_super and thaw_super for the fsfreeze ioctl"). It could be
argued that it is too late to fix the userland ABI breakage caused by that
patch and that the current ABI is the one that should be kept. If this is the
respective maintainer(s) opinion this could be modified to not allow fsfreeze
ioctl nesting.

Changes since version 2:
  - Fix reference count leak in freeze_super when MS_BORN flag is not set in
    the superblock.
  - Protect freeze counter using s_umount and get rid of superblock level
    fsfreeze mutex.

Cc: Josef Bacik <jbacik@fusionio.com>
Cc: Eric Sandeen <sandeen@redhat.com>
Cc: Christoph Hellwig <hch@infradead.org>
Cc: Jan Kara <jack@suse.cz>
Cc: Dave Chinner <dchinner@redhat.com>
Signed-off-by: Fernando Luis Vazquez Cao <fernando@oss.ntt.co.jp>
---

diff -urNp linux-3.6-rc5-orig/fs/block_dev.c linux-3.6-rc5/fs/block_dev.c
--- linux-3.6-rc5-orig/fs/block_dev.c	2012-09-13 18:22:49.176451032 +0900
+++ linux-3.6-rc5/fs/block_dev.c	2012-09-13 18:28:38.036470493 +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,29 +289,32 @@ 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
  *
- * Unlocks the filesystem and marks it writeable again after freeze_bdev().
+ * Unlocks the block device and, if present, the associated filesystem too.
  */
 int thaw_bdev(struct block_device *bdev, struct super_block *sb)
 {
 	int error = -EINVAL;
 
 	mutex_lock(&bdev->bd_fsfreeze_mutex);
+
 	if (!bdev->bd_fsfreeze_count)
 		goto out;
 
-	if (--bdev->bd_fsfreeze_count > 0 || !sb) {
+	bdev->bd_fsfreeze_count--;
+
+	if (!sb) {
 		error = 0;
 		goto out;
 	}
diff -urNp linux-3.6-rc5-orig/fs/gfs2/ops_fstype.c linux-3.6-rc5/fs/gfs2/ops_fstype.c
--- linux-3.6-rc5-orig/fs/gfs2/ops_fstype.c	2012-09-13 18:21:55.628450080 +0900
+++ linux-3.6-rc5/fs/gfs2/ops_fstype.c	2012-09-13 18:28:38.036470493 +0900
@@ -1288,11 +1288,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.6-rc5-orig/fs/nilfs2/super.c linux-3.6-rc5/fs/nilfs2/super.c
--- linux-3.6-rc5-orig/fs/nilfs2/super.c	2012-09-13 18:21:55.828450115 +0900
+++ linux-3.6-rc5/fs/nilfs2/super.c	2012-09-13 18:28:38.036470493 +0900
@@ -1273,11 +1273,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.6-rc5-orig/fs/super.c linux-3.6-rc5/fs/super.c
--- linux-3.6-rc5-orig/fs/super.c	2012-09-13 18:23:00.912451285 +0900
+++ linux-3.6-rc5/fs/super.c	2012-09-13 19:35:20.544495212 +0900
@@ -1024,11 +1024,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);
@@ -1322,8 +1317,11 @@ static void sb_wait_write(struct super_b
  * @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 counts down in thaw_super(). When it becomes 0,
+ * thaw_super() will execute the unfreeze.
  *
  * During this function, sb->s_writers.frozen goes through these values:
  *
@@ -1348,29 +1346,27 @@ static void sb_wait_write(struct super_b
  * freezing. Then we transition to SB_FREEZE_COMPLETE state. This state is
  * mostly auxiliary for filesystems to verify they do not modify frozen fs.
  *
- * sb->s_writers.frozen is protected by sb->s_umount.
+ * sb->s_writers.frozen and sb->s_freeze_count are protected by sb->s_umount.
  */
 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_writers.frozen != SB_UNFROZEN) {
-		deactivate_locked_super(sb);
-		return -EBUSY;
-	}
 
 	if (!(sb->s_flags & MS_BORN)) {
-		up_write(&sb->s_umount);
-		return 0;	/* sic - it's "nothing to do" */
+		atomic_dec(&sb->s_active);
+		goto out_active;	/* sic - it's "nothing to do" */
 	}
 
+	if (++sb->s_freeze_count > 1)
+		goto out_deactivate;
+
 	if (sb->s_flags & MS_RDONLY) {
 		/* Nothing to do really... */
 		sb->s_writers.frozen = SB_FREEZE_COMPLETE;
-		up_write(&sb->s_umount);
-		return 0;
+		goto out_active;
 	}
 
 	/* From now on, no new normal writers can start */
@@ -1402,11 +1398,11 @@ int freeze_super(struct super_block *sb)
 		if (ret) {
 			printk(KERN_ERR
 				"VFS:Filesystem freeze failed\n");
+			sb->s_freeze_count--;
 			sb->s_writers.frozen = SB_UNFROZEN;
 			smp_wmb();
 			wake_up(&sb->s_writers.wait_unfrozen);
-			deactivate_locked_super(sb);
-			return ret;
+			goto out_deactivate;
 		}
 	}
 	/*
@@ -1414,8 +1410,12 @@ int freeze_super(struct super_block *sb)
 	 * sees write activity when frozen is set to SB_FREEZE_COMPLETE.
 	 */
 	sb->s_writers.frozen = SB_FREEZE_COMPLETE;
+out_active:
 	up_write(&sb->s_umount);
-	return 0;
+	return ret;
+out_deactivate:
+	deactivate_locked_super(sb);
+	return ret;
 }
 EXPORT_SYMBOL(freeze_super);
 
@@ -1425,6 +1425,10 @@ EXPORT_SYMBOL(freeze_super);
  *
  * Unlocks the filesystem and marks it writeable again.
  *
+ * Returns -EINVAL if @sb is not frozen, 0 if the filesystem specific unfreeze
+ * function was executed and succeeded or the corresponding error code
+ * otherwise. if the unfreeze fails, @sb is left in the frozen state.
+ *
  * This is the unlocked version of thaw_super, so it is the caller's job to
  * to protect the superblock by grabing the s_umount semaphore in write mode.
  */
@@ -1432,11 +1436,14 @@ int __thaw_super(struct super_block *sb)
 {
 	int error = 0;
 
-	if (sb->s_writers.frozen == SB_UNFROZEN) {
+	if (!sb->s_freeze_count) {
 		error = -EINVAL;
 		goto out;
 	}
 
+	if (--sb->s_freeze_count > 0)
+		goto out;
+
 	if (sb->s_flags & MS_RDONLY)
 		goto out_thaw;
 
@@ -1445,6 +1452,7 @@ int __thaw_super(struct super_block *sb)
 		if (error) {
 			printk(KERN_ERR
 				"VFS:Filesystem thaw failed\n");
+			sb->s_freeze_count++;
 			goto out;
 		}
 	}
diff -urNp linux-3.6-rc5-orig/include/linux/fs.h linux-3.6-rc5/include/linux/fs.h
--- linux-3.6-rc5-orig/include/linux/fs.h	2012-09-13 18:23:00.912451285 +0900
+++ linux-3.6-rc5/include/linux/fs.h	2012-09-13 19:12:38.720491035 +0900
@@ -1578,6 +1578,8 @@ struct super_block {
 
 	/* Being remounted read-only */
 	int s_readonly_remount;
+
+	int			s_freeze_count; /* nr of nested freezes */
 };
 
 /* superblock cache pruning functions */

[PATCH 8/9] 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: Josef Bacik <jbacik@fusionio.com>
Cc: Eric Sandeen <sandeen@redhat.com>
Cc: Christoph Hellwig <hch@infradead.org>
Cc: Jan Kara <jack@suse.cz>
Cc: Dave Chinner <dchinner@redhat.com>
Signed-off-by: Fernando Luis Vazquez Cao <fernando@oss.ntt.co.jp>
---

diff -urNp linux-3.6-rc5-orig/fs/compat_ioctl.c linux-3.6-rc5/fs/compat_ioctl.c
--- linux-3.6-rc5-orig/fs/compat_ioctl.c	2012-07-22 05:58:29.000000000 +0900
+++ linux-3.6-rc5/fs/compat_ioctl.c	2012-09-13 15:40:54.540150687 +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.6-rc5-orig/fs/ioctl.c linux-3.6-rc5/fs/ioctl.c
--- linux-3.6-rc5-orig/fs/ioctl.c	2012-07-22 05:58:29.000000000 +0900
+++ linux-3.6-rc5/fs/ioctl.c	2012-09-13 15:40:54.540150687 +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.6-rc5-orig/fs/super.c linux-3.6-rc5/fs/super.c
--- linux-3.6-rc5-orig/fs/super.c	2012-09-13 15:38:07.356137398 +0900
+++ linux-3.6-rc5/fs/super.c	2012-09-13 15:58:31.516184164 +0900
@@ -1533,3 +1533,8 @@ void emergency_thaw_all(void)
 		schedule_work(work);
 	}
 }
+
+int isfrozen_super(struct super_block *sb)
+{
+	return sb->s_writers.frozen > SB_UNFROZEN ? 1 : 0;
+}
diff -urNp linux-3.6-rc5-orig/include/linux/fs.h linux-3.6-rc5/include/linux/fs.h
--- linux-3.6-rc5-orig/include/linux/fs.h	2012-09-13 15:38:07.356137398 +0900
+++ linux-3.6-rc5/include/linux/fs.h	2012-09-13 15:40:54.540150687 +0900
@@ -342,6 +342,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)
@@ -2086,6 +2087,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 9/9] fsfreeze: add block device ioctl to check freeze state

[Fernando Luis Vázquez Cao]

The BLKISFROZEN ioctl can be used to check the freeze state of a block device
(it is possible to freeze a block device that does not have a filesystem
sitting on top of it).

If allowing the umounting of frozen filesystems is deemed acceptable this ioctl
will be extended to check the state of the associated superblock if any.

Cc: Josef Bacik <jbacik@fusionio.com>
Cc: Eric Sandeen <sandeen@redhat.com>
Cc: Christoph Hellwig <hch@infradead.org>
Cc: Jan Kara <jack@suse.cz>
Cc: Dave Chinner <dchinner@redhat.com>
Signed-off-by: Fernando Luis Vazquez Cao <fernando@oss.ntt.co.jp>
---

diff -urNp linux-3.6-rc5-orig/block/compat_ioctl.c linux-3.6-rc5/block/compat_ioctl.c
--- linux-3.6-rc5-orig/block/compat_ioctl.c	2012-07-22 05:58:29.000000000 +0900
+++ linux-3.6-rc5/block/compat_ioctl.c	2012-09-13 16:22:16.092304889 +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.6-rc5-orig/block/ioctl.c linux-3.6-rc5/block/ioctl.c
--- linux-3.6-rc5-orig/block/ioctl.c	2012-09-12 20:23:55.230047001 +0900
+++ linux-3.6-rc5/block/ioctl.c	2012-09-13 16:22:16.092304889 +0900
@@ -396,6 +396,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.6-rc5-orig/fs/block_dev.c linux-3.6-rc5/fs/block_dev.c
--- linux-3.6-rc5-orig/fs/block_dev.c	2012-09-13 15:38:07.332137396 +0900
+++ linux-3.6-rc5/fs/block_dev.c	2012-09-13 16:22:16.092304889 +0900
@@ -328,6 +328,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.6-rc5-orig/include/linux/fs.h linux-3.6-rc5/include/linux/fs.h
--- linux-3.6-rc5-orig/include/linux/fs.h	2012-09-13 16:15:04.996291015 +0900
+++ linux-3.6-rc5/include/linux/fs.h	2012-09-13 16:22:16.092304889 +0900
@@ -335,6 +335,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 */
@@ -2251,6 +2252,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) {}
@@ -2271,6 +2273,11 @@ static inline int thaw_bdev(struct block
 static inline void iterate_bdevs(void (*f)(struct block_device *, void *), void *arg)
 {
 }
+
+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: [RFC 3/9] fsfreeze: Prevent emergency thaw from looping infinitely

[Eric Sandeen]

On 9/13/12 6:03 AM, 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.
> 
> Return -EINVAL when the filesystem is already unfrozen to avoid this
> problem.
> 
> Cc: Josef Bacik <jbacik@fusionio.com>
> Cc: Eric Sandeen <sandeen@redhat.com>
> Cc: Christoph Hellwig <hch@infradead.org>
> Reviewed-by: Jan Kara <jack@suse.cz>
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> Signed-off-by: Fernando Luis Vazquez Cao <fernando@oss.ntt.co.jp>

Reviewed-by: Eric Sandeen <sandeen@redhat.com>

I actually sent a simpler (but less cleaned up) version of this 2 years ago,
I guess it got missed and I didn't notice.  So thanks!

-Eric

> ---
> 
> diff -urNp vfs-orig/fs/block_dev.c vfs/fs/block_dev.c
> --- vfs-orig/fs/block_dev.c	2012-07-04 18:57:54.000000000 +0900
> +++ vfs/fs/block_dev.c	2012-07-12 13:22:38.124815295 +0900
> @@ -319,22 +319,17 @@ int thaw_bdev(struct block_device *bdev,
>  	if (!bdev->bd_fsfreeze_count)
>  		goto out;
>  
> -	error = 0;
> -	if (--bdev->bd_fsfreeze_count > 0)
> -		goto out;
> -
> -	if (!sb)
> +	if (--bdev->bd_fsfreeze_count > 0 || !sb) {
> +		error = 0;
>  		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

Re: [RFC 2/9] fsfreeze: add unlocked version of thaw_super

[Eric Sandeen]

On 9/13/12 6:01 AM, Fernando Luis Vázquez Cao wrote:
> thaw_super may be called with superblock lock already taken (fsfreeze's
> emergency thaw being one example), so we need an unlocked version to avoid
> lockups.
> 
> Cc: Josef Bacik <jbacik@fusionio.com>
> Cc: Eric Sandeen <sandeen@redhat.com>
> Cc: Christoph Hellwig <hch@infradead.org>
> Cc: Jan Kara <jack@suse.cz>
> Cc: Dave Chinner <dchinner@redhat.com>
> Signed-off-by: Fernando Luis Vazquez Cao <fernando@oss.ntt.co.jp>
> ---
> 
> diff -urNp linux-3.6-rc5-orig/fs/super.c linux-3.6-rc5/fs/super.c
> --- linux-3.6-rc5-orig/fs/super.c	2012-09-12 19:13:00.710047001 +0900
> +++ linux-3.6-rc5/fs/super.c	2012-09-12 19:13:12.185924077 +0900
> @@ -1420,40 +1420,58 @@ int freeze_super(struct super_block *sb)
>  EXPORT_SYMBOL(freeze_super);
>  
>  /**
> - * thaw_super -- unlock filesystem
> + * __thaw_super -- unlock filesystem
>   * @sb: the super to thaw
>   *
> - * Unlocks the filesystem and marks it writeable again after freeze_super().
> + * Unlocks the filesystem and marks it writeable again.
> + *
> + * This is the unlocked version of thaw_super, so it is the caller's job to
> + * to protect the superblock by grabing the s_umount semaphore in write mode.

s/grabing/grabbing/ - and maybe:

+ * and release it again on return, see thaw_super().

But the logic looks fine,

Reviewed-by: Eric Sandeen <sandeen@redhat.com>

>   */
> -int thaw_super(struct super_block *sb)
> +int __thaw_super(struct super_block *sb)
>  {
> -	int error;
> +	int error = 0;
>  
> -	down_write(&sb->s_umount);
>  	if (sb->s_writers.frozen == SB_UNFROZEN) {
> -		up_write(&sb->s_umount);
> -		return -EINVAL;
> +		error = -EINVAL;
> +		goto out;
>  	}
>  
>  	if (sb->s_flags & MS_RDONLY)
> -		goto out;
> +		goto out_thaw;
>  
>  	if (sb->s_op->unfreeze_fs) {
>  		error = sb->s_op->unfreeze_fs(sb);
>  		if (error) {
>  			printk(KERN_ERR
>  				"VFS:Filesystem thaw failed\n");
> -			up_write(&sb->s_umount);
> -			return error;
> +			goto out;
>  		}
>  	}
>  
> -out:
> +out_thaw:
>  	sb->s_writers.frozen = SB_UNFROZEN;
>  	smp_wmb();
>  	wake_up(&sb->s_writers.wait_unfrozen);
> -	deactivate_locked_super(sb);
> +out:
> +	return error;
> +}
>  
> -	return 0;
> +/**
> + * 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)
> +{
> +	int res;
> +	down_write(&sb->s_umount);
> +	res = __thaw_super(sb);
> +	if (!res)
> +		deactivate_locked_super(sb);
> +	else
> +		up_write(&sb->s_umount);
> +	return res;
>  }
>  EXPORT_SYMBOL(thaw_super);
> diff -urNp linux-3.6-rc5-orig/include/linux/fs.h linux-3.6-rc5/include/linux/fs.h
> --- linux-3.6-rc5-orig/include/linux/fs.h	2012-09-12 19:13:00.830047035 +0900
> +++ linux-3.6-rc5/include/linux/fs.h	2012-09-12 19:13:12.197929820 +0900
> @@ -2082,6 +2082,7 @@ extern int user_statfs(const char __user
>  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(struct super_block *super);
>  extern bool our_mnt(struct vfsmount *mnt);
>  
> 
> 

--
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: [RFC 6/9] fsfreeze: move emergency thaw code to fs/super.c

[Josef Bacik]

On Thu, Sep 13, 2012 at 05:08:19AM -0600, Fernando Luis Vázquez Cao wrote:
> 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: Josef Bacik <jbacik@fusionio.com>
> Cc: Eric Sandeen <sandeen@redhat.com>
> Cc: Christoph Hellwig <hch@infradead.org>
> Reviewed-by: Jan Kara <jack@suse.cz>
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> Signed-off-by: Fernando Luis Vazquez Cao <fernando@oss.ntt.co.jp>
> ---
> 
> diff -urNp linux-3.6-rc5-orig/fs/buffer.c linux-3.6-rc5/fs/buffer.c
> --- linux-3.6-rc5-orig/fs/buffer.c	2012-09-12 20:44:13.226112590 +0900
> +++ linux-3.6-rc5/fs/buffer.c	2012-09-12 20:50:25.406058417 +0900
> @@ -511,52 +511,6 @@ repeat:
>  	return err;
>  }
>  
> -static int thaw_super_emergency(struct super_block *sb)
> -{
> -	int res;
> -	/* We were called from __iterate_supers with superblock lock taken
> -	 * so we do not need to do it here. */
> -	res = __thaw_super(sb);
> -	if (!res)
> -		deactivate_locked_super(sb);
> -	else
> -		up_write(&sb->s_umount);
> -	return res;
> -}
> -
> -static void do_thaw_one(struct super_block *sb, void *unused)
> -{
> -	if (sb->s_bdev) {
> -		char b[BDEVNAME_SIZE];
> -		printk(KERN_WARNING "Emergency Thaw on %s.\n",
> -		       bdevname(sb->s_bdev, b));
> -	}
> -	while (!thaw_super_emergency(sb));
> -}
> -
> -static void do_thaw_all(struct work_struct *work)
> -{
> -	__iterate_supers(do_thaw_one, NULL, true);
> -	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.6-rc5-orig/fs/super.c linux-3.6-rc5/fs/super.c
> --- linux-3.6-rc5-orig/fs/super.c	2012-09-12 20:24:10.474041390 +0900
> +++ linux-3.6-rc5/fs/super.c	2012-09-12 20:50:42.546044906 +0900
> @@ -1475,3 +1475,49 @@ int thaw_super(struct super_block *sb)
>  	return res;
>  }
>  EXPORT_SYMBOL(thaw_super);
> +
> +static int thaw_super_emergency(struct super_block *sb)
> +{
> +	int res;
> +	/* We were called from __iterate_supers with superblock lock taken
> +	 * so we do not need to do it here. */
> +	res = __thaw_super(sb);
> +	if (!res)
> +		deactivate_locked_super(sb);
> +	else
> +		up_write(&sb->s_umount);
> +	return res;
> +}

So unless I'm missing something this is wrong.  We do __iterate_supers() which
does down_write(sb) and then call into this.  Lets imagine a perfect world where
the sb was only frozen once.  So we go into __thaw_super() and return 0 because
we were successfull and do deactivate_locked_super() which does
up_write(s_umount), and then we loop because we want to get an -EINVAL to know
we completely unfroze, so we call into __thaw_super(sb) without s_umount held
and then we get our error and do up_write(s_umount) _again_.  So this needs to
be reworked to be correct ;).  Thanks,

Josef
--
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: [RFC 6/9] fsfreeze: move emergency thaw code to fs/super.c

[Eric Sandeen]

On 9/13/12 2:00 PM, Josef Bacik wrote:
> On Thu, Sep 13, 2012 at 05:08:19AM -0600, Fernando Luis Vázquez Cao wrote:
>> 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: Josef Bacik <jbacik@fusionio.com>
>> Cc: Eric Sandeen <sandeen@redhat.com>
>> Cc: Christoph Hellwig <hch@infradead.org>
>> Reviewed-by: Jan Kara <jack@suse.cz>
>> Signed-off-by: Dave Chinner <dchinner@redhat.com>
>> Signed-off-by: Fernando Luis Vazquez Cao <fernando@oss.ntt.co.jp>
>> ---
>>
>> diff -urNp linux-3.6-rc5-orig/fs/buffer.c linux-3.6-rc5/fs/buffer.c
>> --- linux-3.6-rc5-orig/fs/buffer.c	2012-09-12 20:44:13.226112590 +0900
>> +++ linux-3.6-rc5/fs/buffer.c	2012-09-12 20:50:25.406058417 +0900
>> @@ -511,52 +511,6 @@ repeat:
>>  	return err;
>>  }
>>  
>> -static int thaw_super_emergency(struct super_block *sb)
>> -{
>> -	int res;
>> -	/* We were called from __iterate_supers with superblock lock taken
>> -	 * so we do not need to do it here. */
>> -	res = __thaw_super(sb);
>> -	if (!res)
>> -		deactivate_locked_super(sb);
>> -	else
>> -		up_write(&sb->s_umount);
>> -	return res;
>> -}
>> -
>> -static void do_thaw_one(struct super_block *sb, void *unused)
>> -{
>> -	if (sb->s_bdev) {
>> -		char b[BDEVNAME_SIZE];
>> -		printk(KERN_WARNING "Emergency Thaw on %s.\n",
>> -		       bdevname(sb->s_bdev, b));
>> -	}
>> -	while (!thaw_super_emergency(sb));
>> -}
>> -
>> -static void do_thaw_all(struct work_struct *work)
>> -{
>> -	__iterate_supers(do_thaw_one, NULL, true);
>> -	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.6-rc5-orig/fs/super.c linux-3.6-rc5/fs/super.c
>> --- linux-3.6-rc5-orig/fs/super.c	2012-09-12 20:24:10.474041390 +0900
>> +++ linux-3.6-rc5/fs/super.c	2012-09-12 20:50:42.546044906 +0900
>> @@ -1475,3 +1475,49 @@ int thaw_super(struct super_block *sb)
>>  	return res;
>>  }
>>  EXPORT_SYMBOL(thaw_super);
>> +
>> +static int thaw_super_emergency(struct super_block *sb)
>> +{
>> +	int res;
>> +	/* We were called from __iterate_supers with superblock lock taken
>> +	 * so we do not need to do it here. */
>> +	res = __thaw_super(sb);
>> +	if (!res)
>> +		deactivate_locked_super(sb);
>> +	else
>> +		up_write(&sb->s_umount);
>> +	return res;
>> +}
> 	
> So unless I'm missing something this is wrong.  We do __iterate_supers() which
> does down_write(sb) and then call into this.  Lets imagine a perfect world where
> the sb was only frozen once.  So we go into __thaw_super() and return 0 because
> we were successfull and do deactivate_locked_super() which does
> up_write(s_umount), and then we loop because we want to get an -EINVAL to know
> we completely unfroze, so we call into __thaw_super(sb) without s_umount held
> and then we get our error and do up_write(s_umount) _again_.  So this needs to
> be reworked to be correct ;).  Thanks,

The stupid emergency sysrq thing was my fault (at someone else's suggestion) ;)

It's caused a lot of woe, and hasn't worked for two years.  Should we keep it?

-Eric

> Josef
> 

--
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: [RFC 0/9 v3] fsfreeze: miscellaneous fixes and cleanups

[Dave Chinner]

On Thu, Sep 13, 2012 at 07:57:42PM +0900, Fernando Luis Vázquez Cao wrote:
> This patch set 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 following patches are included:
> 
> ---
> [1/9] vfs: add __iterate_supers() helper
> [2/9] fsfreeze: add unlocked version of thaw_super
> 
> Preparatory patches to fix s_umount lockup of emergency thaw code.
> 
> [3/9] fsfreeze: Prevent emergency thaw from looping infinitely
> 
> Fix thaw_bdev so that it propagates the error code properly to the caller.
> This bug caused emergency thaw to loop infinitely. This is a forward port of
> a previous patch by Dave Chinner.
> 
> [4/9] fsfreeze: emergency thaw will deadlock on s_umount
> 
> Avoid emergency thaw deadlock on s_umount by using unlocked version of
> thaw_super() and __iterate_supers()i (introduced in patches 2 and 1
> respectively).

Given the problems with emergency thaw, this interface has never
really worked. In the absence of any obvious need for the
functionality (i.e. nobody has reported that it is broken since it
was introduced), why don't we simply remove it?

IIRC, the emergency thaw code was only added to alleviate
fear-mongering about systems getting stuck with unfreezable ext4
filesystems (after the "freeze w/ timeout" extensions were knocked
back), and time has indicated those fears were unfounded.

So, rather than trying to fix the emergency thaw mess, I say we
nuke it from orbit....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com
--
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: [RFC 6/9] fsfreeze: move emergency thaw code to fs/super.c

[Fernando Luis Vazquez Cao]

Hi Josef,

On 2012/09/14 04:00, Josef Bacik wrote:
> On Thu, Sep 13, 2012 at 05:08:19AM -0600, Fernando Luis Vázquez Cao wrote:
>> 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: Josef Bacik <jbacik@fusionio.com>
>> Cc: Eric Sandeen <sandeen@redhat.com>
>> Cc: Christoph Hellwig <hch@infradead.org>
>> Reviewed-by: Jan Kara <jack@suse.cz>
>> Signed-off-by: Dave Chinner <dchinner@redhat.com>
>> Signed-off-by: Fernando Luis Vazquez Cao <fernando@oss.ntt.co.jp>
>> ---
>>
>> diff -urNp linux-3.6-rc5-orig/fs/buffer.c linux-3.6-rc5/fs/buffer.c
>> --- linux-3.6-rc5-orig/fs/buffer.c	2012-09-12 20:44:13.226112590 +0900
>> +++ linux-3.6-rc5/fs/buffer.c	2012-09-12 20:50:25.406058417 +0900
>> @@ -511,52 +511,6 @@ repeat:
>>   	return err;
>>   }
>>   
>> -static int thaw_super_emergency(struct super_block *sb)
>> -{
>> -	int res;
>> -	/* We were called from __iterate_supers with superblock lock taken
>> -	 * so we do not need to do it here. */
>> -	res = __thaw_super(sb);
>> -	if (!res)
>> -		deactivate_locked_super(sb);
>> -	else
>> -		up_write(&sb->s_umount);
>> -	return res;
>> -}
>> -
>> -static void do_thaw_one(struct super_block *sb, void *unused)
>> -{
>> -	if (sb->s_bdev) {
>> -		char b[BDEVNAME_SIZE];
>> -		printk(KERN_WARNING "Emergency Thaw on %s.\n",
>> -		       bdevname(sb->s_bdev, b));
>> -	}
>> -	while (!thaw_super_emergency(sb));
>> -}
>> -
>> -static void do_thaw_all(struct work_struct *work)
>> -{
>> -	__iterate_supers(do_thaw_one, NULL, true);
>> -	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.6-rc5-orig/fs/super.c linux-3.6-rc5/fs/super.c
>> --- linux-3.6-rc5-orig/fs/super.c	2012-09-12 20:24:10.474041390 +0900
>> +++ linux-3.6-rc5/fs/super.c	2012-09-12 20:50:42.546044906 +0900
>> @@ -1475,3 +1475,49 @@ int thaw_super(struct super_block *sb)
>>   	return res;
>>   }
>>   EXPORT_SYMBOL(thaw_super);
>> +
>> +static int thaw_super_emergency(struct super_block *sb)
>> +{
>> +	int res;
>> +	/* We were called from __iterate_supers with superblock lock taken
>> +	 * so we do not need to do it here. */
>> +	res = __thaw_super(sb);
>> +	if (!res)
>> +		deactivate_locked_super(sb);
>> +	else
>> +		up_write(&sb->s_umount);
>> +	return res;
>> +}
> So unless I'm missing something this is wrong.  We do __iterate_supers() which
> does down_write(sb) and then call into this.  Lets imagine a perfect world where
> the sb was only frozen once.  So we go into __thaw_super() and return 0 because
> we were successfull and do deactivate_locked_super() which does
> up_write(s_umount), and then we loop because we want to get an -EINVAL to know
> we completely unfroze, so we call into __thaw_super(sb) without s_umount held
> and then we get our error and do up_write(s_umount) _again_.  So this needs to
> be reworked to be correct ;).

You are right. I had fixed that locally, but it seems that I sent an
old version of patches 4 and 6.

The current version turns
     while (!thaw_super_emergency(sb));
into
     thaw_super_emergency(sb);
and sets sb->s_freeze_count to 1  before calling __thaw_super(),
which should address the problem you pointed out.

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: [RFC 6/9] fsfreeze: move emergency thaw code to fs/super.c

[Fernando Luis Vazquez Cao]

On 2012/09/14 06:10, Eric Sandeen wrote:
> The stupid emergency sysrq thing was my fault (at someone else's suggestion) ;)
>
> It's caused a lot of woe, and hasn't worked for two years.  Should we keep it?

It turns out that emergency thaw is useful in virtuazalition
environments where a guest's filesystem can be frozen
by a hypervisor controlled guest agent without the guest's
users and administrator being aware of it. In such scenarios
if the guest agent dies leaving a filesystem frozen we are
in trouble. The guest's administrator or root user will eventually
notice that writes to the filesystem that was frozen behind its back
block but it has no way to figure out what is going on since
we do not have check ioctls. In such cases emergency thaw
is very useful. Please notice that even if we managed to restart
the guest agent, in many cases it is stateless so it will not
remember that a froze the filesystem.

Re: [RFC 0/9 v3] fsfreeze: miscellaneous fixes and cleanups

[Fernando Luis Vazquez Cao]

On 2012/09/14 09:57, Dave Chinner wrote:
> On Thu, Sep 13, 2012 at 07:57:42PM +0900, Fernando Luis Vázquez Cao wrote:
>> This patch set 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 following patches are included:
>>
>> ---
>> [1/9] vfs: add __iterate_supers() helper
>> [2/9] fsfreeze: add unlocked version of thaw_super
>>
>> Preparatory patches to fix s_umount lockup of emergency thaw code.
>>
>> [3/9] fsfreeze: Prevent emergency thaw from looping infinitely
>>
>> Fix thaw_bdev so that it propagates the error code properly to the caller.
>> This bug caused emergency thaw to loop infinitely. This is a forward port of
>> a previous patch by Dave Chinner.
>>
>> [4/9] fsfreeze: emergency thaw will deadlock on s_umount
>>
>> Avoid emergency thaw deadlock on s_umount by using unlocked version of
>> thaw_super() and __iterate_supers()i (introduced in patches 2 and 1
>> respectively).
> Given the problems with emergency thaw, this interface has never
> really worked. In the absence of any obvious need for the
> functionality (i.e. nobody has reported that it is broken since it
> was introduced), why don't we simply remove it?
>
> IIRC, the emergency thaw code was only added to alleviate
> fear-mongering about systems getting stuck with unfreezable ext4
> filesystems (after the "freeze w/ timeout" extensions were knocked
> back), and time has indicated those fears were unfounded.
>
> So, rather than trying to fix the emergency thaw mess, I say we
> nuke it from orbit....

As I commented to Eric, In virtualization environments it comes in
handy sometimes. For example, in an emergency case where a guest
agent dies leaving one or more filesystems frozen emergency thaw
is useful.

Hopefully my fix is correct and we can keep this feature.

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: [RFC 0/9 v3] fsfreeze: miscellaneous fixes and cleanups

[Eric Sandeen]

On 9/13/12 9:20 PM, Fernando Luis Vazquez Cao wrote:
> On 2012/09/14 09:57, Dave Chinner wrote:
>> On Thu, Sep 13, 2012 at 07:57:42PM +0900, Fernando Luis Vázquez Cao wrote:
>>> This patch set 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 following patches are included:
>>>
>>> ---
>>> [1/9] vfs: add __iterate_supers() helper
>>> [2/9] fsfreeze: add unlocked version of thaw_super
>>>
>>> Preparatory patches to fix s_umount lockup of emergency thaw code.
>>>
>>> [3/9] fsfreeze: Prevent emergency thaw from looping infinitely
>>>
>>> Fix thaw_bdev so that it propagates the error code properly to the caller.
>>> This bug caused emergency thaw to loop infinitely. This is a forward port of
>>> a previous patch by Dave Chinner.
>>>
>>> [4/9] fsfreeze: emergency thaw will deadlock on s_umount
>>>
>>> Avoid emergency thaw deadlock on s_umount by using unlocked version of
>>> thaw_super() and __iterate_supers()i (introduced in patches 2 and 1
>>> respectively).
>> Given the problems with emergency thaw, this interface has never
>> really worked. In the absence of any obvious need for the
>> functionality (i.e. nobody has reported that it is broken since it
>> was introduced), why don't we simply remove it?
>>
>> IIRC, the emergency thaw code was only added to alleviate
>> fear-mongering about systems getting stuck with unfreezable ext4
>> filesystems (after the "freeze w/ timeout" extensions were knocked
>> back), and time has indicated those fears were unfounded.
>>
>> So, rather than trying to fix the emergency thaw mess, I say we
>> nuke it from orbit....
> 
> As I commented to Eric, In virtualization environments it comes in
> handy sometimes. For example, in an emergency case where a guest
> agent dies leaving one or more filesystems frozen emergency thaw
> is useful.

Except it hasn't actually worked for 2 years, so it really probably
hasn't been handy at all, in practice.

> Hopefully my fix is correct and we can keep this feature.

The fix comes at a cost of quite a lot of complexity and rewriting, though.
We can always write more and more complex code, for weird administrative
corner cases, but is it worth it?  I'm not quite convinced yet.

-Eric

> 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 1/9] vfs: add __iterate_supers() helper

[Eric Sandeen]

On 9/13/12 6:00 AM, Fernando Luis Vázquez Cao wrote:
> iterate_supers() calls a function provided by the caller with the s_umount
> semaphore taken in read mode. However, there may be cases where write mode
> is preferable, so we add a new helper, __iterate_supers(), which lets one
> specify the mode of the lock.
> 
> This will be used to fix the emergency thaw (filesystem unfreeze) code, which
> iterates over the list of superblocks but needs to hold the s_umount semaphore
> in _write_ mode bebore carrying out the actual thaw operation.

I'll go ahead & comment on this even though all of this is to support the
emergency thaw misfeature which I'm liking less and less this evening.

I think this split is a little odd, usually __foo() provides some fundamental
action, and foo_*() functions call it after possibly defining or altering aspects of
that action, or preparing for that action.

So having iterate_supers() do read locks, and __iterate_supers do either read
or write, seems asymmetric, and isn't very well self-documenting.

Why not have i.e. iterate_supers_read() and iterate_supers_write(), each
of which can call __iterate_supers(blah, blah, locktype).

Anyway, it'd mean changing 5 callers but it's a little clearer and more symmetric
to me.  What do you think?

The logic seems fine, just a question about the style.

Thanks,
-Eric

> Cc: Josef Bacik <jbacik@fusionio.com>
> Cc: Eric Sandeen <sandeen@redhat.com>
> Cc: Christoph Hellwig <hch@infradead.org>
> Cc: Jan Kara <jack@suse.cz>
> Cc: Dave Chinner <dchinner@redhat.com>
> Signed-off-by: Fernando Luis Vazquez Cao <fernando@oss.ntt.co.jp>
> ---
> 
> diff -urNp linux-3.6-rc5-orig/fs/super.c linux-3.6-rc5/fs/super.c
> --- linux-3.6-rc5-orig/fs/super.c	2012-09-12 18:45:13.818046999 +0900
> +++ linux-3.6-rc5/fs/super.c	2012-09-12 19:08:58.214034467 +0900
> @@ -537,14 +537,22 @@ void drop_super(struct super_block *sb)
>  EXPORT_SYMBOL(drop_super);
>  
>  /**
> - *	iterate_supers - call function for all active superblocks
> + *	__iterate_supers - call function for all active superblocks
>   *	@f: function to call
>   *	@arg: argument to pass to it
> + *	@wlock: mode of superblock lock (false->read lock, true->write lock)
>   *
>   *	Scans the superblock list and calls given function, passing it
>   *	locked superblock and given argument.
> + *
> + *	When the caller asks for the superblock lock (s_umount semaphore) to be
> + *	taken in write mode, the lock is taken but not released because the
> + *	function provided by the caller may deactivate the superblock itself.
> + *	It is that function's job to unlock the superblock as needed in such a
> + *	case.
>   */
> -void iterate_supers(void (*f)(struct super_block *, void *), void *arg)
> +void __iterate_supers(void (*f)(struct super_block *, void *), void *arg,
> +		      bool wlock)
>  {
>  	struct super_block *sb, *p = NULL;
>  
> @@ -555,10 +563,19 @@ void iterate_supers(void (*f)(struct sup
>  		sb->s_count++;
>  		spin_unlock(&sb_lock);
>  
> -		down_read(&sb->s_umount);
> +		if (wlock)
> +			down_write(&sb->s_umount);
> +		else
> +			down_read(&sb->s_umount);
> +
>  		if (sb->s_root && (sb->s_flags & MS_BORN))
>  			f(sb, arg);
> -		up_read(&sb->s_umount);
> +
> +		/* When the semaphore was taken in write mode the function
> +		 * provided by the caller takes care of unlocking it as
> +		 * needed. See explanation above for details. */
> +		if (!wlock)
> +			up_read(&sb->s_umount);
>  
>  		spin_lock(&sb_lock);
>  		if (p)
> @@ -571,6 +588,19 @@ void iterate_supers(void (*f)(struct sup
>  }
>  
>  /**
> + *	iterate_supers - call function for all active superblocks
> + *	@f: function to call
> + *	@arg: argument to pass to it
> + *
> + *	Scans the superblock list and calls given function, passing it
> + *	a superblock locked in _read_ mode and given argument.
> + */
> +void iterate_supers(void (*f)(struct super_block *, void *), void *arg)
> +{
> +	__iterate_supers(f, arg , false);
> +}
> +
> +/**
>   *	iterate_supers_type - call function for superblocks of given type
>   *	@type: fs type
>   *	@f: function to call
> diff -urNp linux-3.6-rc5-orig/include/linux/fs.h linux-3.6-rc5/include/linux/fs.h
> --- linux-3.6-rc5-orig/include/linux/fs.h	2012-09-12 18:45:14.002047001 +0900
> +++ linux-3.6-rc5/include/linux/fs.h	2012-09-12 18:46:39.466082276 +0900
> @@ -2684,6 +2684,8 @@ extern struct super_block *get_super(str
>  extern struct super_block *get_super_thawed(struct block_device *);
>  extern struct super_block *get_active_super(struct block_device *bdev);
>  extern void drop_super(struct super_block *sb);
> +extern void __iterate_supers(void (*f)(struct super_block *, void *), void *arg,
> +		      bool wlock);
>  extern void iterate_supers(void (*)(struct super_block *, void *), void *);
>  extern void iterate_supers_type(struct file_system_type *,
>  			        void (*)(struct super_block *, void *), void *);
> 
> 

--
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 1/9] vfs: add __iterate_supers() helper

[Fernando Luis Vazquez Cao]

On 2012/09/14 11:36, Eric Sandeen wrote:
> On 9/13/12 6:00 AM, Fernando Luis Vázquez Cao wrote:
>> iterate_supers() calls a function provided by the caller with the s_umount
>> semaphore taken in read mode. However, there may be cases where write mode
>> is preferable, so we add a new helper, __iterate_supers(), which lets one
>> specify the mode of the lock.
>>
>> This will be used to fix the emergency thaw (filesystem unfreeze) code, which
>> iterates over the list of superblocks but needs to hold the s_umount semaphore
>> in _write_ mode bebore carrying out the actual thaw operation.
> I'll go ahead & comment on this even though all of this is to support the
> emergency thaw misfeature which I'm liking less and less this evening.
>
> I think this split is a little odd, usually __foo() provides some fundamental
> action, and foo_*() functions call it after possibly defining or altering aspects of
> that action, or preparing for that action.
>
> So having iterate_supers() do read locks, and __iterate_supers do either read
> or write, seems asymmetric, and isn't very well self-documenting.
>
> Why not have i.e. iterate_supers_read() and iterate_supers_write(), each
> of which can call __iterate_supers(blah, blah, locktype).
>
> Anyway, it'd mean changing 5 callers but it's a little clearer and more symmetric
> to me.  What do you think?
>
> The logic seems fine, just a question about the style.

I agree with you. I will fix the style as you suggest.

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: [RFC 0/9 v3] fsfreeze: miscellaneous fixes and cleanups

[Fernando Luis Vazquez Cao]

On 2012年09月14日 11:33, Eric Sandeen wrote:
> On 9/13/12 9:20 PM, Fernando Luis Vazquez Cao wrote:
>> On 2012/09/14 09:57, Dave Chinner wrote:
>>> On Thu, Sep 13, 2012 at 07:57:42PM +0900, Fernando Luis Vázquez Cao wrote:
>>>> This patch set 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 following patches are included:
>>>>
>>>> ---
>>>> [1/9] vfs: add __iterate_supers() helper
>>>> [2/9] fsfreeze: add unlocked version of thaw_super
>>>>
>>>> Preparatory patches to fix s_umount lockup of emergency thaw code.
>>>>
>>>> [3/9] fsfreeze: Prevent emergency thaw from looping infinitely
>>>>
>>>> Fix thaw_bdev so that it propagates the error code properly to the caller.
>>>> This bug caused emergency thaw to loop infinitely. This is a forward port of
>>>> a previous patch by Dave Chinner.
>>>>
>>>> [4/9] fsfreeze: emergency thaw will deadlock on s_umount
>>>>
>>>> Avoid emergency thaw deadlock on s_umount by using unlocked version of
>>>> thaw_super() and __iterate_supers()i (introduced in patches 2 and 1
>>>> respectively).
>>> Given the problems with emergency thaw, this interface has never
>>> really worked. In the absence of any obvious need for the
>>> functionality (i.e. nobody has reported that it is broken since it
>>> was introduced), why don't we simply remove it?
>>>
>>> IIRC, the emergency thaw code was only added to alleviate
>>> fear-mongering about systems getting stuck with unfreezable ext4
>>> filesystems (after the "freeze w/ timeout" extensions were knocked
>>> back), and time has indicated those fears were unfounded.
>>>
>>> So, rather than trying to fix the emergency thaw mess, I say we
>>> nuke it from orbit....
>> As I commented to Eric, In virtualization environments it comes in
>> handy sometimes. For example, in an emergency case where a guest
>> agent dies leaving one or more filesystems frozen emergency thaw
>> is useful.
> Except it hasn't actually worked for 2 years, so it really probably
> hasn't been handy at all, in practice.

That is why we have been fixing it locally for our systems. :-)


>> Hopefully my fix is correct and we can keep this feature.
> The fix comes at a cost of quite a lot of complexity and rewriting, though.

Hopefully my fix is not too complex...


> We can always write more and more complex code, for weird administrative
> corner cases, but is it worth it?  I'm not quite convinced yet.

Another case where emergency thaw comes in handy is when
one freezes / by mistake which can lead to "funny" situations...
--
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: [RFC 0/9 v3] fsfreeze: miscellaneous fixes and cleanups

[Fernando Luis Vazquez Cao]

I will be sending v4 which fixes the issues pointed out by Eric and include
the right version of patches 4 and 6 (I apologize for the mistake).

- Fernando