[pve-devel] [PATCH storage 1/2] Fix use of worker in vdisk_free

[Fabian Ebner]

On 10/8/19 11:21 AM, Thomas Lamprecht wrote:
> On 10/8/19 10:48 AM, Fabian Ebner wrote:
>> Previously 'free_image' would be executed right away, which is not
>> the intended behaviour.
>>
>> Signed-off-by: Fabian Ebner <f.ebner at proxmox.com>
>> ---
>>
>> This is a followup to [0] but it has nothing to do with the original patch
>> so I didn't put a v2.
>>
>> [0]: https://pve.proxmox.com/pipermail/pve-devel/2019-October/039281.html
>>
>>   PVE/Storage.pm | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/PVE/Storage.pm b/PVE/Storage.pm
>> index 298976f..64b79c9 100755
>> --- a/PVE/Storage.pm
>> +++ b/PVE/Storage.pm
>> @@ -760,7 +760,9 @@ sub vdisk_free {
>>   
>>   	my (undef, undef, undef, undef, undef, $isBase, $format) =
>>   	    $plugin->parse_volname($volname);
>> -	$cleanup_worker = $plugin->free_image($storeid, $scfg, $volname, $isBase, $format);
>> +	$cleanup_worker = sub {
>> +	    $plugin->free_image($storeid, $scfg, $volname, $isBase, $format);
>> +	};
>>       });
>>   
>>       return if !$cleanup_worker;
> but now above never evaluates to false?
> And makes the lock then sense at all? I mean it's used for the
> $volume_is_base_and_used__no_lock check only..
>
> This code is like this since the beginning of the storage plugin system
> commit 1dc01b9f30f4cb201f68a344ff43539623164945 from 2012, and there's
> no explicit info about what was inteded, could also be that the $cleanup_worker
> was left over by mistake instead..
>
> I mean deletion /could/ work without locking on some/most storages..
>
As Fabian pointed out to me offline, my approach is wrong. I didn't
interpret the code here correctly.
For example with the LVMPlugin free_image can return a subroutine
to be spawned as a worker, which is why the code here is as it is.
And we don't always want to spawn a worker (which also results in
a task being created) when we do vdisk_free.