more snapshot improvements [incremental backup saga]

[libvirt] [PATCH 6/8] backup: Add support for filtering based on current moment

Posted by Eric Blake 5 years, 4 months ago

Right now, the snapshot API permits at most one current snapshot, and
includes specific API for getting at that snapshot
(virDomainHasCurrentSnapshot, virDomainSnapshotCurrent,
virDomainSnapshotIsCurrent).  However, with upcoming checkpoints, it
is conceivable that a hypervisor could mark multiple checkpoints as
current (the qemu implementation has only one current checkpoint for
an incremental backup, and computes the set of changes for a
differential backup by merging a chain of checkpoints - but other
hypervisors may treat all differential checkpoints as current).  As
such, it is more desirable to avoid explicit API for getting at the
one current checkpoint, and instead have the List API include a filter
for that purpose.  Still, it is easier to implement that filter
directly in the common virDomainMomentObjList code, since that is the
only code that tracks the one moment that is current (both for
existing snapshots, and for how qemu will be using current
checkpoints).

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 src/conf/virdomainmomentobjlist.h | 11 +++++++++--
 src/conf/virdomainmomentobjlist.c |  9 ++++++++-
 2 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/src/conf/virdomainmomentobjlist.h b/src/conf/virdomainmomentobjlist.h
index 4067e928f4..7628df5e29 100644
--- a/src/conf/virdomainmomentobjlist.h
+++ b/src/conf/virdomainmomentobjlist.h
@@ -78,8 +78,10 @@ typedef enum {
     VIR_DOMAIN_MOMENT_LIST_TOPOLOGICAL = (1 << 1),
     VIR_DOMAIN_MOMENT_LIST_LEAVES      = (1 << 2),
     VIR_DOMAIN_MOMENT_LIST_NO_LEAVES   = (1 << 3),
-    VIR_DOMAIN_MOMENT_LIST_METADATA    = (1 << 4),
-    VIR_DOMAIN_MOMENT_LIST_NO_METADATA = (1 << 5),
+    VIR_DOMAIN_MOMENT_LIST_CURRENT     = (1 << 4),
+    VIR_DOMAIN_MOMENT_LIST_NO_CURRENT  = (1 << 5),
+    VIR_DOMAIN_MOMENT_LIST_METADATA    = (1 << 6),
+    VIR_DOMAIN_MOMENT_LIST_NO_METADATA = (1 << 7),
 } virDomainMomentFilters;

 #define VIR_DOMAIN_MOMENT_FILTERS_METADATA \
@@ -90,10 +92,15 @@ typedef enum {
                (VIR_DOMAIN_MOMENT_LIST_LEAVES | \
                 VIR_DOMAIN_MOMENT_LIST_NO_LEAVES)

+#define VIR_DOMAIN_MOMENT_FILTERS_CURRENT \
+               (VIR_DOMAIN_MOMENT_LIST_CURRENT | \
+                VIR_DOMAIN_MOMENT_LIST_NO_CURRENT)
+
 #define VIR_DOMAIN_MOMENT_FILTERS_ALL \
                (VIR_DOMAIN_MOMENT_LIST_ROOTS | \
                 VIR_DOMAIN_MOMENT_LIST_TOPOLOGICAL | \
                 VIR_DOMAIN_MOMENT_FILTERS_METADATA | \
+                VIR_DOMAIN_MOMENT_FILTERS_CURRENT | \
                 VIR_DOMAIN_MOMENT_FILTERS_LEAVES)

 int virDomainMomentObjListGetNames(virDomainMomentObjListPtr moments,
diff --git a/src/conf/virdomainmomentobjlist.c b/src/conf/virdomainmomentobjlist.c
index 0ea5e9af80..55d1db9fb0 100644
--- a/src/conf/virdomainmomentobjlist.c
+++ b/src/conf/virdomainmomentobjlist.c
@@ -283,6 +283,7 @@ struct virDomainMomentNameData {
     unsigned int flags;
     int count;
     bool error;
+    virDomainMomentObjPtr current; /* The current moment, if any */
     virDomainMomentObjListFilter filter;
     unsigned int filter_flags;
 };
@@ -303,6 +304,11 @@ static int virDomainMomentObjListCopyNames(void *payload,
         return 0;
     if ((data->flags & VIR_DOMAIN_MOMENT_LIST_NO_LEAVES) && !obj->nchildren)
         return 0;
+    if ((data->flags & VIR_DOMAIN_MOMENT_LIST_CURRENT) && obj != data->current)
+        return 0;
+    if ((data->flags & VIR_DOMAIN_MOMENT_LIST_NO_CURRENT) &&
+        obj == data->current)
+        return 0;

     if (!data->filter(obj, data->filter_flags))
         return 0;
@@ -327,7 +333,8 @@ virDomainMomentObjListGetNames(virDomainMomentObjListPtr moments,
                                unsigned int filter_flags)
 {
     struct virDomainMomentNameData data = { names, maxnames, flags, 0,
-                                            false, filter, filter_flags };
+                                            false, moments->current,
+                                            filter, filter_flags };
     size_t i;

     virCheckFlags(VIR_DOMAIN_MOMENT_FILTERS_ALL, -1);
-- 
2.20.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 6/8] backup: Add support for filtering based on current moment

Posted by Peter Krempa 5 years, 4 months ago

On Fri, Jul 05, 2019 at 23:37:33 -0500, Eric Blake wrote:
> Right now, the snapshot API permits at most one current snapshot, and
> includes specific API for getting at that snapshot
> (virDomainHasCurrentSnapshot, virDomainSnapshotCurrent,
> virDomainSnapshotIsCurrent).  However, with upcoming checkpoints, it
> is conceivable that a hypervisor could mark multiple checkpoints as

The same happens to snapshots too. Since you can create an "incomplete"
snapshot by unselecting some disks and then do a complement snapshot of
all the remaining disks then you get to the same situation.

For more fun you can create a snapshot which partially overlaps and then
neither of the "current" snapshots makes sense any more.

> current (the qemu implementation has only one current checkpoint for
> an incremental backup, and computes the set of changes for a
> differential backup by merging a chain of checkpoints - but other
> hypervisors may treat all differential checkpoints as current).  As
> such, it is more desirable to avoid explicit API for getting at the
> one current checkpoint, and instead have the List API include a filter
> for that purpose.  Still, it is easier to implement that filter
> directly in the common virDomainMomentObjList code, since that is the
> only code that tracks the one moment that is current (both for
> existing snapshots, and for how qemu will be using current
> checkpoints).

I don't quite understand why we need the notion of the current
checkpoint anyways. Technically it's just an implementation detal (I
will object to the decisions separately in the patch which implements it
) and all the checkpoints are "current" by getting updates.

It even does not make sense to stop recording the differences at all
since the changed blocks are not kept separately thus if any of the
checkpoints stops being "current" (by stopping updating the bitmap or
chain of bitmaps) it becomes totally worthless since you won't be able
to reconstruct it.

As such I don't understand why we need the notion of the current
checkpoint altogether. It didn't clarify for me from all the
backs-and-forths when I was commenting about it, but you seem to think
it's necessary, but I can't seem to understand why. As long as the
justification is other than 'it's for symmetry with snapshots' (which is
a bogus justification) I'm willing to go with it, but as such since I
don't really undestand why this is necessary I won't be able to maintain
such code.

> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  src/conf/virdomainmomentobjlist.h | 11 +++++++++--
>  src/conf/virdomainmomentobjlist.c |  9 ++++++++-
>  2 files changed, 17 insertions(+), 3 deletions(-)
> 
> diff --git a/src/conf/virdomainmomentobjlist.h b/src/conf/virdomainmomentobjlist.h
> index 4067e928f4..7628df5e29 100644
> --- a/src/conf/virdomainmomentobjlist.h
> +++ b/src/conf/virdomainmomentobjlist.h
> @@ -78,8 +78,10 @@ typedef enum {
>      VIR_DOMAIN_MOMENT_LIST_TOPOLOGICAL = (1 << 1),
>      VIR_DOMAIN_MOMENT_LIST_LEAVES      = (1 << 2),
>      VIR_DOMAIN_MOMENT_LIST_NO_LEAVES   = (1 << 3),
> -    VIR_DOMAIN_MOMENT_LIST_METADATA    = (1 << 4),
> -    VIR_DOMAIN_MOMENT_LIST_NO_METADATA = (1 << 5),
> +    VIR_DOMAIN_MOMENT_LIST_CURRENT     = (1 << 4),
> +    VIR_DOMAIN_MOMENT_LIST_NO_CURRENT  = (1 << 5),
> +    VIR_DOMAIN_MOMENT_LIST_METADATA    = (1 << 6),
> +    VIR_DOMAIN_MOMENT_LIST_NO_METADATA = (1 << 7),

Why the reorder?

>  } virDomainMomentFilters;
> 
>  #define VIR_DOMAIN_MOMENT_FILTERS_METADATA \

