On 01/25/2016 08:37 PM, Roman Bogorodskiy wrote:
> 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! :)
>
Sorry for the delay - backlog grows fast these days.
>>> 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.
>
It would seem this should be a separate patch then since it's not
related to the 'volmode' support. The configure checks aren't my area
of expertise, but as long as you split it out, ACK to that.
Based on the explanation you provide - an ACK for the following change
as well.
John
>>> 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
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list