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

[Roman Bogorodskiy]

  John Ferlan wrote:

> 
> 
> 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...

Heh. Thanks for stepping in! :)

> > 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.

Actually, these two patches are unrelated. I should have been added the
virsh part when I added ZFS driver in the first place, but I forgot
about it at that time.

I remembered about it when I was testing my configure.ac changes on
Linux and saw that virsh -V output does not change.

> > 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.

No, the goal is to check if "volmode" option is supported by the ZFS
implementation we're using. So we don't need any specific information,
just parse usage/help information that 'zfs get' prints out.

In other words, 'zfs get' does not try to receive any information about
the actual pools, volumes etc, its output is always same.

> Also from what I read volmode can have many settings.

Yeah, though if it's available, we always set it to 'dev', and if it's
not available, we just don't care. No need to worry about other possible
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).

Yeah, that's the first idea that I had.

However, I checked the Wikipedia page to see how many OSes support ZFS
and saw a quite lengthy list, including GNU/kFreeBSD, NetBSD, OSX and
SmartOS. I don't know if their implementation is close to FreeBSD's, but
decided that as this feature is not very hard to probe in run-time, I'd
avoid OS-specific quirks.

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

Roman Bogorodskiy