Re: [PATCH] ZFS/other CoW FS save_env support

[Daniel Kiper]

On Tue, Feb 18, 2020 at 11:10:07AM -0800, Paul Dagnelie wrote:
> Hey all, I previously discussed my concept for this patch in my email
> https://lists.gnu.org/archive/html/grub-devel/2020-01/msg00004.html .
> I'm pleased to announce that I've gotten it into a working state and
> it is ready to review.  There are a number of changes here, which I
> will break down below.
>
> First, the concept of "special files" is introduced into grub. These
> are files that are manipulated using different functions from the
> remainder of the filesystem. A filesystem advertises its support for
> these files by filling in new entries in the grub_fs struct.
>
> Second, the loadenv command was modified to use the special file
> functions of the root filesystem if they are detected. If that
> happens, these functions are used to open, read, and write to the
> loadenv file instead of the normal grub file functions.  A variable
> was also added that allows the user to force a file to be used instead
> of the special files, or vice versa.
>
> Third, the grub-editenv command was modified to teach it to probe the
> root filesystem and then check in an array of structures that contain

Why "root" not "boot"?

> functions that will read and modify the filesystem's special file in
> userland. These functions are very similar to normal read and write
> functions, and so don't result in a very different code flow.
>
> Finally, the GRUB ZFS logic was modified to have new special_file
> functions. These functions manipulate the pad2 area of the ZFS label,
> which was previously unused. It now stores a version number, the raw
> contents of the grubenv file, and an embedded checksum for integrity
> purposes. GRUB was taught to read and verify these areas, and also to
> modify them, computing the embeddded checksum appropriately.  In
> addition, if GRUB is build with libzfs support, an entry is added to
> the grub-editenv table that points GRUB to the appropriate libzfs
> functions, and init and fini functions are built into grub-editenv
> itself.
>
> Future work:
> * In the ZFS code could store a packed nvlist instead of a raw file,
> but this would involve further changes to the grub environment file
> code and was deferred for when it would be more useful and important.
> * For the special files code, there is only one special file allowed
> per filesystem, but a specification interface (using either an enum or
> a set of names) could be added in the future.
> * Other filesystem support was not built into this change, but
> extensibility was a primary goal, so adding support for btrfs or other
> filesystems should be relatively straightforward.
> * I did not implement the proposed UEFI variable support, but I did
> develop this with that future in mind, so adding it should be a
> relatively simple extension of these changes.
>
> This patch has been tested on systems with and without ZFS as the boot
> filesystem, and built with and without ZFS libraries installed. It
> worked in each case I tried, including with manual corruption applied
> to the ZFS labels to force GRUB to read from the other label.  This
> was tested in conjunction with
> https://github.com/zfsonlinux/zfs/pull/10009 , the ZoL patch that
> enables ZFS to read from and write to the padding areas we reserved
> for the data.
>
> Let me know if you have any feedback, I'm sure there's some stuff I'm
> doing wrong or in a non-idiomatic fashion.  In addition, if you'd like
> this patch divided into multiple smaller patches, I would also be
> happy to oblige.

Yes, please split the patch into smaller patches. Please do one logical
change per patch.

I quickly went through the patch and pointed things which I spotted at
first sight. I will provide more comments when you split the patch into
separate patches.

Next time please CC following people too: address@hidden,
address@hidden and address@hidden.

>   grub-core/commands/loadenv.c | 122 +++++++++++++++++---
>  grub-core/fs/zfs/zfs.c       | 131 +++++++++++++++++++--
>  grub-core/kern/file.c        |  74 ++++++++----
>  grub-core/lib/envblk.c       |   2 +-
>  include/grub/file.h          |  10 ++
>  include/grub/fs.h            |  12 +-
>  include/grub/lib/envblk.h    |   2 +
>  include/grub/util/install.h  |   3 +
>  include/grub/util/libzfs.h   |   5 +
>  include/grub/zfs/vdev_impl.h |  33 +++---
>  util/editenv.c               |  26 +++--
>  util/grub-editenv.c          | 267 
> +++++++++++++++++++++++++++++++++++++++++--
>  12 files changed, 603 insertions(+), 84 deletions(-)
>
> diff --git a/grub-core/commands/loadenv.c b/grub-core/commands/loadenv.c
> index 3fd664aac..3553c2f94 100644
> --- a/grub-core/commands/loadenv.c
> +++ b/grub-core/commands/loadenv.c
> @@ -79,6 +79,94 @@ open_envblk_file (char *filename,
>    return file;
>  }
>
> +static grub_file_t
> +open_envblk_block (grub_fs_t fs, grub_device_t dev, enum grub_file_type type)
> +{
> +  if (!fs->fs_special_open)
> +    return NULL;
> +  return grub_file_special_open(fs, dev, type);

I think more natural would be...

   if (fs->fs_special_open)
     return grub_file_special_open (fs, dev, type);
   else
     return NULL;

...and it seems to me that "fs_special_open" name is too generic.
What about "fs_envblk_open" or "fs_ext_data_open" (extra data) or
something like that? Same for other functions...

> +}
> +
> +static grub_file_t
> +open_envblk (grub_extcmd_context_t ctxt, enum grub_file_type type)
> +{
> +  struct grub_arg_list *state = ctxt->state;
> +  grub_file_t file = NULL;
> +  grub_device_t device = NULL;
> +  const char *force;
> +  grub_fs_t fs = NULL;
> +  char *filename = (state[0].set) ? state[0].arg : 0;

I think that you can drop parenthesis here. And please use NULL instead of 0.

> +  force = grub_env_get ("grubenv_force");

"grubenv_src" instead of "grubenv_force"?

[...]

> diff --git a/include/grub/fs.h b/include/grub/fs.h
> index 302e48d4b..99a1bf71e 100644
> --- a/include/grub/fs.h
> +++ b/include/grub/fs.h
> @@ -65,7 +65,7 @@ struct grub_fs
>    grub_err_t (*fs_open) (struct grub_file *file, const char *name);
>
>    /* Read LEN bytes data from FILE into BUF.  */
> -  grub_ssize_t (*fs_read) (struct grub_file *file, char *buf, grub_size_t 
> len);
> +  grub_ssize_t (*fs_read) (struct grub_file *file, char *buf, grub_size_t 
> len);
>
>    /* Close the file FILE.  */
>    grub_err_t (*fs_close) (struct grub_file *file);
> @@ -96,6 +96,16 @@ struct grub_fs
>    /* Whether blocklist installs have a chance to work.  */
>    int blocklist_install;
>  #endif
> +
> +  /* The special file functions are defined on filesystems that need to
> +     handle grub-writable files in a special way. This is most commonly the
> +     case for CoW filesystems like btrfs and ZFS.  The normal read and close
> +     functions should detect that they are being called on a special file and
> +     act appropriately.  */

This comment is incorrectly formatted. Please read this [1].

Daniel

[1] https://www.gnu.org/software/grub/manual/grub-dev/grub-dev.html#Comments