3:05 a.m.
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(a)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