| Message ID | a7812cc0b5cf4ea67eee720a020872f55d383c4d.1602459876.git.crobinso@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | qemu: migration: don't open storage driver too early | expand |
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 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 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 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 :|
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; }
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(-) -- 2.28.0