From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.71) id 1RLyf8-0001c1-0w for mharc-grub-devel@gnu.org; Thu, 03 Nov 2011 10:53:02 -0400 Received: from eggs.gnu.org ([140.186.70.92]:38575) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RLyez-0001bi-Mf for grub-devel@gnu.org; Thu, 03 Nov 2011 10:53:00 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RLyes-0005vQ-OG for grub-devel@gnu.org; Thu, 03 Nov 2011 10:52:53 -0400 Received: from mail-ww0-f49.google.com ([74.125.82.49]:46886) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RLyes-0005vB-Gk for grub-devel@gnu.org; Thu, 03 Nov 2011 10:52:46 -0400 Received: by wwe3 with SMTP id 3so1511206wwe.30 for ; Thu, 03 Nov 2011 07:52:45 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=message-id:date:from:user-agent:mime-version:to:subject:references :in-reply-to:x-enigmail-version:content-type; bh=0iXOR/S0yscC2Y591rUBcGWLkjSUiAYOoCnOT6Yh7DI=; b=v19FsNdcDzAfbzNVA47uQKRH360qkVS73bJycZf9KfoMm7H4yLNDxIdCJxcB3xhpPF MBoBOZOTXJ+I7R1OYeDUNBQkl9lxrU8+x2i0scoVj3bjYzgBHzUn27ZtAIxlP5qrGCHN Xj3xrsMbclY91jqk5Xqnr7ehSpsTejGZAAQJc= Received: by 10.227.197.71 with SMTP id ej7mr12218075wbb.15.1320331964169; Thu, 03 Nov 2011 07:52:44 -0700 (PDT) Received: from debian.x201.phnet (gprs45.swisscom-mobile.ch. [193.247.250.45]) by mx.google.com with ESMTPS id em4sm1751304wbb.20.2011.11.03.07.51.08 (version=TLSv1/SSLv3 cipher=OTHER); Thu, 03 Nov 2011 07:52:43 -0700 (PDT) Message-ID: <4EB2AA55.60408@gmail.com> Date: Thu, 03 Nov 2011 15:51:01 +0100 From: =?UTF-8?B?VmxhZGltaXIgJ8+GLWNvZGVyL3BoY29kZXInIFNlcmJpbmVua28=?= User-Agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.2.23) Gecko/20111010 Iceowl/1.0b2 Icedove/3.1.15 MIME-Version: 1.0 To: grub-devel@gnu.org Subject: Re: [Patch] Enable libzfs detection on Linux References: <37B025F9-45D9-4424-8458-6B40D2D5A249@gmail.com> <4E4D42A8.4000503@gmail.com> In-Reply-To: X-Enigmail-Version: 1.1.2 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="------------enig24579A0D5928589BB894077B" X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.6 (newer, 2) X-Received-From: 74.125.82.49 X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.14 Precedence: list Reply-To: The development of GNU GRUB List-Id: The development of GNU GRUB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 03 Nov 2011 14:53:00 -0000 This is an OpenPGP/MIME signed message (RFC 2440 and 3156) --------------enig24579A0D5928589BB894077B Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable 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 '=CF=86-coder/phcoder' Serbinenk= o wrote: >> > On 09.08.2011 19:48, Zachary Bedell wrote: >>> >>=20 >>> >> * Scan /proc/mounts and /etc/mtab in addition to /etc/mnttab to di= scover mounted filesystems in grub_find_zpool_from_dir. >> > /etc/mtab is just a regular file and in many cases is out-of-sync wi= th real state of affairs. Should be ignored altogether. Use of /etc/mntta= b is unfortunate but I know of no other way on other platforms (since I h= aven't looked into it). > Easy enough to remove mtab. > >>> >> These patches have been in use by a number of folks using ZfsOnLin= ux for some time, and they've been robust on those systems. I've tried t= o ensure the changes won't impact non-Linux platforms, though I'm not sur= e I trust my knowledge of autoconf enough to be positive there are no sid= e effects. >>> >>=20 >> > You forget the effect of other code changes (below) >>> >> - FILE *mnttab =3D fopen ("/etc/mnttab", "r"); >>> >> + FILE *mnttab; >>> >> + mnttab =3D 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 lyi= ng). The ifdef around line 1399: > > #if defined(HAVE_STRUCT_STATFS_F_FSTYPENAME) && defined(HAVE_STRUCT_STA= TFS_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 i= t doesn't fire on Solaris and yank the mtab code. > >>> >> -if [ "x`${grub_probe} --device ${GRUB_DEVICE} --target=3Dfs 2>/de= v/null || true`" =3D xbtrfs ]; then >>> >> +LINUX_ROOT_FS=3D`${grub_probe} --device ${GRUB_DEVICE} --target=3D= fs 2>/dev/null || true` >>> >> +LINUX_ROOT_STAT=3D`stat -f --printf=3D%T / || true` >>> >> + >>> >> +if [ "x${LINUX_ROOT_FS}" =3D xbtrfs ] || [ "x${LINUX_ROOT_STAT}" = =3D xbtrfs ]; then >> > This changes logic for btrfs. I don't think it's necessary or good t= o >> > 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 o= riginal btrfs logic from trunk. > >>> >> +if [ "x${LINUX_ROOT_FS}" =3D xzfs ]; then >>> >> + GRUB_CMDLINE_LINUX=3D"boot=3Dzfs \$bootfs ${GRUB_CMDLINE_LINUX}= " >>> >> +fi >>> >> + >>> >> fi >>> >> + if [ "x${LINUX_ROOT_FS}" =3D 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 possib= le >> > 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= =2E >> > The ZFS in question may be on e.g. SAN. You need to figure parameter= s 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 awa= re of). The only thing required in the second half of 10_linux was the c= hange to exclude root=3D=E2=80=A6 and the ro attribute for ZFS. Once gru= b loads kernel & initrd, the initrd does the work of finding the root poo= l and importing it using the full ZFS kernel module, so there's no need t= o 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 > =20 > +# The Solaris Portability Layer is required to link grub against zfs-l= ib on Linux. > +AC_CHECK_LIB([spl], [getextmntent], [ LIBS=3D"$LIBS -pthread -lspl" > + AC_DEFINE([HAVE_LIBSPL], [1], [Define to 1 if you have the SPL libra= ry.])],) > + > # 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/mntt= ab.h sys/mkdev.h) > +AC_CHECK_HEADERS(libzfs.h libnvpair.h sys/param.h sys/mount.h sys/mntt= ab.h sys/mkdev.h,[],[],[$ac_includes_default > +#include ]) > =20 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 > @@ -922,17 +927,19 @@ AC_CHECK_LIB([lzma], [lzma_code], > [Define to 1 if you have the LZMA library.])],= ) > AC_SUBST([LIBLZMA]) > =20 > -AC_CHECK_LIB([zfs], [libzfs_init], > - [LIBZFS=3D"-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=3D"$LIBS -lavl"]) > +AC_CHECK_LIB([efi], [efi_alloc_and_init], [LIBS=3D"$LIBS -lefi"]) > +AC_CHECK_LIB([unicode], [u8_strcmp], [LIBS=3D"$LIBS -lunicode"]) > =20 Why do you need those libraries? I see no reference to these functions. > -AC_CHECK_LIB([nvpair], [nvlist_print], > - [LIBNVPAIR=3D"-lnvpair" > - AC_DEFINE([HAVE_LIBNVPAIR], [1], > - [Define to 1 if you have the NVPAIR library.])= ],) > +AC_CHECK_LIB([nvpair], [nvlist_print], [LIBS=3D"$LIBS -lnvpair" LIBNVP= AIR=3D"$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=3D"$LIBS -lzpool"]) > +AC_CHECK_LIB([zfs], [libzfs_init], [LIBS=3D"$LIBS -lzfs" LIBZFS=3D"$LI= BS" > + AC_DEFINE([HAVE_LIBZFS], [1], [Define to 1 if you have the ZFS libra= ry.])],) > +AC_SUBST([LIBZFS]) > =20 > LIBS=3D"" > =20 > 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 > #include > #include > +#if defined(HAVE_LIBSPL) && defined(__linux__) > +# include > +/* > + * The Solaris Compatibility Layer provides getextmntent on Linux, whi= ch is > + * required for grub-probe to recognize a native ZFS root filesystem o= n > + * 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 > +#endif > =20 > #ifdef HAVE_DEVICE_MAPPER > # include > @@ -598,16 +609,16 @@ grub_guess_root_device (const char *dir) > struct stat st; > dev_t dev; > =20 > -#ifdef __linux__ > - if (!os_dev) > - os_dev =3D grub_find_root_device_from_mountinfo (dir, NULL); > -#endif /* __linux__ */ > - > #if defined(HAVE_LIBZFS) && defined(HAVE_LIBNVPAIR) > if (!os_dev) > os_dev =3D find_root_device_from_libzfs (dir); > #endif > =20 > +#ifdef __linux__ > + if (!os_dev) > + os_dev =3D grub_find_root_device_from_mountinfo (dir, NULL); > +#endif /* __linux__ */ > + > if (os_dev) > { > char *tmp =3D os_dev; > @@ -1399,7 +1410,7 @@ grub_find_zpool_from_dir (const char *dir, char *= *poolname, char **poolfs) > *poolname =3D 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) !=3D 0) > return; > =20 > - FILE *mnttab =3D fopen ("/etc/mnttab", "r"); > + FILE *mnttab =3D NULL; > + > +#ifdef __linux__ > + /* Look in proc only for Linux. Solaris (and anything else with=20 > + HAVE_GETEXTMNTENT) won't need it. */ > + mnttab =3D fopen ("/proc/mounts", "r"); > +#endif > + > + if (! mnttab) > + mnttab =3D fopen ("/etc/mnttab", "r"); > + > if (! mnttab) > return; > =20 > 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=3DUUID=3D${GRUB_DEVICE_UUID} > fi > =20 > -if [ "x`${grub_probe} --device ${GRUB_DEVICE} --target=3Dfs 2>/dev/nul= l || true`" =3D xbtrfs ]; then > +LINUX_ROOT_FS=3D`${grub_probe} --device ${GRUB_DEVICE} --target=3Dfs 2= >/dev/null || true` > +if [ "x${LINUX_ROOT_FS}" =3D xbtrfs ] ; then > rootsubvol=3D"`make_system_path_relative_to_its_root /`" > rootsubvol=3D"${rootsubvol#/}" > if [ "x${rootsubvol}" !=3D x ]; then > GRUB_CMDLINE_LINUX=3D"rootflags=3Dsubvol=3D${rootsubvol} ${GRUB_CM= DLINE_LINUX}" > fi > +elif [ "x${LINUX_ROOT_FS}" =3D xzfs ]; then > + GRUB_CMDLINE_LINUX=3D"boot=3Dzfs ${GRUB_CMDLINE_LINUX}" > fi > =20 > linux_entry () > @@ -113,10 +116,16 @@ EOF > fi > printf '%s\n' "${prepare_boot_cache}" > fi > + if [ "x${LINUX_ROOT_FS}" =3D xzfs ]; then > + # ZFS doesn't want root=3D... or read-only. > + rootentry=3D"" > + else > + rootentry=3D"root=3D${linux_root_device_thisversion} ro" > + fi > message=3D"$(gettext_printf "Loading Linux %s ..." ${version})" > cat << EOF > echo '$message' > - linux ${rel_dirname}/${basename} root=3D${linux_root_device_thisversi= on} ro ${args} > + linux ${rel_dirname}/${basename} ${rootentry} ${args} > EOF > if test -n "${initrd}" ; then > message=3D"$(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 th= is 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 pat= ch 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 --=20 Regards Vladimir '=CF=86-coder/phcoder' Serbinenko --------------enig24579A0D5928589BB894077B Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.11 (GNU/Linux) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/ iF4EAREKAAYFAk6yqlUACgkQNak7dOguQgnnHQD8C1o7KiLfMHOEFqJ9UCfa4rVP mnWDVjPyytRh7uJbmbwBAJkIx/RQUw2+TTAhtqPl+waKhb2qU7FzOTrDrAHnHyhg =sSUJ -----END PGP SIGNATURE----- --------------enig24579A0D5928589BB894077B--