src/qemu/qemu_migration.c | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-)
If storage migration is requested, and the destination storage does
not exist on the remote host, qemu's migration support will call
into the libvirt storage driver to precreate the destination storage.
The storage driver virConnectPtr is opened too early though, adding
an unnecessary dependency on the storage driver for several cases
that don't require it. This currently requires kubevirt to install
the storage driver even though they aren't actually using it.
Push the virGetConnectStorage calls to right before the cases they are
actually needed.
Signed-off-by: Cole Robinson <crobinso@redhat.com>
---
src/qemu/qemu_migration.c | 17 ++++++++++-------
1 file changed, 10 insertions(+), 7 deletions(-)
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 2000c86640..99a6b41483 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -169,8 +169,7 @@ qemuMigrationSrcRestoreDomainState(virQEMUDriverPtr driver, virDomainObjPtr vm)
static int
-qemuMigrationDstPrecreateDisk(virConnectPtr conn,
- virDomainDiskDefPtr disk,
+qemuMigrationDstPrecreateDisk(virDomainDiskDefPtr disk,
unsigned long long capacity)
{
int ret = -1;
@@ -181,6 +180,7 @@ qemuMigrationDstPrecreateDisk(virConnectPtr conn,
g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER;
const char *format = NULL;
unsigned int flags = 0;
+ virConnectPtr conn = NULL;
VIR_DEBUG("Precreate disk type=%s", virStorageTypeToString(disk->src->type));
@@ -204,6 +204,9 @@ qemuMigrationDstPrecreateDisk(virConnectPtr conn,
*volName = '\0';
volName++;
+ if (!(conn = virGetConnectStorage()))
+ goto cleanup;
+
if (!(pool = virStoragePoolLookupByTargetPath(conn, basePath)))
goto cleanup;
format = virStorageFileFormatTypeToString(disk->src->format);
@@ -212,6 +215,9 @@ qemuMigrationDstPrecreateDisk(virConnectPtr conn,
break;
case VIR_STORAGE_TYPE_VOLUME:
+ if (!(conn = virGetConnectStorage()))
+ goto cleanup;
+
if (!(pool = virStoragePoolLookupByName(conn, disk->src->srcpool->pool)))
goto cleanup;
format = virStorageFileFormatTypeToString(disk->src->format);
@@ -270,6 +276,7 @@ qemuMigrationDstPrecreateDisk(virConnectPtr conn,
VIR_FREE(volStr);
virObjectUnref(vol);
virObjectUnref(pool);
+ virObjectUnref(conn);
return ret;
}
@@ -304,13 +311,10 @@ qemuMigrationDstPrecreateStorage(virDomainObjPtr vm,
{
int ret = -1;
size_t i = 0;
- virConnectPtr conn;
if (!nbd || !nbd->ndisks)
return 0;
- if (!(conn = virGetConnectStorage()))
- return -1;
for (i = 0; i < nbd->ndisks; i++) {
virDomainDiskDefPtr disk;
@@ -349,13 +353,12 @@ qemuMigrationDstPrecreateStorage(virDomainObjPtr vm,
VIR_DEBUG("Proceeding with disk source %s", NULLSTR(diskSrcPath));
- if (qemuMigrationDstPrecreateDisk(conn, disk, nbd->disks[i].capacity) < 0)
+ if (qemuMigrationDstPrecreateDisk(disk, nbd->disks[i].capacity) < 0)
goto cleanup;
}
ret = 0;
cleanup:
- virObjectUnref(conn);
return ret;
}
--
2.28.0
On 10/12/20 1:44 AM, Cole Robinson wrote:
> If storage migration is requested, and the destination storage does
> not exist on the remote host, qemu's migration support will call
> into the libvirt storage driver to precreate the destination storage.
>
> The storage driver virConnectPtr is opened too early though, adding
> an unnecessary dependency on the storage driver for several cases
> that don't require it. This currently requires kubevirt to install
> the storage driver even though they aren't actually using it.
>
> Push the virGetConnectStorage calls to right before the cases they are
> actually needed.
>
> Signed-off-by: Cole Robinson <crobinso@redhat.com>
> ---
> src/qemu/qemu_migration.c | 17 ++++++++++-------
> 1 file changed, 10 insertions(+), 7 deletions(-)
>
> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index 2000c86640..99a6b41483 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c
> @@ -169,8 +169,7 @@ qemuMigrationSrcRestoreDomainState(virQEMUDriverPtr driver, virDomainObjPtr vm)
>
>
> static int
> -qemuMigrationDstPrecreateDisk(virConnectPtr conn,
> - virDomainDiskDefPtr disk,
> +qemuMigrationDstPrecreateDisk(virDomainDiskDefPtr disk,
> unsigned long long capacity)
> {
> int ret = -1;
> @@ -181,6 +180,7 @@ qemuMigrationDstPrecreateDisk(virConnectPtr conn,
> g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER;
> const char *format = NULL;
> unsigned int flags = 0;
> + virConnectPtr conn = NULL;
Wee tiny nitpick. If you're touching this might switch it g_auto:
g_autoptr(virConnect) conn = NULL;
and drop the explicit unref.
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Michal
On Mon, Oct 12, 2020 at 09:39:33 +0200, Michal Privoznik wrote:
> On 10/12/20 1:44 AM, Cole Robinson wrote:
> > If storage migration is requested, and the destination storage does
> > not exist on the remote host, qemu's migration support will call
> > into the libvirt storage driver to precreate the destination storage.
> >
> > The storage driver virConnectPtr is opened too early though, adding
> > an unnecessary dependency on the storage driver for several cases
> > that don't require it. This currently requires kubevirt to install
> > the storage driver even though they aren't actually using it.
> >
> > Push the virGetConnectStorage calls to right before the cases they are
> > actually needed.
> >
> > Signed-off-by: Cole Robinson <crobinso@redhat.com>
> > ---
> > src/qemu/qemu_migration.c | 17 ++++++++++-------
> > 1 file changed, 10 insertions(+), 7 deletions(-)
> >
> > diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> > index 2000c86640..99a6b41483 100644
> > --- a/src/qemu/qemu_migration.c
> > +++ b/src/qemu/qemu_migration.c
> > @@ -169,8 +169,7 @@ qemuMigrationSrcRestoreDomainState(virQEMUDriverPtr driver, virDomainObjPtr vm)
> > static int
> > -qemuMigrationDstPrecreateDisk(virConnectPtr conn,
> > - virDomainDiskDefPtr disk,
> > +qemuMigrationDstPrecreateDisk(virDomainDiskDefPtr disk,
> > unsigned long long capacity)
> > {
> > int ret = -1;
> > @@ -181,6 +180,7 @@ qemuMigrationDstPrecreateDisk(virConnectPtr conn,
> > g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER;
> > const char *format = NULL;
> > unsigned int flags = 0;
> > + virConnectPtr conn = NULL;
>
> Wee tiny nitpick. If you're touching this might switch it g_auto:
>
> g_autoptr(virConnect) conn = NULL;
>
> and drop the explicit unref.
In this specific case I'd stay with explicit cleanup. If you look at the
cleanup section we have a 'vol', 'pool' and 'conn' pointer. They are
interconnected. While libvirt internally does refcounting, cleaning them
up in random order might seem wrong.
On 10/12/20 10:01 AM, Peter Krempa wrote:
> On Mon, Oct 12, 2020 at 09:39:33 +0200, Michal Privoznik wrote:
>> On 10/12/20 1:44 AM, Cole Robinson wrote:
>>> If storage migration is requested, and the destination storage does
>>> not exist on the remote host, qemu's migration support will call
>>> into the libvirt storage driver to precreate the destination storage.
>>>
>>> The storage driver virConnectPtr is opened too early though, adding
>>> an unnecessary dependency on the storage driver for several cases
>>> that don't require it. This currently requires kubevirt to install
>>> the storage driver even though they aren't actually using it.
>>>
>>> Push the virGetConnectStorage calls to right before the cases they are
>>> actually needed.
>>>
>>> Signed-off-by: Cole Robinson <crobinso@redhat.com>
>>> ---
>>> src/qemu/qemu_migration.c | 17 ++++++++++-------
>>> 1 file changed, 10 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
>>> index 2000c86640..99a6b41483 100644
>>> --- a/src/qemu/qemu_migration.c
>>> +++ b/src/qemu/qemu_migration.c
>>> @@ -169,8 +169,7 @@ qemuMigrationSrcRestoreDomainState(virQEMUDriverPtr driver, virDomainObjPtr vm)
>>> static int
>>> -qemuMigrationDstPrecreateDisk(virConnectPtr conn,
>>> - virDomainDiskDefPtr disk,
>>> +qemuMigrationDstPrecreateDisk(virDomainDiskDefPtr disk,
>>> unsigned long long capacity)
>>> {
>>> int ret = -1;
>>> @@ -181,6 +180,7 @@ qemuMigrationDstPrecreateDisk(virConnectPtr conn,
>>> g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER;
>>> const char *format = NULL;
>>> unsigned int flags = 0;
>>> + virConnectPtr conn = NULL;
>>
>> Wee tiny nitpick. If you're touching this might switch it g_auto:
>>
>> g_autoptr(virConnect) conn = NULL;
>>
>> and drop the explicit unref.
>
> In this specific case I'd stay with explicit cleanup. If you look at the
> cleanup section we have a 'vol', 'pool' and 'conn' pointer. They are
> interconnected. While libvirt internally does refcounting, cleaning them
> up in random order might seem wrong.
>
Wouldn't the same 'random' order appear again when we switch to g_auto()
for the function anyways? Also, how to order cleanup if we have two
objects, both holding a reference to each other (e.g. virDomainObj and
qemuMonitor). Isn't refcounting designed exactly for cases like these?
Michal
On Mon, Oct 12, 2020 at 10:10:49 +0200, Michal Privoznik wrote:
> On 10/12/20 10:01 AM, Peter Krempa wrote:
> > On Mon, Oct 12, 2020 at 09:39:33 +0200, Michal Privoznik wrote:
> > > On 10/12/20 1:44 AM, Cole Robinson wrote:
> > > > If storage migration is requested, and the destination storage does
> > > > not exist on the remote host, qemu's migration support will call
> > > > into the libvirt storage driver to precreate the destination storage.
> > > >
> > > > The storage driver virConnectPtr is opened too early though, adding
> > > > an unnecessary dependency on the storage driver for several cases
> > > > that don't require it. This currently requires kubevirt to install
> > > > the storage driver even though they aren't actually using it.
> > > >
> > > > Push the virGetConnectStorage calls to right before the cases they are
> > > > actually needed.
> > > >
> > > > Signed-off-by: Cole Robinson <crobinso@redhat.com>
> > > > ---
> > > > src/qemu/qemu_migration.c | 17 ++++++++++-------
> > > > 1 file changed, 10 insertions(+), 7 deletions(-)
> > > >
> > > > diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> > > > index 2000c86640..99a6b41483 100644
> > > > --- a/src/qemu/qemu_migration.c
> > > > +++ b/src/qemu/qemu_migration.c
> > > > @@ -169,8 +169,7 @@ qemuMigrationSrcRestoreDomainState(virQEMUDriverPtr driver, virDomainObjPtr vm)
> > > > static int
> > > > -qemuMigrationDstPrecreateDisk(virConnectPtr conn,
> > > > - virDomainDiskDefPtr disk,
> > > > +qemuMigrationDstPrecreateDisk(virDomainDiskDefPtr disk,
> > > > unsigned long long capacity)
> > > > {
> > > > int ret = -1;
> > > > @@ -181,6 +180,7 @@ qemuMigrationDstPrecreateDisk(virConnectPtr conn,
> > > > g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER;
> > > > const char *format = NULL;
> > > > unsigned int flags = 0;
> > > > + virConnectPtr conn = NULL;
> > >
> > > Wee tiny nitpick. If you're touching this might switch it g_auto:
> > >
> > > g_autoptr(virConnect) conn = NULL;
> > >
> > > and drop the explicit unref.
> >
> > In this specific case I'd stay with explicit cleanup. If you look at the
> > cleanup section we have a 'vol', 'pool' and 'conn' pointer. They are
> > interconnected. While libvirt internally does refcounting, cleaning them
> > up in random order might seem wrong.
> >
>
> Wouldn't the same 'random' order appear again when we switch to g_auto() for
> the function anyways? Also, how to order cleanup if we have two objects,
> both holding a reference to each other (e.g. virDomainObj and qemuMonitor).
I'm actually against senseless conversion to g_auto. There might be code
paths which require explict ordering of cleanups and those obviously
must not be converted.
Obviously the rules for that are almost impossible to codify, but boil
down to carefully considering each case specifically.
> Isn't refcounting designed exactly for cases like these?
Yes it is. As noted in this case it will work.
I'm okay with using g_auto* in this patch provided that the rest of the
function is refactored prior to using it so that we don't have a mixed
cleanup specifically in case of these interconnected objects (yes, I'm
okay with mixed cleanup in case the objects are not interconnected).
On Sun, Oct 11, 2020 at 07:44:36PM -0400, Cole Robinson wrote:
> If storage migration is requested, and the destination storage does
> not exist on the remote host, qemu's migration support will call
> into the libvirt storage driver to precreate the destination storage.
>
> The storage driver virConnectPtr is opened too early though, adding
> an unnecessary dependency on the storage driver for several cases
> that don't require it. This currently requires kubevirt to install
> the storage driver even though they aren't actually using it.
>
> Push the virGetConnectStorage calls to right before the cases they are
> actually needed.
This pushes the connection open attempts inside a loop body. So if the
VM has multiple disks, then we're going to be repeatedly opening and
closing the connection which is not desirable.
I think we need a global connection across all disks, which is lazy
opened on first access.
>
> Signed-off-by: Cole Robinson <crobinso@redhat.com>
> ---
> src/qemu/qemu_migration.c | 17 ++++++++++-------
> 1 file changed, 10 insertions(+), 7 deletions(-)
>
> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index 2000c86640..99a6b41483 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c
> @@ -169,8 +169,7 @@ qemuMigrationSrcRestoreDomainState(virQEMUDriverPtr driver, virDomainObjPtr vm)
>
>
> static int
> -qemuMigrationDstPrecreateDisk(virConnectPtr conn,
> - virDomainDiskDefPtr disk,
> +qemuMigrationDstPrecreateDisk(virDomainDiskDefPtr disk,
> unsigned long long capacity)
> {
> int ret = -1;
> @@ -181,6 +180,7 @@ qemuMigrationDstPrecreateDisk(virConnectPtr conn,
> g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER;
> const char *format = NULL;
> unsigned int flags = 0;
> + virConnectPtr conn = NULL;
>
> VIR_DEBUG("Precreate disk type=%s", virStorageTypeToString(disk->src->type));
>
> @@ -204,6 +204,9 @@ qemuMigrationDstPrecreateDisk(virConnectPtr conn,
> *volName = '\0';
> volName++;
>
> + if (!(conn = virGetConnectStorage()))
> + goto cleanup;
> +
> if (!(pool = virStoragePoolLookupByTargetPath(conn, basePath)))
> goto cleanup;
> format = virStorageFileFormatTypeToString(disk->src->format);
> @@ -212,6 +215,9 @@ qemuMigrationDstPrecreateDisk(virConnectPtr conn,
> break;
>
> case VIR_STORAGE_TYPE_VOLUME:
> + if (!(conn = virGetConnectStorage()))
> + goto cleanup;
> +
> if (!(pool = virStoragePoolLookupByName(conn, disk->src->srcpool->pool)))
> goto cleanup;
> format = virStorageFileFormatTypeToString(disk->src->format);
> @@ -270,6 +276,7 @@ qemuMigrationDstPrecreateDisk(virConnectPtr conn,
> VIR_FREE(volStr);
> virObjectUnref(vol);
> virObjectUnref(pool);
> + virObjectUnref(conn);
> return ret;
> }
>
> @@ -304,13 +311,10 @@ qemuMigrationDstPrecreateStorage(virDomainObjPtr vm,
> {
> int ret = -1;
> size_t i = 0;
> - virConnectPtr conn;
>
> if (!nbd || !nbd->ndisks)
> return 0;
>
> - if (!(conn = virGetConnectStorage()))
> - return -1;
>
> for (i = 0; i < nbd->ndisks; i++) {
> virDomainDiskDefPtr disk;
> @@ -349,13 +353,12 @@ qemuMigrationDstPrecreateStorage(virDomainObjPtr vm,
>
> VIR_DEBUG("Proceeding with disk source %s", NULLSTR(diskSrcPath));
>
> - if (qemuMigrationDstPrecreateDisk(conn, disk, nbd->disks[i].capacity) < 0)
> + if (qemuMigrationDstPrecreateDisk(disk, nbd->disks[i].capacity) < 0)
> goto cleanup;
> }
>
> ret = 0;
> cleanup:
> - virObjectUnref(conn);
> return ret;
> }
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 10/12/20 10:50 AM, Daniel P. Berrangé wrote: > On Sun, Oct 11, 2020 at 07:44:36PM -0400, Cole Robinson wrote: >> If storage migration is requested, and the destination storage does >> not exist on the remote host, qemu's migration support will call >> into the libvirt storage driver to precreate the destination storage. >> >> The storage driver virConnectPtr is opened too early though, adding >> an unnecessary dependency on the storage driver for several cases >> that don't require it. This currently requires kubevirt to install >> the storage driver even though they aren't actually using it. >> >> Push the virGetConnectStorage calls to right before the cases they are >> actually needed. > > This pushes the connection open attempts inside a loop body. So if the > VM has multiple disks, then we're going to be repeatedly opening and > closing the connection which is not desirable. > > I think we need a global connection across all disks, which is lazy > opened on first access. Doesn't virGetConnectStorage() cache the connection? Michal
On Mon, Oct 12, 2020 at 10:55:52AM +0200, Michal Privoznik wrote: > On 10/12/20 10:50 AM, Daniel P. Berrangé wrote: > > On Sun, Oct 11, 2020 at 07:44:36PM -0400, Cole Robinson wrote: > > > If storage migration is requested, and the destination storage does > > > not exist on the remote host, qemu's migration support will call > > > into the libvirt storage driver to precreate the destination storage. > > > > > > The storage driver virConnectPtr is opened too early though, adding > > > an unnecessary dependency on the storage driver for several cases > > > that don't require it. This currently requires kubevirt to install > > > the storage driver even though they aren't actually using it. > > > > > > Push the virGetConnectStorage calls to right before the cases they are > > > actually needed. > > > > This pushes the connection open attempts inside a loop body. So if the > > VM has multiple disks, then we're going to be repeatedly opening and > > closing the connection which is not desirable. > > > > I think we need a global connection across all disks, which is lazy > > opened on first access. > > Doesn't virGetConnectStorage() cache the connection? Yes & no. You can call virSetConnectStorage(conn) to populate the cache with a pre-opened connection. If you haven't done that, then a new connection will be opened every time. The ltter is what this code will be doing. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 10/12/20 5:05 AM, Daniel P. Berrangé wrote: > On Mon, Oct 12, 2020 at 10:55:52AM +0200, Michal Privoznik wrote: >> On 10/12/20 10:50 AM, Daniel P. Berrangé wrote: >>> On Sun, Oct 11, 2020 at 07:44:36PM -0400, Cole Robinson wrote: >>>> If storage migration is requested, and the destination storage does >>>> not exist on the remote host, qemu's migration support will call >>>> into the libvirt storage driver to precreate the destination storage. >>>> >>>> The storage driver virConnectPtr is opened too early though, adding >>>> an unnecessary dependency on the storage driver for several cases >>>> that don't require it. This currently requires kubevirt to install >>>> the storage driver even though they aren't actually using it. >>>> >>>> Push the virGetConnectStorage calls to right before the cases they are >>>> actually needed. >>> >>> This pushes the connection open attempts inside a loop body. So if the >>> VM has multiple disks, then we're going to be repeatedly opening and >>> closing the connection which is not desirable. >>> >>> I think we need a global connection across all disks, which is lazy >>> opened on first access. >> >> Doesn't virGetConnectStorage() cache the connection? > > Yes & no. > > You can call virSetConnectStorage(conn) to populate the cache with a > pre-opened connection. If you haven't done that, then a new connection > will be opened every time. The ltter is what this code will be doing. > What is the use case for not caching the connection by default? virSetConnect* has very few users, but virGetConnect* is sprinkled in many places, including multiple uses in generic code in domain_conf.c. Can this be done as part of virGetConnect*? - Cole
On Mon, Oct 12, 2020 at 09:12:50AM -0400, Cole Robinson wrote: > On 10/12/20 5:05 AM, Daniel P. Berrangé wrote: > > On Mon, Oct 12, 2020 at 10:55:52AM +0200, Michal Privoznik wrote: > >> On 10/12/20 10:50 AM, Daniel P. Berrangé wrote: > >>> On Sun, Oct 11, 2020 at 07:44:36PM -0400, Cole Robinson wrote: > >>>> If storage migration is requested, and the destination storage does > >>>> not exist on the remote host, qemu's migration support will call > >>>> into the libvirt storage driver to precreate the destination storage. > >>>> > >>>> The storage driver virConnectPtr is opened too early though, adding > >>>> an unnecessary dependency on the storage driver for several cases > >>>> that don't require it. This currently requires kubevirt to install > >>>> the storage driver even though they aren't actually using it. > >>>> > >>>> Push the virGetConnectStorage calls to right before the cases they are > >>>> actually needed. > >>> > >>> This pushes the connection open attempts inside a loop body. So if the > >>> VM has multiple disks, then we're going to be repeatedly opening and > >>> closing the connection which is not desirable. > >>> > >>> I think we need a global connection across all disks, which is lazy > >>> opened on first access. > >> > >> Doesn't virGetConnectStorage() cache the connection? > > > > Yes & no. > > > > You can call virSetConnectStorage(conn) to populate the cache with a > > pre-opened connection. If you haven't done that, then a new connection > > will be opened every time. The ltter is what this code will be doing. > > > > What is the use case for not caching the connection by default? It is a matter of figuring out an invalidation strategy, so we don't keep the other daemons running continuously when no client is actually triggering their usage. > virSetConnect* has very few users, but virGetConnect* is sprinkled in > many places, including multiple uses in generic code in domain_conf.c. > Can this be done as part of virGetConnect*? Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
© 2016 - 2024 Red Hat, Inc.