Re: [Patch] Enable libzfs detection on Linux

[Vladimir 'φ-coder/phcoder' Serbinenko]

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
> address@hidden
> https://lists.gnu.org/mailman/listinfo/grub-devel


-- 
Regards
Vladimir 'φ-coder/phcoder' Serbinenko

signature.asc
Description: OpenPGP digital signature