Re: [PATCH 2/2] efivarfs: support freeze/thaw

[Christian Brauner <brauner@kernel.org>]

On Mon, Mar 31, 2025 at 10:46:43AM -0400, James Bottomley wrote:
> On Mon, 2025-03-31 at 14:42 +0200, Christian Brauner wrote:
> > Allow efivarfs to partake to resync variable state during system
> > hibernation and suspend. Add freeze/thaw support.
> > 
> > This is a pretty straightforward implementation. We simply add
> > regular freeze/thaw support for both userspace and the kernel. This
> > works without any big issues and congrats afaict efivars is the first
> > pseudofilesystem that adds support for filesystem freezing and
> > thawing.
> > 
> > The simplicity comes from the fact that we simply always resync
> > variable state after efivarfs has been frozen. It doesn't matter
> > whether that's because of suspend, userspace initiated freeze or
> > hibernation. Efivars is simple enough that it doesn't matter that we
> > walk all dentries. There are no directories and there aren't insane
> > amounts of entries and both freeze/thaw are already heavy-handed
> > operations. We really really don't need to care.
> 
> Just as a point of order: this can't actually work until freeze/thaw is
> actually plumbed into suspend/resume.
> 
> > 
> > Signed-off-by: Christian Brauner <brauner@kernel.org>
> > ---
> >  fs/efivarfs/internal.h |   1 -
> >  fs/efivarfs/super.c    | 196 +++++++++++++--------------------------
> > ----------
> >  2 files changed, 51 insertions(+), 146 deletions(-)
> > 
> > diff --git a/fs/efivarfs/internal.h b/fs/efivarfs/internal.h
> > index ac6a1dd0a6a5..f913b6824289 100644
> > --- a/fs/efivarfs/internal.h
> > +++ b/fs/efivarfs/internal.h
> > @@ -17,7 +17,6 @@ struct efivarfs_fs_info {
> >  	struct efivarfs_mount_opts mount_opts;
> >  	struct super_block *sb;
> >  	struct notifier_block nb;
> > -	struct notifier_block pm_nb;
> >  };
> >  
> >  struct efi_variable {
> > diff --git a/fs/efivarfs/super.c b/fs/efivarfs/super.c
> > index 0486e9b68bc6..567e849a03fe 100644
> > --- a/fs/efivarfs/super.c
> > +++ b/fs/efivarfs/super.c
> > @@ -20,6 +20,7 @@
> >  #include <linux/printk.h>
> >  
> >  #include "internal.h"
> > +#include "../internal.h"
> >  
> >  static int efivarfs_ops_notifier(struct notifier_block *nb, unsigned
> > long event,
> >  				 void *data)
> > @@ -119,12 +120,18 @@ static int efivarfs_statfs(struct dentry
> > *dentry, struct kstatfs *buf)
> >  
> >  	return 0;
> >  }
> > +
> > +static int efivarfs_freeze_fs(struct super_block *sb);
> > +static int efivarfs_unfreeze_fs(struct super_block *sb);
> > +
> >  static const struct super_operations efivarfs_ops = {
> >  	.statfs = efivarfs_statfs,
> >  	.drop_inode = generic_delete_inode,
> >  	.alloc_inode = efivarfs_alloc_inode,
> >  	.free_inode = efivarfs_free_inode,
> >  	.show_options = efivarfs_show_options,
> > +	.freeze_fs = efivarfs_freeze_fs,
> 
> Why is it necessary to have a freeze_fs operation?  The current code in
> super.c:freeze_super() reads:

Fwiw, I've explained this already in prior mails. The same behavior as
for the ioctl where we check whether the filesystem provides either a
->freeze_fs or ->freeze_super method. If neither is provided the
filesystem is assumed to not have freeze support.

> 
> 	if (sb->s_op->freeze_fs) {
> 		ret = sb->s_op->freeze_fs(sb);
> 
> So it would seem that setting this to NULL has exactly the same effect
> as providing a null method.

No, it would cause freeze to not be called.

IOW, any filesystem that doesn't provides neither a freeze_super or
freeze_fs method doesn't support freeze (that's how the ioctls work as
well) which allows us to only call into filesystems that are able to
properly freeze so we don't need pointless FS_* flags. By only providing
thaw it would end up thawing something that was never frozen. Both are
provided and the freeze methods function as the indicator whether
freezing/thawing is supported.

That could be changed but not in this series. We could also provide
noop_freeze just like we have noop_sync but again, not for this series.

> 
> > +	.unfreeze_fs = efivarfs_unfreeze_fs,
> >  };
> >  
> >  /*
> > 
> [...]
> > @@ -608,9 +516,7 @@ static void efivarfs_kill_sb(struct super_block
> > *sb)
> >  {
> >  	struct efivarfs_fs_info *sfi = sb->s_fs_info;
> >  
> > -	blocking_notifier_chain_unregister(&efivar_ops_nh, &sfi-
> > >nb);
> 
> This is an extraneous deletion of an unrelated notifier which efivarfs
> still needs to listen for ops updates from the efi subsystem.

At first I was bewildered because I thought you were talking about pm_nb
for some reason and was ready to explode. Man, I need a post LSFMM
vacation. :)

Thanks for spotting this. This is now fixed by adding back:

blocking_notifier_chain_unregister(&efivar_ops_nh, &sfi->nb);