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.
> -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.
Anyway, I'm all for any alternative!
Luis
