[PATCH] Introduce freeze_super and thaw_super for the fsfreeze ioctl

[PATCH] Introduce freeze_super and thaw_super for the fsfreeze ioctl

[Josef Bacik]

Currently the way we do freezing is by passing sb>s_bdev to freeze_bdev and then
letting it do all the work.  But freezing is more of an fs thing, and doesn't
really have much to do with the bdev at all, all the work gets done with the
super.  In btrfs we do not populate s_bdev, since we can have multiple bdev's
for one fs and setting s_bdev makes removing devices from a pool kind of tricky.
This means that freezing a btrfs filesystem fails, which causes us to corrupt
with things like tux-on-ice which use the fsfreeze mechanism.  So instead of
populating sb->s_bdev with a random bdev in our pool, I've broken the actual fs
freezing stuff into freeze_super and thaw_super.  These just take the
super_block that we're freezing and does the appropriate work.  It's basically
just copy and pasted from freeze_bdev.  I've then converted freeze_bdev over to
use the new super helpers.  I've tested this with ext4 and btrfs and verified
everything continues to work the same as before.

The only new gotcha is multiple calls to the fsfreeze ioctl will return EBUSY if
the fs is already frozen.  I thought this was a better solution than adding a
freeze counter to the super_block, but if everybody hates this idea I'm open to
suggestions.  Thanks,

Signed-off-by: Josef Bacik <josef@redhat.com>
---
 fs/block_dev.c     |   66 ++++++++------------------------------
 fs/ioctl.c         |   18 +++--------
 fs/super.c         |   88 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/fs.h |    2 +
 4 files changed, 109 insertions(+), 65 deletions(-)

diff --git a/fs/block_dev.c b/fs/block_dev.c
index 8bed055..71b6165 100644
--- a/fs/block_dev.c
+++ b/fs/block_dev.c
@@ -245,35 +245,13 @@ struct super_block *freeze_bdev(struct block_device *bdev)
 	sb = get_active_super(bdev);
 	if (!sb)
 		goto out;
-	if (sb->s_flags & MS_RDONLY) {
-		deactivate_locked_super(sb);
-		mutex_unlock(&bdev->bd_fsfreeze_mutex);
-		return sb;
-	}
-
-	sb->s_frozen = SB_FREEZE_WRITE;
-	smp_wmb();
-
-	sync_filesystem(sb);
 
