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@xxxxxxxxxx> 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 -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list