fsfreeze: fix sb vs bdev freeze/thaw b0rkage

RFC PATCH In review Dave Chinner 3 participants 18 messages Jun 10, 2010 – Jun 21, 2010

Subsystems (4)

VFS Device mapper XFS filesystem GFS2 file system

lore

Share & tools

lore

Dave Chinner

author

2010-06-10 07:19 UTC #0 lore raw

cover fsfreeze: fix sb vs bdev freeze/thaw b0rkage

The following series is for to address bugs in the emergency thawing code, as well as mismatcheѕ with freezing at the block layer and the superblock that break the freeze/thaw nesting order.

The first 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.

The remainder of the patches address the bdev/sb mismatch and the fact that sb level freezing does not nest correctly. For all the places that the bdev interfaces are used, we need a superblock anyway so we may as well make freeze/thaw work only at the sb level. As such, this series moves all the nesting code to the sb from the bdev level and removes the freeze_bdev/thaw_bdev interfaces completely. It also converts the emergency thaw to work at the superblock level such that it will now thaw manually frozen filesystems.

A *big* outstanding problem still exists - freezing takes an active reference to the superblock, so unmounting an frozen filesystem has some nasty and unexpected side effects. The existing code results in an unmountable block device:

# mount /dev/vda /mnt/test
# xfs_freeze -f /mnt/test
# umount /mnt/test
# grep test /proc/mounts
# mkfs.xfs -f -l size=128m /dev/vda
mkfs.xfs: /dev/vda contains a mounted filesystem
Usage: mkfs.xfs
....
# mount /dev/vda /mnt/test
mount: /dev/vda already mounted or /mnt/test busy
#

At this point I can't get access to /dev/vda and needs a reboot to get it and /mnt/test back.

This patch series results in the block device being mountable, but remains frozen across unmount/mount:

# mount /dev/vda /mnt/test
# xfs_freeze -f /mnt/test
# umount /mnt/test
# grep test /proc/mounts
# mkfs.xfs -f -l size=128m /dev/vda
mkfs.xfs: /dev/vda contains a mounted filesystem
Usage: mkfs.xfs
....
# mount /dev/vda /mnt/test
# touch /mnt/test/foo &
[1] 2647
#
# xfs_freeze -u /mnt/test
[1]+  Done                    sudo touch /mnt/test/foo
# umount /mnt/test
# mkfs.xfs -f -l size=128m /dev/vda
meta-data=/dev/vda               isize=256    agcount=4, agsize=262144 blks
         =                       sectsz=512   attr=2
data     =                       bsize=4096   blocks=1048576, imaxpct=25
         =                       sunit=0      swidth=0 blks
naming   =version 2              bsize=4096   ascii-ci=0
log      =internal log           bsize=4096   blocks=32768, version=2
         =                       sectsz=512   sunit=0 blks, lazy-count=1
realtime =none                   extsz=4096   blocks=0, rtextents=0
#

This behaviour is only marginally better than the existing behaviour (at least you can release the references). However, I don't really like either option - we used to disallow umount on a frozen filesystems to avoid this problem.

So What is really supposed to happen when we unmount a frozen superblock? Should unmount return EBUSY? Should it be automatically thawed so it doesn't affect block device behaviour after unmount? Something else?

Cheers,

Dave.

Dave Chinner

author

2010-06-10 07:19 UTC #1 lore raw

PATCH 1/5 fsfreeze: Prevent emergency thaw from looping infinitely

From: Dave Chinner <[email protected]>

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.

Signed-off-by Dave Chinner <[email protected]>

---
 fs/block_dev.c |   13 +++++--------
 1 files changed, 5 insertions(+), 8 deletions(-)

fs/block_dev.c +5 -8

index 7346c96..366ac38 100644
@@ -276,22 +276,19 @@ int thaw_bdev(struct block_device *bdev, struct super_block *sb)
 	if (!bdev->bd_fsfreeze_count)
 		goto out;
 
-	error = 0;
-	if (--bdev->bd_fsfreeze_count > 0)
+	if (!sb)
 		goto out;
 
-	if (!sb)
+	error = 0;
+	if (--bdev->bd_fsfreeze_count > 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);

Signature · 1 line

1.7.1

Christoph Hellwig

2010-06-14 15:18 UTC #2 lore raw

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

Quoted 2 lines · Dave Chinner

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

This includes some additional changes in addition to the description, and at least one of them seems incorrect.

fs/block_dev.c int thaw_bdev(struct block_device *bdev, struct super_block *sb) · line 284

Quoted 9 lines

fs/block_dev.c +3 -3

-	error = 0;
-	if (--bdev->bd_fsfreeze_count > 0)
+	if (!sb)
 		goto out;
 
-	if (!sb)
+	error = 0;
+	if (--bdev->bd_fsfreeze_count > 0)
 		goto out;

Here you reorder the sb check to be before the counter decrement. But we do support calling freeze_bdev on a device without a superblock, and you would leak bd_fsfreeze_count for that case and wrongly return -EINVAL on unthaw for these now.

fs/block_dev.c int thaw_bdev(struct block_device *bdev, struct super_block *sb) · line 291

Quoted 7 lines

fs/block_dev.c +1 -4

 	error = thaw_super(sb);
-	if (error) {
+	if (error)
 		bdev->bd_fsfreeze_count++;
-		mutex_unlock(&bdev->bd_fsfreeze_mutex);
-		return error;
-	}

Ok, useful cleanup.

fs/block_dev.c int thaw_bdev(struct block_device *bdev, struct super_block *sb) · line 291

Quoted 4 lines

out:

fs/block_dev.c +1 -1

 	mutex_unlock(&bdev->bd_fsfreeze_mutex);
-	return 0;
+	return error;

And this is the actual fix of course, also looks good.

Dave Chinner

author

2010-06-14 23:20 UTC #3 lore raw

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

Quoted 13 lines · Christoph Hellwig

Quoted 2 lines · Dave Chinner
Return -EINVAL when the filesystem is already unfrozen to avoid this problem.
This includes some additional changes in addition to the description, and at least one of them seems incorrect.
Quoted 5 lines
fs/block_dev.c +1 -2
-	error = 0;
-	if (--bdev->bd_fsfreeze_count > 0)
+	if (!sb)
 		goto out;

Quoted 9 lines

fs/block_dev.c int thaw_bdev(struct block_device *bdev, struct super_block *sb) · line 284
Quoted 4 lines
fs/block_dev.c +2 -1
-	if (!sb)
+	error = 0;
+	if (--bdev->bd_fsfreeze_count > 0)
 		goto out;
Here you reorder the sb check to be before the counter decrement. But we do support calling freeze_bdev on a device without a superblock, and you would leak bd_fsfreeze_count for that case and wrongly return -EINVAL on unthaw for these now.

Ok, will fix it.

Cheers,

Dave.

Signature · 2 lines

Dave Chinner
[email protected]

Dave Chinner

author

2010-06-10 07:19 UTC #4 lore raw

PATCH 2/5 fsfreeze: emergency thaw will deadlock on s_umount

In reply to Dave Chinner

From: Dave Chinner <[email protected]>

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.

Signed-off-by Dave Chinner <[email protected]>

---
 fs/block_dev.c     |   26 ++++++++++++++++++++--
 fs/buffer.c        |    2 +-
 fs/super.c         |   58 +++++++++++++++++++++++++++++++++++++++++++---------
 include/linux/fs.h |    9 ++++++++
 4 files changed, 81 insertions(+), 14 deletions(-)

fs/block_dev.c +23 -3

index 366ac38..a8c8224 100644
@@ -262,13 +262,14 @@ struct super_block *freeze_bdev(struct block_device *bdev)
 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;

@@ -283,15 +284,34 @@ int thaw_bdev(struct block_device *bdev, struct super_block *sb)
 	if (--bdev->bd_fsfreeze_count > 0)
 		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);

fs/buffer.c +1 -1

index d54812b..f0c55d9 100644
@@ -564,7 +564,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));
 }

fs/super.c +48 ~~-10~~

87 lines not on this page · raw message

include/linux/fs.h +9 -0

