Re: Freeze on 27 February

["Vladimir 'φ-coder/phcoder' Serbinenko" <phcoder@gmail.com>]

On 22.02.2012 06:35, Richard Laager wrote:
> On Tue, 2012-02-21 at 17:12 +0100, Vladimir 'φ-coder/phcoder' Serbinenko
> wrote:
>> >  @Richard Laager: Which of ZFS patches aren't committed yet? It's a bit
>> >  tricky to see which ones were superseeded.
> I've attached my current patch set. The patches apply in the order
> listed. They're also roughly ordered by complexity, so I'd recommend
> reviewing them in this order.
>
> Also, if you have libzfs, a --disable-zfs or --without-zfs or similar
> patch is necessary to ensure that the zpool and zfs commands are used
> instead of libzfs.
>
> ---- Not ZFS Related:
>
> Previously submitted, no feedback, trivial:
>      grub-install-whitespace.patch
>
> Not previously submitted, trivial:
>      bzrignore-updates.patch
>
> ---- ZFS Related:
>
> Previously submitted, no feedback:
>      zfs-poolname-spaces.patch
>      zfs-devices.patch
>
> Not previously submitted:
>      zfs-on-linux-rlaager8.patch
>                  With this, you should be able to boot with (native)
>                  ZFS-on-Linux, though you'll have to add whatever rpool
>                  specifiers (if any) required by your initrd.
>
>      zfs-on-linux-rlaager9.patch
>                  Part of this is just to support ZFS roots
>                  (root=ZFS=rpool/ROOT/ubuntu, for example).
>
>                  The other part may need more design work. It moves some
>                  of the btrfs code to inside linux_entry (and likewise,
>                  the ZFS support is added there). Right now, GRUB
>                  supports the concept of multiple kernels. I think that
>                  needs to be extended to multiple root filesystems (in
>                  practice: subvols in btrfs, clones in ZFS). This is the
>                  first step in that process. The missing part is looping
>                  over the additional root filesystems.
>
> Even if we can't get the multiple root filesystems issue figured out,
> I'd really love to see everything else make it into the release. It'd be
> a huge step in the right direction for those of us working with native
> ZFS-on-Linux.
>
> -- Richard
>
>
> bzrignore-updates.patch
>
>
> === modified file '.bzrignore'
> Index: grub/.bzrignore
> ===================================================================
> --- grub.orig/.bzrignore	2012-02-04 17:30:15.295629000 -0600
> +++ grub/.bzrignore	2012-02-04 17:30:49.356454000 -0600
> @@ -104,6 +104,8 @@ partmap_test
>   *.pp
>   po/*.mo
>   po/grub.pot
> +po/POTFILES
> +po/stamp-po
>   stamp-h
>   stamp-h1
>   stamp-h.in
> @@ -132,8 +134,10 @@ contrib
>   grub-core/Makefile.core.am
>   grub-core/Makefile.gcry.def
>   grub-core/contrib
> +grub-core/gdb_grub
>   grub-core/genmod.sh
>   grub-core/gensyminfo.sh
> +grub-core/gmodule.pl
>   grub-core/modinfo.sh
>   grub-core/*.module
>   grub-core/*.pp
>
Committed
> zfs-poolname-spaces.patch
>
>
> Handle pool names with spaces
>
> Index: grub/util/getroot.c
> ===================================================================
> --- grub.orig/util/getroot.c	2012-02-03 05:21:06.838056692 -0600
> +++ grub/util/getroot.c	2012-02-03 05:22:36.227364000 -0600
> @@ -260,7 +260,7 @@
>     char cksum[257], notes[257];
>     unsigned int dummy;
>
> -  cmd = xasprintf ("zpool status %s", poolname);
> +  cmd = xasprintf ("zpool status \"%s\"", poolname);
>     fp = popen (cmd, "r");
This is wrong if poolname contains weird characters. Can you make it 
similar to mdadm-related code?
>     free (cmd);
>
> @@ -285,8 +285,7 @@
>   	      st++;
>   	    break;
>   	  case 1:
> -	    if (!strcmp (name, poolname))
> -	      st++;
> +	    st++;
>   	    break;
>   	  case 2:
>   	    if (strcmp (name, "mirror")&&  !sscanf (name, "mirror-%u",&dummy)
> @@ -420,6 +419,9 @@
>         if (sscanf (sep, "%s %s", entry.fstype, entry.device) != 2)
>   	continue;
>
> +      unescape (entry.fstype);
> +      unescape (entry.device);
> +
You need to increase the size of storage for these fields.
>
> Handle vdevs with full paths
>
> Index: grub/util/getroot.c
> ===================================================================
> --- grub.orig/util/getroot.c	2012-02-03 05:22:36.227364000 -0600
> +++ grub/util/getroot.c	2012-02-03 05:22:41.255135000 -0600
> @@ -301,7 +301,10 @@
>   		    devices = xrealloc (devices, sizeof (devices[0])
>   					* devices_allocated);
>   		  }
> -		devices[ndevices++] = xasprintf ("/dev/%s", name);
> +		if (name[0] == '/')
> +		  devices[ndevices++] = xstrdup (name);
> +		else
> +		  devices[ndevices++] = xasprintf ("/dev/%s", name);
>   	      }
>   	    break;
>   	  }
>
This one is ok other than the missing ChangeLog.
> zfs-on-linux-rlaager8.patch
>
>
> ZFS on Linux Improvements
>
> 1. `zpool status` can output disk names which are under /dev/disk.
> 2. `zpool status` outputs the whole disk device for wholedisk pools,
>     but GRUB needs the partition device.
> 3. Support native ZFS on Linux.
>
> Index: grub/util/getroot.c
> ===================================================================
> --- grub.orig/util/getroot.c	2012-02-03 05:54:39.540539000 -0600
> +++ grub/util/getroot.c	2012-02-03 06:04:29.465275000 -0600
> @@ -304,7 +304,87 @@
>   		if (name[0] == '/')
>   		  devices[ndevices++] = xstrdup (name);
>   		else
> +#ifdef __linux__
> +		  {
> +		    /* The name returned by zpool isn't necessarily directly under /dev. */
> +		    char *device = xasprintf ("/dev/%s", name);
Could you unify this with the scan code we already have? (the one where 
we scan for major/minor)

