Re: [libvirt] [PATCH v2 2/2] storage: btrfs subvolumes for directory pools
Thanks for the review,
10.10.2013 18:57, Daniel P. Berrange kirjoitti:
On Thu, Oct 10, 2013 at 05:52:22PM +0300, Oskari Saarenmaa wrote:
> +if test "$with_storage_btrfs" = "yes" || test
"$with_storage_btrfs" = "check"; then
> + AC_PATH_PROG([BTRFS], [btrfs], [], [$PATH:/sbin:/usr/sbin])
[...]
> +AM_CONDITIONAL([WITH_STORAGE_BTRFS], [test
"$with_storage_btrfs" = "yes"])
Just use 'WITH_BTRFS' here - the WITH_STORAGE_XXXX is for actual
storage backends.
Do we actually need a flag for enabling / disabling btrfs usage as it's
not a separate storage pool? I'm thinking about just calling
AC_PATH_PROG(btrfs) if FS pool is enabled, and using #ifdef BTRFS in the
fs code to use btrfs if available.
> --- 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.
Ok, I'll put this thing somewhere else.
> + 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.
In btrfs's case the note about backing store in an existing subvolume is
informational only; all subvolumes, snapshots or not, are independent
and in case we can't find a symbolic name for the parent volume I think
it's safe to just ignore it. I'll add this note to the comment.
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.
I'm not sure it makes sense to do this until there's another user for
btrfs commands in libvirt, but I can do it if you like. I think it'd be
more useful to have a module which implements copy-on-write operations
for different filesystems (btrfs, zfs, something else?) with a common api.
> 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.
Hmm.. I don't see such a field in virStorageVolType - am I looking at a
wrong enum?
> 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>
I suppose that makes sense, but it would make this incompatible with
existing volume creation. Right now (with the current patch) I can use
existing virsh tooling to run, for example 'virsh vol-create-as default
guest-1 0 --format volume --backing-store guest-base'
> 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
I'll look into this.
Thanks,
Oskari