[libvirt] [PATCH] storage: Refresh pool after creating volume
Martin Kletzander
mkletzan at redhat.com
Sat Jun 1 08:05:52 UTC 2013
On 05/29/2013 06:16 PM, Osier Yang wrote:
> On 30/05/13 00:04, Martin Kletzander wrote:
>> On 05/29/2013 05:55 PM, Osier Yang wrote:
>>> On 29/05/13 20:51, Martin Kletzander wrote:
>>>> On 05/29/2013 11:53 AM, Osier Yang wrote:
>>>>> https://bugzilla.redhat.com/show_bug.cgi?id=965442
>>>>>
>>>>> One has to refresh the pool to get the correct pool info, this
>>>>> patch refreshes the pool after creating a volume in code instead.
>>>>> Pool refreshing failure is fine to ignore with a warning.
>>>>> ---
>>>>> src/storage/storage_driver.c | 6 ++++++
>>>>> 1 file changed, 6 insertions(+)
>>>>>
>>>>> diff --git a/src/storage/storage_driver.c
>>>>> b/src/storage/storage_driver.c
>>>>> index a2b0c21..2a55095 100644
>>>>> --- a/src/storage/storage_driver.c
>>>>> +++ b/src/storage/storage_driver.c
>>>>> @@ -1443,6 +1443,9 @@ storageVolCreateXML(virStoragePoolPtr obj,
>>>>> }
>>>>> + if (backend->refreshPool && backend->refreshPool(obj->conn,
>>>>> pool) < 0)
>>>>> + VIR_WARN("Failed to refresh pool after creating volume");
>>>>> +
>>>>> VIR_INFO("Creating volume '%s' in storage pool '%s'",
>>>>> volobj->name, pool->def->name);
>>>>> ret = volobj;
>>>>> @@ -1606,6 +1609,9 @@ storageVolCreateXMLFrom(virStoragePoolPtr obj,
>>>>> goto cleanup;
>>>>> }
>>>>> + if (backend->refreshPool && backend->refreshPool(obj->conn,
>>>>> pool) < 0)
>>>>> + VIR_WARN("Failed to refresh pool after creating volume");
>>>>> +
>>>>> VIR_INFO("Creating volume '%s' in storage pool '%s'",
>>>>> volobj->name, pool->def->name);
>>>>> ret = volobj;
>>>>>
>>>> I don't want to say NACK just like that, but I think the bug indicates
>>>> there's a problem in the storage driver. It should automatically
>>>> reflect the changes made to that pool.
>>> That's the thing I mentioned long time ago. Using things like inotify
>>> to update the pool's info (though inotify doesn't work for all pool
>>> types, such as iscsi).
>>>
>> I didn't mean responding to user-created volumes. The user has to do
>> pool-refresh after that, that's their business as they do something
>> behind libvirt's back.
>
> Right in principle.
>
>> But when the driver does something (with the
>> pool), it should update its info accordingly without the need to
>> refresh it.
>
> Right too. This applies to deleteVol/resizeVol too. But as I said, though
> calliing refreshPool in these APIs is not the best method, but it's the
> generic
> thing what current storage driver does. Assuming that we won't
> reply on 3rd project/tools, we have to introduce something like event to
> let the pool refresh itself. It's just not much better than calling
> refreshPool
> in the APIs, as it should call refreshPool anyway finally.
>
>>
>>>> What's the structure that gets
>>>> updated only with refresh and not after the vol is created?
>>> Can you explain more about this? I guess you mean the vol is
>>> created outside of libvirt? such as a iscsi target create a new
>>> LUN?
>>>
>>>> Does it do
>>>> with all the drivers?
>>> Not sure what do you mean for "drivers". But I guess you mean
>>> "storage backends" here? if so, yes. All the current storage backends
>>> support "refreshPool"/
>>>
>> Yes, backends. I meant what drivers has this issue.
>>
>>>> Long story short; I think this bug fixes the symptom, not the problem.
>>> As said above, you have a right opinion. The pool should be notified
>>> asynchronously, but it's thing I don't have enough time to do currently.
>>>
>> Not quite what I meant, corrected myself above.
>>
>> Martin
I think we are still talking aout two different things, let me explain on code. This is how I would solve the issue (just for creating the volume, this would have to be similarly adapted to resize, wipe and delete, but that shouldn't be more than few lines):
commit 416247880399f88ec382a16c7cebc5460d6b67ad
Author: Martin Kletzander <mkletzan at redhat.com>
Date: Fri May 31 14:25:48 2013 +0200
test
diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c
index 1a85afc..a119fc4 100644
--- a/src/storage/storage_backend_fs.c
+++ b/src/storage/storage_backend_fs.c
@@ -1,7 +1,7 @@
/*
* storage_backend_fs.c: storage backend for FS and directory handling
*
- * Copyright (C) 2007-2012 Red Hat, Inc.
+ * Copyright (C) 2007-2013 Red Hat, Inc.
* Copyright (C) 2007-2008 Daniel P. Berrange
*
* This library is free software; you can redistribute it and/or
@@ -797,6 +797,27 @@ error:
}
+static int
+virStorageBackendStatVFS(virStoragePoolDefPtr def)
+{
+ struct statvfs sb;
+
+ if (statvfs(def->target.path, &sb) < 0) {
+ virReportSystemError(errno,
+ _("cannot statvfs path '%s'"),
+ def->target.path);
+ return -1;
+ }
+ def->capacity = ((unsigned long long)sb.f_frsize *
+ (unsigned long long)sb.f_blocks);
+ def->available = ((unsigned long long)sb.f_bfree *
+ (unsigned long long)sb.f_frsize);
+ def->allocation = def->capacity - def->available;
+
+ return 0;
+}
+
+
/**
* Iterate over the pool's directory and enumerate all disk images
* within it. This is non-recursive.
@@ -807,7 +828,6 @@ virStorageBackendFileSystemRefresh(virConnectPtr conn ATTRIBUTE_UNUSED,
{
DIR *dir;
struct dirent *ent;
- struct statvfs sb;
virStorageVolDefPtr vol = NULL;
if (!(dir = opendir(pool->def->target.path))) {
@@ -893,18 +913,8 @@ virStorageBackendFileSystemRefresh(virConnectPtr conn ATTRIBUTE_UNUSED,
}
closedir(dir);
-
- if (statvfs(pool->def->target.path, &sb) < 0) {
- virReportSystemError(errno,
- _("cannot statvfs path '%s'"),
- pool->def->target.path);
+ if (virStorageBackendStatVFS(pool->def) < 0)
return -1;
- }
- pool->def->capacity = ((unsigned long long)sb.f_frsize *
- (unsigned long long)sb.f_blocks);
- pool->def->available = ((unsigned long long)sb.f_bfree *
- (unsigned long long)sb.f_frsize);
- pool->def->allocation = pool->def->capacity - pool->def->available;
return 0;
@@ -987,6 +997,13 @@ virStorageBackendFileSystemVolCreate(virConnectPtr conn ATTRIBUTE_UNUSED,
vol->type = VIR_STORAGE_VOL_FILE;
+ if (vol->target.format == VIR_STORAGE_FILE_RAW &&
+ vol->backingStore.path) {
+ virReportError(VIR_ERR_OPERATION_INVALID, "%s",
+ _("raw format doesn't support backing volumes"));
+ return -1;
+ }
+
VIR_FREE(vol->target.path);
if (virAsprintf(&vol->target.path, "%s/%s",
pool->def->target.path,
@@ -1077,6 +1094,10 @@ _virStorageBackendFileSystemVolBuild(virConnectPtr conn,
if (create_func(conn, pool, vol, inputvol, flags) < 0)
return -1;
+
+ if (virStorageBackendStatVFS(pool->def) < 0)
+ return -1;
+
return 0;
}
--
Martin
More information about the libvir-list
mailing list