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

[Thomas Lamprecht]

On 10/9/19 8:47 AM, Fabian Ebner wrote:
> 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.
> 
> 

Yeah, actually only the LVM(thick) plugin uses this mechanism as
opt-in