* [RFC] fs: move freeze/unfreeze_fs hooks before freeze/thaw_super
@ 2014-09-23 2:06 Benjamin Marzinski
2014-09-23 13:38 ` Dave Chinner
0 siblings, 1 reply; 4+ messages in thread
From: Benjamin Marzinski @ 2014-09-23 2:06 UTC (permalink / raw
To: linux-fsdevel
Currently, freezing a filesystem involves calling freeze_super, which locks
sb->s_umount and then calls the fs-specific freeze_fs hook. This makes it
hard for gfs2 (and potentially other cluster filesystems) to use the vfs
freezing code to do freezes on all the cluster nodes.
In order to communicate that a freeze has been requested, and to make sure
that only one node is trying to freeze at a time, gfs2 uses a glock
(sd_freeze_gl). The problem is that there is no hook for gfs2 to acquire
this lock before calling freeze_super. This means that two nodes can
attempt to freeze the filesystem by both calling freeze_super, acquiring
the sb->s_umount lock, and then attempting to grab the cluster glock
sd_freeze_gl. Only one will succeed, and the other will be stuck in
freeze_super, making it impossible to finish freezing the node.
To solve this problem, this patch pushes the freeze/unfreeze_fs hooks to
before freeze/thaw_super, and makes freeze/thaw_super take a callback
function to execute any fs specific code that needs to be done while
s_umount is held. This also means that every filesystem that implements
freeze/unfreeze_fs must call freeze/thaw_super from that function.
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
fs/block_dev.c | 10 ++++++++--
fs/btrfs/super.c | 7 +++----
fs/ext2/super.c | 14 ++++++++++++--
fs/ext3/super.c | 14 ++++++++++++--
fs/ext4/super.c | 14 ++++++++++++--
fs/f2fs/super.c | 7 +++----
fs/gfs2/super.c | 14 ++++++++++++--
fs/gfs2/sys.c | 5 +++--
fs/ioctl.c | 7 +++++--
fs/jfs/super.c | 14 ++++++++++++--
fs/nilfs2/super.c | 14 ++++++++++++--
fs/reiserfs/super.c | 14 ++++++++++++--
fs/super.c | 14 ++++++++------
fs/xfs/xfs_super.c | 18 ++++++++++++++++--
include/linux/fs.h | 6 ++++--
15 files changed, 134 insertions(+), 38 deletions(-)
diff --git a/fs/block_dev.c b/fs/block_dev.c
index 6d72746..668b47d 100644
--- a/fs/block_dev.c
+++ b/fs/block_dev.c
@@ -245,7 +245,10 @@ struct super_block *freeze_bdev(struct block_device *bdev)
sb = get_active_super(bdev);
if (!sb)
goto out;
- error = freeze_super(sb);
+ if (sb->s_op->freeze_fs)
+ error = sb->s_op->freeze_fs(sb);
+ else
+ error = freeze_super(sb, NULL);
if (error) {
deactivate_super(sb);
bdev->bd_fsfreeze_count--;
@@ -282,7 +285,10 @@ int thaw_bdev(struct block_device *bdev, struct super_block *sb)
if (!sb)
goto out;
- error = thaw_super(sb);
+ if (sb->s_op->unfreeze_fs)
+ error = sb->s_op->unfreeze_fs(sb);
+ else
+ error = thaw_super(sb, NULL);
if (error) {
bdev->bd_fsfreeze_count++;
mutex_unlock(&bdev->bd_fsfreeze_mutex);
diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index c4124de..20787f7 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -1808,7 +1808,7 @@ static long btrfs_control_ioctl(struct file *file, unsigned int cmd,
return ret;
}
-static int btrfs_freeze(struct super_block *sb)
+static int do_btrfs_freeze(struct super_block *sb)
{
struct btrfs_trans_handle *trans;
struct btrfs_root *root = btrfs_sb(sb)->tree_root;
@@ -1823,9 +1823,9 @@ static int btrfs_freeze(struct super_block *sb)
return btrfs_commit_transaction(trans, root);
}
-static int btrfs_unfreeze(struct super_block *sb)
+static int btrfs_freeze(struct super_block *sb)
{
- return 0;
+ return freeze_super(sb, do_btrfs_freeze);
}
static int btrfs_show_devname(struct seq_file *m, struct dentry *root)
@@ -1876,7 +1876,6 @@ static const struct super_operations btrfs_super_ops = {
.statfs = btrfs_statfs,
.remount_fs = btrfs_remount,
.freeze_fs = btrfs_freeze,
- .unfreeze_fs = btrfs_unfreeze,
};
static const struct file_operations btrfs_ctl_fops = {
diff --git a/fs/ext2/super.c b/fs/ext2/super.c
index b88edc0..eb10f4b 100644
--- a/fs/ext2/super.c
+++ b/fs/ext2/super.c
@@ -1209,7 +1209,7 @@ static int ext2_sync_fs(struct super_block *sb, int wait)
return 0;
}
-static int ext2_freeze(struct super_block *sb)
+static int do_ext2_freeze(struct super_block *sb)
{
struct ext2_sb_info *sbi = EXT2_SB(sb);
@@ -1231,7 +1231,12 @@ static int ext2_freeze(struct super_block *sb)
return 0;
}
-static int ext2_unfreeze(struct super_block *sb)
+static int ext2_freeze(struct super_block *sb)
+{
+ return freeze_super(sb, do_ext2_freeze);
+}
+
+static int do_ext2_unfreeze(struct super_block *sb)
{
/* Just write sb to clear EXT2_VALID_FS flag */
ext2_write_super(sb);
@@ -1239,6 +1244,11 @@ static int ext2_unfreeze(struct super_block *sb)
return 0;
}
+static int ext2_unfreeze(struct super_block *sb)
+{
+ return thaw_super(sb, do_ext2_unfreeze);
+}
+
void ext2_write_super(struct super_block *sb)
{
if (!(sb->s_flags & MS_RDONLY))
diff --git a/fs/ext3/super.c b/fs/ext3/super.c
index 622e882..a15457d 100644
--- a/fs/ext3/super.c
+++ b/fs/ext3/super.c
@@ -2589,7 +2589,7 @@ static int ext3_sync_fs(struct super_block *sb, int wait)
* LVM calls this function before a (read-only) snapshot is created. This
* gives us a chance to flush the journal completely and mark the fs clean.
*/
-static int ext3_freeze(struct super_block *sb)
+static int do_ext3_freeze(struct super_block *sb)
{
int error = 0;
journal_t *journal;
@@ -2621,11 +2621,16 @@ out:
return error;
}
+static int ext3_freeze(struct super_block *sb)
+{
+ return freeze_super(sb, do_ext3_freeze);
+}
+
/*
* Called by LVM after the snapshot is done. We need to reset the RECOVER
* flag here, even though the filesystem is not technically dirty yet.
*/
-static int ext3_unfreeze(struct super_block *sb)
+static int do_ext3_unfreeze(struct super_block *sb)
{
if (!(sb->s_flags & MS_RDONLY)) {
/* Reser the needs_recovery flag before the fs is unlocked. */
@@ -2636,6 +2641,11 @@ static int ext3_unfreeze(struct super_block *sb)
return 0;
}
+static int ext3_unfreeze(struct super_block *sb)
+{
+ return thaw_super(sb, do_ext3_unfreeze);
+}
+
static int ext3_remount (struct super_block * sb, int * flags, char * data)
{
struct ext3_super_block * es;
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 0b28b36..354bd88 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -4713,7 +4713,7 @@ static int ext4_sync_fs_nojournal(struct super_block *sb, int wait)
* state independently. It relies on upper layer to stop all data & metadata
* modifications.
*/
-static int ext4_freeze(struct super_block *sb)
+static int do_ext4_freeze(struct super_block *sb)
{
int error = 0;
journal_t *journal;
@@ -4743,11 +4743,16 @@ out:
return error;
}
+static int ext4_freeze(struct super_block *sb)
+{
+ return freeze_super(sb, do_ext4_freeze);
+}
+
/*
* Called by LVM after the snapshot is done. We need to reset the RECOVER
* flag here, even though the filesystem is not technically dirty yet.
*/
-static int ext4_unfreeze(struct super_block *sb)
+static int do_ext4_unfreeze(struct super_block *sb)
{
if (sb->s_flags & MS_RDONLY)
return 0;
@@ -4758,6 +4763,11 @@ static int ext4_unfreeze(struct super_block *sb)
return 0;
}
+static int ext4_unfreeze(struct super_block *sb)
+{
+ return thaw_super(sb, do_ext4_unfreeze);
+}
+
/*
* Structure to save mount options for ext4_remount's benefit
*/
diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index 41bdf51..f2abc72 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -474,7 +474,7 @@ int f2fs_sync_fs(struct super_block *sb, int sync)
return 0;
}
-static int f2fs_freeze(struct super_block *sb)
+static int do_f2fs_freeze(struct super_block *sb)
{
int err;
@@ -485,9 +485,9 @@ static int f2fs_freeze(struct super_block *sb)
return err;
}
-static int f2fs_unfreeze(struct super_block *sb)
+static int f2fs_freeze(struct super_block *sb)
{
- return 0;
+ return freeze_super(sb, do_f2fs_freeze);
}
static int f2fs_statfs(struct dentry *dentry, struct kstatfs *buf)
@@ -687,7 +687,6 @@ static struct super_operations f2fs_sops = {
.put_super = f2fs_put_super,
.sync_fs = f2fs_sync_fs,
.freeze_fs = f2fs_freeze,
- .unfreeze_fs = f2fs_unfreeze,
.statfs = f2fs_statfs,
.remount_fs = f2fs_remount,
};
diff --git a/fs/gfs2/super.c b/fs/gfs2/super.c
index a346f56..2a253d8 100644
--- a/fs/gfs2/super.c
+++ b/fs/gfs2/super.c
@@ -954,7 +954,7 @@ static int gfs2_sync_fs(struct super_block *sb, int wait)
*
*/
-static int gfs2_freeze(struct super_block *sb)
+static int do_gfs2_freeze(struct super_block *sb)
{
struct gfs2_sbd *sdp = sb->s_fs_info;
int error;
@@ -983,13 +983,18 @@ static int gfs2_freeze(struct super_block *sb)
return 0;
}
+static int gfs2_freeze(struct super_block *sb)
+{
+ return freeze_super(sb, do_gfs2_freeze);
+}
+
/**
* gfs2_unfreeze - reallow writes to the filesystem
* @sb: the VFS structure for the filesystem
*
*/
-static int gfs2_unfreeze(struct super_block *sb)
+static int do_gfs2_unfreeze(struct super_block *sb)
{
struct gfs2_sbd *sdp = sb->s_fs_info;
@@ -1000,6 +1005,11 @@ static int gfs2_unfreeze(struct super_block *sb)
return 0;
}
+static int gfs2_unfreeze(struct super_block *sb)
+{
+ return thaw_super(sb, do_gfs2_unfreeze);
+}
+
/**
* statfs_fill - fill in the sg for a given RG
* @rgd: the RG
diff --git a/fs/gfs2/sys.c b/fs/gfs2/sys.c
index 3ab566b..9647beb 100644
--- a/fs/gfs2/sys.c
+++ b/fs/gfs2/sys.c
@@ -102,6 +102,7 @@ static ssize_t freeze_show(struct gfs2_sbd *sdp, char *buf)
static ssize_t freeze_store(struct gfs2_sbd *sdp, const char *buf, size_t len)
{
int error;
+ struct super_block *sb = sdp->sd_vfs;
int n = simple_strtol(buf, NULL, 0);
if (!capable(CAP_SYS_ADMIN))
@@ -109,10 +110,10 @@ static ssize_t freeze_store(struct gfs2_sbd *sdp, const char *buf, size_t len)
switch (n) {
case 0:
- error = thaw_super(sdp->sd_vfs);
+ error = sb->s_op->freeze_fs(sb);
break;
case 1:
- error = freeze_super(sdp->sd_vfs);
+ error = sb->s_op->unfreeze_fs(sb);
break;
default:
return -EINVAL;
diff --git a/fs/ioctl.c b/fs/ioctl.c
index 8ac3fad..2f46e078 100644
--- a/fs/ioctl.c
+++ b/fs/ioctl.c
@@ -522,7 +522,7 @@ static int ioctl_fsfreeze(struct file *filp)
return -EOPNOTSUPP;
/* Freeze */
- return freeze_super(sb);
+ return sb->s_op->freeze_fs(sb);
}
static int ioctl_fsthaw(struct file *filp)
@@ -533,7 +533,10 @@ static int ioctl_fsthaw(struct file *filp)
return -EPERM;
/* Thaw */
- return thaw_super(sb);
+ if (sb->s_op->unfreeze_fs)
+ return sb->s_op->unfreeze_fs(sb);
+ else
+ return thaw_super(sb, NULL);
}
/*
diff --git a/fs/jfs/super.c b/fs/jfs/super.c
index adf8cb0..48447b2 100644
--- a/fs/jfs/super.c
+++ b/fs/jfs/super.c
@@ -622,7 +622,7 @@ out_kfree:
return ret;
}
-static int jfs_freeze(struct super_block *sb)
+static int do_jfs_freeze(struct super_block *sb)
{
struct jfs_sb_info *sbi = JFS_SBI(sb);
struct jfs_log *log = sbi->log;
@@ -652,7 +652,12 @@ static int jfs_freeze(struct super_block *sb)
return 0;
}
-static int jfs_unfreeze(struct super_block *sb)
+static int jfs_freeze(struct super_block *sb)
+{
+ return freeze_super(sb, do_jfs_freeze);
+}
+
+static int do_jfs_unfreeze(struct super_block *sb)
{
struct jfs_sb_info *sbi = JFS_SBI(sb);
struct jfs_log *log = sbi->log;
@@ -673,6 +678,11 @@ out:
return rc;
}
+static int jfs_unfreeze(struct super_block *sb)
+{
+ return thaw_super(sb, do_jfs_unfreeze);
+}
+
static struct dentry *jfs_do_mount(struct file_system_type *fs_type,
int flags, const char *dev_name, void *data)
{
diff --git a/fs/nilfs2/super.c b/fs/nilfs2/super.c
index 228f5bd..e34980a 100644
--- a/fs/nilfs2/super.c
+++ b/fs/nilfs2/super.c
@@ -573,7 +573,7 @@ int nilfs_attach_checkpoint(struct super_block *sb, __u64 cno, int curr_mnt,
return err;
}
-static int nilfs_freeze(struct super_block *sb)
+static int do_nilfs_freeze(struct super_block *sb)
{
struct the_nilfs *nilfs = sb->s_fs_info;
int err;
@@ -588,7 +588,12 @@ static int nilfs_freeze(struct super_block *sb)
return err;
}
-static int nilfs_unfreeze(struct super_block *sb)
+static int nilfs_freeze(struct super_block *sb)
+{
+ return freeze_super(sb, do_nilfs_freeze);
+}
+
+static int do_nilfs_unfreeze(struct super_block *sb)
{
struct the_nilfs *nilfs = sb->s_fs_info;
@@ -601,6 +606,11 @@ static int nilfs_unfreeze(struct super_block *sb)
return 0;
}
+static int nilfs_unfreeze(struct super_block *sb)
+{
+ return thaw_super(sb, do_nilfs_unfreeze);
+}
+
static int nilfs_statfs(struct dentry *dentry, struct kstatfs *buf)
{
struct super_block *sb = dentry->d_sb;
diff --git a/fs/reiserfs/super.c b/fs/reiserfs/super.c
index d46e88a..e1b4f6a 100644
--- a/fs/reiserfs/super.c
+++ b/fs/reiserfs/super.c
@@ -126,7 +126,7 @@ static void cancel_old_flush(struct super_block *s)
spin_unlock(&sbi->old_work_lock);
}
-static int reiserfs_freeze(struct super_block *s)
+static int do_reiserfs_freeze(struct super_block *s)
{
struct reiserfs_transaction_handle th;
@@ -149,12 +149,22 @@ static int reiserfs_freeze(struct super_block *s)
return 0;
}
-static int reiserfs_unfreeze(struct super_block *s)
+static int reiserfs_freeze(struct super_block *s)
+{
+ return freeze_super(sb, do_reiserfs_freeeze);
+}
+
+static int do_reiserfs_unfreeze(struct super_block *s)
{
reiserfs_allow_writes(s);
return 0;
}
+static int reiserfs_unfreeze(struct super_block *s)
+{
+ thaw_super(sb, do_reiserfs_unfreeze);
+}
+
extern const struct in_core_key MAX_IN_CORE_KEY;
/*
diff --git a/fs/super.c b/fs/super.c
index b9a214d..f6f6325 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -1283,7 +1283,8 @@ static void sb_wait_write(struct super_block *sb, int level)
*
* sb->s_writers.frozen is protected by sb->s_umount.
*/
-int freeze_super(struct super_block *sb)
+int freeze_super(struct super_block *sb,
+ int (*do_freeze_fs)(struct super_block *sb))
{
int ret;
@@ -1330,8 +1331,8 @@ int freeze_super(struct super_block *sb)
smp_wmb();
sb_wait_write(sb, SB_FREEZE_FS);
- if (sb->s_op->freeze_fs) {
- ret = sb->s_op->freeze_fs(sb);
+ if (do_freeze_fs) {
+ ret = do_freeze_fs(sb);
if (ret) {
printk(KERN_ERR
"VFS:Filesystem freeze failed\n");
@@ -1358,7 +1359,8 @@ EXPORT_SYMBOL(freeze_super);
*
* Unlocks the filesystem and marks it writeable again after freeze_super().
*/
-int thaw_super(struct super_block *sb)
+int thaw_super(struct super_block *sb,
+ int (*do_unfreeze_fs)(struct super_block *sb))
{
int error;
@@ -1371,8 +1373,8 @@ int thaw_super(struct super_block *sb)
if (sb->s_flags & MS_RDONLY)
goto out;
- if (sb->s_op->unfreeze_fs) {
- error = sb->s_op->unfreeze_fs(sb);
+ if (do_unfreeze_fs) {
+ error = do_unfreeze_fs(sb);
if (error) {
printk(KERN_ERR
"VFS:Filesystem thaw failed\n");
diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index b194652..52e1a5c 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -1289,7 +1289,7 @@ xfs_fs_remount(
* record to dirty the log in case of a crash while frozen.
*/
STATIC int
-xfs_fs_freeze(
+do_xfs_fs_freeze(
struct super_block *sb)
{
struct xfs_mount *mp = XFS_M(sb);
@@ -1300,7 +1300,14 @@ xfs_fs_freeze(
}
STATIC int
-xfs_fs_unfreeze(
+xfs_fs_freeze(
+ struct super_block *sb)
+{
+ return freeze_super(sb, do_xfs_fs_freeze);
+}
+
+STATIC int
+do_xfs_fs_unfreeze(
struct super_block *sb)
{
struct xfs_mount *mp = XFS_M(sb);
@@ -1311,6 +1318,13 @@ xfs_fs_unfreeze(
}
STATIC int
+xfs_fs_unfreeze(
+ struct super_block *sb)
+{
+ return thaw_super(sb, do_xfs_fs_unfreeze);
+}
+
+STATIC int
xfs_fs_show_options(
struct seq_file *m,
struct dentry *root)
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 9418772..6bec92e 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1864,8 +1864,10 @@ extern int vfs_statfs(struct path *, struct kstatfs *);
extern int user_statfs(const char __user *, struct kstatfs *);
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 freeze_super(struct super_block *super,
+ int (*do_freeze_fs)(struct super_block *sb));
+extern int thaw_super(struct super_block *super,
+ int (*do_unfreeze_fs)(struct super_block *sb));
extern bool our_mnt(struct vfsmount *mnt);
extern bool fs_fully_visible(struct file_system_type *);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [RFC] fs: move freeze/unfreeze_fs hooks before freeze/thaw_super
2014-09-23 13:38 ` Dave Chinner
@ 2014-09-23 13:24 ` Benjamin Marzinski
2014-09-23 20:50 ` Dave Chinner
0 siblings, 1 reply; 4+ messages in thread
From: Benjamin Marzinski @ 2014-09-23 13:24 UTC (permalink / raw
To: Dave Chinner; +Cc: linux-fsdevel
On Tue, Sep 23, 2014 at 11:38:59PM +1000, Dave Chinner wrote:
That way would certainly require fewer changes. But it seemed to me like
adding a pair of hooks that only gfs2 needed would have less support. If
people would prefer a patch like that, I can easily change it.
thanks.
-Ben
> On Mon, Sep 22, 2014 at 09:06:41PM -0500, Benjamin Marzinski wrote:
> > Currently, freezing a filesystem involves calling freeze_super, which locks
> > sb->s_umount and then calls the fs-specific freeze_fs hook. This makes it
> > hard for gfs2 (and potentially other cluster filesystems) to use the vfs
> > freezing code to do freezes on all the cluster nodes.
> >
> > In order to communicate that a freeze has been requested, and to make sure
> > that only one node is trying to freeze at a time, gfs2 uses a glock
> > (sd_freeze_gl). The problem is that there is no hook for gfs2 to acquire
> > this lock before calling freeze_super. This means that two nodes can
> > attempt to freeze the filesystem by both calling freeze_super, acquiring
> > the sb->s_umount lock, and then attempting to grab the cluster glock
> > sd_freeze_gl. Only one will succeed, and the other will be stuck in
> > freeze_super, making it impossible to finish freezing the node.
> >
> > To solve this problem, this patch pushes the freeze/unfreeze_fs hooks to
> > before freeze/thaw_super, and makes freeze/thaw_super take a callback
> > function to execute any fs specific code that needs to be done while
> > s_umount is held. This also means that every filesystem that implements
> > freeze/unfreeze_fs must call freeze/thaw_super from that function.
>
> Wouldn't just adding a ->prepare_freeze/->prepare_thaw method pair
> and adding gfs2 cluster locking to those new methods be better?
> That way no other filesystem code needs to change at all...
>
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@fromorbit.com
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [RFC] fs: move freeze/unfreeze_fs hooks before freeze/thaw_super
2014-09-23 2:06 [RFC] fs: move freeze/unfreeze_fs hooks before freeze/thaw_super Benjamin Marzinski
@ 2014-09-23 13:38 ` Dave Chinner
2014-09-23 13:24 ` Benjamin Marzinski
0 siblings, 1 reply; 4+ messages in thread
From: Dave Chinner @ 2014-09-23 13:38 UTC (permalink / raw
To: Benjamin Marzinski; +Cc: linux-fsdevel
On Mon, Sep 22, 2014 at 09:06:41PM -0500, Benjamin Marzinski wrote:
> Currently, freezing a filesystem involves calling freeze_super, which locks
> sb->s_umount and then calls the fs-specific freeze_fs hook. This makes it
> hard for gfs2 (and potentially other cluster filesystems) to use the vfs
> freezing code to do freezes on all the cluster nodes.
>
> In order to communicate that a freeze has been requested, and to make sure
> that only one node is trying to freeze at a time, gfs2 uses a glock
> (sd_freeze_gl). The problem is that there is no hook for gfs2 to acquire
> this lock before calling freeze_super. This means that two nodes can
> attempt to freeze the filesystem by both calling freeze_super, acquiring
> the sb->s_umount lock, and then attempting to grab the cluster glock
> sd_freeze_gl. Only one will succeed, and the other will be stuck in
> freeze_super, making it impossible to finish freezing the node.
>
> To solve this problem, this patch pushes the freeze/unfreeze_fs hooks to
> before freeze/thaw_super, and makes freeze/thaw_super take a callback
> function to execute any fs specific code that needs to be done while
> s_umount is held. This also means that every filesystem that implements
> freeze/unfreeze_fs must call freeze/thaw_super from that function.
Wouldn't just adding a ->prepare_freeze/->prepare_thaw method pair
and adding gfs2 cluster locking to those new methods be better?
That way no other filesystem code needs to change at all...
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [RFC] fs: move freeze/unfreeze_fs hooks before freeze/thaw_super
2014-09-23 13:24 ` Benjamin Marzinski
@ 2014-09-23 20:50 ` Dave Chinner
0 siblings, 0 replies; 4+ messages in thread
From: Dave Chinner @ 2014-09-23 20:50 UTC (permalink / raw
To: Benjamin Marzinski; +Cc: linux-fsdevel
[please don't top post]
On Tue, Sep 23, 2014 at 08:24:23AM -0500, Benjamin Marzinski wrote:
> On Tue, Sep 23, 2014 at 11:38:59PM +1000, Dave Chinner wrote:
> > On Mon, Sep 22, 2014 at 09:06:41PM -0500, Benjamin Marzinski wrote:
> > > Currently, freezing a filesystem involves calling freeze_super, which locks
> > > sb->s_umount and then calls the fs-specific freeze_fs hook. This makes it
> > > hard for gfs2 (and potentially other cluster filesystems) to use the vfs
> > > freezing code to do freezes on all the cluster nodes.
> > >
> > > In order to communicate that a freeze has been requested, and to make sure
> > > that only one node is trying to freeze at a time, gfs2 uses a glock
> > > (sd_freeze_gl). The problem is that there is no hook for gfs2 to acquire
> > > this lock before calling freeze_super. This means that two nodes can
> > > attempt to freeze the filesystem by both calling freeze_super, acquiring
> > > the sb->s_umount lock, and then attempting to grab the cluster glock
> > > sd_freeze_gl. Only one will succeed, and the other will be stuck in
> > > freeze_super, making it impossible to finish freezing the node.
> > >
> > > To solve this problem, this patch pushes the freeze/unfreeze_fs hooks to
> > > before freeze/thaw_super, and makes freeze/thaw_super take a callback
> > > function to execute any fs specific code that needs to be done while
> > > s_umount is held. This also means that every filesystem that implements
> > > freeze/unfreeze_fs must call freeze/thaw_super from that function.
> >
> > Wouldn't just adding a ->prepare_freeze/->prepare_thaw method pair
> > and adding gfs2 cluster locking to those new methods be better?
> > That way no other filesystem code needs to change at all...
>
> That way would certainly require fewer changes. But it seemed to me like
> adding a pair of hooks that only gfs2 needed would have less support. If
> people would prefer a patch like that, I can easily change it.
It's much better than changing the meaning and usage of an existing
method and silently breaking every out-of-tree module that uses the
freezefs/unfreezefs methods. Adding new methods is simple, these new
methods have very little impact on the structure and logic of the
code, and it can be ignored by the majority of filesystems. Seems
like the right thing to do to me...
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2014-09-23 20:50 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-09-23 2:06 [RFC] fs: move freeze/unfreeze_fs hooks before freeze/thaw_super Benjamin Marzinski
2014-09-23 13:38 ` Dave Chinner
2014-09-23 13:24 ` Benjamin Marzinski
2014-09-23 20:50 ` Dave Chinner
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.