index 471e1ff..e246389 100644
@@ -1803,6 +1803,7 @@ extern int iterate_mounts(int (*)(struct vfsmount *, void *), void *,
 extern int vfs_statfs(struct dentry *, 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 int current_umask(void);

@@ -1953,6 +1954,8 @@ extern int sync_blockdev(struct block_device *bdev);
 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) {}

@@ -1968,6 +1971,12 @@ static inline int thaw_bdev(struct block_device *bdev, struct super_block *sb)
 {
 	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;

Signature · 1 line

1.7.1

Christoph Hellwig

2010-06-14 15:20 UTC #5 lore raw

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

Quoted 11 lines · Dave Chinner

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.

This is correct as long as no one calls thaw_super directly, which is not the case currently.

Dave Chinner

author

2010-06-14 23:21 UTC #6 lore raw

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

Quoted 13 lines · Christoph Hellwig

Quoted 11 lines · Dave Chinner
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.

Quoted 2 lines

This is correct as long as no one calls thaw_super directly, which is not the case currently.

Yeah, the idea of the first two patches is that they can be applied to a current tree and backported and prevent the infinite loop or deadlock. The problem of thaw_bdev/thaw_super is what the rest of the patches are supposed to address.

Cheers,

Dave.

Signature · 2 lines

Dave Chinner
[email protected]

Dave Chinner

author

2010-06-21 01:58 UTC #7 lore raw

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

In reply to Christoph Hellwig

Quoted 13 lines · Christoph Hellwig

Quoted 11 lines · Dave Chinner
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.

Quoted 2 lines

This is correct as long as no one calls thaw_super directly, which is not the case currently.

This patch doesn't try to deal with the bdev/super mismatches; all it does is prevent an obvious deadlock. Calling freeze/thaw_super directly will serialise on the s_umount lock, calling freeze/thaw_bdev() will serialise on the bdev freeze mutex, and if we mix the two they'll serialise on the s_umount lock. So I think with this patch serialisation will still occur correctly but avoid the current deadlock.

I'll change the commit message to explain this better.

Cheers,

Dave.

Signature · 2 lines

Dave Chinner
[email protected]

Christoph Hellwig

2010-06-21 07:47 UTC #8 lore raw

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

Quoted 9 lines · Dave Chinner

This patch doesn't try to deal with the bdev/super mismatches; all it does is prevent an obvious deadlock. Calling freeze/thaw_super directly will serialise on the s_umount lock, calling freeze/thaw_bdev() will serialise on the bdev freeze mutex, and if we mix the two they'll serialise on the s_umount lock. So I think with this patch serialisation will still occur correctly but avoid the current deadlock.
I'll change the commit message to explain this better.

I don;t think the explanation alone is enough.

Right now thaw_super itself is only serialized by exclusive shared
s_umount. thaw_bdev it also serialized by bd_fsfreeze_mutex, but
there are callers of thaw_super that do not go through thaw_bdev, so our locking is not enough here.

Dave Chinner

author

2010-06-10 07:19 UTC #9 lore raw

PATCH 3/5 fsfreeze: freeze_super and thaw_bdev don't play well together

In reply to Dave Chinner

From: Dave Chinner <[email protected]>

thaw_bdev() has re-enterency 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.

Signed-off-by Dave Chinner <[email protected]>

---
 fs/block_dev.c       |   59 ++++----------------------------------
 fs/gfs2/ops_fstype.c |   12 --------
 fs/super.c           |   77 ++++++++++++++++++++++++++++++-------------------
 include/linux/fs.h   |    8 ++---
 4 files changed, 56 insertions(+), 100 deletions(-)

fs/block_dev.c +6 ~~-53~~

index a8c8224..84899b3 100644
@@ -230,71 +230,22 @@ struct super_block *freeze_bdev(struct block_device *bdev)
 	struct super_block *sb;
 	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;
-	}
-
 	sb = get_active_super(bdev);
 	if (!sb)
 		goto out;
 	error = freeze_super(sb);
 	if (error) {
 		deactivate_super(sb);
-		bdev->bd_fsfreeze_count--;
-		mutex_unlock(&bdev->bd_fsfreeze_mutex);
 		return ERR_PTR(error);
 	}
 	deactivate_super(sb);
  out:
 	sync_blockdev(bdev);
-	mutex_unlock(&bdev->bd_fsfreeze_mutex);
 	return sb;	/* thaw_bdev releases s->s_umount */
 }
 EXPORT_SYMBOL(freeze_bdev);
 
 /**
- * __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().
- */
-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 (!sb)
-		goto out;
-
-	error = 0;
-	if (--bdev->bd_fsfreeze_count > 0)
-		goto out;
-
-	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

@@ -303,13 +254,17 @@ out:
  */
 int thaw_bdev(struct block_device *bdev, struct super_block *sb)
 {
-	return __thaw_bdev(bdev, sb, 0);
+	if (!sb)
+		return -EINVAL;
+	return thaw_super(sb);
 }
 EXPORT_SYMBOL(thaw_bdev);
 
 int thaw_bdev_emergency(struct block_device *bdev, struct super_block *sb)
 {
-	return __thaw_bdev(bdev, sb, 1);
+	if (!sb)
+		return -EINVAL;
+	return thaw_super_emergency(sb);
 }
 
 static int blkdev_writepage(struct page *page, struct writeback_control *wbc)

@@ -434,8 +389,6 @@ static void init_once(void *foo)
 	INIT_LIST_HEAD(&bdev->bd_holder_list);
 #endif
 	inode_init_once(&ei->vfs_inode);
-	/* Initialize mutex for freeze. */
-	mutex_init(&bdev->bd_fsfreeze_mutex);
 }
 
 static inline void __bd_forget(struct inode *inode)

fs/gfs2/ops_fstype.c +0 ~~-12~~

index 3593b3a..5acc907 100644
@@ -1304,19 +1304,7 @@ static int gfs2_get_sb(struct file_system_type *fs_type, int flags,
 	if (IS_ERR(bdev))
 		return PTR_ERR(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);
-		error = -EBUSY;
-		goto error_bdev;
-	}
 	s = sget(fs_type, test_gfs2_super, set_gfs2_super, bdev);
-	mutex_unlock(&bdev->bd_fsfreeze_mutex);
 	error = PTR_ERR(s);
 	if (IS_ERR(s))
 		goto error_bdev;

fs/super.c +47 ~~-30~~

147 lines not on this page · raw message

include/linux/fs.h +3 -5

index e246389..f92b077 100644
@@ -672,11 +672,6 @@ struct block_device {
 	 * care to not mess up bd_private for that case.
 	 */
 	unsigned long		bd_private;
-
-	/* The counter of freeze processes */
-	int			bd_fsfreeze_count;
-	/* Mutex for freeze */
-	struct mutex		bd_fsfreeze_mutex;
 };
 
 /*

@@ -1382,6 +1377,9 @@ struct super_block {
 	 * generic_show_options()
 	 */
 	char *s_options;
+
+	int			s_freeze_count;	/* nr of nested freezes */
+	struct mutex		s_freeze_mutex;	/* nesting lock */
 };
 
 extern struct timespec current_fs_time(struct super_block *sb);

Signature · 1 line

1.7.1

Christoph Hellwig

2010-06-14 15:22 UTC #10 lore raw

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

This breaks the "feature" that we can freeze a block device that doesn't have a filesystem mounted yet. For filesystems using get_sb_bdev that prevents a new filesystem to be mounted on them.

I'm not sure it's a particularly useful feature, but it's been there since day 1 of the freeze support. The easiest way to not break it would be to keep the per-sb freeze count only for that case and only check it during mount.

Dave Chinner

author

2010-06-15 00:01 UTC #11 lore raw

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

Quoted 8 lines · Christoph Hellwig

This breaks the "feature" that we can freeze a block device that doesn't have a filesystem mounted yet. For filesystems using get_sb_bdev that prevents a new filesystem to be mounted on them.
I'm not sure it's a particularly useful feature, but it's been there since day 1 of the freeze support. The easiest way to not break it would be to keep the per-sb freeze count only for that case and only check it during mount.

You mean the per-bdev freeze count, right? So freeze/thaw_bdev would have to remain?

Cheers,

Dave.

Signature · 2 lines

Dave Chinner
[email protected]

Christoph Hellwig

2010-06-15 06:24 UTC #12 lore raw

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

Quoted 5 lines · Dave Chinner

Quoted 4 lines · Christoph Hellwig
This breaks the "feature" that we can freeze a block device that doesn't have a filesystem mounted yet. For filesystems using get_sb_bdev that prevents a new filesystem to be mounted on them.

Quoted 7 lines

Quoted 4 lines
I'm not sure it's a particularly useful feature, but it's been there since day 1 of the freeze support. The easiest way to not break it would be to keep the per-sb freeze count only for that case and only check it during mount.
You mean the per-bdev freeze count, right? So freeze/thaw_bdev would have to remain?

Yes.

Dave Chinner

author

2010-06-10 07:20 UTC #13 lore raw

PATCH 4/5 fsfreeze: switch to using super methods everywhere

In reply to Dave Chinner

From: Dave Chinner <[email protected]>

freeze/thaw_bdev are now just trivial wrappers around freeze/thaw_super(). Convert all users of the bdev interfaces to use the superblock interfaces instead, and remove the bdev interfaces.

Signed-off-by Dave Chinner <[email protected]>

