From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [45.144.208.40]) by lore.proxmox.com (Postfix) with ESMTPS id DEE111FF13A for ; Wed, 22 Jul 2026 18:22:25 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 20D612148A; Wed, 22 Jul 2026 18:22:25 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20251104; t=1784737301; x=1785342101; darn=lists.proxmox.com; h=subject:content-transfer-encoding:content-type:mime-version :references:in-reply-to:message-id:date:cc:to:from:from:to:cc :subject:date:message-id:reply-to:content-type; bh=Lcy+wereZozmCkT+TY4BIe7I8IZNUuy/NWQh6rbEqvk=; b=GJDmtnNW9/AnOVuzF+//3FCW/PWnTE8Eb+f4cx4J+UJM7+tLhDLh3sJ5USeMR3W2UY WfLH2gNy7/o7JgkWoPMr3I/10btg4jF39xRULMlHPPet9SVSz4LoFXQsL3iKdRukaTBu nG+QwmrXFwqnLvLmtrD71Jrj3vY2rAtRno5PAMxg3vcz9RNTCBZkcO5zfrzZ8F/7dMMu LniIcjlXte5N1Eoxfll4IFFAxveNJDW1pQXqw79uLrfV2zT/csGbSPAcQN79hJ6hg/Kw pX62NTDR8zh0/8ASWba8Qz+hubpSK9DlcVrFLuI1RfUPU4kW6sx23/TKNe9/yMHGmRfB TTzA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1784737301; x=1785342101; h=subject:content-transfer-encoding:content-type:mime-version :references:in-reply-to:message-id:date:cc:to:from:x-gm-gg :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to :content-type; bh=Lcy+wereZozmCkT+TY4BIe7I8IZNUuy/NWQh6rbEqvk=; b=irt3CD3uirdpfEfKWF3FkrDWUqA6PF/H+4N0i0vyvOazid4LHTdGZVNn2qIQ3lL1EK dKCmcFZrO+Dq1jAYYX1m9lqVaAn1nSdIVhPh2MphBAZ4Y4lXxKZ36WZGf8GYoUqe5Ilp uKYTYHPl+xZw1++yvVNuMnyLluSyWtxJMgJpRpXuQasEw7ER/wwrG7EBqc22/Y47uvn7 D52/04/b8khPgQR8Z+jy+A2X4KUVOjAm7MQ7xYDkiR2PdHhbIWJVYSzCbRV6o5W2c55z ER4K2Jn35XE5MwFYl7TUY6U/fuAJaXv9UHDCKKstb8UdM7deIgHz3uKFg6j6+FR3zkrG zYkw== X-Gm-Message-State: AOJu0YwXz8yYR8rVi5QFyjv00XBQyiNjda4ZPKudHfP8x1lVDIzH3z2k rb2H3efN7jde6at3t+lwfrhzFZxUCq/0iQtsQNbDxmQeFnBv8gRJtbZC X-Gm-Gg: AR+sD11Z0PRvJhWeZxAyoCD7SifiiPF1k49GWAUW0YGGB9666sBvvO9piPToM1zSDFW kyCDajq663ZPhBwoMrw/blpuFGILwZUA1hq/7m+OE14kB6wmEje+omxsQTw69lFQh7O9Ka7hzQP jbksENSHdLEa1xbPFqofWQDADeB/yW6HQkStxuhPtFngjmIIpB3Yl82b0MGW6nJSnuWjB5auyq7 J2aGJAMxl6WFCdUefm4uC2oEaW7+zWYVe/zqRGOM1/PLVzZ4LlbaMrs5yTniE/Y+D/KxTRnwss3 RtBhiOlNJo5J+91mnvPEiNlnw0gju4/5SBKzVGhB6XVR1pal6CrBFl/rr7ZfA50a3o3CyedzH9t rs5orUlTWolpR7BH3dMDDKmpDNCtl9E3WTv7eoRxEgrFLrVuxQctHqbASx32bCXHD9wR9Eb2j X-Received: by 2002:a05:6a21:68e:b0:3b2:8674:9830 with SMTP id adf61e73a8af0-3c4292df0a6mr5001761637.14.1784737299952; Wed, 22 Jul 2026 09:21:39 -0700 (PDT) From: Ciro Iriarte To: pve-devel@lists.proxmox.com Date: Wed, 22 Jul 2026 12:34:28 -0300 Message-ID: <20260722.rbdflattenracefix.copyoffload@cyruspy.gmail.com> In-Reply-To: <20260720.0.copyoffload@cyruspy.gmail.com> References: <20260720.0.copyoffload@cyruspy.gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Subject: [RFC PATCH storage] rbd: fix copy-offload flatten status race on fast small copies X-SPAM-LEVEL: Spam detection results: 0 AWL 0.080 Adjusted score from AWL reputation of From: address DKIM_SIGNED 0.1 Message has a DKIM or DK signature, not necessarily valid DKIM_VALID -0.1 Message has at least one valid DKIM or DK signature DKIM_VALID_AU -0.1 Message has a valid DKIM or DK signature from author's domain DKIM_VALID_EF -0.1 Message has a valid DKIM or DK signature from envelope-from domain DMARC_PASS -0.1 DMARC pass policy FREEMAIL_FROM 0.001 Sender email is commonly abused enduser mail provider RCVD_IN_DNSWL_NONE -0.0001 Sender listed at https://www.dnswl.org/, no trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: NDQVR5DWOMIULXV3WD6NAU2AW6TOOVWX X-Message-ID-Hash: NDQVR5DWOMIULXV3WD6NAU2AW6TOOVWX X-MailFrom: cyruspy@gmail.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: copy_image_status() read the image parent, then looked up the manager flatten task, and declared the flatten dead ("no longer running and the image is still a clone") whenever the task was missing. But the flatten task is only a liveness signal; the image parent link is the completion signal. A fast flatten of a small, near-empty image can vanish from `rbd task list` between the parent read and the task lookup, so a copy that actually completed was misreported as a dead flatten and the whole clone was rolled back. This surfaced live on a running multi-disk clone (3 offloaded 2 GiB disks): one disk flattened and was reaped cleanly while another, polled a moment later, hit the empty-task window and failed the clone. Re-read the authoritative parent when the task is missing: if the flatten just finished the parent is now gone (complete); only when the parent persists AND a second task look-up is still empty is the flatten really dead. A genuinely dead flatten still fails as before. Add copy_offload_rbd_status_test.pm, the first unit test on the rbd offload path: it mocks the rbd/ceph command output to pin the race (empty task + parent cleared on re-read => complete), the genuinely-dead case (=> dies), and the plain running/complete states. It fails against the pre-fix code with the exact error. Found by the freeze-bracket live-validation harness (pve-FCLUPlugin #22). Generated-By: Claude (https://claude.ai) Signed-off-by: Ciro Iriarte Co-Authored-By: Claude --- Applies on top of the v1 series; fixes the 2/5 rbd copy-offload. Found while live-validating the freeze bracket on a running multi-disk clone (details in the commit message). Not a v2 -- the rest of the series is unchanged. src/PVE/Storage/RBDPlugin.pm | 117 +++++++++++++---------- src/test/copy_offload_rbd_status_test.pm | 98 +++++++++++++++++++ src/test/run_plugin_tests.pl | 1 + 3 files changed, 167 insertions(+), 49 deletions(-) create mode 100644 src/test/copy_offload_rbd_status_test.pm diff --git a/src/PVE/Storage/RBDPlugin.pm b/src/PVE/Storage/RBDPlugin.pm index 31e9414..7f0dc00 100644 --- a/src/PVE/Storage/RBDPlugin.pm +++ b/src/PVE/Storage/RBDPlugin.pm @@ -919,61 +919,80 @@ sub copy_image_status { my (undef, $name) = $class->parse_volname($volname); - my $cmd = $rbd_cmd->($scfg, $storeid, 'info', $name, '--format', 'json'); - my $info = ''; - run_rbd_command($cmd, errmsg => "rbd info '$name' error", outfunc => sub { $info .= shift }); - - my $parent = eval { decode_json($info)->{parent} }; - - # No parent: the flatten finished and the image is INDEPENDENT -- which is the - # difference that matters. A clone is readable from the moment it exists, but - # deleting the source before the flatten completes would destroy it. - if (!$parent) { - # Drop the snapshot this copy was taken from, on the SOURCE image. Once the - # flatten completes the target no longer records where it came from, so this is - # the last chance to find it -- and leaving it behind pins space on the source - # and blocks deleting that volume later. - # - # Only ever our own: the name is derived from the target, so a caller-supplied - # snapshot can never match and is left untouched. - if ($source) { - my $own_snap = $rbd_copy_snap->($volname); - my (undef, $src_name) = $class->parse_volname($source->{volname}); - eval { - my $c = $rbd_cmd->( - $source->{scfg}, $source->{storeid}, - 'snap', 'rm', $src_name, '--snap', $own_snap, - ); - run_rbd_command($c, errmsg => "rbd snap rm error", outfunc => sub { }); - }; - } - # The placeholder copy_image_start() parked. Deliberately not removed there: - # 'rbd rm' walks every object and that call can run with a guest frozen. Best - # effort -- the copy is already complete and correct, and dying here would make - # the caller free it. - my $parked = copy_parked_name($name); - if (rbd_volume_exists($scfg, $storeid, $parked)) { - eval { - my $c = $rbd_cmd->($scfg, $storeid, 'rm', $parked); - run_rbd_command($c, errmsg => "rbd rm placeholder '$parked' error"); + # The image's parent link is the source of truth for completion: no parent means the + # flatten finished and the image is INDEPENDENT -- the difference that matters. A + # clone is readable from the moment it exists, but deleting the source before the + # flatten completes would destroy it. + my $read_parent = sub { + my $cmd = $rbd_cmd->($scfg, $storeid, 'info', $name, '--format', 'json'); + my $info = ''; + run_rbd_command($cmd, errmsg => "rbd info '$name' error", outfunc => sub { $info .= shift }); + return eval { decode_json($info)->{parent} }; + }; + + my $parent = $read_parent->(); + + # Still a clone: the flatten is running, has just finished, or died. The manager task + # is only a LIVENESS signal, not the completion signal. A fast flatten of a small + # image can vanish from 'rbd task list' between the info read above and this task + # lookup, so a MISSING task is ambiguous -- re-read the authoritative parent before + # concluding the flatten died. Without this, a copy that actually completed is + # misreported as a dead flatten, the race that surfaces on concurrent multi-disk + # clones of small images. + if ($parent) { + my $task = $rbd_flatten_task->($scfg, $name); + if ($task) { + my $progress = $task->{progress}; + return { + state => 'pending', + (defined($progress) ? (progress => int($progress * 100)) : ()), }; - warn $@ if $@; } - return { state => 'complete' }; + # No task: re-read the parent. If the flatten just finished it is now gone. + $parent = $read_parent->(); + if ($parent) { + # Parent persists with no task. One more task look-up guards a transient + # empty 'rbd task list' while the flatten is genuinely still running; only + # when that is also empty is the flatten really dead. + die "flatten of '$volname' is no longer running and the image is still a clone\n" + if !$rbd_flatten_task->($scfg, $name); + return { state => 'pending' }; + } + # Parent cleared -- the flatten completed; fall through to the completion cleanup. } - # Still parented. Distinguish a running flatten from one that died: without this a - # dead flatten leaves the image parented forever and the caller polls indefinitely. - my $task = $rbd_flatten_task->($scfg, $name); - die "flatten of '$volname' is no longer running and the image is still a clone\n" - if !$task; + # INDEPENDENT. Drop the snapshot this copy was taken from, on the SOURCE image. Once + # the flatten completes the target no longer records where it came from, so this is + # the last chance to find it -- and leaving it behind pins space on the source and + # blocks deleting that volume later. + # + # Only ever our own: the name is derived from the target, so a caller-supplied + # snapshot can never match and is left untouched. + if ($source) { + my $own_snap = $rbd_copy_snap->($volname); + my (undef, $src_name) = $class->parse_volname($source->{volname}); + eval { + my $c = $rbd_cmd->( + $source->{scfg}, $source->{storeid}, + 'snap', 'rm', $src_name, '--snap', $own_snap, + ); + run_rbd_command($c, errmsg => "rbd snap rm error", outfunc => sub { }); + }; + } + # The placeholder copy_image_start() parked. Deliberately not removed there: 'rbd rm' + # walks every object and that call can run with a guest frozen. Best effort -- the + # copy is already complete and correct, and dying here would make the caller free it. + my $parked = copy_parked_name($name); + if (rbd_volume_exists($scfg, $storeid, $parked)) { + eval { + my $c = $rbd_cmd->($scfg, $storeid, 'rm', $parked); + run_rbd_command($c, errmsg => "rbd rm placeholder '$parked' error"); + }; + warn $@ if $@; + } - my $progress = $task->{progress}; - return { - state => 'pending', - (defined($progress) ? (progress => int($progress * 100)) : ()), - }; + return { state => 'complete' }; } sub alloc_image { diff --git a/src/test/copy_offload_rbd_status_test.pm b/src/test/copy_offload_rbd_status_test.pm new file mode 100644 index 0000000..d895f63 --- /dev/null +++ b/src/test/copy_offload_rbd_status_test.pm @@ -0,0 +1,98 @@ +package PVE::Storage::TestCopyOffloadRbdStatus; + +use strict; +use warnings; + +use lib qw(..); + +use PVE::Storage::RBDPlugin; +use PVE::CephConfig; +use Test::More; + +# copy_image_status() must treat the image PARENT LINK -- not the manager flatten task -- +# as the completion signal. A fast flatten of a small image can vanish from +# `rbd task list` between the parent read and the task look-up, so a missing task while +# the image still shows a parent is AMBIGUOUS: re-read the parent before declaring the +# flatten dead. This pins that race (a copy that finished must not be reported as a dead +# flatten -- the failure seen live on a concurrent multi-disk clone) plus the +# unambiguous states around it. + +my $scfg = { type => 'rbd', pool => 'testpool' }; +my $storeid = 'rbd-a'; +my $volname = 'vm-100-disk-0'; + +# Scripted command output, consumed in call order. +my @info_json; # what each 'rbd info' returns (parent present or not) +my @task_json; # what each 'ceph rbd task list' returns + +{ + no warnings 'redefine'; + + # Keep $rbd_cmd from touching the ceph config / filesystem. + *PVE::CephConfig::ceph_connect_option = sub { return (); }; + + # Every rbd subcommand copy_image_status touches goes through run_rbd_command: + # info -> the scripted parent state + # ls -> '[]' (rbd_volume_exists: the parked placeholder never exists here) + # other -> succeed silently (snap rm / rm are best-effort cleanup) + *PVE::Storage::RBDPlugin::run_rbd_command = sub { + my ($cmd, %param) = @_; + my $op = join(' ', @$cmd); + my $out = $param{outfunc} // sub { }; + if ($op =~ /\binfo\b/) { + $out->(shift(@info_json) // '{}'); + } elsif ($op =~ /\bls\b/) { + $out->('[]'); + } + return 0; + }; + + # $rbd_flatten_task runs `ceph rbd task list` through run_command. + *PVE::Storage::RBDPlugin::run_command = sub { + my ($cmd, %param) = @_; + my $out = $param{outfunc} // sub { }; + $out->(shift(@task_json) // '[]'); + return 0; + }; +} + +my $PARENT = '{"parent":{"pool":"testpool","image":"src","snapshot":"__copy_vm-100-disk-0"}}'; +my $NOPARENT = '{}'; +my $A_TASK = + '[{"refs":{"action":"flatten","image_name":"vm-100-disk-0","pool_name":"testpool"},"progress":0.5}]'; +my $NO_TASK = '[]'; + +sub status { + return PVE::Storage::RBDPlugin->copy_image_status($scfg, $storeid, $volname, undef); +} + +# 1. Independent on the first poll: no parent -> complete. +@info_json = ($NOPARENT); +@task_json = (); +is(status()->{state}, 'complete', 'no parent on the first poll => complete'); + +# 2. Flatten still running: parent + a live task -> pending (with progress). +@info_json = ($PARENT); +@task_json = ($A_TASK); +my $st = status(); +is($st->{state}, 'pending', 'parent + a running flatten task => pending'); +is($st->{progress}, 50, ' progress surfaced from the task'); + +# 3. THE RACE: parent seen, task list momentarily empty, re-read shows the flatten just +# finished. Must be complete, NOT a fatal "flatten no longer running". +@info_json = ($PARENT, $NOPARENT); # info#1 parent, info#2 (re-read) parent gone +@task_json = ($NO_TASK); # task look-up empty +is(status()->{state}, 'complete', + 'empty task + parent-cleared-on-reread => complete (the fixed race)'); + +# 4. Genuinely dead flatten: parent persists across the re-read and no task on either +# look-up -> still fails, as before. +@info_json = ($PARENT, $PARENT); +@task_json = ($NO_TASK, $NO_TASK); # first lookup + the guard lookup both empty +my $died = !eval { status(); 1 }; +ok($died, 'parent persists + no task on both look-ups => dies (real failure still fails)'); +like($@, qr/no longer running and the image is still a clone/, ' with the flatten-dead error'); + +done_testing(); + +1; diff --git a/src/test/run_plugin_tests.pl b/src/test/run_plugin_tests.pl index 84995fe..0c55832 100755 --- a/src/test/run_plugin_tests.pl +++ b/src/test/run_plugin_tests.pl @@ -20,6 +20,7 @@ my $res = $harness->runtests( "copy_offload_test.pm", "copy_offload_naming_test.pm", "copy_offload_feature_test.pm", + "copy_offload_rbd_status_test.pm", ); exit -1 if !$res || $res->{failed} || $res->{parse_errors}; -- 2.54.0