Re: [pve-devel] [PATCH v3 qemu-server 11/11] qcow2: add external snapshot support

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

[-- Attachment #1: Type: message/rfc822, Size: 15710 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 qemu-server 11/11] qcow2: add external snapshot support
Date: Mon, 13 Jan 2025 18:07:43 +0000
Message-ID: <09c1a7f90fe54d4423d7b3e81ffc618aac999ef2.camel@groupe-cyllene.com>

> 
> 
> > > should this maybe have been vdisk_alloc and it just works by
> accident?
> It's not works fine with vdisk_alloc, because the volume need to be
> created without the size specified but with backing file param
> instead.
> (if I remember, qemu-img is looking at the backing file size+metadas
> and set the correct total size for the new volume)

>>I am not sure I follow.. we create a snapshot, but then we pretend it
>>isn't a file with backing file when passing it to qemu? this seems
>>wrong.. IMHO we should just allocate (+format) here, and then let
>>qemu do the backing link up instead of this confusing (and error-
>>prone, as it masks problems that should be a hard error!) call..
>>
>>
>>
>>
> >>Maybe a better way could be to reuse vdisk_alloc, and add backing
> >>file
> >>as param ?
>>
>>that seems.. wrong as well? the file can never be bigger just because
>>it has a backing file right? why can't we just allocate and format
>>the regular volume?

I need to redo tests as I don't remember exactly.

From memory (I have wrote it 2month ago, sorry ^_^ ) , it was maybe :
 - related with metadatas prealloc size + lvm size).

 or (but I need to verify)

 The "blockdev-snasphot" qmp later, only change the backing-file in
memory in the blockgraph, but not inside the file itself. (so after a
restart of the process, the chain is borken).


> > +    my $new_file_blockdev = generate_file_blockdev($storecfg,
> > $drive, $new_current_file_nodename);
> > +    my $new_fmt_blockdev = generate_format_blockdev($storecfg,
> > $drive, $new_current_fmt_nodename, $new_file_blockdev);
> > +
> > +    $new_fmt_blockdev->{backing} = undef;
> 
> > > generate_format_blockdev doesn't set backing? 
> yes, it's adding backing
> 
> > > maybe this should be >>converted into an assertion?
> 
> but they are a limitation of the qmp blockdev-ad ++blockdev-snapshot
> where the backing attribute need undef in the blockdev-add or the
> blockdev-snapshot will fail because it's trying itself to set the
> backing file when doing the switch.
> 
> From my test, it was related to this

>>yeah, what I mean is:
>>
>>generate_format_blockdev doesn't (and should never) set backing. so
>>setting it to undef has no effect. but we might want to assert that
>>it *is* undef, so that if we ever mistakenly change
>>generate_format_blockdev to set backing, it will be caught instead of
>>silently being papered over..

you need it if you want to have block node-names for snapshots.
if backing is no defined, the backing chain is autogenerated with
random #block node-names.   (so rename/block reopen can't work)






[-- 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