LinuxLists.cc - [PATCH 3/6] fs: distinguish between user initiated freeze and kernel initiated freeze

2023-05-08 01:45:35

Subject: [PATCH 3/6] fs: distinguish between user initiated freeze and kernel initiated freeze

Userspace can initiate a freeze call using ioctls. If the kernel decides
to freeze a filesystem later it must be able to distinguish if userspace
had initiated the freeze, so that it does not unfreeze it later
automatically on resume.

Likewise if the kernel is initiating a freeze on its own it should *not*
fail to freeze a filesystem if a user had already frozen it on our behalf.
This same concept applies to thawing, even if its not possible for
userspace to beat the kernel in thawing a filesystem. This logic however
has never applied to userspace freezing and thawing, two consecutive
userspace freeze calls will results in only the first one succeeding, so
we must retain the same behaviour in userspace.

This doesn't implement yet kernel initiated filesystem freeze calls,
this will be done in subsequent calls. This change should introduce
no functional changes, it just extends the definitions of a frozen
filesystem to account for future kernel initiated filesystem freeze
and let's us keep record of when userpace initiated it so the kernel
can respect a userspace initiated freeze upon kernel initiated freeze
and its respective thaw cycle.

Reviewed-by: Jan Kara <[email protected]>
Signed-off-by: Luis Chamberlain <[email protected]>
---
block/bdev.c | 4 ++--
fs/f2fs/gc.c | 4 ++--
fs/gfs2/glops.c | 2 +-
fs/gfs2/super.c | 2 +-
fs/gfs2/sys.c | 4 ++--
fs/gfs2/util.c | 2 +-
fs/ioctl.c | 4 ++--
fs/super.c | 29 +++++++++++++++++++++++++----
include/linux/fs.h | 16 ++++++++++++++--
9 files changed, 50 insertions(+), 17 deletions(-)

diff --git a/block/bdev.c b/block/bdev.c
index dc54a2a1c46e..04f7b2c99845 100644
--- a/block/bdev.c
+++ b/block/bdev.c
@@ -250,7 +250,7 @@ int freeze_bdev(struct block_device *bdev)
if (sb->s_op->freeze_super)
error = sb->s_op->freeze_super(sb);
else
- error = freeze_super(sb);
+ error = freeze_super(sb, true);
deactivate_locked_super(sb);

