On Wed, Jun 07, 2023 at 10:24:32PM -0700, Christoph Hellwig wrote:
> 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?
Yeah, I'll do that.
> > -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.
Instead of __bitwise, I'll make freeze_super() take an enum, since
callers can only initiate one at a time, and the compiler can (in
theory) catch people passing garbage inputs.
> > /**
> > - * 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.
Agreed.
> > +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?
Yes, will do. I think Jan's simplification will condense this too.
> > +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;
> }
<nod>
> Btw, same comment about the wrappers as on the freeze side.
<nod>
--D