-	sb->s_frozen = SB_FREEZE_TRANS;
-	smp_wmb();
-
-	sync_blockdev(sb->s_bdev);
-
-	if (sb->s_op->freeze_fs) {
-		error = sb->s_op->freeze_fs(sb);
-		if (error) {
-			printk(KERN_ERR
-				"VFS:Filesystem freeze failed\n");
-			sb->s_frozen = SB_UNFROZEN;
-			deactivate_locked_super(sb);
-			bdev->bd_fsfreeze_count--;
-			mutex_unlock(&bdev->bd_fsfreeze_mutex);
-			return ERR_PTR(error);
-		}
+	error = freeze_super(sb, 1);
+	if (error) {
+		bdev->bd_fsfreeze_count--;
+		mutex_unlock(&bdev->bd_fsfreeze_mutex);
+		return ERR_PTR(error);
 	}
-	up_write(&sb->s_umount);
 
  out:
 	sync_blockdev(bdev);
@@ -295,40 +273,24 @@ int thaw_bdev(struct block_device *bdev, struct super_block *sb)
 
 	mutex_lock(&bdev->bd_fsfreeze_mutex);
 	if (!bdev->bd_fsfreeze_count)
-		goto out_unlock;
+		goto out;
 
 	error = 0;
 	if (--bdev->bd_fsfreeze_count > 0)
-		goto out_unlock;
+		goto out;
 
 	if (!sb)
-		goto out_unlock;
+		goto out;
 
 	BUG_ON(sb->s_bdev != bdev);
-	down_write(&sb->s_umount);
-	if (sb->s_flags & MS_RDONLY)
-		goto out_deactivate;
-
-	if (sb->s_op->unfreeze_fs) {
-		error = sb->s_op->unfreeze_fs(sb);
-		if (error) {
-			printk(KERN_ERR
-				"VFS:Filesystem thaw failed\n");
-			sb->s_frozen = SB_FREEZE_TRANS;
-			bdev->bd_fsfreeze_count++;
-			mutex_unlock(&bdev->bd_fsfreeze_mutex);
-			return error;
-		}
+	error = thaw_super(sb);
+	if (error) {
+		bdev->bd_fsfreeze_count++;
+		mutex_unlock(&bdev->bd_fsfreeze_mutex);
+		return error;
 	}
 
-	sb->s_frozen = SB_UNFROZEN;
-	smp_wmb();
-	wake_up(&sb->s_wait_unfrozen);
-
-out_deactivate:
-	if (sb)
-		deactivate_locked_super(sb);
-out_unlock:
+out:
 	mutex_unlock(&bdev->bd_fsfreeze_mutex);
 	return 0;
 }
diff --git a/fs/ioctl.c b/fs/ioctl.c
index 6c75110..a065eff 100644
--- a/fs/ioctl.c
+++ b/fs/ioctl.c
@@ -503,6 +503,7 @@ static int ioctl_fioasync(unsigned int fd, struct file *filp,
 static int ioctl_fsfreeze(struct file *filp)
 {
 	struct super_block *sb = filp->f_path.dentry->d_inode->i_sb;
+	int ret;
 
 	if (!capable(CAP_SYS_ADMIN))
 		return -EPERM;
@@ -511,15 +512,10 @@ static int ioctl_fsfreeze(struct file *filp)
 	if (sb->s_op->freeze_fs == NULL)
 		return -EOPNOTSUPP;
 
-	/* If a blockdevice-backed filesystem isn't specified, return. */
-	if (sb->s_bdev == NULL)
-		return -EINVAL;
-
 	/* Freeze */
-	sb = freeze_bdev(sb->s_bdev);
-	if (IS_ERR(sb))
-		return PTR_ERR(sb);
-	return 0;
+	ret = freeze_super(sb, 0);
+
+	return ret;
 }
 
 static int ioctl_fsthaw(struct file *filp)
@@ -529,12 +525,8 @@ static int ioctl_fsthaw(struct file *filp)
 	if (!capable(CAP_SYS_ADMIN))
 		return -EPERM;
 
-	/* If a blockdevice-backed filesystem isn't specified, return EINVAL. */
-	if (sb->s_bdev == NULL)
-		return -EINVAL;
-
 	/* Thaw */
-	return thaw_bdev(sb->s_bdev, sb);
+	return thaw_super(sb);
 }
 
 /*
diff --git a/fs/super.c b/fs/super.c
index 19eb70b..305d475 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -971,6 +971,94 @@ out:
 
 EXPORT_SYMBOL_GPL(vfs_kern_mount);
 
+/**
+ * freeze_super -- lock the filesystem and force it into a consistent state
+ * @super: the super to lock
+ *
+ * Syncs the super to make sure the filesystem is consistent and calls the fs's
+ * freeze_fs.  We hold the s_umount semaphore in order to make sure the fs is
+ * not unmounted until after we thaw the fs.  This cannot be called multiple
+ * times like freeze_bdev, if we're already frozen we'll return -EBUSY.
+ */
+int freeze_super(struct super_block *sb, int locked)
+{
+	int ret;
+
+	if (!locked) {
+		spin_lock(&sb_lock);
+		ret = grab_super(sb);
+		if (!ret)
+			return 0;
+	}
+
+	if (sb->s_flags & MS_RDONLY) {
+		deactivate_locked_super(sb);
+		return 0;
+	}
+
+	if (sb->s_frozen) {
+		deactivate_locked_super(sb);
+		return -EBUSY;
+	}
+
+	sb->s_frozen = SB_FREEZE_WRITE;
+	smp_wmb();
+
+	sync_filesystem(sb);
+
+	sb->s_frozen = SB_FREEZE_TRANS;
+	smp_wmb();
+
+	sync_blockdev(sb->s_bdev);
+	if (sb->s_op->freeze_fs) {
+		ret = sb->s_op->freeze_fs(sb);
+		if (ret) {
+			printk(KERN_ERR
+				"VFS:Filesystem freeze failed\n");
+			sb->s_frozen = SB_UNFROZEN;
+			deactivate_locked_super(sb);
+			return ret;
+		}
+	}
+	up_write(&sb->s_umount);
+	return 0;
+}
+EXPORT_SYMBOL(freeze_super);
+
+/**
+ * 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 error;
+
+	down_write(&sb->s_umount);
+	if (sb->s_flags & MS_RDONLY)
+		goto out;
+
+	if (sb->s_op->unfreeze_fs) {
+		error = sb->s_op->unfreeze_fs(sb);
+		if (error) {
+			printk(KERN_ERR
+				"VFS:Filesystem thaw failed\n");
+			sb->s_frozen = SB_FREEZE_TRANS;
+			return error;
+		}
+	}
+
+	sb->s_frozen = SB_UNFROZEN;
+	smp_wmb();
+	wake_up(&sb->s_wait_unfrozen);
+
+out:
+	deactivate_locked_super(sb);
+	return 0;
+}
+EXPORT_SYMBOL(thaw_super);
+
 static struct vfsmount *fs_set_subtype(struct vfsmount *mnt, const char *fstype)
 {
 	int err;
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 2620a8c..a5778ae 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1802,6 +1802,8 @@ extern int may_umount(struct vfsmount *);
 extern long do_mount(char *, char *, char *, unsigned long, void *);
 extern struct vfsmount *collect_mounts(struct path *);
 extern void drop_collected_mounts(struct vfsmount *);
+extern int freeze_super(struct super_block *super, int locked);
+extern int thaw_super(struct super_block *super);
 
 extern int vfs_statfs(struct dentry *, struct kstatfs *);
 
-- 
1.6.6.1

Re: [PATCH] Introduce freeze_super and thaw_super for the fsfreeze ioctl

[Al Viro]

On Tue, Mar 23, 2010 at 10:22:00AM -0400, Josef Bacik wrote:
> Currently the way we do freezing is by passing sb>s_bdev to freeze_bdev and then
> letting it do all the work.  But freezing is more of an fs thing, and doesn't
> really have much to do with the bdev at all, all the work gets done with the
> super.  In btrfs we do not populate s_bdev, since we can have multiple bdev's
> for one fs and setting s_bdev makes removing devices from a pool kind of tricky.
> This means that freezing a btrfs filesystem fails, which causes us to corrupt
> with things like tux-on-ice which use the fsfreeze mechanism.  So instead of
> populating sb->s_bdev with a random bdev in our pool, I've broken the actual fs
> freezing stuff into freeze_super and thaw_super.  These just take the
> super_block that we're freezing and does the appropriate work.  It's basically
> just copy and pasted from freeze_bdev.  I've then converted freeze_bdev over to
> use the new super helpers.  I've tested this with ext4 and btrfs and verified
> everything continues to work the same as before.
> 
> The only new gotcha is multiple calls to the fsfreeze ioctl will return EBUSY if
> the fs is already frozen.  I thought this was a better solution than adding a
> freeze counter to the super_block, but if everybody hates this idea I'm open to
> suggestions.  Thanks,

Locking is all wrong there.  We don't need to worry about umount; we *already*
have an active reference.  And leaving a kernel object with semaphore held
when ioctl returns is completely wrong.

Re: [PATCH] Introduce freeze_super and thaw_super for the fsfreeze ioctl

[Josef Bacik]

On Tue, Mar 23, 2010 at 02:28:44PM +0000, Al Viro wrote:
> On Tue, Mar 23, 2010 at 10:22:00AM -0400, Josef Bacik wrote:
> > Currently the way we do freezing is by passing sb>s_bdev to freeze_bdev and then
> > letting it do all the work.  But freezing is more of an fs thing, and doesn't
> > really have much to do with the bdev at all, all the work gets done with the
> > super.  In btrfs we do not populate s_bdev, since we can have multiple bdev's
> > for one fs and setting s_bdev makes removing devices from a pool kind of tricky.
> > This means that freezing a btrfs filesystem fails, which causes us to corrupt
> > with things like tux-on-ice which use the fsfreeze mechanism.  So instead of
> > populating sb->s_bdev with a random bdev in our pool, I've broken the actual fs
> > freezing stuff into freeze_super and thaw_super.  These just take the
> > super_block that we're freezing and does the appropriate work.  It's basically
> > just copy and pasted from freeze_bdev.  I've then converted freeze_bdev over to
> > use the new super helpers.  I've tested this with ext4 and btrfs and verified
> > everything continues to work the same as before.
> > 
> > The only new gotcha is multiple calls to the fsfreeze ioctl will return EBUSY if
> > the fs is already frozen.  I thought this was a better solution than adding a
> > freeze counter to the super_block, but if everybody hates this idea I'm open to
> > suggestions.  Thanks,
> 
> Locking is all wrong there.  We don't need to worry about umount; we *already*
> have an active reference.  And leaving a kernel object with semaphore held
> when ioctl returns is completely wrong.

Hmm sorry my comment is incorrect, we don't actually hold the semaphore until
the thaw.  Here is the version with the fixed up comment, sorry about that.

Signed-off-by: Josef Bacik <josef@redhat.com>
---
 fs/block_dev.c     |   66 ++++++++-------------------------------
 fs/ioctl.c         |   18 +++--------
 fs/super.c         |   87 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/fs.h |    2 +
 4 files changed, 108 insertions(+), 65 deletions(-)

diff --git a/fs/block_dev.c b/fs/block_dev.c
index 8bed055..71b6165 100644
--- a/fs/block_dev.c
+++ b/fs/block_dev.c
@@ -245,35 +245,13 @@ struct super_block *freeze_bdev(struct block_device *bdev)
 	sb = get_active_super(bdev);
 	if (!sb)
 		goto out;
-	if (sb->s_flags & MS_RDONLY) {
-		deactivate_locked_super(sb);
-		mutex_unlock(&bdev->bd_fsfreeze_mutex);
-		return sb;
-	}
-
-	sb->s_frozen = SB_FREEZE_WRITE;
-	smp_wmb();
-
-	sync_filesystem(sb);
 
-	sb->s_frozen = SB_FREEZE_TRANS;
-	smp_wmb();
-
-	sync_blockdev(sb->s_bdev);
-
-	if (sb->s_op->freeze_fs) {
-		error = sb->s_op->freeze_fs(sb);
-		if (error) {
-			printk(KERN_ERR
-				"VFS:Filesystem freeze failed\n");
-			sb->s_frozen = SB_UNFROZEN;
-			deactivate_locked_super(sb);
-			bdev->bd_fsfreeze_count--;
-			mutex_unlock(&bdev->bd_fsfreeze_mutex);
-			return ERR_PTR(error);
-		}
+	error = freeze_super(sb, 1);
+	if (error) {
+		bdev->bd_fsfreeze_count--;
+		mutex_unlock(&bdev->bd_fsfreeze_mutex);
+		return ERR_PTR(error);
 	}
-	up_write(&sb->s_umount);
 
  out:
 	sync_blockdev(bdev);
@@ -295,40 +273,24 @@ int thaw_bdev(struct block_device *bdev, struct super_block *sb)
 
 	mutex_lock(&bdev->bd_fsfreeze_mutex);
 	if (!bdev->bd_fsfreeze_count)
-		goto out_unlock;
+		goto out;
 
 	error = 0;
 	if (--bdev->bd_fsfreeze_count > 0)
-		goto out_unlock;
+		goto out;
 
 	if (!sb)
-		goto out_unlock;
+		goto out;
 
 	BUG_ON(sb->s_bdev != bdev);
-	down_write(&sb->s_umount);
-	if (sb->s_flags & MS_RDONLY)
-		goto out_deactivate;
-
-	if (sb->s_op->unfreeze_fs) {
-		error = sb->s_op->unfreeze_fs(sb);
-		if (error) {
-			printk(KERN_ERR
-				"VFS:Filesystem thaw failed\n");
-			sb->s_frozen = SB_FREEZE_TRANS;
-			bdev->bd_fsfreeze_count++;
-			mutex_unlock(&bdev->bd_fsfreeze_mutex);
-			return error;
-		}
+	error = thaw_super(sb);
+	if (error) {
+		bdev->bd_fsfreeze_count++;
+		mutex_unlock(&bdev->bd_fsfreeze_mutex);
+		return error;
 	}
 
-	sb->s_frozen = SB_UNFROZEN;
-	smp_wmb();
-	wake_up(&sb->s_wait_unfrozen);
-
-out_deactivate:
-	if (sb)
-		deactivate_locked_super(sb);
-out_unlock:
+out:
 	mutex_unlock(&bdev->bd_fsfreeze_mutex);
 	return 0;
 }
diff --git a/fs/ioctl.c b/fs/ioctl.c
index 6c75110..a065eff 100644
--- a/fs/ioctl.c
+++ b/fs/ioctl.c
@@ -503,6 +503,7 @@ static int ioctl_fioasync(unsigned int fd, struct file *filp,
 static int ioctl_fsfreeze(struct file *filp)
 {
 	struct super_block *sb = filp->f_path.dentry->d_inode->i_sb;
+	int ret;
 
 	if (!capable(CAP_SYS_ADMIN))
 		return -EPERM;
@@ -511,15 +512,10 @@ static int ioctl_fsfreeze(struct file *filp)
 	if (sb->s_op->freeze_fs == NULL)
 		return -EOPNOTSUPP;
 
-	/* If a blockdevice-backed filesystem isn't specified, return. */
-	if (sb->s_bdev == NULL)
-		return -EINVAL;
-
 	/* Freeze */
-	sb = freeze_bdev(sb->s_bdev);
-	if (IS_ERR(sb))
-		return PTR_ERR(sb);
-	return 0;
+	ret = freeze_super(sb, 0);
+
+	return ret;
 }
 
 static int ioctl_fsthaw(struct file *filp)
@@ -529,12 +525,8 @@ static int ioctl_fsthaw(struct file *filp)
 	if (!capable(CAP_SYS_ADMIN))
 		return -EPERM;
 
-	/* If a blockdevice-backed filesystem isn't specified, return EINVAL. */
-	if (sb->s_bdev == NULL)
-		return -EINVAL;
-
 	/* Thaw */
-	return thaw_bdev(sb->s_bdev, sb);
+	return thaw_super(sb);
 }
 
 /*
diff --git a/fs/super.c b/fs/super.c
index 19eb70b..91c734c 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -971,6 +971,93 @@ out:
 
 EXPORT_SYMBOL_GPL(vfs_kern_mount);
 
+/**
+ * freeze_super -- lock the filesystem and force it into a consistent state
+ * @super: 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.
+ */
+int freeze_super(struct super_block *sb, int locked)
+{
+	int ret;
+
+	if (!locked) {
+		spin_lock(&sb_lock);
+		ret = grab_super(sb);
+		if (!ret)
+			return 0;
+	}
+
+	if (sb->s_flags & MS_RDONLY) {
+		deactivate_locked_super(sb);
+		return 0;
+	}
+
+	if (sb->s_frozen) {
+		deactivate_locked_super(sb);
+		return -EBUSY;
+	}
+
+	sb->s_frozen = SB_FREEZE_WRITE;
+	smp_wmb();
+
+	sync_filesystem(sb);
+
+	sb->s_frozen = SB_FREEZE_TRANS;
+	smp_wmb();
+
+	sync_blockdev(sb->s_bdev);
+	if (sb->s_op->freeze_fs) {
+		ret = sb->s_op->freeze_fs(sb);
+		if (ret) {
+			printk(KERN_ERR
+				"VFS:Filesystem freeze failed\n");
+			sb->s_frozen = SB_UNFROZEN;
+			deactivate_locked_super(sb);
+			return ret;
+		}
+	}
+	up_write(&sb->s_umount);
+	return 0;
+}
+EXPORT_SYMBOL(freeze_super);
+
+/**
+ * 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 error;
+
+	down_write(&sb->s_umount);
+	if (sb->s_flags & MS_RDONLY)
+		goto out;
+
+	if (sb->s_op->unfreeze_fs) {
+		error = sb->s_op->unfreeze_fs(sb);
+		if (error) {
+			printk(KERN_ERR
+				"VFS:Filesystem thaw failed\n");
+			sb->s_frozen = SB_FREEZE_TRANS;
+			return error;
+		}
+	}
+
+	sb->s_frozen = SB_UNFROZEN;
+	smp_wmb();
+	wake_up(&sb->s_wait_unfrozen);
+
+out:
+	deactivate_locked_super(sb);
+	return 0;
+}
+EXPORT_SYMBOL(thaw_super);
+
 static struct vfsmount *fs_set_subtype(struct vfsmount *mnt, const char *fstype)
 {
 	int err;
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 2620a8c..a5778ae 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1802,6 +1802,8 @@ extern int may_umount(struct vfsmount *);
 extern long do_mount(char *, char *, char *, unsigned long, void *);
 extern struct vfsmount *collect_mounts(struct path *);
 extern void drop_collected_mounts(struct vfsmount *);
+extern int freeze_super(struct super_block *super, int locked);
+extern int thaw_super(struct super_block *super);
 
 extern int vfs_statfs(struct dentry *, struct kstatfs *);
 
-- 
1.6.6.1

Re: [PATCH] Introduce freeze_super and thaw_super for the fsfreeze ioctl

[Al Viro]

On Tue, Mar 23, 2010 at 10:34:56AM -0400, Josef Bacik wrote:
> +++ b/fs/block_dev.c
> @@ -245,35 +245,13 @@ struct super_block *freeze_bdev(struct block_device *bdev)
>  	sb = get_active_super(bdev);

sb is an active locked reference

> +	error = freeze_super(sb, 1);
> +	if (error) {
> +		bdev->bd_fsfreeze_count--;
> +		mutex_unlock(&bdev->bd_fsfreeze_mutex);
> +		return ERR_PTR(error);
>  	}
> -	up_write(&sb->s_umount);
>  
>   out:
>  	sync_blockdev(bdev);

>  static int ioctl_fsfreeze(struct file *filp)
>  {
>  	struct super_block *sb = filp->f_path.dentry->d_inode->i_sb;

sb is an active reference

> +	ret = freeze_super(sb, 0);
> +
> +	return ret;
>  }

> +int freeze_super(struct super_block *sb, int locked)
> +{
> +	int ret;
> +
> +	if (!locked) {
> +		spin_lock(&sb_lock);
> +		ret = grab_super(sb);

What in hell for?  We already hold an active reference here.  That's leaving
aside the obvious comments about argument-dependent locking state...

Re: [PATCH] Introduce freeze_super and thaw_super for the fsfreeze ioctl

[Josef Bacik]

On Tue, Mar 23, 2010 at 02:48:28PM +0000, Al Viro wrote:
> On Tue, Mar 23, 2010 at 10:34:56AM -0400, Josef Bacik wrote:
> > +++ b/fs/block_dev.c
> > @@ -245,35 +245,13 @@ struct super_block *freeze_bdev(struct block_device *bdev)
> >  	sb = get_active_super(bdev);
> 
> sb is an active locked reference
> 
> > +	error = freeze_super(sb, 1);
> > +	if (error) {
> > +		bdev->bd_fsfreeze_count--;
> > +		mutex_unlock(&bdev->bd_fsfreeze_mutex);
> > +		return ERR_PTR(error);
> >  	}
> > -	up_write(&sb->s_umount);
> >  
> >   out:
> >  	sync_blockdev(bdev);
> 
> >  static int ioctl_fsfreeze(struct file *filp)
> >  {
> >  	struct super_block *sb = filp->f_path.dentry->d_inode->i_sb;
> 
> sb is an active reference
> 

I don't understand how this is an active reference?  We are talking about
s_active right?

> > +	ret = freeze_super(sb, 0);
> > +
> > +	return ret;
> >  }
> 
> > +int freeze_super(struct super_block *sb, int locked)
> > +{
> > +	int ret;
> > +
> > +	if (!locked) {
> > +		spin_lock(&sb_lock);
> > +		ret = grab_super(sb);
> 
> What in hell for?  We already hold an active reference here.  That's leaving
> aside the obvious comments about argument-dependent locking state...

We need to have an active (s_active++) reference to the super throughout the
freeze to make sure somebody doesn't come along and umount the fs until after we
do the thaw.  Now, most of this code is copy and pasted, so I don't claim to
understand why it was done this way to begin with, I was just trying to not
change as much as possible.  But from what I can tell, we _need_ to have the
active reference on the sb until we thaw.  Freeze_bdev already gets this active
reference via get_active_super(), but from what I can tell the ioctl_fsfreeze
doesn't have an active reference, hence the locked argument, so we can do a
grab_super and get the active reference.  Would you prefer that I have a
__freeze_super that expects the sb to already have an active reference, and then
make freeze_super do the grab_super instead?  Thanks,

Josef

Re: [PATCH] Introduce freeze_super and thaw_super for the fsfreeze ioctl

[Al Viro]

On Tue, Mar 23, 2010 at 11:03:01AM -0400, Josef Bacik wrote:
> > sb is an active reference
> > 
> 
> I don't understand how this is an active reference?  We are talking about
> s_active right?

It's an opened file, for crying out loud!  If there is anything that makes
sure that superblock will stay alive, that is it...

And lose the "locked" argument, please.  The sane solution is to make
get_active_super() return it unlocked and have your freeze_bdev() simply
grab s_umount.  Unconditionally.  I'll do the first part in #untested in
a minute or so (and make it grab s_umount in the current variant of code in
fs/block_dev.c); then your patch would shift taking s_umount down into
freeze_super().

Re: [PATCH] Introduce freeze_super and thaw_super for the fsfreeze ioctl

[Al Viro]

On Tue, Mar 23, 2010 at 03:09:23PM +0000, Al Viro wrote:
> On Tue, Mar 23, 2010 at 11:03:01AM -0400, Josef Bacik wrote:
> > > sb is an active reference
> > > 
> > 
> > I don't understand how this is an active reference?  We are talking about
> > s_active right?
> 
> It's an opened file, for crying out loud!  If there is anything that makes
> sure that superblock will stay alive, that is it...
> 
> And lose the "locked" argument, please.  The sane solution is to make
> get_active_super() return it unlocked and have your freeze_bdev() simply
> grab s_umount.  Unconditionally.  I'll do the first part in #untested in
> a minute or so (and make it grab s_umount in the current variant of code in
> fs/block_dev.c); then your patch would shift taking s_umount down into
> freeze_super().

Done.  See the tree on hera (or wait a few until it propagates to git.k.o)

Re: [PATCH] Introduce freeze_super and thaw_super for the fsfreeze ioctl

[Al Viro]

On Tue, Mar 23, 2010 at 03:12:46PM +0000, Al Viro wrote:
> On Tue, Mar 23, 2010 at 03:09:23PM +0000, Al Viro wrote:
> > On Tue, Mar 23, 2010 at 11:03:01AM -0400, Josef Bacik wrote:
> > > > sb is an active reference
> > > > 
> > > 
> > > I don't understand how this is an active reference?  We are talking about
> > > s_active right?
> > 
> > It's an opened file, for crying out loud!  If there is anything that makes
> > sure that superblock will stay alive, that is it...

Grr...  Make that "is alive and can be aliased without any problems, so
atomic_inc(sb->s_active) in caller would do just fine."

Re: [PATCH] Introduce freeze_super and thaw_super for the fsfreeze ioctl

[Sunil Mushran]

Thanks for doing this. This helps ocfs2 too because we need to be able
to freeze the fs both from the user-context (node the ioctl was issued)
and the kernel-context (other nodes). In the older scheme, it was tricky
to handle nodes racing to freeze the fs. This will make it a lot easier.

Sunil

Josef Bacik wrote:
> Currently the way we do freezing is by passing sb>s_bdev to freeze_bdev and then
> letting it do all the work.  But freezing is more of an fs thing, and doesn't
> really have much to do with the bdev at all, all the work gets done with the
> super.  In btrfs we do not populate s_bdev, since we can have multiple bdev's
> for one fs and setting s_bdev makes removing devices from a pool kind of tricky.
> This means that freezing a btrfs filesystem fails, which causes us to corrupt
> with things like tux-on-ice which use the fsfreeze mechanism.  So instead of
> populating sb->s_bdev with a random bdev in our pool, I've broken the actual fs
> freezing stuff into freeze_super and thaw_super.  These just take the
> super_block that we're freezing and does the appropriate work.  It's basically
> just copy and pasted from freeze_bdev.  I've then converted freeze_bdev over to
> use the new super helpers.  I've tested this with ext4 and btrfs and verified
> everything continues to work the same as before.
>
> The only new gotcha is multiple calls to the fsfreeze ioctl will return EBUSY if
> the fs is already frozen.  I thought this was a better solution than adding a
> freeze counter to the super_block, but if everybody hates this idea I'm open to
> suggestions.  Thanks,
>
> Signed-off-by: Josef Bacik <josef@redhat.com>
> ---
>  fs/block_dev.c     |   66 ++++++++------------------------------
>  fs/ioctl.c         |   18 +++--------
>  fs/super.c         |   88 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/fs.h |    2 +
>  4 files changed, 109 insertions(+), 65 deletions(-)
>
> diff --git a/fs/block_dev.c b/fs/block_dev.c
> index 8bed055..71b6165 100644
> --- a/fs/block_dev.c
> +++ b/fs/block_dev.c
> @@ -245,35 +245,13 @@ struct super_block *freeze_bdev(struct block_device *bdev)
>  	sb = get_active_super(bdev);
>  	if (!sb)
>  		goto out;
> -	if (sb->s_flags & MS_RDONLY) {
> -		deactivate_locked_super(sb);
> -		mutex_unlock(&bdev->bd_fsfreeze_mutex);
> -		return sb;
> -	}
> -
> -	sb->s_frozen = SB_FREEZE_WRITE;
> -	smp_wmb();
> -
> -	sync_filesystem(sb);
>  
> -	sb->s_frozen = SB_FREEZE_TRANS;
> -	smp_wmb();
> -
> -	sync_blockdev(sb->s_bdev);
> -
> -	if (sb->s_op->freeze_fs) {
> -		error = sb->s_op->freeze_fs(sb);
> -		if (error) {
> -			printk(KERN_ERR
> -				"VFS:Filesystem freeze failed\n");
> -			sb->s_frozen = SB_UNFROZEN;
> -			deactivate_locked_super(sb);
> -			bdev->bd_fsfreeze_count--;
> -			mutex_unlock(&bdev->bd_fsfreeze_mutex);
> -			return ERR_PTR(error);
> -		}
> +	error = freeze_super(sb, 1);
> +	if (error) {
> +		bdev->bd_fsfreeze_count--;
> +		mutex_unlock(&bdev->bd_fsfreeze_mutex);
> +		return ERR_PTR(error);
>  	}
> -	up_write(&sb->s_umount);
>  
>   out:
>  	sync_blockdev(bdev);
> @@ -295,40 +273,24 @@ int thaw_bdev(struct block_device *bdev, struct super_block *sb)
>  
>  	mutex_lock(&bdev->bd_fsfreeze_mutex);
>  	if (!bdev->bd_fsfreeze_count)
> -		goto out_unlock;
> +		goto out;
>  
>  	error = 0;
>  	if (--bdev->bd_fsfreeze_count > 0)
> -		goto out_unlock;
> +		goto out;
>  
>  	if (!sb)
> -		goto out_unlock;
> +		goto out;
>  
>  	BUG_ON(sb->s_bdev != bdev);
> -	down_write(&sb->s_umount);
> -	if (sb->s_flags & MS_RDONLY)
> -		goto out_deactivate;
> -
> -	if (sb->s_op->unfreeze_fs) {
> -		error = sb->s_op->unfreeze_fs(sb);
> -		if (error) {
> -			printk(KERN_ERR
> -				"VFS:Filesystem thaw failed\n");
> -			sb->s_frozen = SB_FREEZE_TRANS;
> -			bdev->bd_fsfreeze_count++;
> -			mutex_unlock(&bdev->bd_fsfreeze_mutex);
> -			return error;
> -		}
> +	error = thaw_super(sb);
> +	if (error) {
> +		bdev->bd_fsfreeze_count++;
> +		mutex_unlock(&bdev->bd_fsfreeze_mutex);
> +		return error;
>  	}
>  
> -	sb->s_frozen = SB_UNFROZEN;
> -	smp_wmb();
> -	wake_up(&sb->s_wait_unfrozen);
> -
> -out_deactivate:
> -	if (sb)
> -		deactivate_locked_super(sb);
> -out_unlock:
> +out:
>  	mutex_unlock(&bdev->bd_fsfreeze_mutex);
>  	return 0;
>  }
> diff --git a/fs/ioctl.c b/fs/ioctl.c
> index 6c75110..a065eff 100644
> --- a/fs/ioctl.c
> +++ b/fs/ioctl.c
> @@ -503,6 +503,7 @@ static int ioctl_fioasync(unsigned int fd, struct file *filp,
>  static int ioctl_fsfreeze(struct file *filp)
>  {
>  	struct super_block *sb = filp->f_path.dentry->d_inode->i_sb;
> +	int ret;
>  
>  	if (!capable(CAP_SYS_ADMIN))
>  		return -EPERM;
> @@ -511,15 +512,10 @@ static int ioctl_fsfreeze(struct file *filp)
>  	if (sb->s_op->freeze_fs == NULL)
>  		return -EOPNOTSUPP;
>  
> -	/* If a blockdevice-backed filesystem isn't specified, return. */
> -	if (sb->s_bdev == NULL)
> -		return -EINVAL;
> -
>  	/* Freeze */
> -	sb = freeze_bdev(sb->s_bdev);
> -	if (IS_ERR(sb))
> -		return PTR_ERR(sb);
> -	return 0;
> +	ret = freeze_super(sb, 0);
> +
> +	return ret;
>  }
>  
>  static int ioctl_fsthaw(struct file *filp)
> @@ -529,12 +525,8 @@ static int ioctl_fsthaw(struct file *filp)
>  	if (!capable(CAP_SYS_ADMIN))
>  		return -EPERM;
>  
> -	/* If a blockdevice-backed filesystem isn't specified, return EINVAL. */
> -	if (sb->s_bdev == NULL)
> -		return -EINVAL;
> -
>  	/* Thaw */
> -	return thaw_bdev(sb->s_bdev, sb);
> +	return thaw_super(sb);
>  }
>  
>  /*
> diff --git a/fs/super.c b/fs/super.c
> index 19eb70b..305d475 100644
> --- a/fs/super.c
> +++ b/fs/super.c
> @@ -971,6 +971,94 @@ out:
>  
>  EXPORT_SYMBOL_GPL(vfs_kern_mount);
>  
> +/**
> + * freeze_super -- lock the filesystem and force it into a consistent state
> + * @super: the super to lock
> + *
> + * Syncs the super to make sure the filesystem is consistent and calls the fs's
> + * freeze_fs.  We hold the s_umount semaphore in order to make sure the fs is
> + * not unmounted until after we thaw the fs.  This cannot be called multiple
> + * times like freeze_bdev, if we're already frozen we'll return -EBUSY.
> + */
> +int freeze_super(struct super_block *sb, int locked)
> +{
> +	int ret;
> +
> +	if (!locked) {
> +		spin_lock(&sb_lock);
> +		ret = grab_super(sb);
> +		if (!ret)
> +			return 0;
> +	}
> +
> +	if (sb->s_flags & MS_RDONLY) {
> +		deactivate_locked_super(sb);
> +		return 0;
> +	}
> +
> +	if (sb->s_frozen) {
> +		deactivate_locked_super(sb);
> +		return -EBUSY;
> +	}
> +
> +	sb->s_frozen = SB_FREEZE_WRITE;
> +	smp_wmb();
> +
> +	sync_filesystem(sb);
> +
> +	sb->s_frozen = SB_FREEZE_TRANS;
> +	smp_wmb();
> +
> +	sync_blockdev(sb->s_bdev);
> +	if (sb->s_op->freeze_fs) {
> +		ret = sb->s_op->freeze_fs(sb);
> +		if (ret) {
> +			printk(KERN_ERR
> +				"VFS:Filesystem freeze failed\n");
> +			sb->s_frozen = SB_UNFROZEN;
> +			deactivate_locked_super(sb);
> +			return ret;
> +		}
> +	}
> +	up_write(&sb->s_umount);
> +	return 0;
> +}
> +EXPORT_SYMBOL(freeze_super);
> +
> +/**
> + * 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 error;
> +
> +	down_write(&sb->s_umount);
> +	if (sb->s_flags & MS_RDONLY)
> +		goto out;
> +
> +	if (sb->s_op->unfreeze_fs) {
> +		error = sb->s_op->unfreeze_fs(sb);
> +		if (error) {
> +			printk(KERN_ERR
> +				"VFS:Filesystem thaw failed\n");
> +			sb->s_frozen = SB_FREEZE_TRANS;
> +			return error;
> +		}
> +	}
> +
> +	sb->s_frozen = SB_UNFROZEN;
> +	smp_wmb();
> +	wake_up(&sb->s_wait_unfrozen);
> +
> +out:
> +	deactivate_locked_super(sb);
> +	return 0;
> +}
> +EXPORT_SYMBOL(thaw_super);
> +
>  static struct vfsmount *fs_set_subtype(struct vfsmount *mnt, const char *fstype)
>  {
>  	int err;
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 2620a8c..a5778ae 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -1802,6 +1802,8 @@ extern int may_umount(struct vfsmount *);
>  extern long do_mount(char *, char *, char *, unsigned long, void *);
>  extern struct vfsmount *collect_mounts(struct path *);
>  extern void drop_collected_mounts(struct vfsmount *);
> +extern int freeze_super(struct super_block *super, int locked);
> +extern int thaw_super(struct super_block *super);
>  
>  extern int vfs_statfs(struct dentry *, struct kstatfs *);
>  

Re: [PATCH] Introduce freeze_super and thaw_super for the fsfreeze ioctl

[Nigel Cunningham]

Hi.

A cc would have been nice :)

Thankfully the subject caught my eye.

Nigel

Re: [PATCH] Introduce freeze_super and thaw_super for the fsfreeze ioctl

[Nigel Cunningham]

Hi.

On 24/03/10 02:09, Al Viro wrote:
> On Tue, Mar 23, 2010 at 11:03:01AM -0400, Josef Bacik wrote:
>>> sb is an active reference
>>>
>>
>> I don't understand how this is an active reference?  We are talking about
>> s_active right?
>
> It's an opened file, for crying out loud!  If there is anything that makes
> sure that superblock will stay alive, that is it...
>
> And lose the "locked" argument, please.  The sane solution is to make
> get_active_super() return it unlocked and have your freeze_bdev() simply
> grab s_umount.  Unconditionally.  I'll do the first part in #untested in
> a minute or so (and make it grab s_umount in the current variant of code in
> fs/block_dev.c); then your patch would shift taking s_umount down into
> freeze_super().

Since TuxOnIce was mentioned, I guess he's thinking on the following 
routine.

Regards,

Nigel

/**
  * freeze_filesystems - lock all filesystems and force them into a 
consistent
  * state
  * @which:      What combination of fuse & non-fuse to freeze.
  */
