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

[Christoph Hellwig]

On Mon, May 22, 2023 at 04:42:00PM -0700, Darrick J. Wong wrote:
> 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.

I think this is fundamentally the right thing.  Can you send this as
a standalone thread in a separate thread to make it sure it gets
expedited?

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

Is the unsigned short really worth it?  Even if it's just two values
I'd also go for a __bitwise type to get the typechecking as that tends
to help a lot goind down the road.

>  /**
> - * 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.
>   */

There's really no point in having a kerneldoc for a static function.
Either this moves to the actual exported functions, or it should
become a normal non-kerneldoc comment.  But I'm not even sre this split
makes much sense to start with.  I'd just add a the who argument
to freeze_super given that we have only very few callers anyway,
and it is way easier to follow than thse rappers hardoding the argument.

> +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) {

Nit, but maybe split evetything inside this branch into a
freeze_frozen_super helper?

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

To me this screams for another 'is_partial_thaw' or so helper, whith
which we could doe something like:

	if (sbw->frozen != SB_FREEZE_COMPLETE) {
		ret = -EINVAL;
		goto out_unlock;
	}
	ret = is_partial_thaw(sb, who);
	if (ret) {
		if (ret == 1) {
			sbw->freeze_holders &= ~who;
			ret = 0
		}
		goto out_unlock;
	}

Btw, same comment about the wrappers as on the freeze side.