Re: [libvirt] [PATCH RFC] storage: ZFS support

[Roman Bogorodskiy]

  Ján Tomko wrote:

> On 07/22/2014 07:08 PM, Roman Bogorodskiy wrote:
> > Implement ZFS storage backend driver. Currently supported
> > only on FreeBSD because of ZFS limitations on Linux.
> > 
> > Features supported:
> > 
> >  - pool-start, pool-stop
> >  - pool-info
> >  - vol-list
> >  - vol-create / vol-delete
> > 
> > Pool definition looks like that:
> > 
> >  
> >   myzfspool
> >   
> > actualpoolname
> >   
> >  
> > 
> > The 'actualpoolname' value is a name of the pool on the system,
> > such as shown by 'zpool list' command. Target makes no sense
> > here because volumes path is always /dev/zvol/$poolname/$volname.
> > 
> > Users has to create a pool on his own, this driver doesn't
> > support pool creation currently.
> > 
> > A volume could be used with Qemu by adding an entry like this:
> > 
> > 
> >   
> >   
> >   
> > 
> > ---
> 
> Looks good to me, just a few nits below:

Thanks for the review!

I'll address the issues pointed here.

> Also, is source.name/vol.name unique enough, wouldn't the full target.path be
> better?

I thought about that, but wasn't sure if it's a good thing to use
absolute device path. On the other hand, it's hardcoded anyway, so,
probably it's not a big deal.

PS I've been looking at ZFS support on Linux. I had no success with
zfs-fuse (when I was creating a volume it reported it's not supported
and segfaulted). Then I tried zfsonlinux and it looked more stable and
supported the basic operations I've tested. Unfortunately, it misses
some handy command args, such as -H and -p for some commands to provide
scriptable output... Probably I'll try to workaround that when I have
time.

Roman Bogorodskiy


pgpepyvtg653M.pgp
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH RFC] storage: ZFS support

[Ján Tomko]

On 07/22/2014 07:08 PM, Roman Bogorodskiy wrote:
> Implement ZFS storage backend driver. Currently supported
> only on FreeBSD because of ZFS limitations on Linux.
> 
> Features supported:
> 
>  - pool-start, pool-stop
>  - pool-info
>  - vol-list
>  - vol-create / vol-delete
> 
> Pool definition looks like that:
> 
>  
>   myzfspool
>   
> actualpoolname
>   
>  
> 
> The 'actualpoolname' value is a name of the pool on the system,
> such as shown by 'zpool list' command. Target makes no sense
> here because volumes path is always /dev/zvol/$poolname/$volname.
> 
> Users has to create a pool on his own, this driver doesn't
> support pool creation currently.
> 
> A volume could be used with Qemu by adding an entry like this:
> 
> 
>   
>   
>   
> 
> ---

Looks good to me, just a few nits below:

>  configure.ac  |  43 +
>  include/libvirt/libvirt.h.in  |   1 +
>  po/POTFILES.in|   1 +
>  src/Makefile.am   |   8 +
>  src/conf/storage_conf.c   |  11 +-
>  src/conf/storage_conf.h   |   4 +-
>  src/qemu/qemu_conf.c  |   1 +
>  src/storage/storage_backend.c |   6 +
>  src/storage/storage_backend_zfs.c | 344 
> ++
>  src/storage/storage_backend_zfs.h |  29 
>  src/storage/storage_driver.c  |   1 +
>  tools/virsh-pool.c|   3 +
>  12 files changed, 449 insertions(+), 3 deletions(-)
>  create mode 100644 src/storage/storage_backend_zfs.c
>  create mode 100644 src/storage/storage_backend_zfs.h
> 

Missing additions to docs/schemas and docs/storage.html.in

