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

Christoph Hellwig
2023-06-12T03:58:52Z

On Sun, Jun 11, 2023 at 08:15:22PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
>
> 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@kernel.org
> Cc: jack@suse.cz
> Cc: hch@infradead.org
> Cc: ruansy.fnst@fujitsu.com
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
> 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@lst.de>