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. But when the driver does something (with the pool), it should update its info accordingly without the need to refresh it.

...

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

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

On Mon, Jun 03, 2013 at 11:47:10PM +0800, Osier Yang wrote:

On 01/06/13 16:05, Martin Kletzander wrote:

...

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

Okay, I know your meaning now.

...

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; +} +

Though this is only for fs backend, I could see your thought (only refresh the pool's capacity, available, and allocation, without collecting the volumes), right? This makes sense for fs backend indeed, but not for some of the other backends, for instance, scsi backend pool, it populates the pool's capacity, available, allocation while scanning the LUNs...

This would also mean that we do heaps of pool refreshes when creating lots of volumes which can be an expensive operation. Shouldn't we rather only update the information from the information we have from the newly created volume? Cheers, -- Guido

On 06/06/2013 12:13 PM, Daniel P. Berrange wrote:

On Thu, Jun 06, 2013 at 11:19:18AM +0200, Guido Günther wrote:

...

On Mon, Jun 03, 2013 at 11:47:10PM +0800, Osier Yang wrote:

...

On 01/06/13 16:05, Martin Kletzander wrote:

...

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; +} +

Though this is only for fs backend, I could see your thought (only refresh the pool's capacity, available, and allocation, without collecting the volumes), right? This makes sense for fs backend indeed, but not for some of the other backends, for instance, scsi backend pool, it populates the pool's capacity, available, allocation while scanning the LUNs...

This would also mean that we do heaps of pool refreshes when creating lots of volumes which can be an expensive operation. Shouldn't we rather only update the information from the information we have from the newly created volume?

Yeah, when creating a volume, we certainly shouldn't do a full pool refresh. We want to update the pool from the metadata that we already have about the volume we're creating.

That is what I'm trying to suggest the whole time :) One check for the volume and one statvfs call is very reasonable (I'd say) to run when creating a volume. And it removes similar problems with information freshness. Martin