Re: [PATCH 1/4] vfs: allow filesystem freeze callers to denote who froze the fs

[Luis Chamberlain <mcgrof@kernel.org>]

On Tue, May 02, 2023 at 08:02:18PM -0700, Darrick J. Wong wrote:
> diff --git a/fs/super.c b/fs/super.c
> index 04bc62ab7dfe..01891f9e6d5e 100644
> --- a/fs/super.c
> +++ b/fs/super.c
> @@ -1736,18 +1747,33 @@ int freeze_super(struct super_block *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 thawing the fs will return
> + * -EBUSY.  See the comment for __freeze_super for more information.
> + */
> +int freeze_super(struct super_block *sb)
> +{
> +	return __freeze_super(sb, USERSPACE_FREEZE_COOKIE);
> +}
>  EXPORT_SYMBOL(freeze_super);
>  
> -static int thaw_super_locked(struct super_block *sb)
> +static int thaw_super_locked(struct super_block *sb, unsigned long cookie)
>  {
>  	int error;
>  
> -	if (sb->s_writers.frozen != SB_FREEZE_COMPLETE) {
> +	if (sb->s_writers.frozen != SB_FREEZE_COMPLETE ||
> +	    sb->s_writers.freeze_cookie != cookie) {
>  		up_write(&sb->s_umount);
>  		return -EINVAL;

We get the same by just having drivers use freeze_super(sb, true) in the
patches I have, ie, we treat it a user-initiated.

On freeze() we have:

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;
	...
}

On thaw we end up with:

int thaw_super(struct super_block *sb, bool usercall)
{
	int error;

	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;
	...
}

As I had it, I had made the drivers and the bdev freeze use the usercall as
true and so there is no change.

In case there is a filesystem already frozen then which was initiated by
the filesystem, for whatever reason, the filesystem the kernel auto-freeze
will chug on happy with the system freeze, it bails out withour error
and moves on to the next filesystem to freeze.

Upon thaw, the kernel auto-thaw will detect that the filesystem was
frozen by user on sb_is_frozen_by_user() and so will just bail and not
thaw it.

If the mechanism you want to introduce is to allow a filesystem to even
prevent kernel auto-freeze with -EBUSY it begs the question if that
shouldn't also prevent suspend. Because it would anyway as you have it
right now with your patch but it would return -EINVAL. I also ask because of
the possible issues with the filesystem not going to suspend but the backing
or other possible related devices going to suspend.

Since I think the goal is to prevent the kernel auto-freeze due to
online fsck to complete, then I think you *do* want to prevent full
system suspend from moving forward. In that case, why not just have
the filesystem check for that and return -EBUSY on its respective
filesystem sb->s_op->freeze_fs(sb) callback?

  Luis