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.
_______________________________________________
kexec mailing list
kexec@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/kexec
