On 14.09.2011 20:39, Zachary Bedell wrote:
> Finally getting back to this & trying address concerns below:
>
> On Aug 18, 2011, at 12:49 PM, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
>> > On 09.08.2011 19:48, Zachary Bedell wrote:
>>> >>
>>> >> * Scan /proc/mounts and /etc/mtab in addition to /etc/mnttab to discover
>>> >> mounted filesystems in grub_find_zpool_from_dir.
>> > /etc/mtab is just a regular file and in many cases is out-of-sync with
>> > real state of affairs. Should be ignored altogether. Use of /etc/mnttab is
>> > unfortunate but I know of no other way on other platforms (since I haven't
>> > looked into it).
> Easy enough to remove mtab.
>
>>> >> These patches have been in use by a number of folks using ZfsOnLinux for
>>> >> some time, and they've been robust on those systems. I've tried to
>>> >> ensure the changes won't impact non-Linux platforms, though I'm not sure
>>> >> I trust my knowledge of autoconf enough to be positive there are no side
>>> >> effects.
>>> >>
>> > You forget the effect of other code changes (below)
>>> >> - FILE *mnttab = fopen ("/etc/mnttab", "r");
>>> >> + FILE *mnttab;
>>> >> + mnttab = fopen ("/proc/mounts", "r");
>> > /proc on FreeBSD is very different from Linux one. Don't try
>> > /proc/mounts except if you have Linux.
> If I'm reading the pre-existing ifdef's there, the code added for
> /proc/mounts wouldn't apply on FreeBSD (assuming the comments there aren't
> lying). The ifdef around line 1399:
>
> #if defined(HAVE_STRUCT_STATFS_F_FSTYPENAME) &&
> defined(HAVE_STRUCT_STATFS_F_MNTFROMNAME)
> /* FreeBSD and GNU/kFreeBSD. */
>
> would be hit on BSD and thus exclude the /proc/mounts code. That said, easy
> enough to add an extra '#ifdef __linux__' around the proc code so it doesn't
> fire on Solaris and yank the mtab code.
>
>>> >> -if [ "x`${grub_probe} --device ${GRUB_DEVICE} --target=fs 2>/dev/null
>>> >> || true`" = xbtrfs ]; then
>>> >> +LINUX_ROOT_FS=`${grub_probe} --device ${GRUB_DEVICE} --target=fs
>>> >> 2>/dev/null || true`
>>> >> +LINUX_ROOT_STAT=`stat -f --printf=%T / || true`
>>> >> +
>>> >> +if [ "x${LINUX_ROOT_FS}" = xbtrfs ] || [ "x${LINUX_ROOT_STAT}" = xbtrfs
>>> >> ]; then
>> > This changes logic for btrfs. I don't think it's necessary or good to
>> > just change it.
> Looking back, I think this may have been the result of forward porting this
> patch from an older Grub codebase. I've changed it to restore the original
> btrfs logic from trunk.
>
>>> >> +if [ "x${LINUX_ROOT_FS}" = xzfs ]; then
>>> >> + GRUB_CMDLINE_LINUX="boot=zfs \$bootfs ${GRUB_CMDLINE_LINUX}"
>>> >> +fi
>>> >> +
>>> >> fi
>>> >> + if [ "x${LINUX_ROOT_FS}" = xzfs ]; then
>>> >> + cat << EOF
>>> >> + insmod zfsinfo
>>> >> + zfs-bootfs (\$root) bootfs
>>> >> +EOF
>> > In this place $root refers to whereever kernel is. So if /boot is
>> > separate it will be wrong. Moreover you completely forget the possible
>> > subvolumes. One could have e.g.
>> > FreeBSD in /freebsd/@/...
>> > GNU/Linux in /gnu/linux/@
>> > /boot in /boot/@
>> > In this case $bootfs has to take subvolume into account.
>> > Also nothing guarantees that / is accessible from GRUB proper at all.
>> > The ZFS in question may be on e.g. SAN. You need to figure parameters in
>> > 10_linux, not on boot time.
> Taking a closer look at these, I don't think they're necessary with the
> initrd scripts used by the current Linux implementations (one in Ubuntu & the
> Dracut scripts in the ZFSonLinux distribution being the two I'm aware of).
> The only thing required in the second half of 10_linux was the change to
> exclude root=… and the ro attribute for ZFS. Once grub loads kernel &
> initrd, the initrd does the work of finding the root pool and importing it
> using the full ZFS kernel module, so there's no need to use Grub's logic to
> do the same.
>
> Remixed patch is attached.
>
> grub-zfs-linux.patch
>
>
> diff --git a/configure.ac b/configure.ac
> index e6d7265..3137869 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -295,9 +295,14 @@ else
> AC_PATH_PROG(HELP2MAN, help2man)
> fi
>
> +# The Solaris Portability Layer is required to link grub against zfs-lib on
> Linux.
> +AC_CHECK_LIB([spl], [getextmntent], [ LIBS="$LIBS -pthread -lspl"
> + AC_DEFINE([HAVE_LIBSPL], [1], [Define to 1 if you have the SPL
> library.])],)
> +
> # Check for functions and headers.
> AC_CHECK_FUNCS(posix_memalign memalign asprintf vasprintf getextmntent)
> -AC_CHECK_HEADERS(libzfs.h libnvpair.h sys/param.h sys/mount.h sys/mnttab.h
> sys/mkdev.h)
> +AC_CHECK_HEADERS(libzfs.h libnvpair.h sys/param.h sys/mount.h sys/mnttab.h
> sys/mkdev.h,[],[],[$ac_includes_default
> +#include <zfs-linux/zfs_config.h>])
>
This will fail on Solaris or FreeBSD due to missing zfs-linux/zfs_config.h.
> AC_CHECK_MEMBERS([struct statfs.f_fstypename],,,[$ac_includes_default
> #include <sys/param.h>
> @@ -922,17 +927,19 @@ AC_CHECK_LIB([lzma], [lzma_code],
> [Define to 1 if you have the LZMA library.])],)
> AC_SUBST([LIBLZMA])
>
> -AC_CHECK_LIB([zfs], [libzfs_init],
> - [LIBZFS="-lzfs"
> - AC_DEFINE([HAVE_LIBZFS], [1],
> - [Define to 1 if you have the ZFS library.])],)
> -AC_SUBST([LIBZFS])
> +# These libraries and zpool below are external to libzfs on Linux,
> +# but usually internal or intrinsic on other platforms.
> +AC_CHECK_LIB([avl], [avl_create], [LIBS="$LIBS -lavl"])
> +AC_CHECK_LIB([efi], [efi_alloc_and_init], [LIBS="$LIBS -lefi"])
> +AC_CHECK_LIB([unicode], [u8_strcmp], [LIBS="$LIBS -lunicode"])
>
Why do you need those libraries? I see no reference to these functions.
> -AC_CHECK_LIB([nvpair], [nvlist_print],
> - [LIBNVPAIR="-lnvpair"
> - AC_DEFINE([HAVE_LIBNVPAIR], [1],
> - [Define to 1 if you have the NVPAIR library.])],)
> +AC_CHECK_LIB([nvpair], [nvlist_print], [LIBS="$LIBS -lnvpair"
> LIBNVPAIR="$LIBS"
> + AC_DEFINE([HAVE_LIBNVPAIR], [1], [Define to 1 if you have the NVPAIR
> library.])],)
> AC_SUBST([LIBNVPAIR])
> +AC_CHECK_LIB([zpool], [zfs_prop_init], [LIBS="$LIBS -lzpool"])
> +AC_CHECK_LIB([zfs], [libzfs_init], [LIBS="$LIBS -lzfs" LIBZFS="$LIBS"
> + AC_DEFINE([HAVE_LIBZFS], [1], [Define to 1 if you have the ZFS
> library.])],)
> +AC_SUBST([LIBZFS])
>
> LIBS=""
>
> diff --git a/util/getroot.c b/util/getroot.c
> index 7106458..592a02f 100644
> --- a/util/getroot.c
> +++ b/util/getroot.c
> @@ -34,6 +34,17 @@
> #include <stdint.h>
> #include <grub/util/misc.h>
> #include <grub/cryptodisk.h>
> +#if defined(HAVE_LIBSPL) && defined(__linux__)
> +# include <sys/ioctl.h>
> +/*
> + * The Solaris Compatibility Layer provides getextmntent on Linux, which is
> + * required for grub-probe to recognize a native ZFS root filesystem on
> + * a Linux system. This typedef is required because including the SPL
> + * types.h here conflicts with an earlier Linux types.h inclusion.
> + */
> + typedef unsigned int uint_t;
> +# include <libspl/sys/mnttab.h>
> +#endif
>
> #ifdef HAVE_DEVICE_MAPPER
> # include <libdevmapper.h>
> @@ -598,16 +609,16 @@ grub_guess_root_device (const char *dir)
> struct stat st;
> dev_t dev;
>
> -#ifdef __linux__
> - if (!os_dev)
> - os_dev = grub_find_root_device_from_mountinfo (dir, NULL);
> -#endif /* __linux__ */
> -
> #if defined(HAVE_LIBZFS) && defined(HAVE_LIBNVPAIR)
> if (!os_dev)
> os_dev = find_root_device_from_libzfs (dir);
> #endif
>
> +#ifdef __linux__
> + if (!os_dev)
> + os_dev = grub_find_root_device_from_mountinfo (dir, NULL);
> +#endif /* __linux__ */
> +
> if (os_dev)
> {
> char *tmp = os_dev;
> @@ -1399,7 +1410,7 @@ grub_find_zpool_from_dir (const char *dir, char
> **poolname, char **poolfs)
> *poolname = xstrdup (mnt.f_mntfromname);
> }
> #elif defined(HAVE_GETEXTMNTENT)
> - /* Solaris. */
> + /* Solaris and ZFSonLinux (but not FUSE). */
> {
> struct stat st;
> struct extmnttab mnt;
> @@ -1407,7 +1418,17 @@ grub_find_zpool_from_dir (const char *dir, char
> **poolname, char **poolfs)
> if (stat (dir, &st) != 0)
> return;
>
> - FILE *mnttab = fopen ("/etc/mnttab", "r");
> + FILE *mnttab = NULL;
> +
> +#ifdef __linux__
> + /* Look in proc only for Linux. Solaris (and anything else with
> + HAVE_GETEXTMNTENT) won't need it. */
> + mnttab = fopen ("/proc/mounts", "r");
> +#endif
> +
> + if (! mnttab)
> + mnttab = fopen ("/etc/mnttab", "r");
> +
> if (! mnttab)
> return;
>
> diff --git a/util/grub.d/10_linux.in b/util/grub.d/10_linux.in
> index 97e7c65..5624607 100644
> --- a/util/grub.d/10_linux.in
> +++ b/util/grub.d/10_linux.in
> @@ -51,12 +51,15 @@ else
> LINUX_ROOT_DEVICE=UUID=${GRUB_DEVICE_UUID}
> fi
>
> -if [ "x`${grub_probe} --device ${GRUB_DEVICE} --target=fs 2>/dev/null ||
> true`" = xbtrfs ]; then
> +LINUX_ROOT_FS=`${grub_probe} --device ${GRUB_DEVICE} --target=fs 2>/dev/null
> || true`
> +if [ "x${LINUX_ROOT_FS}" = xbtrfs ] ; then
> rootsubvol="`make_system_path_relative_to_its_root /`"
> rootsubvol="${rootsubvol#/}"
> if [ "x${rootsubvol}" != x ]; then
> GRUB_CMDLINE_LINUX="rootflags=subvol=${rootsubvol} ${GRUB_CMDLINE_LINUX}"
> fi
> +elif [ "x${LINUX_ROOT_FS}" = xzfs ]; then
> + GRUB_CMDLINE_LINUX="boot=zfs ${GRUB_CMDLINE_LINUX}"
> fi
>
> linux_entry ()
> @@ -113,10 +116,16 @@ EOF
> fi
> printf '%s\n' "${prepare_boot_cache}"
> fi
> + if [ "x${LINUX_ROOT_FS}" = xzfs ]; then
> + # ZFS doesn't want root=... or read-only.
> + rootentry=""
> + else
> + rootentry="root=${linux_root_device_thisversion} ro"
> + fi
> message="$(gettext_printf "Loading Linux %s ..." ${version})"
> cat << EOF
> echo '$message'
> - linux ${rel_dirname}/${basename}
> root=${linux_root_device_thisversion} ro ${args}
> + linux ${rel_dirname}/${basename} ${rootentry} ${args}
> EOF
> if test -n "${initrd}" ; then
> message="$(gettext_printf "Loading initial ramdisk ...")"
>
>
>
>
> FWIW, my commit comment locally for this was:
> * Adjusts autoconf logic to properly detect libzfs on Linux.
> * Includes additional headers necessary for libspl.
> * Changes order that filesystems are detected in to allow ZFS a chance to be
> found.
> * Add necessary boot parameters & detection logic to grub.d script for Linux.
>
> Sorry for the interminable delay in getting back to this. Debugging this
> took a rather "creative" turn as I found a few more cases where pools that
> worked in the ZFS driver were rejected by Grub. I have a second patch that
> fixes a number of these cases which I'll be posting shortly.
>
> Best regards,
> Zac Bedell
>
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel-- Regards Vladimir 'φ-coder/phcoder' Serbinenko
signature.asc
Description: OpenPGP digital signature
_______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
