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

[James Bottomley]

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@xxxxxxxxxx>
> ---
>  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:

	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.

> +	.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.

Regards,

James