The backing store indexes were not bound to the storage sources in any
way. To allow us to bind a given alias to a given storage source we need
to save the index in virStorageSource. The backing store ids are now
generated when detecting the backing chain.
Since we don't re-detect the backing chain after snapshots, the
numbering needs to be fixed there.
---
src/conf/domain_conf.c | 22 +++++++++++++++-------
src/qemu/qemu_driver.c | 14 ++++++++++++++
src/storage/storage_source.c | 9 ++++++---
src/util/virstoragefile.c | 1 +
src/util/virstoragefile.h | 1 +
5 files changed, 37 insertions(+), 10 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index f7c9160b4..45fa57a14 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -8269,6 +8269,7 @@ virDomainDiskBackingStoreParse(xmlXPathContextPtr ctxt,
xmlNodePtr source;
char *type = NULL;
char *format = NULL;
+ char *idx = NULL;
int ret = -1;
if (!(ctxt->node = virXPathNode("./backingStore[*]", ctxt))) {
@@ -8279,6 +8280,13 @@ virDomainDiskBackingStoreParse(xmlXPathContextPtr ctxt,
if (VIR_ALLOC(backingStore) < 0)
goto cleanup;
+ if (!(flags & VIR_DOMAIN_DEF_PARSE_INACTIVE) &&
+ (idx = virXMLPropString(ctxt->node, "index")) &&
+ virStrToLong_uip(idx, NULL, 10, &backingStore->id) < 0) {
+ virReportError(VIR_ERR_XML_ERROR, _("invalid disk index '%s'"), idx);
+ goto cleanup;
+ }
+
if (!(type = virXMLPropString(ctxt->node, "type"))) {
virReportError(VIR_ERR_XML_ERROR, "%s",
_("missing disk backing store type"));
@@ -21898,8 +21906,7 @@ virDomainDiskSourceFormat(virBufferPtr buf,
static int
virDomainDiskBackingStoreFormat(virBufferPtr buf,
virStorageSourcePtr backingStore,
- const char *backingStoreRaw,
- unsigned int idx)
+ const char *backingStoreRaw)
{
const char *type;
const char *format;
@@ -21926,8 +21933,10 @@ virDomainDiskBackingStoreFormat(virBufferPtr buf,
return -1;
}
- virBufferAsprintf(buf, "<backingStore type='%s' index='%u'>\n",
- type, idx);
+ virBufferAsprintf(buf, "<backingStore type='%s'", type);
+ if (backingStore->id != 0)
+ virBufferAsprintf(buf, " index='%u'", backingStore->id);
+ virBufferAddLit(buf, ">\n");
virBufferAdjustIndent(buf, 2);
virBufferAsprintf(buf, "<format type='%s'/>\n", format);
@@ -21935,8 +21944,7 @@ virDomainDiskBackingStoreFormat(virBufferPtr buf,
if (virDomainDiskSourceFormatInternal(buf, backingStore, 0, 0, true) < 0 ||
virDomainDiskBackingStoreFormat(buf,
backingStore->backingStore,
- backingStore->backingStoreRaw,
- idx + 1) < 0)
+ backingStore->backingStoreRaw) < 0)
return -1;
virBufferAdjustIndent(buf, -2);
@@ -22072,7 +22080,7 @@ virDomainDiskDefFormat(virBufferPtr buf,
* persistent storage of backing chains is ready. */
if (!(flags & VIR_DOMAIN_DEF_FORMAT_INACTIVE) &&
virDomainDiskBackingStoreFormat(buf, def->src->backingStore,
- def->src->backingStoreRaw, 1) < 0)
+ def->src->backingStoreRaw) < 0)
return -1;
virBufferEscapeString(buf, "<backenddomain name='%s'/>\n", def->domain_name);
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 7c6f1674a..0260c40ef 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -14442,6 +14442,17 @@ qemuDomainSnapshotDiskDataCollect(virQEMUDriverPtr driver,
}
+static void
+qemuDomainSnapshotUpdateDiskSourcesRenumber(virStorageSourcePtr src)
+{
+ virStorageSourcePtr next;
+ unsigned int idx = 1;
+
+ for (next = src->backingStore; next; next = next->backingStore)
+ next->id = idx++;
+}
+
+
/**
* qemuDomainSnapshotUpdateDiskSources:
* @dd: snapshot disk data object
@@ -14464,6 +14475,9 @@ qemuDomainSnapshotUpdateDiskSources(qemuDomainSnapshotDiskDataPtr dd,
VIR_STEAL_PTR(dd->src->backingStore, dd->disk->src);
VIR_STEAL_PTR(dd->disk->src, dd->src);
+ /* fix numbering of disks */
+ qemuDomainSnapshotUpdateDiskSourcesRenumber(dd->disk->src);
+
if (dd->persistdisk) {
VIR_STEAL_PTR(dd->persistsrc->backingStore, dd->persistdisk->src);
VIR_STEAL_PTR(dd->persistdisk->src, dd->persistsrc);
diff --git a/src/storage/storage_source.c b/src/storage/storage_source.c
index bf4762237..864c69928 100644
--- a/src/storage/storage_source.c
+++ b/src/storage/storage_source.c
@@ -396,7 +396,8 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src,
uid_t uid, gid_t gid,
bool allow_probe,
bool report_broken,
- virHashTablePtr cycle)
+ virHashTablePtr cycle,
+ unsigned int depth)
{
int ret = -1;
const char *uniqueName;
@@ -474,7 +475,7 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src,
if ((ret = virStorageFileGetMetadataRecurse(backingStore, parent,
uid, gid,
allow_probe, report_broken,
- cycle)) < 0) {
+ cycle, depth + 1)) < 0) {
if (report_broken)
goto cleanup;
@@ -489,6 +490,8 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src,
ret = 0;
cleanup:
+ if (src->backingStore)
+ src->backingStore->id = depth;
VIR_FREE(buf);
virStorageFileDeinit(src);
virStorageSourceFree(backingStore);
@@ -543,7 +546,7 @@ virStorageFileGetMetadata(virStorageSourcePtr src,
}
ret = virStorageFileGetMetadataRecurse(src, src, uid, gid,
- allow_probe, report_broken, cycle);
+ allow_probe, report_broken, cycle, 1);
virHashFree(cycle);
return ret;
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index dd4494940..2b9f4c892 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -2030,6 +2030,7 @@ virStorageSourceCopy(const virStorageSource *src,
if (VIR_ALLOC(ret) < 0)
return NULL;
+ ret->id = src->id;
ret->type = src->type;
ret->protocol = src->protocol;
ret->format = src->format;
diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
index 74dee10f2..d3bafefc3 100644
--- a/src/util/virstoragefile.h
+++ b/src/util/virstoragefile.h
@@ -227,6 +227,7 @@ typedef virStorageSource *virStorageSourcePtr;
* IMPORTANT: When adding fields to this struct it's also necessary to add
* appropriate code to the virStorageSourceCopy deep copy function */
struct _virStorageSource {
+ unsigned int id; /* backing chain identifier, 0 is unset */
int type; /* virStorageType */
char *path;
int protocol; /* virStorageNetProtocol */
--
2.14.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 10/12/2017 02:07 PM, Peter Krempa wrote:
> The backing store indexes were not bound to the storage sources in any
> way. To allow us to bind a given alias to a given storage source we need
> to save the index in virStorageSource. The backing store ids are now
> generated when detecting the backing chain.
>
> Since we don't re-detect the backing chain after snapshots, the
> numbering needs to be fixed there.
> ---
> src/conf/domain_conf.c | 22 +++++++++++++++-------
> src/qemu/qemu_driver.c | 14 ++++++++++++++
> src/storage/storage_source.c | 9 ++++++---
> src/util/virstoragefile.c | 1 +
> src/util/virstoragefile.h | 1 +
> 5 files changed, 37 insertions(+), 10 deletions(-)
>
> +++ b/src/qemu/qemu_driver.c
> @@ -14442,6 +14442,17 @@ qemuDomainSnapshotDiskDataCollect(virQEMUDriverPtr driver,
> }
>
>
> +static void
> +qemuDomainSnapshotUpdateDiskSourcesRenumber(virStorageSourcePtr src)
> +{
> + virStorageSourcePtr next;
> + unsigned int idx = 1;
> +
> + for (next = src->backingStore; next; next = next->backingStore)
> + next->id = idx++;
> +}
> +
Is renumbering going to affect API?
It's the difference between starting with:
back <- mid <- active
having numbers 3, 2, 1, vs. starting with:
back <- mid
then creating active via snapshot, and now having numbers 2, 1, 3.
In other words, do we try to preserve that index 1 is tied to the file
we opened first, no matter what order it is currently in the chain, or
do we state that our use of "vda[1]" to denote a member of the chain may
mean different files at different points in time based on what other
chain manipulations have caused renumbering along the way?
When we were always regenerating the chain on the fly, there wasn't much
stability to worry about. But now that we are going to try to preserve
the index across domain reboots, we need to make sure we know which way
we want things to work.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, Oct 12, 2017 at 14:57:36 -0500, Eric Blake wrote:
> On 10/12/2017 02:07 PM, Peter Krempa wrote:
> > The backing store indexes were not bound to the storage sources in any
> > way. To allow us to bind a given alias to a given storage source we need
> > to save the index in virStorageSource. The backing store ids are now
> > generated when detecting the backing chain.
> >
> > Since we don't re-detect the backing chain after snapshots, the
> > numbering needs to be fixed there.
> > ---
> > src/conf/domain_conf.c | 22 +++++++++++++++-------
> > src/qemu/qemu_driver.c | 14 ++++++++++++++
> > src/storage/storage_source.c | 9 ++++++---
> > src/util/virstoragefile.c | 1 +
> > src/util/virstoragefile.h | 1 +
> > 5 files changed, 37 insertions(+), 10 deletions(-)
> >
>
> > +++ b/src/qemu/qemu_driver.c
> > @@ -14442,6 +14442,17 @@ qemuDomainSnapshotDiskDataCollect(virQEMUDriverPtr driver,
> > }
> >
> >
> > +static void
> > +qemuDomainSnapshotUpdateDiskSourcesRenumber(virStorageSourcePtr src)
> > +{
> > + virStorageSourcePtr next;
> > + unsigned int idx = 1;
> > +
> > + for (next = src->backingStore; next; next = next->backingStore)
> > + next->id = idx++;
> > +}
> > +
>
> Is renumbering going to affect API?
>
> It's the difference between starting with:
>
> back <- mid <- active
>
> having numbers 3, 2, 1, vs. starting with:
>
> back <- mid
>
> then creating active via snapshot, and now having numbers 2, 1, 3.
In case when we will not use blockdev-add, the order will be 1,2,3
always, since they will be in order. If blockdev add will be used it
should be 4,1,3, so if you pop out 2 of the chain, it will never appear
back.
>
> In other words, do we try to preserve that index 1 is tied to the file
> we opened first, no matter what order it is currently in the chain, or
> do we state that our use of "vda[1]" to denote a member of the chain may
> mean different files at different points in time based on what other
> chain manipulations have caused renumbering along the way?
Exactly, but possible only when tracking the full chain.
>
> When we were always regenerating the chain on the fly, there wasn't much
> stability to worry about. But now that we are going to try to preserve
> the index across domain reboots, we need to make sure we know which way
> we want things to work.
I don't really want to go as far as guaranteeing the numbers across
reboots. I think keeping them across one lifetime should be good enough.
>
>
> --
> Eric Blake, Principal Software Engineer
> Red Hat, Inc. +1-919-301-3266
> Virtualization: qemu.org | libvirt.org
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 10/13/2017 12:43 AM, Peter Krempa wrote: > On Thu, Oct 12, 2017 at 14:57:36 -0500, Eric Blake wrote: >> On 10/12/2017 02:07 PM, Peter Krempa wrote: >>> The backing store indexes were not bound to the storage sources in any >>> way. To allow us to bind a given alias to a given storage source we need >>> to save the index in virStorageSource. The backing store ids are now >>> generated when detecting the backing chain. >>> >>> Since we don't re-detect the backing chain after snapshots, the >>> numbering needs to be fixed there. >>> --- >> Is renumbering going to affect API? >> >> It's the difference between starting with: >> >> back <- mid <- active >> >> having numbers 3, 2, 1, vs. starting with: >> >> back <- mid >> >> then creating active via snapshot, and now having numbers 2, 1, 3. > > In case when we will not use blockdev-add, the order will be 1,2,3 > always, since they will be in order. If blockdev add will be used it > should be 4,1,3, so if you pop out 2 of the chain, it will never appear > back. > >> >> In other words, do we try to preserve that index 1 is tied to the file >> we opened first, no matter what order it is currently in the chain, or >> do we state that our use of "vda[1]" to denote a member of the chain may >> mean different files at different points in time based on what other >> chain manipulations have caused renumbering along the way? > > Exactly, but possible only when tracking the full chain. > >> >> When we were always regenerating the chain on the fly, there wasn't much >> stability to worry about. But now that we are going to try to preserve >> the index across domain reboots, we need to make sure we know which way >> we want things to work. > > I don't really want to go as far as guaranteeing the numbers across > reboots. I think keeping them across one lifetime should be good enough. Unless anyone else has strong opinions about disk indices not always being stable, I can live with this as your design goal. As I mentioned in 4/9, I didn't see anything wrong with the code itself, so as long as we don't have a problem with the design implications: Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Fri, Oct 13, 2017 at 08:36:54 -0500, Eric Blake wrote: > On 10/13/2017 12:43 AM, Peter Krempa wrote: > > On Thu, Oct 12, 2017 at 14:57:36 -0500, Eric Blake wrote: > >> On 10/12/2017 02:07 PM, Peter Krempa wrote: [..] > >> When we were always regenerating the chain on the fly, there wasn't much > >> stability to worry about. But now that we are going to try to preserve > >> the index across domain reboots, we need to make sure we know which way > >> we want things to work. > > > > I don't really want to go as far as guaranteeing the numbers across > > reboots. I think keeping them across one lifetime should be good enough. > > Unless anyone else has strong opinions about disk indices not always > being stable, I can live with this as your design goal. As I mentioned > in 4/9, I didn't see anything wrong with the code itself, so as long as > we don't have a problem with the design implications: The idea for the indexes to always refer to the same image was there from the beginning, we only cut corners when implementing it the first time. I'm now paying back the technical debt with a lot of interest. > Reviewed-by: Eric Blake <eblake@redhat.com> Thanks for the review. I've pushed these. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2025 Red Hat, Inc.