[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