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 <[email protected]>
Thanks for the review!
_______________________________________________
pve-devel mailing list
[email protected]
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
