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

[Daniel Kiper]

Hi Paul,

On Wed, Mar 11, 2020 at 10:37:08AM -0700, 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
> 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 patchset 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.

First of all I would like to thank you for doing this work. This is very
interesting feature. I went quickly through the code and it looks quite
nice. However, I realized that it touches a lot of critical places in the
GRUB code. ...and we are in freeze right now. So, at this point I think
it is dangerous to merge that code. Simply we can make a lot of fallout
which we may not be able to catch and clear before the release. Hence,
I would like to suggest to defer this work until the release. Sorry about
that but I do not want to risk GRUB code breakage at this point. I hope
this is not a problem for you.

Daniel