---
 drivers/md/dm.c    |   13 +++++++----
 fs/block_dev.c     |   54 ----------------------------------------------------
 fs/buffer.c        |    2 +-
 fs/super.c         |    1 +
 fs/xfs/xfs_fsops.c |   10 ++------
 include/linux/fs.h |   19 ------------------
 6 files changed, 13 insertions(+), 86 deletions(-)

drivers/md/dm.c +8 -5

index d21e128..70d5fd6 100644
@@ -2427,15 +2427,18 @@ static int lock_fs(struct mapped_device *md)
 
 	WARN_ON(md->frozen_sb);
 
-	md->frozen_sb = freeze_bdev(md->bdev);
-	if (IS_ERR(md->frozen_sb)) {
-		r = PTR_ERR(md->frozen_sb);
+	md->frozen_sb = get_active_super(md->bdev);
+	if (!md->frozen_sb)
+		return -EIO;
+	r = freeze_super(md->frozen_sb);
+	if (r) {
+		deactivate_super(md->frozen_sb);
 		md->frozen_sb = NULL;
 		return r;
 	}
+	deactivate_super(md->frozen_sb);
 
 	set_bit(DMF_FROZEN, &md->flags);
-
 	return 0;
 }

@@ -2444,7 +2447,7 @@ static void unlock_fs(struct mapped_device *md)
 	if (!test_bit(DMF_FROZEN, &md->flags))
 		return;
 
-	thaw_bdev(md->bdev, md->frozen_sb);
+	thaw_super(md->frozen_sb);
 	md->frozen_sb = NULL;
 	clear_bit(DMF_FROZEN, &md->flags);
 }

fs/block_dev.c +0 ~~-54~~

60 lines not on this page · raw message

fs/buffer.c +1 -1

index f0c55d9..b095fc1 100644
@@ -564,7 +564,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));
 }

fs/super.c +1 -0

index 5f13431..81a4034 100644
@@ -976,6 +976,7 @@ int freeze_super(struct super_block *sb)
 		}
 	}
 out_active:
+	sync_blockdev(sb->s_bdev);
 	up_write(&sb->s_umount);
 out_unlock:
 	mutex_unlock(&sb->s_freeze_mutex);

fs/xfs/xfs_fsops.c +3 -7

index 37a6f62..cc993a5 100644
@@ -642,16 +642,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;

include/linux/fs.h +0 ~~-19~~

index f92b077..39bf4ac 100644
@@ -1951,30 +1951,11 @@ extern void invalidate_bdev(struct block_device *);
 extern int sync_blockdev(struct block_device *bdev);
 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) {}
 static inline int sync_blockdev(struct block_device *bdev) { return 0; }
 static inline void invalidate_bdev(struct block_device *bdev) {}
