On Thu, Jun 08, 2023 at 01:26:19PM -0700, Luis Chamberlain wrote:
> On Mon, May 22, 2023 at 04:42:00PM -0700, Darrick J. Wong wrote:
> > How about this as an alternative patch?
>
> I'm all for it, this is low hanging fruit and I try to get back to it
> as no one else does, so I'm glad someone else is looking and trying too!
>
> Hopefully dropping patch 1 and 2 would help with this.
>
> Comments below.
>
> > From: Darrick J. Wong <djwong@xxxxxxxxxx>
> > Subject: fs: distinguish between user initiated freeze and kernel initiated freeze
> >
> > 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.
>
> This mix-match stuff is also important to document so we can get
> userspace to understand what is allowed and we get a sense of direction
> written / documented. Without this trying to navigate around this is
> all implied. We may need to adjust things with time for thing we may
> not have considered.
That's captured in the kernledoc for freeze_super, which is no longer
getting cut up into __freeze_super here.
> > -int freeze_super(struct super_block *sb)
> > +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) {
>
> <-- snip -->
>
> > + case FREEZE_HOLDER_USERSPACE:
> > + if (sbw->freeze_holders & FREEZE_HOLDER_USERSPACE) {
> > + /*
> > + * Userspace freeze already in effect; tell
> > + * the caller we're busy.
> > + */
> > + deactivate_locked_super(sb);
> > + return -EBUSY;
>
> I'm thinking some userspace might find this OK so thought maybe
> something like -EALREADY would be better, to then allow userspace
> to decide, however, since userspace would not control the thaw it
> seems like risky business to support that.
It already has to, since we've been returning EBUSY for "fs already
frozen or being frozen" for years.
--D
> Anyway, I'm all for any alternative!
>
> Luis