if (error) {
@@ -295,7 +295,7 @@ int thaw_bdev(struct block_device *bdev)
if (sb->s_op->thaw_super)
error = sb->s_op->thaw_super(sb);
else
- error = thaw_super(sb);
+ error = thaw_super(sb, true);
if (error)
bdev->bd_fsfreeze_count++;
else
diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
index e31d6791d3e3..a5891055d85d 100644
--- a/fs/f2fs/gc.c
+++ b/fs/f2fs/gc.c
@@ -2168,7 +2168,7 @@ int f2fs_resize_fs(struct f2fs_sb_info *sbi, __u64 block_count)

if (!get_active_super(sbi->sb->s_bdev))
return -ENOTTY;
- freeze_super(sbi->sb);
+ freeze_super(sbi->sb, true);

f2fs_down_write(&sbi->gc_lock);
f2fs_down_write(&sbi->cp_global_sem);
@@ -2221,7 +2221,7 @@ int f2fs_resize_fs(struct f2fs_sb_info *sbi, __u64 block_count)
f2fs_up_write(&sbi->cp_global_sem);
f2fs_up_write(&sbi->gc_lock);
/* We use the same active reference from freeze */
- thaw_super(sbi->sb);
+ thaw_super(sbi->sb, true);
deactivate_locked_super(sbi->sb);
return err;
}
diff --git a/fs/gfs2/glops.c b/fs/gfs2/glops.c
index 01d433ed6ce7..8fd37508f9a0 100644
--- a/fs/gfs2/glops.c
+++ b/fs/gfs2/glops.c
@@ -584,7 +584,7 @@ static int freeze_go_sync(struct gfs2_glock *gl)
if (gl->gl_state == LM_ST_SHARED && !gfs2_withdrawn(sdp) &&
!test_bit(SDF_NORECOVERY, &sdp->sd_flags)) {
atomic_set(&sdp->sd_freeze_state, SFS_STARTING_FREEZE);
- error = freeze_super(sdp->sd_vfs);
+ error = freeze_super(sdp->sd_vfs, true);
if (error) {
fs_info(sdp, "GFS2: couldn't freeze filesystem: %d\n",
error);
diff --git a/fs/gfs2/super.c b/fs/gfs2/super.c
index e57cb593e2f3..f2641891de43 100644
--- a/fs/gfs2/super.c
+++ b/fs/gfs2/super.c
@@ -687,7 +687,7 @@ void gfs2_freeze_func(struct work_struct *work)
gfs2_assert_withdraw(sdp, 0);
} else {
atomic_set(&sdp->sd_freeze_state, SFS_UNFROZEN);
- error = thaw_super(sb);
+ error = thaw_super(sb, true);
if (error) {
fs_info(sdp, "GFS2: couldn't thaw filesystem: %d\n",
error);
diff --git a/fs/gfs2/sys.c b/fs/gfs2/sys.c
index e80c827acd09..9e0398f99674 100644
--- a/fs/gfs2/sys.c
+++ b/fs/gfs2/sys.c
@@ -169,10 +169,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 = thaw_super(sdp->sd_vfs, true);
break;
case 1:
- error = freeze_super(sdp->sd_vfs);
+ error = freeze_super(sdp->sd_vfs, true);
break;
default:
deactivate_locked_super(sb);
diff --git a/fs/gfs2/util.c b/fs/gfs2/util.c
index 3a0cd5e9ad84..be9705d618ec 100644
--- a/fs/gfs2/util.c
+++ b/fs/gfs2/util.c
@@ -191,7 +191,7 @@ static void signal_our_withdraw(struct gfs2_sbd *sdp)
/* Make sure gfs2_unfreeze works if partially-frozen */
flush_work(&sdp->sd_freeze_work);
atomic_set(&sdp->sd_freeze_state, SFS_FROZEN);
- thaw_super(sdp->sd_vfs);
+ thaw_super(sdp->sd_vfs, true);
} else {
wait_on_bit(&i_gl->gl_flags, GLF_DEMOTE,
TASK_UNINTERRUPTIBLE);
diff --git a/fs/ioctl.c b/fs/ioctl.c
index 1d20af762e0d..3cc79b82a5dc 100644
--- a/fs/ioctl.c
+++ b/fs/ioctl.c
@@ -401,7 +401,7 @@ static int ioctl_fsfreeze(struct file *filp)
/* Freeze */
if (sb->s_op->freeze_super)
ret = sb->s_op->freeze_super(sb);
- ret = freeze_super(sb);
+ ret = freeze_super(sb, true);

deactivate_locked_super(sb);

@@ -418,7 +418,7 @@ static int ioctl_fsthaw(struct file *filp)
/* Thaw */
if (sb->s_op->thaw_super)
return sb->s_op->thaw_super(sb);
- return thaw_super(sb);
+ return thaw_super(sb, true);
}

static int ioctl_file_dedupe_range(struct file *file,
diff --git a/fs/super.c b/fs/super.c
index 46c6475fc765..16ccbb9dd230 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -1026,7 +1026,7 @@ static void do_thaw_all_callback(struct super_block *sb)
return;
if (sb->s_root && sb->s_flags & SB_BORN) {
emergency_thaw_bdev(sb);
- thaw_super(sb);
+ thaw_super(sb, true);
}
deactivate_locked_super(sb);
}
@@ -1636,6 +1636,8 @@ static void sb_freeze_unlock(struct super_block *sb, int level)
/**
* freeze_super - force a filesystem backed by a block device into a consistent state
* @sb: the super to lock
+ * @usercall: whether or not userspace initiated this via an ioctl or if it
+ * was a kernel freeze
*
* Used by filesystems and the kernel to freeze a fileystem backed by a block
* device into a consistent state. Callers must use get_active_super(bdev) to
@@ -1669,10 +1671,13 @@ static void sb_freeze_unlock(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, bool usercall)
{
int ret;

+ if (!usercall && sb_is_frozen(sb))
+ return 0;
+
if (!sb_is_unfrozen(sb))
return -EBUSY;

@@ -1682,6 +1687,7 @@ int freeze_super(struct super_block *sb)
if (sb_rdonly(sb)) {
/* Nothing to do really... */
sb->s_writers.frozen = SB_FREEZE_COMPLETE;
+ sb->s_writers.frozen_by_user = usercall;
return 0;
}

@@ -1699,6 +1705,7 @@ int freeze_super(struct super_block *sb)
ret = sync_filesystem(sb);
if (ret) {
sb->s_writers.frozen = SB_UNFROZEN;
+ sb->s_writers.frozen_by_user = false;
sb_freeze_unlock(sb, SB_FREEZE_PAGEFAULT);
wake_up(&sb->s_writers.wait_unfrozen);
return ret;
@@ -1724,6 +1731,7 @@ int freeze_super(struct super_block *sb)
* when frozen is set to SB_FREEZE_COMPLETE, and for thaw_super().
*/
sb->s_writers.frozen = SB_FREEZE_COMPLETE;
+ sb->s_writers.frozen_by_user = usercall;
lockdep_sb_freeze_release(sb);
return 0;
}
@@ -1732,21 +1740,33 @@ EXPORT_SYMBOL(freeze_super);
/**
* thaw_super -- unlock a filesystem backed by a block device
* @sb: the super to thaw
+ * @usercall: whether or not userspace initiated this thaw or if it was the
+ * kernel which initiated it
*
* Used by filesystems and the kernel to thaw a fileystem backed by a block
* device. Callers must use get_active_super(bdev) to lock the @sb and when
* done must unlock it with deactivate_locked_super(). Once done, this marks
* the filesystem as writeable.
*/
-int thaw_super(struct super_block *sb)
+int thaw_super(struct super_block *sb, bool usercall)
{
int error;

- if (sb->s_writers.frozen != SB_FREEZE_COMPLETE)
+ if (!usercall) {
+ /*
+ * If userspace initiated the freeze don't let the kernel
+ * thaw it on return from a kernel initiated freeze.
+ */
+ if (sb_is_unfrozen(sb) || sb_is_frozen_by_user(sb))
+ return 0;
+ }
+
+ if (!sb_is_frozen(sb))
return -EINVAL;

if (sb_rdonly(sb)) {
sb->s_writers.frozen = SB_UNFROZEN;
+ sb->s_writers.frozen_by_user = false;
goto out;
}

@@ -1763,6 +1783,7 @@ int thaw_super(struct super_block *sb)
}

sb->s_writers.frozen = SB_UNFROZEN;
+ sb->s_writers.frozen_by_user = false;
sb_freeze_unlock(sb, SB_FREEZE_FS);
out:
wake_up(&sb->s_writers.wait_unfrozen);
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 90b5bdc4071a..d9b46c858103 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1146,6 +1146,7 @@ enum {

struct sb_writers {
int frozen; /* Is sb frozen? */
+ bool frozen_by_user; /* User freeze? */
wait_queue_head_t wait_unfrozen; /* wait for thaw */
struct percpu_rw_semaphore rw_sem[SB_FREEZE_LEVELS];
};
@@ -1632,6 +1633,17 @@ static inline bool sb_is_frozen(struct super_block *sb)
return sb->s_writers.frozen == SB_FREEZE_COMPLETE;
}

+/**
+ * sb_is_frozen_by_user - was the superblock frozen by userspace?
+ * @sb: the super to check
+ *
+ * Returns true if the super is frozen by userspace, such as an ioctl.
+ */
+static inline bool sb_is_frozen_by_user(struct super_block *sb)
+{
+ return sb_is_frozen(sb) && sb->s_writers.frozen_by_user;
+}
+
/**
* sb_is_unfrozen - is superblock unfrozen
* @sb: the super to check
@@ -2308,8 +2320,8 @@ extern int unregister_filesystem(struct file_system_type *);
extern int vfs_statfs(const struct path *, struct kstatfs *);
extern int user_statfs(const char __user *, struct kstatfs *);
extern int fd_statfs(int, 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, bool usercall);
+extern int thaw_super(struct super_block *super, bool usercall);
extern __printf(2, 3)
int super_setup_bdi_name(struct super_block *sb, char *fmt, ...);
extern int super_setup_bdi(struct super_block *sb);
--
2.39.2

2023-05-16 15:30:54

by Darrick J. Wong

[permalink] [raw]

Subject: Re: [PATCH 3/6] fs: distinguish between user initiated freeze and kernel initiated freeze

On Sun, May 07, 2023 at 06:17:14PM -0700, Luis Chamberlain wrote:
> Userspace can initiate a freeze call using ioctls. If the kernel decides
> to freeze a filesystem later it must be able to distinguish if userspace
> had initiated the freeze, so that it does not unfreeze it later
> automatically on resume.
>
> Likewise if the kernel is initiating a freeze on its own it should *not*
> fail to freeze a filesystem if a user had already frozen it on our behalf.
> This same concept applies to thawing, even if its not possible for
> userspace to beat the kernel in thawing a filesystem. This logic however
> has never applied to userspace freezing and thawing, two consecutive
> userspace freeze calls will results in only the first one succeeding, so
> we must retain the same behaviour in userspace.
>
> This doesn't implement yet kernel initiated filesystem freeze calls,
> this will be done in subsequent calls. This change should introduce
> no functional changes, it just extends the definitions of a frozen
> filesystem to account for future kernel initiated filesystem freeze
> and let's us keep record of when userpace initiated it so the kernel
> can respect a userspace initiated freeze upon kernel initiated freeze
> and its respective thaw cycle.
>
> Reviewed-by: Jan Kara <[email protected]>
> Signed-off-by: Luis Chamberlain <[email protected]>
> ---
> block/bdev.c | 4 ++--
> fs/f2fs/gc.c | 4 ++--
> fs/gfs2/glops.c | 2 +-
> fs/gfs2/super.c | 2 +-
> fs/gfs2/sys.c | 4 ++--
> fs/gfs2/util.c | 2 +-
> fs/ioctl.c | 4 ++--
> fs/super.c | 29 +++++++++++++++++++++++++----
> include/linux/fs.h | 16 ++++++++++++++--
> 9 files changed, 50 insertions(+), 17 deletions(-)
>
> diff --git a/block/bdev.c b/block/bdev.c
> index dc54a2a1c46e..04f7b2c99845 100644
> --- a/block/bdev.c
> +++ b/block/bdev.c
> @@ -250,7 +250,7 @@ int freeze_bdev(struct block_device *bdev)
> if (sb->s_op->freeze_super)
> error = sb->s_op->freeze_super(sb);
> else
> - error = freeze_super(sb);
> + error = freeze_super(sb, true);
> deactivate_locked_super(sb);
>
> if (error) {
> @@ -295,7 +295,7 @@ int thaw_bdev(struct block_device *bdev)
> if (sb->s_op->thaw_super)
> error = sb->s_op->thaw_super(sb);
> else
> - error = thaw_super(sb);
> + error = thaw_super(sb, true);
> if (error)
> bdev->bd_fsfreeze_count++;
> else
> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
> index e31d6791d3e3..a5891055d85d 100644
> --- a/fs/f2fs/gc.c
> +++ b/fs/f2fs/gc.c
> @@ -2168,7 +2168,7 @@ int f2fs_resize_fs(struct f2fs_sb_info *sbi, __u64 block_count)
>
> if (!get_active_super(sbi->sb->s_bdev))
> return -ENOTTY;
> - freeze_super(sbi->sb);
> + freeze_super(sbi->sb, true);
>
> f2fs_down_write(&sbi->gc_lock);
> f2fs_down_write(&sbi->cp_global_sem);
> @@ -2221,7 +2221,7 @@ int f2fs_resize_fs(struct f2fs_sb_info *sbi, __u64 block_count)
> f2fs_up_write(&sbi->cp_global_sem);
> f2fs_up_write(&sbi->gc_lock);
> /* We use the same active reference from freeze */
> - thaw_super(sbi->sb);
> + thaw_super(sbi->sb, true);
> deactivate_locked_super(sbi->sb);
> return err;
> }
> diff --git a/fs/gfs2/glops.c b/fs/gfs2/glops.c
> index 01d433ed6ce7..8fd37508f9a0 100644
> --- a/fs/gfs2/glops.c
> +++ b/fs/gfs2/glops.c
> @@ -584,7 +584,7 @@ static int freeze_go_sync(struct gfs2_glock *gl)
> if (gl->gl_state == LM_ST_SHARED && !gfs2_withdrawn(sdp) &&
> !test_bit(SDF_NORECOVERY, &sdp->sd_flags)) {
> atomic_set(&sdp->sd_freeze_state, SFS_STARTING_FREEZE);
> - error = freeze_super(sdp->sd_vfs);
> + error = freeze_super(sdp->sd_vfs, true);
> if (error) {
> fs_info(sdp, "GFS2: couldn't freeze filesystem: %d\n",
> error);
> diff --git a/fs/gfs2/super.c b/fs/gfs2/super.c
> index e57cb593e2f3..f2641891de43 100644
> --- a/fs/gfs2/super.c
> +++ b/fs/gfs2/super.c
> @@ -687,7 +687,7 @@ void gfs2_freeze_func(struct work_struct *work)
> gfs2_assert_withdraw(sdp, 0);
> } else {
> atomic_set(&sdp->sd_freeze_state, SFS_UNFROZEN);
> - error = thaw_super(sb);
> + error = thaw_super(sb, true);
> if (error) {
> fs_info(sdp, "GFS2: couldn't thaw filesystem: %d\n",
> error);
> diff --git a/fs/gfs2/sys.c b/fs/gfs2/sys.c
> index e80c827acd09..9e0398f99674 100644
> --- a/fs/gfs2/sys.c
> +++ b/fs/gfs2/sys.c
> @@ -169,10 +169,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 = thaw_super(sdp->sd_vfs, true);
> break;
> case 1:
> - error = freeze_super(sdp->sd_vfs);
> + error = freeze_super(sdp->sd_vfs, true);
> break;
> default:
> deactivate_locked_super(sb);
> diff --git a/fs/gfs2/util.c b/fs/gfs2/util.c
> index 3a0cd5e9ad84..be9705d618ec 100644
> --- a/fs/gfs2/util.c
> +++ b/fs/gfs2/util.c
> @@ -191,7 +191,7 @@ static void signal_our_withdraw(struct gfs2_sbd *sdp)
> /* Make sure gfs2_unfreeze works if partially-frozen */
> flush_work(&sdp->sd_freeze_work);
> atomic_set(&sdp->sd_freeze_state, SFS_FROZEN);
> - thaw_super(sdp->sd_vfs);
> + thaw_super(sdp->sd_vfs, true);
> } else {
> wait_on_bit(&i_gl->gl_flags, GLF_DEMOTE,
> TASK_UNINTERRUPTIBLE);
> diff --git a/fs/ioctl.c b/fs/ioctl.c
> index 1d20af762e0d..3cc79b82a5dc 100644
> --- a/fs/ioctl.c
> +++ b/fs/ioctl.c
> @@ -401,7 +401,7 @@ static int ioctl_fsfreeze(struct file *filp)
> /* Freeze */
> if (sb->s_op->freeze_super)
> ret = sb->s_op->freeze_super(sb);
> - ret = freeze_super(sb);
> + ret = freeze_super(sb, true);
>
> deactivate_locked_super(sb);
>
> @@ -418,7 +418,7 @@ static int ioctl_fsthaw(struct file *filp)
> /* Thaw */
> if (sb->s_op->thaw_super)
> return sb->s_op->thaw_super(sb);
> - return thaw_super(sb);
> + return thaw_super(sb, true);
> }
>
> static int ioctl_file_dedupe_range(struct file *file,
> diff --git a/fs/super.c b/fs/super.c
> index 46c6475fc765..16ccbb9dd230 100644
> --- a/fs/super.c
> +++ b/fs/super.c
> @@ -1026,7 +1026,7 @@ static void do_thaw_all_callback(struct super_block *sb)
> return;
> if (sb->s_root && sb->s_flags & SB_BORN) {
> emergency_thaw_bdev(sb);
> - thaw_super(sb);
> + thaw_super(sb, true);
> }
> deactivate_locked_super(sb);
> }
> @@ -1636,6 +1636,8 @@ static void sb_freeze_unlock(struct super_block *sb, int level)
> /**
> * freeze_super - force a filesystem backed by a block device into a consistent state
> * @sb: the super to lock
> + * @usercall: whether or not userspace initiated this via an ioctl or if it
> + * was a kernel freeze
> *
> * Used by filesystems and the kernel to freeze a fileystem backed by a block
> * device into a consistent state. Callers must use get_active_super(bdev) to
> @@ -1669,10 +1671,13 @@ static void sb_freeze_unlock(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, bool usercall)
> {
> int ret;
>
> + if (!usercall && sb_is_frozen(sb))
> + return 0;

If we try a kernel freeze but the fs was already frozen by userspace, we
return ... zero?

What if userspace thaws the fs immediately afterwards? The kernel
caller is still running, and now it erroneously thinks the fs is frozen.
Won't that break the suspend freezer?

TBH I was more thinking about fscounters scrub, which will report false
corruptions if the fs gets unfrozen while it is running.

--D

> +
> if (!sb_is_unfrozen(sb))
> return -EBUSY;
>
> @@ -1682,6 +1687,7 @@ int freeze_super(struct super_block *sb)
> if (sb_rdonly(sb)) {
> /* Nothing to do really... */
> sb->s_writers.frozen = SB_FREEZE_COMPLETE;
> + sb->s_writers.frozen_by_user = usercall;
> return 0;
> }
>
> @@ -1699,6 +1705,7 @@ int freeze_super(struct super_block *sb)
> ret = sync_filesystem(sb);
> if (ret) {
> sb->s_writers.frozen = SB_UNFROZEN;
> + sb->s_writers.frozen_by_user = false;
> sb_freeze_unlock(sb, SB_FREEZE_PAGEFAULT);
> wake_up(&sb->s_writers.wait_unfrozen);
> return ret;
> @@ -1724,6 +1731,7 @@ int freeze_super(struct super_block *sb)
> * when frozen is set to SB_FREEZE_COMPLETE, and for thaw_super().
> */
> sb->s_writers.frozen = SB_FREEZE_COMPLETE;
> + sb->s_writers.frozen_by_user = usercall;
> lockdep_sb_freeze_release(sb);
> return 0;
> }
> @@ -1732,21 +1740,33 @@ EXPORT_SYMBOL(freeze_super);
> /**
> * thaw_super -- unlock a filesystem backed by a block device
> * @sb: the super to thaw
> + * @usercall: whether or not userspace initiated this thaw or if it was the
> + * kernel which initiated it
> *
> * Used by filesystems and the kernel to thaw a fileystem backed by a block
> * device. Callers must use get_active_super(bdev) to lock the @sb and when
> * done must unlock it with deactivate_locked_super(). Once done, this marks
> * the filesystem as writeable.
> */
> -int thaw_super(struct super_block *sb)
> +int thaw_super(struct super_block *sb, bool usercall)
> {
> int error;
>
> - if (sb->s_writers.frozen != SB_FREEZE_COMPLETE)
> + if (!usercall) {
> + /*
> + * If userspace initiated the freeze don't let the kernel
> + * thaw it on return from a kernel initiated freeze.
> + */
> + if (sb_is_unfrozen(sb) || sb_is_frozen_by_user(sb))
> + return 0;
> + }
> +
> + if (!sb_is_frozen(sb))
> return -EINVAL;
>
> if (sb_rdonly(sb)) {
> sb->s_writers.frozen = SB_UNFROZEN;
> + sb->s_writers.frozen_by_user = false;
> goto out;
> }
>
> @@ -1763,6 +1783,7 @@ int thaw_super(struct super_block *sb)
> }
>
> sb->s_writers.frozen = SB_UNFROZEN;
> + sb->s_writers.frozen_by_user = false;
> sb_freeze_unlock(sb, SB_FREEZE_FS);
> out:
> wake_up(&sb->s_writers.wait_unfrozen);
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 90b5bdc4071a..d9b46c858103 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -1146,6 +1146,7 @@ enum {
>
> struct sb_writers {
> int frozen; /* Is sb frozen? */
> + bool frozen_by_user; /* User freeze? */
> wait_queue_head_t wait_unfrozen; /* wait for thaw */
> struct percpu_rw_semaphore rw_sem[SB_FREEZE_LEVELS];
> };
> @@ -1632,6 +1633,17 @@ static inline bool sb_is_frozen(struct super_block *sb)
> return sb->s_writers.frozen == SB_FREEZE_COMPLETE;
> }
>
> +/**
> + * sb_is_frozen_by_user - was the superblock frozen by userspace?
> + * @sb: the super to check
> + *
> + * Returns true if the super is frozen by userspace, such as an ioctl.
> + */
> +static inline bool sb_is_frozen_by_user(struct super_block *sb)
> +{
> + return sb_is_frozen(sb) && sb->s_writers.frozen_by_user;
> +}
> +
> /**
> * sb_is_unfrozen - is superblock unfrozen
> * @sb: the super to check
> @@ -2308,8 +2320,8 @@ extern int unregister_filesystem(struct file_system_type *);
> extern int vfs_statfs(const struct path *, struct kstatfs *);
> extern int user_statfs(const char __user *, struct kstatfs *);
> extern int fd_statfs(int, 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, bool usercall);
> +extern int thaw_super(struct super_block *super, bool usercall);
> extern __printf(2, 3)
> int super_setup_bdi_name(struct super_block *sb, char *fmt, ...);
> extern int super_setup_bdi(struct super_block *sb);
> --
> 2.39.2
>

2023-05-23 01:13:24

by Darrick J. Wong

[permalink] [raw]

Subject: Re: [PATCH 3/6] fs: distinguish between user initiated freeze and kernel initiated freeze

How about this as an alternative patch? Kernel and userspace freeze
state are stored in s_writers; each type cannot block the other (though
you still can't have nested kernel or userspace freezes); and the freeze
is maintained until /both/ freeze types are dropped.

AFAICT this should work for the two other usecases (quiescing pagefaults
for fsdax pmem pre-removal; and freezing fses during suspend) besides
online fsck for xfs.

--D

From: Darrick J. Wong <[email protected]>
Subject: fs: distinguish between user initiated freeze and kernel initiated freeze

Userspace can freeze a filesystem using the FIFREEZE ioctl or by
suspending the block device; this state persists until userspace thaws
the filesystem with the FITHAW ioctl or resuming the block device.
Since commit 18e9e5104fcd ("Introduce freeze_super and thaw_super for
the fsfreeze ioctl") we only allow the first freeze command to succeed.

The kernel may decide that it is necessary to freeze a filesystem for
its own internal purposes, such as suspends in progress, filesystem fsck
activities, or quiescing a device prior to removal. Userspace thaw
commands must never break a kernel freeze, and kernel thaw commands
shouldn't undo userspace's freeze command.

Introduce a couple of freeze holder flags and wire it into the
sb_writers state. One kernel and one userspace freeze are allowed to
coexist at the same time; the filesystem will not thaw until both are
lifted.

Inspired-by: Luis Chamberlain <[email protected]>
Signed-off-by: Darrick J. Wong <[email protected]>
---
fs/super.c | 172 +++++++++++++++++++++++++++++++++++++++++++++++++---
include/linux/fs.h | 8 ++
2 files changed, 170 insertions(+), 10 deletions(-)

diff --git a/fs/super.c b/fs/super.c
index 34afe411cf2b..7496d51affb9 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -39,7 +39,7 @@
#include <uapi/linux/mount.h>
#include "internal.h"

-static int thaw_super_locked(struct super_block *sb);
+static int thaw_super_locked(struct super_block *sb, unsigned short who);

static LIST_HEAD(super_blocks);
static DEFINE_SPINLOCK(sb_lock);
@@ -1027,7 +1027,7 @@ static void do_thaw_all_callback(struct super_block *sb)
down_write(&sb->s_umount);
if (sb->s_root && sb->s_flags & SB_BORN) {
emergency_thaw_bdev(sb);
- thaw_super_locked(sb);
+ thaw_super_locked(sb, FREEZE_HOLDER_USERSPACE);
} else {
up_write(&sb->s_umount);
}
@@ -1636,13 +1636,21 @@ static void sb_freeze_unlock(struct super_block *sb, int level)
}

/**
- * freeze_super - lock the filesystem and force it into a consistent state
+ * __freeze_super - lock the filesystem and force it into a consistent state
* @sb: the super to lock
+ * @who: FREEZE_HOLDER_USERSPACE if userspace wants to freeze the fs;
+ * FREEZE_HOLDER_KERNEL if the kernel wants to freeze it
*
* 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
+ * freeze_fs. Subsequent calls to this without first thawing the fs may return
* -EBUSY.
*
+ * The @who argument distinguishes between the kernel and userspace trying to
+ * freeze the filesystem. Although there cannot be multiple kernel freezes or
+ * multiple userspace freezes in effect at any given time, the kernel and
+ * userspace can both hold a filesystem frozen. The filesystem remains frozen
+ * until there are no kernel or userspace freezes in effect.
+ *
* During this function, sb->s_writers.frozen goes through these values:
*
* SB_UNFROZEN: File system is normal, all writes progress as usual.
@@ -1668,12 +1676,61 @@ static void sb_freeze_unlock(struct super_block *sb, int level)
*
* sb->s_writers.frozen is protected by sb->s_umount.
*/
-int freeze_super(struct super_block *sb)
+static int __freeze_super(struct super_block *sb, unsigned short who)
{
+ struct sb_writers *sbw = &sb->s_writers;
int ret;

atomic_inc(&sb->s_active);
down_write(&sb->s_umount);
+
+ if (sbw->frozen == SB_FREEZE_COMPLETE) {
+ switch (who) {
+ case FREEZE_HOLDER_KERNEL:
+ if (sbw->freeze_holders & FREEZE_HOLDER_KERNEL) {
+ /*
+ * Kernel freeze already in effect; caller can
+ * try again.
+ */
+ deactivate_locked_super(sb);
+ return -EBUSY;
+ }
+ if (sbw->freeze_holders & FREEZE_HOLDER_USERSPACE) {
+ /*
+ * Share the freeze state with the userspace
+ * freeze already in effect.
+ */
+ sbw->freeze_holders |= who;
+ deactivate_locked_super(sb);
+ return 0;
+ }
+ break;
+ case FREEZE_HOLDER_USERSPACE:
+ if (sbw->freeze_holders & FREEZE_HOLDER_USERSPACE) {
+ /*
+ * Userspace freeze already in effect; tell
+ * the caller we're busy.
+ */
+ deactivate_locked_super(sb);
+ return -EBUSY;
+ }
+ if (sbw->freeze_holders & FREEZE_HOLDER_KERNEL) {
+ /*
+ * Share the freeze state with the kernel
+ * freeze already in effect.
+ */
+ sbw->freeze_holders |= who;
+ deactivate_locked_super(sb);
+ return 0;
+ }
+ break;
+ default:
+ BUG();
+ deactivate_locked_super(sb);
+ return -EINVAL;
+ }
+ }
+
if (sb->s_writers.frozen != SB_UNFROZEN) {
deactivate_locked_super(sb);
return -EBUSY;
@@ -1686,6 +1743,7 @@ int freeze_super(struct super_block *sb)

if (sb_rdonly(sb)) {
/* Nothing to do really... */
+ sb->s_writers.freeze_holders |= who;
sb->s_writers.frozen = SB_FREEZE_COMPLETE;
up_write(&sb->s_umount);
return 0;
@@ -1731,23 +1789,103 @@ int freeze_super(struct super_block *sb)
* For debugging purposes so that fs can warn if it sees write activity
* when frozen is set to SB_FREEZE_COMPLETE, and for thaw_super().
*/
+ sb->s_writers.freeze_holders |= who;
sb->s_writers.frozen = SB_FREEZE_COMPLETE;
lockdep_sb_freeze_release(sb);
up_write(&sb->s_umount);
return 0;
}
+
+/*
+ * freeze_super - lock the filesystem and force it into a consistent state
+ * @sb: the super to lock
+ *
+ * Syncs the super to make sure the filesystem is consistent and calls the fs's
+ * freeze_fs. Subsequent calls to this without first calling thaw_super will
+ * return -EBUSY. See the comment for __freeze_super for more information.
+ */
+int freeze_super(struct super_block *sb)
+{
+ return __freeze_super(sb, FREEZE_HOLDER_USERSPACE);
+}
EXPORT_SYMBOL(freeze_super);

-static int thaw_super_locked(struct super_block *sb)
+/**
+ * freeze_super_kernel - lock the filesystem for an internal kernel operation
+ * and force it into a consistent state.
+ * @sb: the super to lock
+ *
+ * Syncs the super to make sure the filesystem is consistent and calls the fs's
+ * freeze_fs. Subsequent calls to this without first calling thaw_super_excl
+ * will return -EBUSY.
+ */
+int freeze_super_kernel(struct super_block *sb)
{
+ return __freeze_super(sb, FREEZE_HOLDER_KERNEL);
+}
+EXPORT_SYMBOL_GPL(freeze_super_kernel);
+
+/*
+ * Undoes the effect of a freeze_super_locked call. If the filesystem is
+ * frozen both by userspace and the kernel, a thaw call from either source
+ * removes that state without releasing the other state or unlocking the
+ * filesystem.
+ */
+static int thaw_super_locked(struct super_block *sb, unsigned short who)
+{
+ struct sb_writers *sbw = &sb->s_writers;
int error;

+ if (sbw->frozen == SB_FREEZE_COMPLETE) {
+ switch (who) {
+ case FREEZE_HOLDER_KERNEL:
+ if (!(sbw->freeze_holders & FREEZE_HOLDER_KERNEL)) {
+ /* Caller doesn't hold a kernel freeze. */
+ up_write(&sb->s_umount);
+ return -EINVAL;
+ }
+ if (sbw->freeze_holders & FREEZE_HOLDER_USERSPACE) {
+ /*
+ * We were sharing the freeze with userspace,
+ * so drop the userspace freeze but exit
+ * without unfreezing.
+ */
+ sbw->freeze_holders &= ~who;
+ up_write(&sb->s_umount);
+ return 0;
+ }
+ break;
+ case FREEZE_HOLDER_USERSPACE:
+ if (!(sbw->freeze_holders & FREEZE_HOLDER_USERSPACE)) {
+ /* Caller doesn't hold a userspace freeze. */
+ up_write(&sb->s_umount);
+ return -EINVAL;
+ }
+ if (sbw->freeze_holders & FREEZE_HOLDER_KERNEL) {
+ /*
+ * We were sharing the freeze with the kernel,
+ * so drop the kernel freeze but exit without
+ * unfreezing.
+ */
+ sbw->freeze_holders &= ~who;
+ up_write(&sb->s_umount);
+ return 0;
+ }
+ break;
+ default:
+ BUG();
+ up_write(&sb->s_umount);
+ return -EINVAL;
+ }
+ }
+
if (sb->s_writers.frozen != SB_FREEZE_COMPLETE) {
up_write(&sb->s_umount);
return -EINVAL;
}

if (sb_rdonly(sb)) {
+ sb->s_writers.freeze_holders &= ~who;
sb->s_writers.frozen = SB_UNFROZEN;
goto out;
}
@@ -1765,6 +1903,7 @@ static int thaw_super_locked(struct super_block *sb)
}
}

+ sb->s_writers.freeze_holders &= ~who;
sb->s_writers.frozen = SB_UNFROZEN;
sb_freeze_unlock(sb, SB_FREEZE_FS);
out:
@@ -1774,18 +1913,33 @@ static int thaw_super_locked(struct super_block *sb)
}

/**
- * thaw_super -- unlock filesystem
+ * thaw_super -- unlock filesystem frozen with freeze_super
* @sb: the super to thaw
*
- * Unlocks the filesystem and marks it writeable again after freeze_super().
+ * Unlocks the filesystem after freeze_super, and make it writeable again if
+ * there is not a freeze_super_kernel still in effect.
*/
int thaw_super(struct super_block *sb)
{
down_write(&sb->s_umount);
- return thaw_super_locked(sb);
+ return thaw_super_locked(sb, FREEZE_HOLDER_USERSPACE);
}
EXPORT_SYMBOL(thaw_super);

+/**
+ * thaw_super_kernel -- unlock filesystem frozen with freeze_super_kernel
+ * @sb: the super to thaw
+ *
+ * Unlocks the filesystem after freeze_super_kernel, and make it writeable
+ * again if there is not a freeze_super still in effect.
+ */
+int thaw_super_kernel(struct super_block *sb)
+{
+ down_write(&sb->s_umount);
+ return thaw_super_locked(sb, FREEZE_HOLDER_KERNEL);
+}
+EXPORT_SYMBOL_GPL(thaw_super_kernel);
+
/*
* Create workqueue for deferred direct IO completions. We allocate the
* workqueue when it's first needed. This avoids creating workqueue for
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 21a981680856..147644b5d648 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1145,11 +1145,15 @@ enum {
#define SB_FREEZE_LEVELS (SB_FREEZE_COMPLETE - 1)

struct sb_writers {
- int frozen; /* Is sb frozen? */
+ unsigned short frozen; /* Is sb frozen? */
+ unsigned short freeze_holders; /* Who froze fs? */
wait_queue_head_t wait_unfrozen; /* wait for thaw */
struct percpu_rw_semaphore rw_sem[SB_FREEZE_LEVELS];
};

