On Thu, Oct 10, 2013 at 05:52:22PM +0300, Oskari Saarenmaa wrote:
> This commit adds support for btrfs subvolumes as directory volumes. The
> directory storage pool can be used to manage btrfs subvolumes on an existing
> btrfs filesystem. The driver can create new blank subvolumes and snapshots
> of existing subvolumes as well as delete existing subvolumes.
>
> The subvolumes created are automatically made visible on the host side
> and can be attached to domains using the <filesystem> tags as defined in
> 'format domain' documentation.
>
> Subvolumes do not implement quotas at the moment because the current
> (btrfs-progs-0.20.rc1.20130501git7854c8b-4.fc20.x86_64) support for quota
> management in btrfs-progs is lacking the necessary features, for example
> it's not possible to see the quota assigned to a certain subvolume and
> usage information is only updated on syncfs(2). Quota support will be
> implemented once the tools gain the necessary features.
>
> Signed-off-by: Oskari Saarenmaa <os@xxxxxxx>
> ---
> I did not add a new virDirIsExecutable function as it wasn't really needed
> by this patch set, it's sufficient to just verify that the backing volume
> is a directory before running btrfs subvolume snapshot.
>
> Part 1/2 still applies cleanly on today's git master so not resending it.
>
> configure.ac | 25 ++-
> docs/schemas/storagevol.rng | 1 +
> docs/storage.html.in | 30 ++-
> libvirt.spec.in | 4 +
> src/conf/storage_conf.c | 3 +
> src/conf/storage_conf.h | 1 +
> src/storage/storage_backend.c | 15 +-
> src/storage/storage_backend_fs.c | 219 ++++++++++++++++++++--
> src/util/virstoragefile.c | 4 +-
> src/util/virstoragefile.h | 1 +
> tests/storagevolxml2xmlin/vol-btrfs-snapshot.xml | 13 ++
> tests/storagevolxml2xmlin/vol-btrfs.xml | 9 +
> tests/storagevolxml2xmlout/vol-btrfs-snapshot.xml | 26 +++
> tests/storagevolxml2xmlout/vol-btrfs.xml | 17 ++
> tests/storagevolxml2xmltest.c | 2 +
> 15 files changed, 340 insertions(+), 30 deletions(-)
> create mode 100644 tests/storagevolxml2xmlin/vol-btrfs-snapshot.xml
> create mode 100644 tests/storagevolxml2xmlin/vol-btrfs.xml
> create mode 100644 tests/storagevolxml2xmlout/vol-btrfs-snapshot.xml
> create mode 100644 tests/storagevolxml2xmlout/vol-btrfs.xml
>
> diff --git a/configure.ac b/configure.ac
> index 1993fab..a82640c 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -1646,6 +1646,10 @@ AC_ARG_WITH([storage-sheepdog],
> [AS_HELP_STRING([--with-storage-sheepdog],
> [with Sheepdog backend for the storage driver @<:@default=check@:>@])],
> [],[with_storage_sheepdog=check])
> +AC_ARG_WITH([storage-btrfs],
> + [AS_HELP_STRING([--with-storage-btrfs],
> + [with Btrfs backend for the storage driver @<:@default=check@:>@])],
> + [],[with_storage_btrfs=check])
We no longer have a backend for btrfs - it is just a part of the fs driver.
>
> if test "$with_libvirtd" = "no"; then
> with_storage_dir=no
> @@ -1858,6 +1862,24 @@ fi
> AM_CONDITIONAL([WITH_STORAGE_SHEEPDOG],
> [test "$with_storage_sheepdog" = "yes"])
>
> +if test "$with_storage_btrfs" = "yes" || test "$with_storage_btrfs" = "check"; then
> + AC_PATH_PROG([BTRFS], [btrfs], [], [$PATH:/sbin:/usr/sbin])
> +
> + if test "$with_storage_btrfs" = "yes" ; then
> + if test -z "$BTRFS" ; then AC_MSG_ERROR([We need btrfs -command for btrfs storage driver]) ; fi
> + else
> + if test -z "$BTRFS" ; then with_storage_btrfs=no ; fi
> + if test "$with_storage_btrfs" = "check" ; then with_storage_btrfs=yes ; fi
> + fi
> +
> + if test "$with_storage_btrfs" = "yes" ; then
> + AC_DEFINE_UNQUOTED([WITH_STORAGE_BTRFS], 1, [whether Btrfs backend for storage driver is enabled])
> + AC_DEFINE_UNQUOTED([BTRFS],["$BTRFS"],[Location of btrfs program])
> + fi
> +fi
> +AM_CONDITIONAL([WITH_STORAGE_BTRFS], [test "$with_storage_btrfs" = "yes"])
Just use 'WITH_BTRFS' here - the WITH_STORAGE_XXXX is for actual
storage backends.
> +
> +
>
> LIBPARTED_CFLAGS=
> LIBPARTED_LIBS=
> @@ -1945,7 +1967,7 @@ AC_SUBST([DEVMAPPER_CFLAGS])
> AC_SUBST([DEVMAPPER_LIBS])
>
> with_storage=no
> -for backend in dir fs lvm iscsi scsi mpath rbd disk; do
> +for backend in dir fs lvm iscsi scsi mpath rbd disk btrfs; do
> if eval test \$with_storage_$backend = yes; then
> with_storage=yes
> break
> @@ -2670,6 +2692,7 @@ AC_MSG_NOTICE([ mpath: $with_storage_mpath])
> AC_MSG_NOTICE([ Disk: $with_storage_disk])
> AC_MSG_NOTICE([ RBD: $with_storage_rbd])
> AC_MSG_NOTICE([Sheepdog: $with_storage_sheepdog])
> +AC_MSG_NOTICE([ Btrfs: $with_storage_btrfs])
Again not needed here.
> AC_MSG_NOTICE([])
> AC_MSG_NOTICE([Security Drivers])
> AC_MSG_NOTICE([])
> diff --git a/docs/schemas/storagevol.rng b/docs/schemas/storagevol.rng
> index 8bc5907..8814973 100644
> --- a/docs/schemas/storagevol.rng
> +++ b/docs/schemas/storagevol.rng
> @@ -201,6 +201,7 @@
> <value>qed</value>
> <value>vmdk</value>
> <value>vpc</value>
> + <value>volume</value>
> </choice>
> </define>
>
> diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
> index 975e662..b349c3f 100644
> --- a/src/conf/storage_conf.c
> +++ b/src/conf/storage_conf.c
> @@ -314,11 +314,14 @@ virStorageVolDefFree(virStorageVolDefPtr def)
> VIR_FREE(def->source.extents);
>
> VIR_FREE(def->target.compat);
> + VIR_FREE(def->target.uuid);
> virBitmapFree(def->target.features);
> VIR_FREE(def->target.path);
> VIR_FREE(def->target.perms.label);
> VIR_FREE(def->target.timestamps);
> virStorageEncryptionFree(def->target.encryption);
> + VIR_FREE(def->backingStore.compat);
> + VIR_FREE(def->backingStore.uuid);
> VIR_FREE(def->backingStore.path);
> VIR_FREE(def->backingStore.perms.label);
> VIR_FREE(def->backingStore.timestamps);
> diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h
> index 62ff1fd..6414c74 100644
> --- a/src/conf/storage_conf.h
> +++ b/src/conf/storage_conf.h
> @@ -90,6 +90,7 @@ struct _virStorageVolTarget {
> virStorageEncryptionPtr encryption;
> virBitmapPtr features;
> char *compat;
> + char *uuid;
> };
This struct represents the public XML data format - adding random fields
to it for individual driver use is not allowed.
> diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c
> index f9b0326..2cf300f 100644
> --- a/src/storage/storage_backend_fs.c
> +++ b/src/storage/storage_backend_fs.c
> @@ -51,6 +51,7 @@
> #include "virfile.h"
> #include "virlog.h"
> #include "virstring.h"
> +#include "dirname.h"
>
> #define VIR_FROM_THIS VIR_FROM_STORAGE
>
> @@ -801,6 +802,33 @@ error:
> }
>
>
> +#if WITH_STORAGE_BTRFS
> +static int
> +btrfsVolInfo(virStoragePoolObjPtr pool ATTRIBUTE_UNUSED,
> + char **groups,
> + void *data)
> +{
> + virStorageVolDefPtr vol = data;
> +
> + if (STREQ(groups[0], "uuid")) {
> + vol->type = VIR_STORAGE_VOL_DIR;
> + vol->target.format = VIR_STORAGE_FILE_VOLUME;
> + VIR_FREE(vol->target.uuid);
> + if (VIR_STRDUP(vol->target.uuid, groups[1]) < 0)
> + return -1;
> + } else if (STREQ(groups[0], "Parent uuid") && STRNEQ(groups[1], "-")) {
> + vol->backingStore.format = VIR_STORAGE_FILE_VOLUME;
> + VIR_FREE(vol->backingStore.path);
> + vol->backingStore.path = NULL;
> + VIR_FREE(vol->backingStore.uuid);
> + if (VIR_STRDUP(vol->backingStore.uuid, groups[1]) < 0)
> + return -1;
> + }
> +
> + return 0;
> +}
> +#endif
> +
> /**
> * Iterate over the pool's directory and enumerate all disk images
> * within it. This is non-recursive.
> @@ -809,10 +837,17 @@ static int
> virStorageBackendFileSystemRefresh(virConnectPtr conn ATTRIBUTE_UNUSED,
> virStoragePoolObjPtr pool)
> {
> - DIR *dir;
> + int ret = -1;
> + DIR *dir = NULL;
> struct dirent *ent;
> struct statvfs sb;
> virStorageVolDefPtr vol = NULL;
> +#if WITH_STORAGE_BTRFS
> + int missing_subvolume_info = 0;
> + char *fstype = NULL;
> +
> + fstype = virFileFsType(pool->def->target.path);
> +#endif
>
> if (!(dir = opendir(pool->def->target.path))) {
> virReportSystemError(errno,
> @@ -822,7 +857,7 @@ virStorageBackendFileSystemRefresh(virConnectPtr conn ATTRIBUTE_UNUSED,
> }
>
> while ((ent = readdir(dir)) != NULL) {
> - int ret;
> + int probe;
> char *backingStore;
> int backingStoreFormat;
>
> @@ -842,19 +877,19 @@ virStorageBackendFileSystemRefresh(virConnectPtr conn ATTRIBUTE_UNUSED,
> if (VIR_STRDUP(vol->key, vol->target.path) < 0)
> goto cleanup;
>
> - if ((ret = virStorageBackendProbeTarget(&vol->target,
> - &backingStore,
> - &backingStoreFormat,
> - &vol->allocation,
> - &vol->capacity,
> - &vol->target.encryption)) < 0) {
> - if (ret == -2) {
> + if ((probe = virStorageBackendProbeTarget(&vol->target,
> + &backingStore,
> + &backingStoreFormat,
> + &vol->allocation,
> + &vol->capacity,
> + &vol->target.encryption)) < 0) {
> + if (probe == -2) {
> /* Silently ignore non-regular files,
> * eg '.' '..', 'lost+found', dangling symbolic link */
> virStorageVolDefFree(vol);
> vol = NULL;
> continue;
> - } else if (ret == -3) {
> + } else if (probe == -3) {
> /* The backing file is currently unavailable, its format is not
> * explicitly specified, the probe to auto detect the format
> * failed: continue with faked RAW format, since AUTO will
> @@ -865,6 +900,23 @@ virStorageBackendFileSystemRefresh(virConnectPtr conn ATTRIBUTE_UNUSED,
> goto cleanup;
> }
>
> +#if WITH_STORAGE_BTRFS
> + /* check for subvolumes */
> + if (vol->target.format == VIR_STORAGE_FILE_DIR &&
> + fstype != NULL && STREQ(fstype, "btrfs")) {
> + int vars[] = {2};
> + const char *regexes[] = {"^\\s*([A-Za-z ]+):\\s*(.+)\\s*$"};
> + virCommandPtr cmd = virCommandNewArgList(
> + "btrfs", "subvolume", "show", vol->target.path, NULL);
> + if (cmd == NULL)
> + goto cleanup;
> + virStorageBackendRunProgRegex(NULL, cmd, 1, regexes, vars,
> + btrfsVolInfo, vol, NULL);
> + if (vol->backingStore.format == VIR_STORAGE_FILE_VOLUME)
> + missing_subvolume_info ++;
You're incrementing this multiple times
> + }
> +#endif
> +
> /* directory based volume */
> if (vol->target.format == VIR_STORAGE_FILE_DIR)
> vol->type = VIR_STORAGE_VOL_DIR;
> @@ -896,13 +948,44 @@ virStorageBackendFileSystemRefresh(virConnectPtr conn ATTRIBUTE_UNUSED,
> vol = NULL;
> }
> closedir(dir);
> -
> + dir = NULL;
> +
> +#if WITH_STORAGE_BTRFS
> + if (missing_subvolume_info) {
But only checking for non-zero, so this should be a 'bool' not 'int'
> + /* update snapshot information */
> + virStorageVolDefPtr volv, volb;
> + int idxv, idxb;
Use 'i' and 'j' for loop iterators and make them size_t.
> +
> + for (idxv = 0; idxv < pool->volumes.count; idxv ++) {
No whitespace before '++' operator
> + volv = pool->volumes.objs[idxv];
> + if (volv->backingStore.format == VIR_STORAGE_FILE_VOLUME &&
> + volv->backingStore.uuid != NULL) {
> + for (idxb = 0; idxb < pool->volumes.count; idxb ++) {
Again whitespace.
> + volb = pool->volumes.objs[idxb];
> + if (volb->target.format == VIR_STORAGE_FILE_VOLUME &&
> + volb->target.uuid != NULL &&
> + STREQ(volb->target.uuid, volv->backingStore.uuid)) {
> + if (VIR_STRDUP(volv->backingStore.path, volb->target.path) < 0)
> + goto cleanup;
> + break;
> + }
> + }
> + if (volv->backingStore.path == NULL) {
> + /* backing store not in the pool, ignore it */
Backing stores for volumes are not required to be in the same pool as
the source. For example it is valid to have a qcow2 file backed by
a lvm volume.
> +static int createVolumeDir(virConnectPtr conn ATTRIBUTE_UNUSED,
> + virStoragePoolObjPtr pool ATTRIBUTE_UNUSED,
> + virStorageVolDefPtr vol,
> + virStorageVolDefPtr inputvol,
> + unsigned int flags)
> +{
> + int ret = -1;
> + char *vol_dir_name = NULL;
> + char *fstype = NULL;
> + virCommandPtr cmd = NULL;
> + struct stat st;
> +
> + virCheckFlags(0, -1);
> +
> + if (inputvol) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("cannot copy from volume to a subvolume"));
> + return -1;
> + }
> +
> + if ((vol_dir_name = mdir_name(vol->target.path)) == NULL)
> + goto cleanup;
> +
> + fstype = virFileFsType(vol_dir_name);
> + if (fstype == NULL) {
> + virReportSystemError(errno,
> + _("cannot get filesystem type for %s"),
> + vol->target.path);
> + goto cleanup;
> + }
> +#if WITH_STORAGE_BTRFS
> + else if (STREQ(fstype, "btrfs")) {
> + cmd = virCommandNew("btrfs");
> + if (!cmd)
> + goto cleanup;
> +
> + if (vol->backingStore.path == NULL) {
> + virCommandAddArgList(cmd, "subvolume", "create", vol->target.path, NULL);
> + } else {
> + if (!virFileIsDir(vol->backingStore.path)) {
> + virReportError(VIR_ERR_OPERATION_INVALID,
> + _("backing store %s is not a directory"),
> + vol->backingStore.path);
> + goto cleanup;
> + }
> +
> + virCommandAddArgList(cmd, "subvolume", "snapshot", vol->backingStore.path,
> + vol->target.path, NULL);
> + }
> + if (virCommandRun(cmd, NULL) < 0)
> + goto cleanup;
> + }
> +#endif
> + else {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("subvolumes are not supported in %s"),
> + fstype);
> + goto cleanup;
> + }
> +
> + if (stat(vol->target.path, &st) < 0) {
> + virReportSystemError(errno,
> + _("failed to create %s"), vol->target.path);
> + goto cleanup;
> + }
> + ret = 0;
> +
> +cleanup:
> + VIR_FREE(vol_dir_name);
> + VIR_FREE(fstype);
> + virCommandFree(cmd);
> + return ret;
> +}
> +
> static int createFileDir(virConnectPtr conn ATTRIBUTE_UNUSED,
> virStoragePoolObjPtr pool,
> virStorageVolDefPtr vol,
> @@ -1061,6 +1229,8 @@ _virStorageBackendFileSystemVolBuild(virConnectPtr conn,
> create_func = virStorageBackendCreateRaw;
> } else if (vol->target.format == VIR_STORAGE_FILE_DIR) {
> create_func = createFileDir;
> + } else if (vol->target.format == VIR_STORAGE_FILE_VOLUME) {
> + create_func = createVolumeDir;
> } else if ((tool_type = virStorageBackendFindFSImageTool(NULL)) != -1) {
> create_func = virStorageBackendFSImageToolTypeToFunc(tool_type);
>
> @@ -1133,6 +1303,23 @@ virStorageBackendFileSystemVolDelete(virConnectPtr conn ATTRIBUTE_UNUSED,
> }
> break;
> case VIR_STORAGE_VOL_DIR:
> +#if WITH_STORAGE_BTRFS
> + if (vol->target.format == VIR_STORAGE_FILE_VOLUME) {
> + char *fstype = virFileFsType(vol->target.path);
> + if (fstype != NULL && STREQ(fstype, "btrfs")) {
> + virCommandPtr cmd = NULL;
> + int ret;
> +
> + cmd = virCommandNewArgList("btrfs", "subvolume", "delete",
> + vol->target.path, NULL);
> + ret = (virCommandRun(cmd, NULL) < 0) ? -1 : 0;
> + virCommandFree(cmd);
> + VIR_FREE(fstype);
> + return ret;
> + }
I'm thinking it would be nice to have a dedicate file with APIs for btrfs
eg src/util/virbtrfs.{h,c} and have it contain things like
virBtrFSCreateVolume(....)
virBtrFSCreateVolume(....)
virBtrFS...(....)
and so on, so we don't have all these virCommand calls littering this
file.
> diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
> index 48d5fbb..d0231b6 100644
> --- a/src/util/virstoragefile.c
> +++ b/src/util/virstoragefile.c
> @@ -57,7 +57,7 @@ VIR_ENUM_IMPL(virStorageFileFormat,
> "raw", "dir", "bochs",
> "cloop", "cow", "dmg", "iso",
> "qcow", "qcow2", "qed", "vmdk", "vpc",
> - "fat", "vhd", "vdi")
> + "fat", "vhd", "vdi", "volume")
>
> VIR_ENUM_IMPL(virStorageFileFeature,
> VIR_STORAGE_FILE_FEATURE_LAST,
> @@ -232,6 +232,8 @@ static struct FileTypeInfo const fileTypeInfo[] = {
> -1, {0}, 0, 0, 0, 0, NULL, NULL },
> [VIR_STORAGE_FILE_VHD] = { 0, NULL, NULL, LV_LITTLE_ENDIAN,
> -1, {0}, 0, 0, 0, 0, NULL, NULL },
> + [VIR_STORAGE_FILE_VOLUME] = { 0, NULL, NULL, LV_LITTLE_ENDIAN,
> + -1, {0}, 0, 0, 0, 0, NULL, NULL },
> };
> verify(ARRAY_CARDINALITY(fileTypeInfo) == VIR_STORAGE_FILE_LAST);
>
> diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
> index d5effa4..4dc6c81 100644
> --- a/src/util/virstoragefile.h
> +++ b/src/util/virstoragefile.h
> @@ -46,6 +46,7 @@ enum virStorageFileFormat {
> VIR_STORAGE_FILE_FAT,
> VIR_STORAGE_FILE_VHD,
> VIR_STORAGE_FILE_VDI,
> + VIR_STORAGE_FILE_VOLUME,
This isn't right - volumes already have a type={file,block,dir,volume}
and we shouldn't duplicate this in the format attribute too.
>
> VIR_STORAGE_FILE_LAST,
> };
> diff --git a/tests/storagevolxml2xmlin/vol-btrfs-snapshot.xml b/tests/storagevolxml2xmlin/vol-btrfs-snapshot.xml
> new file mode 100644
> index 0000000..5a58b56
> --- /dev/null
> +++ b/tests/storagevolxml2xmlin/vol-btrfs-snapshot.xml
> @@ -0,0 +1,13 @@
> +<volume>
> + <name>clone</name>
> + <capacity unit="bytes">0</capacity>
> + <allocation unit="bytes">0</allocation>
> + <target>
> + <path>/lxc/clone</path>
> + <format type='volume'/>
> + </target>
> + <backingStore>
> + <path>/lxc/vanilla</path>
> + <format type='volume'/>
> + </backingStore>
> +</volume>
Using 'format' in this way is wrong. What we should be doing
is exposing the volume 'type' as an attribute eg
<volume type='volume'>
...
</volume>
> diff --git a/tests/storagevolxml2xmlout/vol-btrfs-snapshot.xml b/tests/storagevolxml2xmlout/vol-btrfs-snapshot.xml
> new file mode 100644
> index 0000000..75830d3
> --- /dev/null
> +++ b/tests/storagevolxml2xmlout/vol-btrfs-snapshot.xml
> @@ -0,0 +1,26 @@
> +<volume>
> + <name>clone</name>
> + <key>(null)</key>
If you're getting (null) here, then something has gone wrong
> + <source>
> + </source>
> + <capacity unit='bytes'>0</capacity>
> + <allocation unit='bytes'>0</allocation>
> + <target>
> + <path>/lxc/clone</path>
> + <format type='volume'/>
> + <permissions>
> + <mode>0600</mode>
> + <owner>4294967295</owner>
> + <group>4294967295</group>
> + </permissions>
> + </target>
> + <backingStore>
> + <path>/lxc/vanilla</path>
> + <format type='volume'/>
> + <permissions>
> + <mode>0600</mode>
> + <owner>4294967295</owner>
> + <group>4294967295</group>
> + </permissions>
> + </backingStore>
> +</volume>
> diff --git a/tests/storagevolxml2xmlout/vol-btrfs.xml b/tests/storagevolxml2xmlout/vol-btrfs.xml
> new file mode 100644
> index 0000000..fbad915
> --- /dev/null
> +++ b/tests/storagevolxml2xmlout/vol-btrfs.xml
> @@ -0,0 +1,17 @@
> +<volume>
> + <name>vanilla</name>
> + <key>(null)</key>
Again.
> + <source>
> + </source>
> + <capacity unit='bytes'>0</capacity>
> + <allocation unit='bytes'>0</allocation>
> + <target>
> + <path>/lxc/vanilla</path>
> + <format type='volume'/>
> + <permissions>
> + <mode>0600</mode>
> + <owner>4294967295</owner>
> + <group>4294967295</group>
> + </permissions>
> + </target>
> +</volume>
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list