ACK
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 6/8] backup: Add support for filtering based on current moment

Posted by Eric Blake 5 years, 4 months ago

On 7/8/19 3:27 AM, Peter Krempa wrote:
> On Fri, Jul 05, 2019 at 23:37:33 -0500, Eric Blake wrote:
>> Right now, the snapshot API permits at most one current snapshot, and
>> includes specific API for getting at that snapshot
>> (virDomainHasCurrentSnapshot, virDomainSnapshotCurrent,
>> virDomainSnapshotIsCurrent).  However, with upcoming checkpoints, it
>> is conceivable that a hypervisor could mark multiple checkpoints as
> 
> The same happens to snapshots too. Since you can create an "incomplete"
> snapshot by unselecting some disks and then do a complement snapshot of
> all the remaining disks then you get to the same situation.
> 
> For more fun you can create a snapshot which partially overlaps and then
> neither of the "current" snapshots makes sense any more.

For now, I'm deferring this patch to later (or maybe even dropping it),
based on the v9 review comments about using the one-and-only leaf node
snapshot as the default parent at least for the short term where we
interlock against having snapshots. If we find that changing a
checkpoint current node makes sense with snapshots, we can always add
additional filtering at that time, but right now is premature.  While it
is more churn for me, I agree with the goal of stripping down the
initial implementation to the bare minimum needed, and this does not
qualify in that regards.

>> @@ -78,8 +78,10 @@ typedef enum {
>>      VIR_DOMAIN_MOMENT_LIST_TOPOLOGICAL = (1 << 1),
>>      VIR_DOMAIN_MOMENT_LIST_LEAVES      = (1 << 2),
>>      VIR_DOMAIN_MOMENT_LIST_NO_LEAVES   = (1 << 3),
>> -    VIR_DOMAIN_MOMENT_LIST_METADATA    = (1 << 4),
>> -    VIR_DOMAIN_MOMENT_LIST_NO_METADATA = (1 << 5),
>> +    VIR_DOMAIN_MOMENT_LIST_CURRENT     = (1 << 4),
>> +    VIR_DOMAIN_MOMENT_LIST_NO_CURRENT  = (1 << 5),
>> +    VIR_DOMAIN_MOMENT_LIST_METADATA    = (1 << 6),
>> +    VIR_DOMAIN_MOMENT_LIST_NO_METADATA = (1 << 7),
> 
> Why the reorder?

Because if we like the idea of filtering on a current node better than
the idea of no_metadata, then patches that add current filtering in v9
use bit 6 right away, while bits for metadata filtering of checkpoints
sit unused, and it looks odd to have a gap in the numbering. The
snapshot code already has to do mapping between internal bits from the
public API flags, but my initial goal was to have the internal bits
match the checkpoint public bits.  That said, since I'm now working on
stripping the notion of current filtering out of checkpoints before the
initial implementation, the question is moot until (or if) this patch
ever gets revived.

> 
> 
>>  } virDomainMomentFilters;
>>
>>  #define VIR_DOMAIN_MOMENT_FILTERS_METADATA \
> 
> ACK

Thanks for the review, even if later patches changed our minds.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 1/8] snapshot: Rename qemu domain snapshot test files
[libvirt] [PATCH 2/8] snapshot: Fix virDomainUndefineFlags docs regarding snapshots
[libvirt] [PATCH 3/8] snapshot: Add internal option to validate XML against schema
[libvirt] [PATCH 4/8] snapshot: Add VIR_DOMAIN_SNAPSHOT_CREATE_VALIDATE flag
[libvirt] [PATCH 5/8] snapshot: Factor out redefine cycle validation
[libvirt] [PATCH 6/8] backup: Add support for filtering based on current moment
[libvirt] [PATCH 7/8] snapshot: Add ListAll filters for current snapshot
[libvirt] [PATCH 8/8] snapshot: Expose new filter flags in virsh