+#define FREEZE_HOLDER_USERSPACE (1U << 1) /* userspace froze fs */
+#define FREEZE_HOLDER_KERNEL (1U << 2) /* kernel froze fs */
+
struct super_block {
struct list_head s_list; /* Keep this first */
dev_t s_dev; /* search index; _not_ kdev_t */
@@ -2288,6 +2292,8 @@ extern int user_statfs(const char __user *, struct kstatfs *);
extern int fd_statfs(int, struct kstatfs *);
extern int freeze_super(struct super_block *super);
extern int thaw_super(struct super_block *super);
+extern int freeze_super_kernel(struct super_block *super);
+extern int thaw_super_kernel(struct super_block *super);
extern __printf(2, 3)
int super_setup_bdi_name(struct super_block *sb, char *fmt, ...);
extern int super_setup_bdi(struct super_block *sb);

2023-05-25 14:36:01

On Tue, Jun 06, 2023 at 10:19:56AM -0700, Darrick J. Wong wrote:
> Luis hasn't responded to my question, so I stopped.

Sorry for the delay, by all means alternatives are hugely
welcomed. I just worked on it as it was back logged work and
not a high priority thing for me, so I try to get to it when I
can. Having someone with more immediate needs give it a stab
is hugely appreciated!

Luis

2023-06-08 22:11:13

by Darrick J. Wong

[permalink] [raw]

Subject: Re: [PATCH 3/6] fs: distinguish between user initiated freeze and kernel initiated freeze

On Thu, Jun 08, 2023 at 01:26:19PM -0700, Luis Chamberlain wrote:
> On Mon, May 22, 2023 at 04:42:00PM -0700, Darrick J. Wong wrote:
> > How about this as an alternative patch?
>
> I'm all for it, this is low hanging fruit and I try to get back to it
> as no one else does, so I'm glad someone else is looking and trying too!
>
> Hopefully dropping patch 1 and 2 would help with this.
>
> Comments below.
>
> > From: Darrick J. Wong <[email protected]>
> > Subject: fs: distinguish between user initiated freeze and kernel initiated freeze
> >
> > Userspace can freeze a filesystem using the FIFREEZE ioctl or by
> > suspending the block device; this state persists until userspace thaws
> > the filesystem with the FITHAW ioctl or resuming the block device.
> > Since commit 18e9e5104fcd ("Introduce freeze_super and thaw_super for
> > the fsfreeze ioctl") we only allow the first freeze command to succeed.
> >
> > The kernel may decide that it is necessary to freeze a filesystem for
> > its own internal purposes, such as suspends in progress, filesystem fsck
> > activities, or quiescing a device prior to removal. Userspace thaw
> > commands must never break a kernel freeze, and kernel thaw commands
> > shouldn't undo userspace's freeze command.
> >
> > Introduce a couple of freeze holder flags and wire it into the
> > sb_writers state. One kernel and one userspace freeze are allowed to
> > coexist at the same time; the filesystem will not thaw until both are
> > lifted.
>
> This mix-match stuff is also important to document so we can get
> userspace to understand what is allowed and we get a sense of direction
> written / documented. Without this trying to navigate around this is
> all implied. We may need to adjust things with time for thing we may
> not have considered.

That's captured in the kernledoc for freeze_super, which is no longer
getting cut up into __freeze_super here.

> > -int freeze_super(struct super_block *sb)
> > +static int __freeze_super(struct super_block *sb, unsigned short who)
> > {
> > + struct sb_writers *sbw = &sb->s_writers;
> > int ret;
> >
> > atomic_inc(&sb->s_active);
> > down_write(&sb->s_umount);
> > +
> > + if (sbw->frozen == SB_FREEZE_COMPLETE) {
> > + switch (who) {
>
> <-- snip -->
>
> > + case FREEZE_HOLDER_USERSPACE:
> > + if (sbw->freeze_holders & FREEZE_HOLDER_USERSPACE) {
> > + /*
> > + * Userspace freeze already in effect; tell
> > + * the caller we're busy.
> > + */
> > + deactivate_locked_super(sb);
> > + return -EBUSY;
>
> I'm thinking some userspace might find this OK so thought maybe
> something like -EALREADY would be better, to then allow userspace
> to decide, however, since userspace would not control the thaw it
> seems like risky business to support that.

It already has to, since we've been returning EBUSY for "fs already
frozen or being frozen" for years.

--D

> Anyway, I'm all for any alternative!
>
> Luis