> diff --git a/src/storage/storage_backend_zfs.c 
> b/src/storage/storage_backend_zfs.c
> new file mode 100644
> index 000..1079bc7
> --- /dev/null
> +++ b/src/storage/storage_backend_zfs.c
> @@ -0,0 +1,344 @@
> +/*
> + * storage_backend_zfs.c: storage backend for ZFS handling
> + *
> + * Copyright (C) 2014 Roman Bogorodskiy
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library.  If not, see
> + * .
> + *
> + */
> +
> +#include 
> +
> +#include "viralloc.h"
> +#include "virerror.h"
> +#include "virfile.h"
> +#include "storage_backend_zfs.h"
> +#include "virlog.h"
> +#include "virstring.h"
> +
> +#define VIR_FROM_THIS VIR_FROM_STORAGE
> +
> +VIR_LOG_INIT("storage.storage_backend_zfs");
> +
> +/*
> + * Some common flags of zfs and zpool commands we use:
> + * -H -- don't print headers and separate fields by tab
> + * -p -- show exact numbers instead of human-readable, i.e.
> + *   for size, show just a number instead of 2G etc
> + */
> +
> +
> +static int
> +virStorageBackendZFSCheckPool(virConnectPtr conn ATTRIBUTE_UNUSED,
> +  virStoragePoolObjPtr pool ATTRIBUTE_UNUSED,
> +  bool *isActive)
> +{
> +char *devpath;
> +
> +if (virAsprintf(&devpath, "/dev/zvol/%s",
> +pool->def->source.name) == -1)
> +return -1;
> +*isActive = virFileIsDir(devpath);
> +VIR_FREE(devpath);
> +
> +return 0;
> +}
> +

> +static int
> +virStorageBackendZFSStartPool(virConnectPtr conn ATTRIBUTE_UNUSED,
> +  virStoragePoolObjPtr pool ATTRIBUTE_UNUSED)
> +{
> +return 0;
> +}

This doesn't need to be implemented if it doesn't do anything (same for 
StopPool)

> +static int
> +virStorageBackendZFSRefreshPool(virConnectPtr conn ATTRIBUTE_UNUSED,
> +virStoragePoolObjPtr pool ATTRIBUTE_UNUSED)
> +{
> +virCommandPtr cmd = NULL;
> +char *zpool_props = NULL;
> +char **lines = NULL;
> +size_t i;
> +
> +/**
> + * $ zpool get -Hp health,size,free,allocated test
> + * testhealth  ONLINE  -
> + * testsize199715979264-
> + * testfree198899976704-
> + * testallocated   816002560   -
> + * $
> + *
> + * Here we just provide a list of properties we want to see
> + */
> +cmd = virCommandNewArgList(ZPOOL,
> +   "get", "-Hp",
> +   "health,size,free,allocated",
> +   pool->def->source.name,
> +   NULL);
> +virCommandSetOutputBuffer(cmd, &zpool_props);
> +if (virCommandRun(cmd, NULL) < 0)
> +goto cleanup;
> +
> 

[libvirt] [PATCH RFC] storage: ZFS support

[Roman Bogorodskiy]

Implement ZFS storage backend driver. Currently supported
only on FreeBSD because of ZFS limitations on Linux.

Features supported:

 - pool-start, pool-stop
 - pool-info
 - vol-list
 - vol-create / vol-delete

Pool definition looks like that:

 
  myzfspool
  
actualpoolname
  
 

The 'actualpoolname' value is a name of the pool on the system,
such as shown by 'zpool list' command. Target makes no sense
here because volumes path is always /dev/zvol/$poolname/$volname.

Users has to create a pool on his own, this driver doesn't
support pool creation currently.

A volume could be used with Qemu by adding an entry like this:


  
  
  

---
 configure.ac  |  43 +
 include/libvirt/libvirt.h.in  |   1 +
 po/POTFILES.in|   1 +
 src/Makefile.am   |   8 +
 src/conf/storage_conf.c   |  11 +-
 src/conf/storage_conf.h   |   4 +-
 src/qemu/qemu_conf.c  |   1 +
 src/storage/storage_backend.c |   6 +
 src/storage/storage_backend_zfs.c | 344 ++
 src/storage/storage_backend_zfs.h |  29 
 src/storage/storage_driver.c  |   1 +
 tools/virsh-pool.c|   3 +
 12 files changed, 449 insertions(+), 3 deletions(-)
 create mode 100644 src/storage/storage_backend_zfs.c
 create mode 100644 src/storage/storage_backend_zfs.h

diff --git a/configure.ac b/configure.ac
index f37c716..7e46460 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1734,6 +1734,10 @@ AC_ARG_WITH([storage-gluster],
   [AS_HELP_STRING([--with-storage-gluster],
 [with Gluster backend for the storage driver @<:@default=check@:>@])],
   [],[with_storage_gluster=check])
+AC_ARG_WITH([storage-zfs],
+  [AS_HELP_STRING([--with-storage-zfs],
+[with ZFS backend for the storage driver @<:@default=check@:>@])],
+  [],[with_storage_zfs=check])
 
 if test "$with_libvirtd" = "no"; then
   with_storage_dir=no
@@ -1746,6 +1750,7 @@ if test "$with_libvirtd" = "no"; then
   with_storage_rbd=no
   with_storage_sheepdog=no
   with_storage_gluster=no
+  with_storage_zfs=no
 fi
 if test "$with_storage_dir" = "yes" ; then
   AC_DEFINE_UNQUOTED([WITH_STORAGE_DIR], 1, [whether directory backend for 
storage driver is enabled])
@@ -1963,6 +1968,43 @@ 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])
+  AC_PATH_PROG([ZPOOL], [zpool], [], [$PATH:/sbin:/usr/sbin])
+
+  if test "$with_storage_zfs" = "yes"; then
+if test -z "$ZFS" || test -z "$ZPOOL"; then
+  AC_MSG_ERROR([We need zfs and zpool for ZFS storage driver])
+fi
+  else
+if test -z "$ZFS" || test -z "$ZPOOL"; then
+  with_storage_zfs=no
+fi
+
+if test "$with_storage_zfs" = "check"; then
+  with_storage_zfs=yes
+fi
+  fi
+
+  if test "$with_storage_zfs" = "yes"; then
+AC_DEFINE_UNQUOTED([WITH_STORAGE_ZFS], 1,
+  [whether ZFS backend for storage driver is enabled])
+AC_DEFINE_UNQUOTED([ZFS], ["$ZFS"], [Location of zfs program])
+AC_DEFINE_UNQUOTED([ZPOOL], ["$ZPOOL"], [Location of zpool program])
+  fi
+fi
+AM_CONDITIONAL([WITH_STORAGE_ZFS],
+  [test "$with_storage_zfs" = "yes"])
+
 if test "$with_storage_fs" = "yes" ||
