[libvirt] [PATCH v2 2/2] storage: btrfs subvolumes for directory pools

[Oskari Saarenmaa]

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