@@ -478,7 +558,8 @@
        if (!*entries[i].device)
  	continue;

-      if (grub_strcmp (entries[i].fstype, "fuse.zfs") == 0)
+      if (grub_strcmp (entries[i].fstype, "fuse.zfs") == 0 ||
+          grub_strcmp (entries[i].fstype, "zfs") == 0)
  	{
  	  char *slash;
  	  slash = strchr (entries[i].device, '/');
This should go as a separate patch

>
> zfs-on-linux-rlaager9.patch
>
>
> Pass boot=zfs and zfs-bootfs=... when / is ZFS on Linux
>
> Index: grub/util/grub.d/10_linux.in
> ===================================================================
> --- grub.orig/util/grub.d/10_linux.in	2012-02-02 03:34:48.512644650 -0600
> +++ grub/util/grub.d/10_linux.in	2012-02-03 01:12:57.608355000 -0600
> @@ -56,13 +56,11 @@
>     LINUX_ROOT_DEVICE=UUID=${GRUB_DEVICE_UUID}
>   fi
>
> -if [ "x`${grub_probe} --device ${GRUB_DEVICE} --target=fs 2>/dev/null || true`" = xbtrfs ] \
> -    || [ "x`stat -f --printf=%T /`" = 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
> +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}" = xzfs ]; then
> +  RPOOL=`${grub_probe} --device ${GRUB_DEVICE} --target=fs_label 2>/dev/null || true`
>   fi
>
>   for word in $GRUB_CMDLINE_LINUX_DEFAULT; do
> @@ -193,6 +191,19 @@
>     version=`echo $basename | sed -e "s,^[^0-9]*-,,g"`
>     alt_version=`echo $version | sed -e "s,\.old$,,g"`
>     linux_root_device_thisversion="${LINUX_ROOT_DEVICE}"
> +  cmdline=""
> +  if [ "x${LINUX_ROOT_FS}" = xbtrfs -o "x${LINUX_ROOT_STAT}" = xbtrfs ]; then
> +    rootsubvol="`make_system_path_relative_to_its_root /`"
> +    rootsubvol="${rootsubvol#/}"
> +    if [ "x${rootsubvol}" != x ]; then
> +      cmdline="rootflags=subvol=${rootsubvol} ${cmdline}"
> +    fi
> +  fi
> +  if [ "x${LINUX_ROOT_FS}" = xzfs ]; then
> +    bootfs="`make_system_path_relative_to_its_root / | sed -e "s,@$,,"`"
> +    linux_root_device_thisversion="ZFS=${RPOOL}${bootfs}"
> +    cmdline="boot=zfs rpool=${RPOOL} bootfs=${RPOOL}${bootfs} ${cmdline}"
> +  fi
Please keep this at the beginning, we don't need to execute it for every 
kernel.
>
>     initrd=
>     for i in "initrd.img-${version}" "initrd-${version}.img" "initrd-${version}.gz" \
> @@ -229,7 +240,7 @@
>     fi
>
>     linux_entry "${OS}" "${version}" false \
> -      "${GRUB_CMDLINE_LINUX} ${GRUB_CMDLINE_EXTRA} ${GRUB_CMDLINE_LINUX_DEFAULT}" \
> +      "${cmdline} ${GRUB_CMDLINE_LINUX} ${GRUB_CMDLINE_EXTRA} ${GRUB_CMDLINE_LINUX_DEFAULT}" \
>         quiet
>     if [ "x${GRUB_DISABLE_RECOVERY}" != "xtrue" ]; then
>       if [ -x /lib/recovery-mode/recovery-menu ]; then


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