Re: [Patch] Enable libzfs detection on Linux

[Zachary Bedell <pendorbound@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3632 bytes --]

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.


[-- Attachment #2: grub-zfs-linux.patch --]
[-- Type: application/octet-stream, Size: 5610 bytes --]

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>])
 
 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"])
 
-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 ...")"

[-- Attachment #3: Type: text/plain, Size: 654 bytes --]



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