Re: [Patch] Enable libzfs detection on Linux

[Zachary Bedell]

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
Description: Binary data

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