Re: [pve-devel] [PATCH v3 pve-storage 1/3] qcow2: add external snapshot support

["DERUMIER, Alexandre via pve-devel" <pve-devel@lists.proxmox.com>]

[-- Attachment #1: Type: message/rfc822, Size: 15532 bytes --]

From: "DERUMIER, Alexandre" <alexandre.derumier@groupe-cyllene.com>
To: "pve-devel@lists.proxmox.com" <pve-devel@lists.proxmox.com>, "f.gruenbichler@proxmox.com" <f.gruenbichler@proxmox.com>
Subject: Re: [pve-devel] [PATCH v3 pve-storage 1/3] qcow2: add external snapshot support
Date: Fri, 10 Jan 2025 11:51:35 +0000
Message-ID: <1caecaa23e5d57030a9e31f2f0e67648f1930d6a.camel@groupe-cyllene.com>

>>yes, for the "first" snapshot that is true (since that one is
>>basically the baseline data, which will often be huge compared to the
>>snapshot delta). but streaming (rebasing) saves us the rename, which
>>makes the error handling a lot easier/less risky. maybe we could
>special case the first snapshot as a performance optimization? ;)

Ah, that's a good point indeed. Yes, I think it's a good idea, commit
to "first" snapshot, and rebase for others. I'll look to implement
this.

> 
> 
> This one was if we want to do retry in case of error, if we have
> multiple disks. (for example, first snapshot delete api call,  the
> first disk remove the snapshot, but a bug occur and second disk don't
> remove the snapshot). 
> 
> User could want to unlock the vm-snaphot lock and  and fix it
> manually
> with calling again the snapshot delete.
> 
> I'm not sure how to handle this correctly ?

>>I think the force parameter for snapshot deletion covers this
>>already, and it should be fine for this to die..

Ah, ok, I was not aware about this parameter ! thanks.

> 
> 
> > + } else {
> > +     print"commit $snappath\n";
> > +     $cmd = ['/usr/bin/qemu-img', 'commit', $snappath];
> 
> > > leftover from previous version? not used/overwritten below ;)
> 
> no, this is really to commit the the snapshot to parent

>>but it is not executed..

Ah, ok ! sorrry ! I think I have dropped some code during rebase before
sending patches, because I had tested it a lot of time !



> this is for this usecase :
> 
> A<----B<----C.
> 
> you commit B to A,  then you need to change the backing file of C to
> A
> (instead B)
> 
> A<----C

>>but this is the wrong semantics.. the writes/delta in B need to go to
>>C (they happened after A), not to A!

I think they can go to A (commit) or C (stream)

here an example:

current (1TB)
 - take snap A

(A (1TB)<------new current 500MB (backing file A))

- take snap B

(A (1TB)<------B 500MB (backingfile A)<------new current 10MB
(backingfile B))


Then, you want to delete B.


so, you stream it to current. (so copy 500MB to current in this
example)

Then, you want to delete snapshot A
you don't want stream A to current, because A is the big initial image.
So, instead, you need to commit the current to A (with the extra 500MB)


So, if you have a lot of snapshot to delete, you are going do a copy
same datas each time to the upper snapshot for nothing, because at the
end we are going to commit to the initial "first" snapshot/image.






[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel