[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] storage: Refresh pool after creating volume



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 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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]