-
-static inline struct super_block *freeze_bdev(struct block_device *sb)
-{
-	return NULL;
-}
-
-static inline int thaw_bdev(struct block_device *bdev, struct super_block *sb)
-{
-	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;

Signature · 1 line

1.7.1

Christoph Hellwig

2010-06-14 15:23 UTC #14 lore raw

Re: [PATCH 4/5] fsfreeze: switch to using super methods everywhere

Quoted 5 lines · Dave Chinner

From: Dave Chinner <[email protected]>
freeze/thaw_bdev are now just trivial wrappers around freeze/thaw_super(). Convert all users of the bdev interfaces to use the superblock interfaces instead, and remove the bdev interfaces.

This requires exporting get_active_super. Buit if we want to keep suppoting freezing of unmounted block devices we need to keep the helpers anyway.

Dave Chinner

author

2010-06-10 07:20 UTC #15 lore raw

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

In reply to Dave Chinner

From: Dave Chinner <[email protected]>

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.

Signed-off-by Dave Chinner <[email protected]>

---
 fs/buffer.c        |   31 -------------------------------
 fs/super.c         |   37 +++++++++++++++++++++++++++++++++----
 include/linux/fs.h |    1 -
 3 files changed, 33 insertions(+), 36 deletions(-)

fs/buffer.c +0 ~~-31~~

index b095fc1..61bd994 100644
@@ -561,37 +561,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

fs/super.c +33 -4

index 81a4034..680b8d5 100644
@@ -1059,7 +1059,7 @@ out_unlock:
 }
 
 /**
- * thaw_super -- unlock filesystem
+ * __thaw_super -- unlock filesystem
  * @sb: the super to thaw
  *
  * Unlocks the filesystem and marks it writeable again after freeze_super().

@@ -1075,11 +1075,40 @@ 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.
+ */
+static void thaw_super_emergency(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(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
  */
-int thaw_super_emergency(struct super_block *sb)
+void emergency_thaw_all(void)
 {
-	return __thaw_super(sb, 1);
+	struct work_struct *work;
+
+	work = kmalloc(sizeof(*work), GFP_ATOMIC);
+	if (work) {
+		INIT_WORK(work, do_thaw_all);
+		schedule_work(work);
+	}
 }
 
 static struct vfsmount *fs_set_subtype(struct vfsmount *mnt, const char *fstype)

include/linux/fs.h +0 -1

index 39bf4ac..a704062 100644
@@ -1801,7 +1801,6 @@ extern int iterate_mounts(int (*)(struct vfsmount *, void *), void *,
 extern int vfs_statfs(struct dentry *, 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 int current_umask(void);

Signature · 1 line

1.7.1

Christoph Hellwig

2010-06-14 15:25 UTC #16 lore raw

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

Quoted 6 lines · Dave Chinner

From: Dave Chinner <[email protected]>
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.

fs/super.c EXPORT_SYMBOL(thaw_super); · line 1088

Quoted 9 lines

fs/super.c +9 -0

+static void thaw_super_emergency(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(sb, 1));

Please move the ; to a separate line to make the loop better redable.

Josef Bacik

2010-06-10 12:38 UTC #17 lore raw

In reply to Dave Chinner

Quoted 71 lines · Dave Chinner

The following series is for to address bugs in the emergency thawing code, as well as mismatcheѕ with freezing at the block layer and the superblock that break the freeze/thaw nesting order.
The first 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.
The remainder of the patches address the bdev/sb mismatch and the fact that sb level freezing does not nest correctly. For all the places that the bdev interfaces are used, we need a superblock anyway so we may as well make freeze/thaw work only at the sb level. As such, this series moves all the nesting code to the sb from the bdev level and removes the freeze_bdev/thaw_bdev interfaces completely. It also converts the emergency thaw to work at the superblock level such that it will now thaw manually frozen filesystems.
A *big* outstanding problem still exists - freezing takes an active reference to the superblock, so unmounting an frozen filesystem has some nasty and unexpected side effects. The existing code results in an unmountable block device:
# mount /dev/vda /mnt/test
# xfs_freeze -f /mnt/test
# umount /mnt/test
# grep test /proc/mounts
# mkfs.xfs -f -l size=128m /dev/vda
mkfs.xfs: /dev/vda contains a mounted filesystem
Usage: mkfs.xfs
....
# mount /dev/vda /mnt/test
mount: /dev/vda already mounted or /mnt/test busy
#
At this point I can't get access to /dev/vda and needs a reboot to get it and /mnt/test back.
This patch series results in the block device being mountable, but remains frozen across unmount/mount:
# mount /dev/vda /mnt/test
# xfs_freeze -f /mnt/test
# umount /mnt/test
# grep test /proc/mounts
# mkfs.xfs -f -l size=128m /dev/vda
mkfs.xfs: /dev/vda contains a mounted filesystem
Usage: mkfs.xfs
....
# mount /dev/vda /mnt/test
# touch /mnt/test/foo &
[1] 2647
#
# xfs_freeze -u /mnt/test
[1]+  Done                    sudo touch /mnt/test/foo
# umount /mnt/test
# mkfs.xfs -f -l size=128m /dev/vda
meta-data=/dev/vda               isize=256    agcount=4, agsize=262144 blks
         =                       sectsz=512   attr=2
data     =                       bsize=4096   blocks=1048576, imaxpct=25
         =                       sunit=0      swidth=0 blks
naming   =version 2              bsize=4096   ascii-ci=0
log      =internal log           bsize=4096   blocks=32768, version=2
         =                       sectsz=512   sunit=0 blks, lazy-count=1
realtime =none                   extsz=4096   blocks=0, rtextents=0
#
This behaviour is only marginally better than the existing behaviour (at least you can release the references). However, I don't really like either option - we used to disallow umount on a frozen filesystems to avoid this problem.

Quoted 5 lines

So What is really supposed to happen when we unmount a frozen superblock? Should unmount return EBUSY? Should it be automatically thawed so it doesn't affect block device behaviour after unmount? Something else?

My vote is for EBUSY, that way we don't get this weird unexpected behavior thing. Plus if dm is doing a snapshot or whatever, unfreezing the fs to let it unmount in the middle of it doing its thing could be unfun. Thanks,

Josef
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html