[libvirt] [PATCH 1/2] storage: zfs: enable on Linux

[John Ferlan]

On 01/02/2016 09:25 PM, Roman Bogorodskiy wrote:
> ZFS-on-Linux implementation of ZFS starting with version 0.6.4
> contains all the features we use, so there's no reason to limit
> libvirt ZFS storage backend to FreeBSD only.
> 
> There's still one difference between these implementations: ZFS on
> FreeBSD requires to set 'volmode' option to 'dev' when creating
> volumes, while ZFS-on-Linux does not need that.
> 
> Handle it by checking if 'volmode' option is needed by parsing usage
> information of the 'zfs get' command.
> ---
>  configure.ac                      |  8 ------
>  src/storage/storage_backend_zfs.c | 51 +++++++++++++++++++++++++++++++++++++--
>  2 files changed, 49 insertions(+), 10 deletions(-)
> 

Caveat - I know very little about zfs, FreeBSD...  But it's languishing
so...

> diff --git a/configure.ac b/configure.ac
> index a566f5b..d768341 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -2005,14 +2005,6 @@ if test "$with_storage_gluster" = "yes"; then
>  fi
>  AM_CONDITIONAL([WITH_STORAGE_GLUSTER], [test "$with_storage_gluster" = "yes"])
>  
> -if test "$with_storage_zfs" = "check"; then
> -    with_storage_zfs=$with_freebsd
> -fi
> -
> -if test "$with_storage_zfs" = "yes" && test "$with_freebsd" = "no"; then
> -    AC_MSG_ERROR([The ZFS storage driver can be enabled on FreeBSD only.])
> -fi
> -
>  if test "$with_storage_zfs" = "yes" ||
>     test "$with_storage_zfs" = "check"; then
>    AC_PATH_PROG([ZFS], [zfs], [], [$PATH:/sbin:/usr/sbin])

So it would seem this hunk and patch 2 would be more related.  That is
the virsh WITH_STORAGE_ZFS. The order of the patches is up to you.


> diff --git a/src/storage/storage_backend_zfs.c b/src/storage/storage_backend_zfs.c
> index cb2662a..6bf7963 100644
> --- a/src/storage/storage_backend_zfs.c
> +++ b/src/storage/storage_backend_zfs.c
> @@ -39,6 +39,47 @@ VIR_LOG_INIT("storage.storage_backend_zfs");
>   *       for size, show just a number instead of 2G etc
>   */
>  
> +/**
> + * virStorageBackendZFSVolModeNeeded:
> + *
> + * Checks if it's necessary to specify 'volmode' (i.e. that
> + * we're working with BSD ZFS implementation).
> + *
> + * Returns 1 if 'volmode' is need, 0 if not needed, -1 on error
> + */

Does volmode only exist in/on one environment? FreeBSD?

> +static int
> +virStorageBackendZFSVolModeNeeded(void)
> +{
> +    virCommandPtr cmd = NULL;
> +    int ret = -1, exit = -1;
> +    char *error = NULL;
> +
> +    /* 'zfs get' without arguments prints out
> +     * usage information to stderr, including
> +     * list of supported options, and exits with
> +     * exit code 2
> +     */
> +    cmd = virCommandNewArgList(ZFS, "get", NULL);
> +    virCommandAddEnvString(cmd, "LC_ALL=C");
> +    virCommandSetErrorBuffer(cmd, &error);

Why isn't this something like zfs get volmode $target? (examples that
google returns are tank/<something>).  And I would assume $target would
be pool->def->source.name & vol->name (from the caller).

It would seem that all that's being checking is whether the variable
exists "somewhere" (in "some" $target) as opposed to the specific one.

Also from what I read volmode can have many settings.

> +
> +    ret = virCommandRun(cmd, &exit);
> +    if ((ret < 0) || (exit != 2)) {
> +        VIR_WARN("Command 'zfs get' either failed "
> +                 "to run or exited with unexpected status");
> +        goto cleanup;
> +    }
> +
> +    if (strstr(error, " volmode "))
> +        ret = 1;
> +    else
> +        ret = 0;
> +
> + cleanup:
> +    virCommandFree(cmd);
> +    VIR_FREE(error);
> +    return ret;
> +}
>  
>  static int
>  virStorageBackendZFSCheckPool(virStoragePoolObjPtr pool ATTRIBUTE_UNUSED,
> @@ -258,6 +299,7 @@ virStorageBackendZFSCreateVol(virConnectPtr conn ATTRIBUTE_UNUSED,
>  {
>      virCommandPtr cmd = NULL;
>      int ret = -1;
> +    int volmode_needed = -1;
>  
>      vol->type = VIR_STORAGE_VOL_BLOCK;
>  
> @@ -273,6 +315,9 @@ virStorageBackendZFSCreateVol(virConnectPtr conn ATTRIBUTE_UNUSED,
>      if (VIR_STRDUP(vol->key, vol->target.path) < 0)
>          goto cleanup;
>  
> +    volmode_needed = virStorageBackendZFSVolModeNeeded();
> +    if (volmode_needed < 0)
> +        goto cleanup;
>      /**
>       * $ zfs create -o volmode=dev -V 10240K test/volname
>       *
> @@ -281,8 +326,10 @@ virStorageBackendZFSCreateVol(virConnectPtr conn ATTRIBUTE_UNUSED,
>       *                   will lookup vfs.zfs.vol.mode sysctl value
>       * -V -- tells to create a volume with the specified size
>       */
> -    cmd = virCommandNewArgList(ZFS, "create", "-o", "volmode=dev",
> -                               "-V", NULL);
> +    cmd = virCommandNewArgList(ZFS, "create", NULL);

Why not just encase this in

#if WITH_FREEBSD
    virCommandAddArgList(cmd, "-o", "volmode=dev", NULL);
#endif

Although perhaps there's some that may not like that approach... It just
seems 'safer' than a get command which searches the output for the
parameter being present. Who knows what implications for those "other"
Linuxes that are about to be allowed to use ZFS (or just were depending
on patch order).

John
> +    if (volmode_needed)
> +        virCommandAddArgList(cmd, "-o", "volmode=dev", NULLq);
> +    virCommandAddArg(cmd, "-V");
>      virCommandAddArgFormat(cmd, "%lluK",
>                             VIR_DIV_UP(vol->target.capacity, 1024));
>      virCommandAddArgFormat(cmd, "%s/%s",
>