Re: [RFC 3/6] fs: add automatic kernel fs freeze / thaw and remove kthread freezing

[Christian Brauner]

On Wed, Mar 26, 2025 at 07:53:10AM -0400, James Bottomley wrote:
> On Wed, 2025-03-26 at 04:22 -0700, Luis Chamberlain wrote:
> > Add support to automatically handle freezing and thawing filesystems
> > during the kernel's suspend/resume cycle.
> > 
> > This is needed so that we properly really stop IO in flight without
> > races after userspace has been frozen. Without this we rely on
> > kthread freezing and its semantics are loose and error prone.
> > For instance, even though a kthread may use try_to_freeze() and end
> > up being frozen we have no way of being sure that everything that
> > has been spawned asynchronously from it (such as timers) have also
> > been stopped as well.
> > 
> > A long term advantage of also adding filesystem freeze / thawing
> > supporting during suspend / hibernation is that long term we may
> > be able to eventually drop the kernel's thread freezing completely
> > as it was originally added to stop disk IO in flight as we hibernate
> > or suspend.
> > 
> > This does not remove the superfluous freezer calls on all
> > filesystems.
> > Each filesystem must remove all the kthread freezer stuff and peg
> > the fs_type flags as supporting auto-freezing with the FS_AUTOFREEZE
> > flag.
> > 
> > Subsequent patches remove the kthread freezer usage from each
> > filesystem, one at a time to make all this work bisectable.
> > Once all filesystems remove the usage of the kthread freezer we
> > can remove the FS_AUTOFREEZE flag.
> > 
> > Reviewed-by: Jan Kara <jack@xxxxxxx>
> > Signed-off-by: Luis Chamberlain <mcgrof@xxxxxxxxxx>
> > ---
> >  fs/super.c             | 50
> > ++++++++++++++++++++++++++++++++++++++++++
> >  include/linux/fs.h     | 14 ++++++++++++
> >  kernel/power/process.c | 15 ++++++++++++-
> >  3 files changed, 78 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/super.c b/fs/super.c
> > index 9995546cf159..7428f0b2251c 100644
> > --- a/fs/super.c
> > +++ b/fs/super.c
> > @@ -2279,3 +2279,53 @@ int sb_init_dio_done_wq(struct super_block
> > *sb)
> >  	return 0;
> >  }
> >  EXPORT_SYMBOL_GPL(sb_init_dio_done_wq);
> > +
> > +#ifdef CONFIG_PM_SLEEP
> > +static bool super_should_freeze(struct super_block *sb)
> > +{
> > +	if (!(sb->s_type->fs_flags & FS_AUTOFREEZE))
> > +		return false;
> > +	/*
> > +	 * We don't freeze virtual filesystems, we skip those
> > filesystems with
> > +	 * no backing device.
> > +	 */
> > +	if (sb->s_bdi == &noop_backing_dev_info)
> > +		return false;
> 
> 
> This logic won't work for me because efivarfs is a pseudofilesystem and
> will have a noop bdi (or simply a null s_bdev, which is easier to check
> for).  I was thinking of allowing freeze/thaw to continue for a s_bdev
> == NULL filesystem if it provided a freeze or thaw callback, which will
> cover efivarfs.

Filesystem freezing isn't dependent on backing devices. I'm not sure
where that impression comes from. The FS_AUTOFREEZE shouldn't be
necessary once all filesystems have been fixed up (which I guess this is
about). The logic should just be similar to what we do for the freeze
ioctl.

IOW, we skip filesystems without any freeze method. That excludes any fs
that isn't prepared to be frozen:

The easiest way is very likely to give efivarfs a ->freeze_super() and
->thaw_super() method since it likely doesn't all of the fanciness that
freeze_super() adds.

Then we have two approaches:

(1) Change the iterator to take a reference while holding the super_lock() and
    then calling a helper to freeze the fs.
(2) Pass the information that s_umount is held down to the freeze methods.

For example (2) would be something like:

diff --git a/include/linux/fs.h b/include/linux/fs.h
index be3ad155ec9f..7ad515ad6934 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2272,6 +2272,7 @@ enum freeze_holder {
        FREEZE_HOLDER_KERNEL    = (1U << 0),
        FREEZE_HOLDER_USERSPACE = (1U << 1),
        FREEZE_MAY_NEST         = (1U << 2),
+       FREEZE_SUPER_LOCKED     = (1U << 3),
 };

 struct super_operations {

static int freeze_super_locked(struct file *filp)
{
	/* If filesystem doesn't support freeze feature, return. */
	if (sb->s_op->freeze_fs == NULL && sb->s_op->freeze_super == NULL)
		return 0;

	if (sb->s_op->freeze_super)
		return sb->s_op->freeze_super(sb, FREEZE_HOLDER_KERNEL | FREEZE_SUPER_LOCKED);
	return freeze_super(sb, FREEZE_HOLDER_KERNEL | FREEZE_SUPER_LOCKED);
}

Why do you care about efivarfs taking part in system suspend though?

> 
> > +
> > +	return true;
> > +}
> > +
> > +int fs_suspend_freeze_sb(struct super_block *sb, void *priv)
> > +{
> > +	int error = 0;
> > +
> > +	if (!super_should_freeze(sb))
> > +		goto out;
> > +
> > +	pr_info("%s (%s): freezing\n", sb->s_type->name, sb->s_id);
> > +
> > +	error = freeze_super(sb, false);
> 
> This is actually not wholly correct now.  If the fs provides a sb-
> >freeze() method, you should use that instead of freeze_super() ... see
> how fs_bdev_freeze() is doing it.

> 
> Additionally, the first thing freeze_super() does is take the
> superblock lock exclusively.  Since you've already taken it exclusively
> in your iterate super, how does this not deadlock?

It will deadlock.