On Sun, Jun 11, 2023 at 08:15:22PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@xxxxxxxxxx>
>
> 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.
>
> I wonder if the f2fs/gfs2 code should be using a kernel freeze here, but
> for now we'll use FREEZE_HOLDER_USERSPACE to preserve existing
> behaviors.
>
> Cc: mcgrof@xxxxxxxxxx
> Cc: jack@xxxxxxx
> Cc: hch@xxxxxxxxxxxxx
> Cc: ruansy.fnst@xxxxxxxxxxx
> Signed-off-by: Darrick J. Wong <djwong@xxxxxxxxxx>
> ---
> Documentation/filesystems/vfs.rst | 4 +-
> block/bdev.c | 8 ++--
> fs/f2fs/gc.c | 4 +-
> fs/gfs2/glops.c | 2 -
> fs/gfs2/super.c | 6 +--
> fs/gfs2/sys.c | 4 +-
> fs/gfs2/util.c | 2 -
> fs/ioctl.c | 8 ++--
> fs/super.c | 79 +++++++++++++++++++++++++++++++++----
> include/linux/fs.h | 15 +++++--
> 10 files changed, 100 insertions(+), 32 deletions(-)
>
>
> diff --git a/Documentation/filesystems/vfs.rst b/Documentation/filesystems/vfs.rst
> index 769be5230210..41cf2a56cbca 100644
> --- a/Documentation/filesystems/vfs.rst
> +++ b/Documentation/filesystems/vfs.rst
> @@ -260,9 +260,9 @@ filesystem. The following members are defined:
> void (*evict_inode) (struct inode *);
> void (*put_super) (struct super_block *);
> int (*sync_fs)(struct super_block *sb, int wait);
> - int (*freeze_super) (struct super_block *);
> + int (*freeze_super) (struct super_block *, enum freeze_holder who);
> int (*freeze_fs) (struct super_block *);
> - int (*thaw_super) (struct super_block *);
> + int (*thaw_super) (struct super_block *, enum freeze_wholder who);
Nit: Can you spell out the sb paramter as well and avoid the overly long
lines here?
> +static int freeze_frozen_super(struct super_block *sb, enum freeze_holder who)
> +{
> + /* Someone else already holds this type of freeze */
> + if (sb->s_writers.freeze_holders & who)
> + return -EBUSY;
> +
> + WARN_ON(sb->s_writers.freeze_holders == 0);
> +
> + sb->s_writers.freeze_holders |= who;
> + return 0;
> +}
So with the simplification I'm not even sure we need this helper
anymore. But I could live with it either way.
> /**
> * 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
I think the cnonstants should use a % prefix for kerneldoc to notice
them. Also I suspect something like:
* @who: context that wants to free
and then in the body:
* @who should be:
* * %FREEZE_HOLDER_USERSPACE if userspace wants to freeze the fs
* * %FREEZE_HOLDER_KERNEL if the kernel wants to freeze it
for better rendering of the comments. Same applies for the thaw side.
> +static int thaw_super_locked(struct super_block *sb, enum freeze_holder who)
> {
> int error;
>
> + if (sb->s_writers.frozen == SB_FREEZE_COMPLETE) {
> + error = try_thaw_shared_super(sb, who);
> + if (error != 1) {
> + up_write(&sb->s_umount);
> + return error;
> + }
> + }
> +
> if (sb->s_writers.frozen != SB_FREEZE_COMPLETE) {
Make this and
} else {
instead of checking the same condition twice?
> +extern int freeze_super(struct super_block *super, enum freeze_holder who);
> +extern int thaw_super(struct super_block *super, enum freeze_holder who);
.. and drop the pointless externs here.
Except for these various nitpicks this looks good:
Reviewed-by: Christoph Hellwig <hch@xxxxxx>
