[pve-devel] [RFC storage] lvm plugin: fix locking for rollback when using CLI

[pve-devel] [RFC storage] lvm plugin: fix locking for rollback when using CLI

[Fiona Ebner]

For some reason still to be determined, doing a rollback via CLI on an
LVM storage with snapshot-as-volume-chain and saferemove would run
into locking issues (likely trying to re-acquire the lock it already
holds a second time). The same issue does not happen when the rollback
is done via UI.

Avoid doing fork_cleanup_worker() inside the locked section to avoid
this.

Fixes: 8eabcc7 ("lvm plugin: snapshot-as-volume-chain: use locking for snapshot operations")
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 src/PVE/Storage/LVMPlugin.pm | 24 +++++++++++++-----------
 1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/src/PVE/Storage/LVMPlugin.pm b/src/PVE/Storage/LVMPlugin.pm
index 97f7bf4..164437a 100644
--- a/src/PVE/Storage/LVMPlugin.pm
+++ b/src/PVE/Storage/LVMPlugin.pm
@@ -1117,23 +1117,17 @@ sub volume_rollback_is_possible {
 }
 
 my sub volume_snapshot_rollback_locked {
-    my ($class, $scfg, $storeid, $volname, $snap) = @_;
+    my ($class, $scfg, $storeid, $volname, $snap, $cleanup_worker) = @_;
 
     my $format = ($class->parse_volname($volname))[6];
 
     die "can't rollback snapshot for '$format' volume\n" if $format ne 'qcow2';
 
-    my $cleanup_worker = eval { free_snap_image($class, $storeid, $scfg, $volname, 'current'); };
+    $cleanup_worker->$* = eval { free_snap_image($class, $storeid, $scfg, $volname, 'current'); };
     die "error deleting snapshot $snap $@\n" if $@;
 
     eval { alloc_snap_image($class, $storeid, $scfg, $volname, $snap) };
-    my $alloc_err = $@;
-
-    fork_cleanup_worker($cleanup_worker);
-
-    if ($alloc_err) {
-        die "can't allocate new volume $volname: $alloc_err\n";
-    }
+    die "can't allocate new volume $volname: $@\n" if $@;
 
     return undef;
 }
@@ -1141,14 +1135,22 @@ my sub volume_snapshot_rollback_locked {
 sub volume_snapshot_rollback {
     my ($class, $scfg, $storeid, $volname, $snap) = @_;
 
-    return $class->cluster_lock_storage(
+    my $cleanup_worker;
+
+    $class->cluster_lock_storage(
         $storeid,
         $scfg->{shared},
         undef,
         sub {
-            return volume_snapshot_rollback_locked($class, $scfg, $storeid, $volname, $snap);
+            volume_snapshot_rollback_locked(
+                $class, $scfg, $storeid, $volname, $snap, \$cleanup_worker,
+            );
         },
     );
+
+    fork_cleanup_worker($cleanup_worker);
+
+    return;
 }
 
 sub volume_snapshot_delete {
-- 
2.47.3



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

Re: [pve-devel] [RFC storage] lvm plugin: fix locking for rollback when using CLI

[Fiona Ebner]

Hi Andrei,

Am 20.11.25 um 9:03 AM schrieb Andrei Perepiolkin:
> Hi Fiona,
> 
> Regarding lock problem that you have mentioned.
> Can it be related to series of calls:
> 
> volume_snapshot_rollback https://github.com/proxmox/pve-storage/blob/
> a52a1ef526e289d54a5bf44c7467dd4ee49aecab/src/PVE/Storage/
> LVMPlugin.pm#L1141C1-L1152C2
> |
> volume_snapshot_rollback_locked https://github.com/proxmox/pve-storage/
> blob/a52a1ef526e289d54a5bf44c7467dd4ee49aecab/src/PVE/Storage/
> LVMPlugin.pm#L1119C1-L1126C98
> |
> free_snap_image https://github.com/proxmox/pve-storage/blob/
> a52a1ef526e289d54a5bf44c7467dd4ee49aecab/src/PVE/Storage/
> LVMPlugin.pm#L763C1-L768C2
> |
> free_lvm_volumes https://github.com/proxmox/pve-storage/blob/
> a52a1ef526e289d54a5bf44c7467dd4ee49aecab/src/PVE/Storage/
> LVMPlugin.pm#L374C1-L382C15
> 
> Best regards,
> Andrei
> 

yes, it is those calls. That's why the patch moves the spawning of the
cleanup worker outside of the locked rollback section. The open question
was why it happens via CLI, but not via UI. I didn't have the time to
look into the reason yesterday, but looking at fork_worker() in
RESTEnvironment.pm is pretty telling, which has a note:

# NOTE: we simulate running in foreground if ($self->{type} eq 'cli')

so while it does fork() and spawn a second task, it waits for that
synchronously and therefore does not drop the lock.

> On 11/19/25 12:22, Fiona Ebner wrote:
>> @@ -1141,14 +1135,22 @@ my sub volume_snapshot_rollback_locked {
>>   sub volume_snapshot_rollback {
>>       my ($class, $scfg, $storeid, $volname, $snap) = @_;
>>   -    return $class->cluster_lock_storage(
>> +    my $cleanup_worker;
>> +
>> +    $class->cluster_lock_storage(
>>           $storeid,
>>           $scfg->{shared},
>>           undef,
>>           sub {
>> -            return volume_snapshot_rollback_locked($class, $scfg,
>> $storeid, $volname, $snap);
>> +            volume_snapshot_rollback_locked(
>> +                $class, $scfg, $storeid, $volname, $snap, \
>> $cleanup_worker,
>> +            );
>>           },
>>       );

I forgot to put an eval around here to make sure we spawn the cleanup
worker in the error case too. I'll send a v2 with that and more context
in the commit message

>> +
>> +    fork_cleanup_worker($cleanup_worker);
>> +
>> +    return;
>>   }
>>     sub volume_snapshot_delete {
> 

Best Regards,
Fiona



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

[pve-devel] superseded: [RFC storage] lvm plugin: fix locking for rollback when using CLI

[Fiona Ebner]

superseded by:
https://lore.proxmox.com/pve-devel/20251120101742.24843-1-f.ebner@proxmox.com/T/



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