public inbox for pve-devel@lists.proxmox.com
 help / color / mirror / Atom feed
From: "Fabian Grünbichler" <f.gruenbichler@proxmox.com>
To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>
Subject: Re: [pve-devel] [PATCH qemu-server 6/6] fix #6543: use qcow2 'discard-no-unref' option when using snapshot-as-volume-chain
Date: Fri, 25 Jul 2025 09:38:54 +0200	[thread overview]
Message-ID: <1753428793.n5phbfd5ll.astroid@yuna.none> (raw)
In-Reply-To: <20250724135956.112138-7-f.ebner@proxmox.com>

On July 24, 2025 3:59 pm, Fiona Ebner wrote:
> Without the 'discard-no-unref', a qcow2 file can grow beyond what
> 'qemu-img measure' reports, because of fragmentation. This can lead to
> IO errors with qcow2 on top of LVM storages, where the containing LV
> is allocated with that size. Guard enabling the option with
> having 'snapshot-as-volume-chain' in the storage configuration for
> now. Enabling it always should be evaluated a bit more and tested on
> different storages. It is a runtime-only option just affecting how
> referencing clusters is handled during discard in qcow2 and nothing
> else, so it is also fine for existing images and migration streams.
> 
> While 'snapshot-as-volume-chain' is not the perfect proxy, as that's
> not only for LVM, it's an experimental feature that covers the LVM
> case and it seems like a nice fit to try out the new option on
> file-based storages too.
> 
> Suggested-by: Alexandre Derumier <alexandre.derumier@groupe-cyllene.com>
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
>  src/PVE/QemuServer/Blockdev.pm                |  7 ++++++
>  src/PVE/QemuServer/QemuImage.pm               | 19 +++++++++++++++
>  src/test/cfg2cmd/simple-backingchain.conf.cmd |  2 +-
>  src/test/run_qemu_img_convert_tests.pl        | 24 ++++++++++++++-----
>  4 files changed, 45 insertions(+), 7 deletions(-)
> 
> diff --git a/src/PVE/QemuServer/Blockdev.pm b/src/PVE/QemuServer/Blockdev.pm
> index 8528a587..1487bc99 100644
> --- a/src/PVE/QemuServer/Blockdev.pm
> +++ b/src/PVE/QemuServer/Blockdev.pm
> @@ -372,6 +372,13 @@ my sub generate_format_blockdev {
>          $blockdev->{size} = int($options->{size});
>      }
>  
> +    # see bug #6543: without this option, fragmentation can lead to the qcow2 file growing larger
> +    # than what qemu-img measure reports, which is problematic for qcow2-on-top-of-LVM
> +    # TODO test and consider enabling this in general
> +    if ($scfg && $scfg->{'snapshot-as-volume-chain'}) {
> +        $blockdev->{'discard-no-unref'} = JSON::true if $format eq 'qcow2';
> +    }
> +
>      return $blockdev;
>  }
>  
> diff --git a/src/PVE/QemuServer/QemuImage.pm b/src/PVE/QemuServer/QemuImage.pm
> index 026c24e9..7f6d5f01 100644
> --- a/src/PVE/QemuServer/QemuImage.pm
> +++ b/src/PVE/QemuServer/QemuImage.pm
> @@ -3,6 +3,9 @@ package PVE::QemuServer::QemuImage;
>  use strict;
>  use warnings;
>  
> +use Fcntl qw(S_ISBLK);
> +use File::stat;
> +
>  use PVE::Format qw(render_bytes);
>  use PVE::Storage;
>  use PVE::Tools;
> @@ -27,6 +30,18 @@ sub convert_iscsi_path {
>      die "cannot convert iscsi path '$path', unknown format\n";
>  }
>  
> +my sub qcow2_target_image_opts {
> +    my ($path, @qcow2_opts) = @_;
> +
> +    my $st = File::stat::stat($path) or die "stat for '$path' failed - $!\n";

right now this is only called for PVE-managed volumes.. so we could
actually call qemu_blockdev_options instead in `convert` below, and use
the driver (and possibly other things?) from there?

> +
> +    my $driver = S_ISBLK($st->mode) ? 'host_device' : 'file';
> +
> +    my $qcow2_opts_str = ',' . join(',', @qcow2_opts);
> +
> +    return "driver=qcow2$qcow2_opts_str,file.driver=$driver,file.filename=$path";
> +}
> +
>  # The possible options are:
>  # bwlimit - The bandwidth limit in KiB/s.
>  # is-zero-initialized - If the destination image is zero-initialized.
> @@ -71,6 +86,7 @@ sub convert {
>      my $dst_format = checked_volume_format($storecfg, $dst_volid);
>      my $dst_path = PVE::Storage::path($storecfg, $dst_volid);
>      my $dst_is_iscsi = ($dst_path =~ m|^iscsi://|);
> +    my $dst_needs_discard_no_unref = $dst_scfg->{'snapshot-as-volume-chain'};

&& $dst_format eq 'qcow2'

as above in Blockdev.pm?

>      my $support_qemu_snapshots = PVE::Storage::volume_qemu_snapshot_method($storecfg, $src_volid);
>  
>      my $cmd = [];
> @@ -94,6 +110,9 @@ sub convert {
>      if ($dst_is_iscsi) {
>          push @$cmd, '--target-image-opts';
>          $dst_path = convert_iscsi_path($dst_path);
> +    } elsif ($dst_needs_discard_no_unref) {
> +        push @$cmd, '--target-image-opts';
> +        $dst_path = qcow2_target_image_opts($dst_path, 'discard-no-unref=true');
>      } else {
>          push @$cmd, '-O', $dst_format;
>      }


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


  parent reply	other threads:[~2025-07-25  7:37 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-07-24 13:59 [pve-devel] [PATCH-SERIES qemu-server 0/6] blockdev and snapshot-as-volume-chain on LVM fixes Fiona Ebner
2025-07-24 13:59 ` [pve-devel] [PATCH qemu-server 1/6] blockdev: helper to add common options Fiona Ebner
2025-07-24 13:59 ` [pve-devel] [PATCH qemu-server 2/6] blockdev: fix discard Fiona Ebner
2025-07-24 13:59 ` [pve-devel] [PATCH qemu-server 3/6] tests: image convert: avoid hard-coded VM ID in result Fiona Ebner
2025-07-24 13:59 ` [pve-devel] [PATCH qemu-server 4/6] tests: image convert: properly set snapshot-as-volume-chain option Fiona Ebner
2025-07-24 13:59 ` [pve-devel] [PATCH qemu-server 5/6] tests: image convert: add tests where storages with 'snapshot-as-volume-chain' are the target Fiona Ebner
2025-07-24 13:59 ` [pve-devel] [PATCH qemu-server 6/6] fix #6543: use qcow2 'discard-no-unref' option when using snapshot-as-volume-chain Fiona Ebner
2025-07-24 18:01   ` DERUMIER, Alexandre via pve-devel
2025-07-25  7:38   ` Fabian Grünbichler [this message]
2025-07-25  8:24     ` Fiona Ebner
2025-07-25  7:40 ` [pve-devel] [PATCH-SERIES qemu-server 0/6] blockdev and snapshot-as-volume-chain on LVM fixes Fabian Grünbichler

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1753428793.n5phbfd5ll.astroid@yuna.none \
    --to=f.gruenbichler@proxmox.com \
    --cc=pve-devel@lists.proxmox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal