Re: [pve-devel] [PATCH-SERIES v8 pve-storage/qemu-server] add external qcow2 snapshot support

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

[-- Attachment #1: Type: message/rfc822, Size: 23664 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>
Cc: "w.bumiller@proxmox.com" <w.bumiller@proxmox.com>, "t.lamprecht@proxmox.com" <t.lamprecht@proxmox.com>
Subject: Re: [pve-devel] [PATCH-SERIES v8 pve-storage/qemu-server] add external qcow2 snapshot support
Date: Thu, 10 Jul 2025 15:46:35 +0000
Message-ID: <c6c3457906642a30ddffc0f6b9d28ea6a745ac7c.camel@groupe-cyllene.com>

Hi Fabian,

I'll try to fix all your comments for next week.

I'm going on holiday end of the next week, the 18th july to around 10
August, so I think It'll be the last time I can work on it before next
month. But feel free to improve my patches during this time.




-------- Message initial --------
De: Fabian Grünbichler <f.gruenbichler@proxmox.com>
À: Proxmox VE development discussion <pve-devel@lists.proxmox.com>
Cc: Alexandre Derumier <alexandre.derumier@groupe-cyllene.com>,
Wolfgang Bumiller <w.bumiller@proxmox.com>, Thomas Lamprecht
<t.lamprecht@proxmox.com>
Objet: Re: [pve-devel] [PATCH-SERIES v8 pve-storage/qemu-server] add
external qcow2 snapshot support
Date: 10/07/2025 17:13:43


> Alexandre Derumier via pve-devel <pve-devel@lists.proxmox.com> hat am
> 09.07.2025 18:21 CEST geschrieben:
> This patch series implement qcow2 external snapshot support for files
> && lvm volumes
> 
> The current internal qcow2 snapshots have bad write performance
> because no metadatas can be preallocated.
> 
> This is particulary visible on a shared filesystem like ocfs2 or
> gfs2.
> 
> Also other bugs are freeze/lock reported by users since years on
> snapshots delete on nfs
> (The disk access seem to be frozen during all the delete duration)
> 
> This also open doors for remote snapshot export-import for storage
> replication.
> 
> Changelog v8:
>     storage :
>      - fix Fabian comments
>      - add rename_snapshot   
>      - add qemu_resize
>      - plugin: restrict volnames
>      - plugin: use 'external-snapshots' instead 'snapext'
>     qemu-server:
>      - fix efi test with wrong volnames vm-disk-100-0.raw
>      - use rename_snapshot
> MAIN TODO:
>     - add snapshots tests in both pve-storage && qemu-server
>     - better handle snapshot failure with multiple disks

sent a few follow-ups, as I didn't manage to fully test things and
depending on the outcome of such tests, it might be okay to apply the
series with those follow-ups, and fix up the rest later, or not..

some open issues that I discovered that still need fixing:

1. missing activation when snapshotting an LVM volume if the VM is not
running

snapshotting 'drive-scsi0' (test:123/vm-123-disk-0.qcow2)
snapshotting 'drive-scsi1' (lvm:vm-123-disk-0.qcow2)
  Renamed "vm-123-disk-0.qcow2" to "snap_vm-123-disk-0_test.qcow2" in
volume group "lvm"
failed to stat '/dev/lvm/snap_vm-123-disk-0_test.qcow2'   <============
Use of uninitialized value $size in division (/) at
/usr/share/perl5/PVE/Storage/LVMPlugin.pm line 671.
  Rounding up size to full physical extent 4.00 MiB
  Logical volume "vm-123-disk-0.qcow2" created.
  Logical volume "vm-123-disk-0.qcow2" successfully removed.
  Renamed "snap_vm-123-disk-0_test.qcow2" to "vm-123-disk-0.qcow2" in
volume group "lvm"

2. storage migration with external snapshots needs to be implemented or
disabled (LVM is raw-only at the moment)
3. moving a 'raw' lvm volume to the same storage with format 'qcow2'
fails with "you can't move to the same storage with same format (500)"
(UI and CLI, other way round works)
4. all snapshot volumes on extsnap dir storages will print warnings
like

`this volume filename is not supported anymore`

when hitting `parse_namedir` - those can likely be avoided by skipping
the warning if the name matches the snapshot format and external-
snapshots are enabled..

5. the backing file paths are not relative for LVM
6. it's fairly easy to accidentally create qcow2-formatted LVM volumes,
as opposed to the requirement to enable a non-UI storage option at
storage creation time for dir storages, we might want to add some
warning to the UI at least? or we could also guard usage of the format
with a config option..
7. the snapshot name related helpers being public would be nice to
avoid - one way would be to inline them and duplicate
volume_snapshot_info in the LVM plugin, but if a better option is found
that would be great
8. renaming a volume needs to be forbidden if snapshots exist, or the
whole chain needs to be renamed (this is currently caught higher up in
the stack, not sure if we need/want to also check in the storage
layer?)

the parse_namedir change also need a close look to see if some other
plugins get broken.. (@Wolfgang - since you are working on related
changes!)

I am sure there are more rough edges to be found, so don't consider the
list above complete!

> 
> 
> pve-storage:
> 
> Alexandre Derumier (13):
>   plugin: add qemu_img_create
>   plugin: add qemu_img_create_qcow2_backed
>   plugin: add qemu_img_info
>   plugin: add qemu_img_measure
>   plugin: add qemu_img_resize
>   rbd && zfs : create_base : remove $running param from
> volume_snapshot
>   storage: volume_snapshot: add $running param
>   storage: add rename_snapshot method
>   storage: add volume_support_qemu_snapshot
>   plugin: fix volname parsing
>   qcow2: add external snapshot support
>   lvmplugin: add qcow2 snapshot
>   tests: add lvmplugin test
> 
>  ApiChangeLog                         |  15 +
>  src/PVE/Storage.pm                   |  45 ++-
>  src/PVE/Storage/BTRFSPlugin.pm       |   6 +
>  src/PVE/Storage/CIFSPlugin.pm        |   1 +
>  src/PVE/Storage/Common.pm            |  33 ++
>  src/PVE/Storage/DirPlugin.pm         |  11 +
>  src/PVE/Storage/ESXiPlugin.pm        |   8 +-
>  src/PVE/Storage/ISCSIDirectPlugin.pm |   2 +-
>  src/PVE/Storage/LVMPlugin.pm         | 571 +++++++++++++++++++++----
> -
>  src/PVE/Storage/LvmThinPlugin.pm     |   8 +-
>  src/PVE/Storage/NFSPlugin.pm         |   1 +
>  src/PVE/Storage/PBSPlugin.pm         |   2 +-
>  src/PVE/Storage/Plugin.pm            | 518 +++++++++++++++++++++---
>  src/PVE/Storage/RBDPlugin.pm         |  18 +-
>  src/PVE/Storage/ZFSPlugin.pm         |   4 +-
>  src/PVE/Storage/ZFSPoolPlugin.pm     |   8 +-
>  src/test/Makefile                    |   5 +-
>  src/test/run_test_lvmplugin.pl       | 577
> +++++++++++++++++++++++++++
>  18 files changed, 1662 insertions(+), 171 deletions(-)
>  create mode 100755 src/test/run_test_lvmplugin.pl
> 
> qemu-server :
> 
> Alexandre Derumier (4):
>   qemu_img convert : add external snapshot support
>   blockdev: add backing_chain support
>   qcow2: add external snapshot support
>   tests: fix efi vm-disk-100-0.raw -> vm-100-disk-0.raw
> 
>  src/PVE/QemuConfig.pm                         |   4 +-
>  src/PVE/QemuServer.pm                         | 132 +++++--
>  src/PVE/QemuServer/Blockdev.pm                | 345
> +++++++++++++++++-
>  src/PVE/QemuServer/QemuImage.pm               |   6 +-
>  src/test/cfg2cmd/efi-raw-old.conf             |   2 +-
>  src/test/cfg2cmd/efi-raw-old.conf.cmd         |   2 +-
>  src/test/cfg2cmd/efi-raw-template.conf        |   2 +-
>  src/test/cfg2cmd/efi-raw-template.conf.cmd    |   2 +-
>  src/test/cfg2cmd/efi-raw.conf                 |   2 +-
>  src/test/cfg2cmd/efi-raw.conf.cmd             |   2 +-
>  src/test/cfg2cmd/efi-secboot-and-tpm-q35.conf |   2 +-
>  .../cfg2cmd/efi-secboot-and-tpm-q35.conf.cmd  |   2 +-
>  src/test/cfg2cmd/efi-secboot-and-tpm.conf     |   2 +-
>  src/test/cfg2cmd/efi-secboot-and-tpm.conf.cmd |   2 +-
>  src/test/cfg2cmd/sev-es.conf                  |   2 +-
>  src/test/cfg2cmd/sev-es.conf.cmd              |   2 +-
>  src/test/cfg2cmd/sev-std.conf                 |   2 +-
>  src/test/cfg2cmd/sev-std.conf.cmd             |   2 +-
>  src/test/cfg2cmd/simple-backingchain.conf     |  25 ++
>  src/test/cfg2cmd/simple-backingchain.conf.cmd |  33 ++
>  src/test/run_config2command_tests.pl          |  47 +++
>  src/test/run_qemu_img_convert_tests.pl        |  59 +++
>  src/test/snapshot-test.pm                     |   4 +-
>  23 files changed, 634 insertions(+), 49 deletions(-)
>  create mode 100644 src/test/cfg2cmd/simple-backingchain.conf
>  create mode 100644 src/test/cfg2cmd/simple-backingchain.conf.cmd
> 
> 
> -- 
> 2.39.5


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