test "$with_storage_gluster" = "yes"; then
   AC_PATH_PROG([GLUSTER_CLI], [gluster], [], [$PATH:/sbin:/usr/sbin])
@@ -2806,6 +2848,7 @@ AC_MSG_NOTICE([Disk: $with_storage_disk])
 AC_MSG_NOTICE([ RBD: $with_storage_rbd])
 AC_MSG_NOTICE([Sheepdog: $with_storage_sheepdog])
 AC_MSG_NOTICE([ Gluster: $with_storage_gluster])
+AC_MSG_NOTICE([ ZFS: $with_storage_zfs])
 AC_MSG_NOTICE([])
 AC_MSG_NOTICE([Security Drivers])
 AC_MSG_NOTICE([])
diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
index ad6785f..47ea695 100644
--- a/include/libvirt/libvirt.h.in
+++ b/include/libvirt/libvirt.h.in
@@ -3170,6 +3170,7 @@ typedef enum {
 VIR_CONNECT_LIST_STORAGE_POOLS_RBD   = 1 << 14,
 VIR_CONNECT_LIST_STORAGE_POOLS_SHEEPDOG  = 1 << 15,
 VIR_CONNECT_LIST_STORAGE_POOLS_GLUSTER   = 1 << 16,
+VIR_CONNECT_LIST_STORAGE_POOLS_ZFS   = 1 << 17,
 } virConnectListAllStoragePoolsFlags;
 
 int virConnectListAllStoragePools(virConnectPtr conn,
diff --git a/po/POTFILES.in b/po/POTFILES.in
index d10e892..2646bcc 100644
--- a/po/POTFILES.in
+++ b/po/POTFILES.in
@@ -148,6 +148,7 @@ src/storage/storage_backend_mpath.c
 src/storage/storage_backend_rbd.c
 src/storage/storage_backend_scsi.c
 src/storage/storage_backend_sheepdog.c
+src/storage/storage_backend_zfs.c
 src/sto