void freeze_filesystems(int which)
{
         struct super_block *sb;

         lockdep_off();

         /*
          * Freeze in reverse order so filesystems dependant upon others are
          * frozen in the right order (eg. loopback on ext3).
          */
         list_for_each_entry_reverse(sb, &super_blocks, s_list) {
                 FS_PRINTK(KERN_INFO "Considering %s.%s: (root %p, bdev 
%x)",
                         sb->s_type->name ? sb->s_type->name : "?",
                         sb->s_subtype ? sb->s_subtype : "", sb->s_root,
                         sb->s_bdev ? sb->s_bdev->bd_dev : 0);

                 if (sb->s_type->fs_flags & FS_IS_FUSE &&
                     sb->s_frozen == SB_UNFROZEN &&
                     which & FS_FREEZER_FUSE) {
                         sb->s_frozen = SB_FREEZE_TRANS;
                         sb->s_flags |= MS_FROZEN;
                         FS_PRINTK("Fuse filesystem done.\n");
                         continue;
                 }

                 if (!sb->s_root || !sb->s_bdev ||
                     (sb->s_frozen == SB_FREEZE_TRANS) ||
                     (sb->s_flags & MS_RDONLY) ||
                     (sb->s_flags & MS_FROZEN) ||
                     !(which & FS_FREEZER_NORMAL)) {
                         FS_PRINTK(KERN_INFO "Nope.\n");
                         continue;
                 }

                 FS_PRINTK(KERN_INFO "Freezing %x... ", sb->s_bdev->bd_dev);
                 freeze_bdev(sb->s_bdev);
                 sb->s_flags |= MS_FROZEN;
                 FS_PRINTK(KERN_INFO "Done.\n");
         }

         lockdep_on();
}

Re: [PATCH] Introduce freeze_super and thaw_super for the fsfreeze ioctl

[Al Viro]

On Wed, Mar 24, 2010 at 09:31:51AM +1100, Nigel Cunningham wrote:
> /**
>  * freeze_filesystems - lock all filesystems and force them into a
> consistent
>  * state
>  * @which:      What combination of fuse & non-fuse to freeze.
>  */
> void freeze_filesystems(int which)
> {
>         struct super_block *sb;
> 
>         lockdep_off();
> 
>         /*
>          * Freeze in reverse order so filesystems dependant upon others are
>          * frozen in the right order (eg. loopback on ext3).
>          */

[snip the horror]

	a) traversing superblock list without any locking whatsoever
	b) accessing superblock fields <....>
	c) <.........................> without making sure that it's not
going to disappear
	d) calling freeze_bdev() without any warranties that its argument
is not going to be freed under you
	e) layering violations all over the place
etc.

I've stayed away from TuxOnIce flamefests and I've no idea how representative
that snippet is, but if it *does* match the general code quality in there...
Ouch.

Re: [PATCH] Introduce freeze_super and thaw_super for the fsfreeze ioctl

[Al Viro]

On Tue, Mar 23, 2010 at 11:18:03PM +0000, Al Viro wrote:
> [snip the horror]
> 
> 	a) traversing superblock list without any locking whatsoever
> 	b) accessing superblock fields <....>
> 	c) <.........................> without making sure that it's not
> going to disappear
> 	d) calling freeze_bdev() without any warranties that its argument
> is not going to be freed under you
> 	e) layering violations all over the place
> etc.

BTW, superblock can be kept alive by an opened file.  umount -l, for example,
will give that.  So it can get freed whenever the last reference to struct
file can go away (AF_UNIX garbage collection, etc.)

Re: [PATCH] Introduce freeze_super and thaw_super for the fsfreeze ioctl

[Nigel Cunningham]

Hi.

On 24/03/10 10:18, Al Viro wrote:
> On Wed, Mar 24, 2010 at 09:31:51AM +1100, Nigel Cunningham wrote:
>> /**
>>   * freeze_filesystems - lock all filesystems and force them into a
>> consistent
>>   * state
>>   * @which:      What combination of fuse&  non-fuse to freeze.
>>   */
>> void freeze_filesystems(int which)
>> {
>>          struct super_block *sb;
>>
>>          lockdep_off();
>>
>>          /*
>>           * Freeze in reverse order so filesystems dependant upon others are
>>           * frozen in the right order (eg. loopback on ext3).
>>           */
>
> [snip the horror]
>
> 	a) traversing superblock list without any locking whatsoever
> 	b) accessing superblock fields<....>
> 	c)<.........................>  without making sure that it's not
> going to disappear
> 	d) calling freeze_bdev() without any warranties that its argument
> is not going to be freed under you
> 	e) layering violations all over the place
> etc.
>
> I've stayed away from TuxOnIce flamefests and I've no idea how representative
> that snippet is, but if it *does* match the general code quality in there...
> Ouch.

Locking isn't necessary because of the freezing.

Regards,

Nigel

Re: [PATCH] Introduce freeze_super and thaw_super for the fsfreeze ioctl

[Nigel Cunningham]

Hi again.

On 24/03/10 10:52, Nigel Cunningham wrote:
>> I've stayed away from TuxOnIce flamefests and I've no idea how
>> representative
>> that snippet is, but if it *does* match the general code quality in
>> there...
>> Ouch.
>
> Locking isn't necessary because of the freezing.

Err, freezer, I mean.

Regards,

Nigel

Re: [PATCH] Introduce freeze_super and thaw_super for the fsfreeze ioctl

[Nigel Cunningham]

Hi again.

On 24/03/10 10:18, Al Viro wrote:
> I've stayed away from TuxOnIce flamefests and I've no idea how representative
> that snippet is, but if it *does* match the general code quality in there...

PS: I wish you wouldn't. I know I'm not the guru coder some of you guys 
are, and have no problem with being told there's a better way to do 
things (as long as you are better to deal with than some others have 
sometimes been in the past!)

Nigel

Re: [PATCH] Introduce freeze_super and thaw_super for the fsfreeze ioctl

[Al Viro]

On Wed, Mar 24, 2010 at 10:55:45AM +1100, Nigel Cunningham wrote:
> Hi again.
> 
> On 24/03/10 10:52, Nigel Cunningham wrote:
> >>I've stayed away from TuxOnIce flamefests and I've no idea how
> >>representative
> >>that snippet is, but if it *does* match the general code quality in
> >>there...
> >>Ouch.
> >
> >Locking isn't necessary because of the freezing.
> 
> Err, freezer, I mean.

Even leaving aside the accuracy of that (and I seriously doubt it), this
is completely wrong since *code* *gets* *copied*.

Re: [PATCH] Introduce freeze_super and thaw_super for the fsfreeze ioctl

[Nigel Cunningham]

Hi.

On 24/03/10 11:21, Al Viro wrote:
> On Wed, Mar 24, 2010 at 10:55:45AM +1100, Nigel Cunningham wrote:
>> Hi again.
>>
>> On 24/03/10 10:52, Nigel Cunningham wrote:
>>>> I've stayed away from TuxOnIce flamefests and I've no idea how
>>>> representative
>>>> that snippet is, but if it *does* match the general code quality in
>>>> there...
>>>> Ouch.
>>>
>>> Locking isn't necessary because of the freezing.
>>
>> Err, freezer, I mean.
>
> Even leaving aside the accuracy of that (and I seriously doubt it), this
> is completely wrong since *code* *gets* *copied*.

Ok. That's a fair enough point. I'll add locking.

Regards,

Nigel

Re: [PATCH] Introduce freeze_super and thaw_super for the fsfreeze ioctl

[Josef Bacik]

On Wed, Mar 24, 2010 at 09:25:39AM +1100, Nigel Cunningham wrote:
> Hi.
>
> A cc would have been nice :)
>
> Thankfully the subject caught my eye.
>

Well it's not really tux-on-ice's fault, freeze wouldn't work _at all_ via the
ioctl or any other method, we just happened to notice it was broken because
people using tux-on-ice were getting corrupt filesystems after suspend :).  Btw,
it may be good to have freezer_sync set to 1 by default, otherwise if an fs
doesnt support freezing it will likely end up corrupted too.  Thanks,

Josef

Re: [PATCH] Introduce freeze_super and thaw_super for the fsfreeze ioctl

[Nigel Cunningham]

Hi.

On 24/03/10 12:17, Josef Bacik wrote:
> On Wed, Mar 24, 2010 at 09:25:39AM +1100, Nigel Cunningham wrote:
>> Hi.
>>
>> A cc would have been nice :)
>>
>> Thankfully the subject caught my eye.
>>
>
> Well it's not really tux-on-ice's fault, freeze wouldn't work _at all_ via the
> ioctl or any other method, we just happened to notice it was broken because
> people using tux-on-ice were getting corrupt filesystems after suspend :).  Btw,
> it may be good to have freezer_sync set to 1 by default, otherwise if an fs
> doesnt support freezing it will likely end up corrupted too.  Thanks,

Corrupted because the sys_sync was disabled? (We were recently 
discussing adding a tuneable to swsusp for disabling sys_sync there too).

I've just committed a patch making freezer_sync default to 1.

Regards,

Nigel