Re: [libvirt RFC 20/24] qemu_snapshot: prepare data for external snapshot deletion

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Sep 06, 2022 at 12:19:38PM +0200, Peter Krempa wrote:
> On Tue, Aug 23, 2022 at 18:32:23 +0200, Pavel Hrdina wrote:
> > In order to save some CPU cycles we will collect all the necessary data
> > to delete external snapshot before we even start. They will be later
> > used by code that deletes the snapshots and updates metadata when
> > needed.
> > 
> > With external snapshots we need data that libvirt gets from running QEMU
> > process so if the VM is not running we need to start paused QEMU process
> > for the snapshot deletion and kill at afterwards.
> > 
> > Signed-off-by: Pavel Hrdina <phrdina@xxxxxxxxxx>
> > ---
> >  src/qemu/qemu_snapshot.c | 144 ++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 142 insertions(+), 2 deletions(-)
> 
> First a few general notes for all upcoming patches.
> 
> 1) All of the non-trivial functions dealing with snapshots should have
> documentation explaing what they do. The snapshot code, as you certainly
> know, is very complex and in many cases un-obvious. Having them will
> save us (or anybody else wanting to maintain the code) some headaches in
> the future.
> 
> 2) I'd also suggest that you add a knowledge base internals article
> outlining how snapshot deletion is supposed to work. In case there are
> some user-visible caveats, we also have an user-focused article on
> snapshots ( kbase/snapshots.rst ).
> 
> 3) I'll try to assess the code also from the point of view of changes
> needed to implement reversion of snapshots and the intricacies that
> might bring for deletion.
> 
> > diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
> > index da9b4c30f0..dade5dcea4 100644
> > --- a/src/qemu/qemu_snapshot.c
> > +++ b/src/qemu/qemu_snapshot.c
> > @@ -2232,6 +2232,120 @@ qemuSnapshotRevert(virDomainObj *vm,
> >  }
> >  
> >  
> > +typedef struct {
> > +    virDomainMomentObj *parentSnap;
> > +    virDomainSnapshotDiskDef *snapDisk;
> > +    virDomainDiskDef *domDisk;
> > +    virDomainDiskDef *parentDomDisk;
> > +    virStorageSource *diskSrc;
> > +    virStorageSource *parentDiskSrc;
> > +    virStorageSource *prevDiskSrc;
> > +    qemuBlockJobData *job;
> > +} qemuSnapshotDeleteExternalData;
> > +
> > +
> > +static virDomainMomentObj*
> > +qemuSnapshotFindParentSnapForDisk(virDomainMomentObj *snap,
> > +                                  virDomainSnapshotDiskDef *snapDisk)
> > +{
> > +    virDomainMomentObj *parentSnap = snap->parent;
> > +
> > +    while (parentSnap) {
> > +        ssize_t i;
> > +        virDomainSnapshotDef *parentSnapdef = virDomainSnapshotObjGetDef(parentSnap);
> > +
> > +        if (!parentSnapdef)
> > +            break;
> > +
> > +        for (i = 0; i < parentSnapdef->ndisks; i++) {
> > +            virDomainSnapshotDiskDef *parentSnapDisk = &(parentSnapdef->disks[i]);
> > +
> > +            if (parentSnapDisk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_NO &&
> > +                STREQ(snapDisk->name, parentSnapDisk->name)) {
> > +                return parentSnap;
> > +            }
> > +        }
> > +
> > +        parentSnap = parentSnap->parent;
> > +    }
> > +
> > +    return NULL;
> > +}
> > +
> > +
> > +static GPtrArray*
> > +qemuSnapshotDeleteExternalPrepare(virDomainObj *vm,
> > +                                  virDomainMomentObj *snap)
> > +{
> > +    ssize_t i;
> > +    virDomainSnapshotDef *snapdef = virDomainSnapshotObjGetDef(snap);
> > +    g_autoptr(GPtrArray) ret = g_ptr_array_new_full(0, g_free);
> 
> Is there any specific requirement to access these randomly?
> (specifically why GPtrArray is used instead of e.g. a G(S)List?

There is no need to access these randomly but G(S)List doesn't work that
nicely with g_autoptr and dynamically allocated elements. For this case
we would have to use g_list_free_full().

> > +    for (i = 0; i < snapdef->ndisks; i++) {
> > +        g_autofree qemuSnapshotDeleteExternalData *data = NULL;
> > +        virDomainSnapshotDiskDef *snapDisk = &(snapdef->disks[i]);
> > +
> > +        if (snapDisk->snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_NO)
> > +            continue;
> 
> [...]
> 
> >  typedef struct _virQEMUMomentReparent virQEMUMomentReparent;
> >  struct _virQEMUMomentReparent {
> >      const char *dir;
> > @@ -2504,9 +2618,9 @@ qemuSnapshotDeleteValidate(virDomainMomentObj *snap,
> >      }
> >  
> >      if (virDomainSnapshotIsExternal(snap) &&
> > -        !(flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY)) {
> > +        (flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN)) {
> >          virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> > -                       _("deletion of external disk snapshots not supported"));
> > +                       _("deletion of external disk snapshot with internal children disk snapshots not supported"));
> 
> This seems a bit unrelated to this patch.
> 
> >          return -1;
> >      }
> >  
> > @@ -2523,6 +2637,8 @@ qemuSnapshotDelete(virDomainObj *vm,
> >      int ret = -1;
> >      virDomainMomentObj *snap = NULL;
> >      bool metadata_only = !!(flags & VIR_DOMAIN_SNAPSHOT_DELETE_METADATA_ONLY);
> > +    bool stop_qemu = false;
> > +    g_autoptr(GPtrArray) externalData = NULL;
> >  
> >      virCheckFlags(VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN |
> >                    VIR_DOMAIN_SNAPSHOT_DELETE_METADATA_ONLY |
> > @@ -2537,6 +2653,25 @@ qemuSnapshotDelete(virDomainObj *vm,
> >      if (!metadata_only) {
> >          if (qemuSnapshotDeleteValidate(snap, flags) < 0)
> >              goto endjob;
> > +
> > +        if (virDomainSnapshotIsExternal(snap) &&
> > +            !(flags & (VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN |
> > +                       VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY))) {
> > +            if (!virDomainObjIsActive(vm)) {
> > +                if (qemuProcessStart(NULL, driver, vm, NULL, VIR_ASYNC_JOB_NONE,
> > +                                     NULL, -1, NULL, NULL,
> > +                                     VIR_NETDEV_VPORT_PROFILE_OP_CREATE,
> > +                                     VIR_QEMU_PROCESS_START_PAUSED) < 0) {
> 
> Note that a user might attempt to call a 'virsh resume' here. This might
> be a problem especially if you went the route of asynchronously handling
> the deletion.
> 
> > +                    goto endjob;
> > +                }
> > +
> > +                stop_qemu = true;
> > +            }
> > +
> > +            externalData = qemuSnapshotDeleteExternalPrepare(vm, snap);
> > +            if (!externalData)
> > +                goto endjob;
> 
> Preferrably this operation will be done before starting qemu to do the
> actual deletion. I understand that it has to be done after, to have
> correct pointers after the copy of the definition needed for the
> startup.
> 
> In this instance it probably is still better to do the checks first,
> throw them away if qemu startup is needed and re-do them after, as the
> startup of qemu is a waaay more expensive operation.

Yeah I don't like this ordering as well, I'll check what parts of the
qemuSnapshotDeleteExternalPrepare() can be done without qemu running.

> > +        }
> >      }
> >  
> >      if (flags & (VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN |
> > @@ -2547,6 +2682,11 @@ qemuSnapshotDelete(virDomainObj *vm,
> >      }
> >  
> >   endjob:
> > +    if (stop_qemu) {
> > +        qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_SHUTDOWN,
> > +                        VIR_ASYNC_JOB_NONE, 0);
> 
> So definitely user interaction of the VM needs to be forbidden during
> this API because it would be bad if the VM gets terminated here.

Good point, will look into it.

Pavel

Attachment: signature.asc
Description: PGP signature


[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux