From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id B045B91456 for ; Wed, 3 Apr 2024 17:07:59 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 8A9DE1B8C9 for ; Wed, 3 Apr 2024 17:07:29 +0200 (CEST) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Wed, 3 Apr 2024 17:07:28 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 9188844B1E for ; Wed, 3 Apr 2024 17:07:28 +0200 (CEST) From: Hannes Duerr To: pve-devel@lists.proxmox.com Date: Wed, 3 Apr 2024 17:07:12 +0200 Message-Id: <20240403150712.262773-1-h.duerr@proxmox.com> X-Mailer: git-send-email 2.39.2 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.021 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [plugin.pm, proxmox.com] Subject: [pve-devel] [PATCH pve-storage 1/1] storage/plugin: implement ref-counting for disknames in get_next_vm_diskname X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 03 Apr 2024 15:07:59 -0000 As Fabian has already mentioned here[0], there can be a race between two parallel imports. More specifically, if both imports have --allow-rename set and the desired name already exists, then it can happen that both imports get the same name. The reason for this is that we currently only check which names have already been assigned in the vm config and then use the next free one, and do not check whether a running process has already been assigned the same name. However, while writing and testing the patch, I found that this is often not a problem, as - in most cases the source VM config is locked and therefore only one process can generate a new name (migrate, clone, move disk, update config) - a name must be specified and therefore no new name is generated (pvesm alloc) - the timeframe for the race is very short. At the same time, it is possible that I have not considered all edge cases and that there are other cases where the race can occur, especially with regard to remote_migrations. You can provoke the problem with two parallel imports on a host where local-lvm:vm-100-disk-0 already exists: pvesm import local-lvm:vm-100-disk-0 raw+size --allow-rename 1 Now that I've looked into the problem a bit, I'm not sure this patch is even necessary as it adds more complexity. So I wanted to ask for your opinion, wether you think it makes sense to add this change or not. The patch introduces a tmp file which stores the newly assigned disk names and the pid of the process which requested the disk name. If a second process is assigned the same name, it will see from the file that the name has already been assigned to another process, and will take the next available name. Reading and writing to the tmp file requires a lock to prevent races. [0] https://lists.proxmox.com/pipermail/pve-devel/2024-January/061526.html Signed-off-by: Hannes Duerr --- src/PVE/Storage/Plugin.pm | 99 ++++++++++++++++++++++++++++++++++----- 1 file changed, 86 insertions(+), 13 deletions(-) diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm index 7456c8e..f76550a 100644 --- a/src/PVE/Storage/Plugin.pm +++ b/src/PVE/Storage/Plugin.pm @@ -10,8 +10,10 @@ use File::chdir; use File::Path; use File::Basename; use File::stat qw(); +use File::Copy; use PVE::Tools qw(run_command); +use PVE::ProcFSTools; use PVE::JSONSchema qw(get_standard_option register_standard_option); use PVE::Cluster qw(cfs_register_file); @@ -779,23 +781,94 @@ my $get_vm_disk_number = sub { sub get_next_vm_diskname { my ($disk_list, $storeid, $vmid, $fmt, $scfg, $add_fmt_suffix) = @_; - $fmt //= ''; - my $prefix = ($fmt eq 'subvol') ? 'subvol' : 'vm'; - my $suffix = $add_fmt_suffix ? ".$fmt" : ''; + my $code = sub { + my $reserved_names_file = "/var/tmp/pve-reserved-volnames"; + my $tmp_file = "/var/tmp/pve-reserved-volnames.tmp"; - my $disk_ids = {}; - foreach my $disk (@$disk_list) { - my $disknum = $get_vm_disk_number->($disk, $scfg, $vmid, $suffix); - $disk_ids->{$disknum} = 1 if defined($disknum); - } + $fmt //= ''; + my $prefix = ($fmt eq 'subvol') ? 'subvol' : 'vm'; + my $suffix = $add_fmt_suffix ? ".$fmt" : ''; + my $disk_id; + my $disk_ids = {}; - for (my $i = 0; $i < $MAX_VOLUMES_PER_GUEST; $i++) { - if (!$disk_ids->{$i}) { - return "$prefix-$vmid-disk-$i$suffix"; + foreach my $disk (@$disk_list) { + my $disknum = $get_vm_disk_number->($disk, $scfg, $vmid, $suffix); + $disk_ids->{$disknum} = 1 if defined($disknum); } - } - die "unable to allocate an image name for VM $vmid in storage '$storeid'\n" + for (my $i = 0; $i < $MAX_VOLUMES_PER_GUEST; $i++) { + if (!$disk_ids->{$i}) { + $disk_id = $i; + last; + } + } + + if (! -e $reserved_names_file) { + my $create_h = IO::File->new($reserved_names_file, "w") || + die "can't open or create'$reserved_names_file' - $!\n"; + print $create_h "$storeid $vmid $disk_id $$"; + $create_h->close; + + return "$prefix-$vmid-disk-$disk_id$suffix"; + } else { + my $collision; + my $pid; + + my $in_h = IO::File->new($reserved_names_file, "r") || + die "can't open or create'$reserved_names_file' - $!\n"; + my $out_h = IO::File->new($tmp_file, "w") || + die "can't open or create'$tmp_file' - $!\n"; + + # remove entries when the process does not exist anymore + while (my $line = <$in_h>) { + ($pid) = $line =~ /$PVE::JSONSchema::PVE_ID \d+ \d+ (\d+)/; + next if (!defined $pid || !defined PVE::ProcFSTools::check_process_running($pid)); + print $out_h $line; + } + + $in_h->close; + $out_h->close; + move($tmp_file, $reserved_names_file); + + for (my $y = $disk_id; $y < $disk_id + 5; $y++) { + undef $collision; + + my $input_h = IO::File->new($reserved_names_file, "r") || + die "can't open or create'$reserved_names_file' - $!\n"; + + while (my $line = <$input_h>) { + if ($disk_ids->{$y} || $line =~ /$storeid $vmid $y \d+/) { + $collision=1; + last; + } + } + + # Found new free file name so we add it to the reserved file + if (!defined $collision) { + my $append_h = IO::File->new($reserved_names_file, "a") || + die "can't open or create'$reserved_names_file' - $!\n"; + + print $append_h "$storeid $vmid $disk_id $$"; + $append_h->close; + + return "$prefix-$vmid-disk-$disk_id$suffix"; + } + + $input_h->close; + } + + return; + } + + }; + + my $new_disk_name = PVE::Tools::lock_file('/var/lock/pve-disk-names.lck', 10, $code); + die $@ if $@; + + if (!$new_disk_name) { + die "unable to allocate an image name for VM $vmid in storage '$storeid'\n"; + } + return $new_disk_name; } sub find_free_diskname { -- 2.39.2