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

[Fiona Ebner <f.ebner@proxmox.com>]

Am 20.11.25 um 2:21 PM schrieb Fabian Grünbichler:
> On November 20, 2025 11:17 am, Fiona Ebner wrote:
>> @@ -1141,14 +1135,29 @@ my sub volume_snapshot_rollback_locked {
>>  sub volume_snapshot_rollback {
>>      my ($class, $scfg, $storeid, $volname, $snap) = @_;
>>  
>> -    return $class->cluster_lock_storage(
>> -        $storeid,
>> -        $scfg->{shared},
>> -        undef,
>> -        sub {
>> -            return volume_snapshot_rollback_locked($class, $scfg, $storeid, $volname, $snap);
>> -        },
>> -    );
>> +    my $cleanup_worker;
>> +
>> +    eval {
>> +        $class->cluster_lock_storage(
>> +            $storeid,
>> +            $scfg->{shared},
>> +            undef,
>> +            sub {
>> +                volume_snapshot_rollback_locked(
>> +                    $class, $scfg, $storeid, $volname, $snap, \$cleanup_worker,
>> +                );
>> +            },
>> +        );
>> +    };
>> +    my $err = $@;
>> +
>> +    # Spawn outside of the locked section, because with 'saferemove', the cleanup worker also needs
>> +    # to obtain the lock, and in CLI context, it will be awaited synchronously, see fork_worker().
>> +    fork_cleanup_worker($cleanup_worker);
> 
> I mean, this just worked because the forked cleanup sub waited long
> enough by chance for the lock/before acquiring the lock to allow the
> parent to release it in the non-CLI case, right? i.e., it was still
> wrong, because volume_snapshot_rollback_locked might have done more work
> after forking the cleanup worker while still holding the lock.. 

Yes, that is true.

> all the other code paths here have this pattern:
> 
> my $cleanup_worker = eval { lock and do something };
> fork_cleanup_worker($cleanup_worker)
> 
> which is exactly what this patch switches to, so consider this
> 
> Reviewed-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>

Thanks for the review!


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