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

2020-03-25 Thread Paul Dagnelie
Not a problem, I suspected as much. We don't have any strong need to
get this work integrated ASAP, since we are going to have to build a
custom version of GRUB for internal use until this makes its way into
a release that lands in an Ubuntu LTS. We just want to make sure that
we get some good reviews on the changes from people with experience in
the codebase, and that one day we will be able to stop maintaining our
fork.

On Wed, Mar 25, 2020 at 10:06 AM Daniel Kiper  wrote:
>
> 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/msg4.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



-- 
Paul Dagnelie

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel


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

2020-03-25 Thread 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/msg4.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

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel