[pve-devel] [PATCH 2/7] use qemu_drive_mirror_monitor in live full clone

[Alexandre DERUMIER]

>>That was what the code did previously. In cose of clone_vm I'm not sure how
>>much sense that made before. A job count should be easy to implement though?
>>(Add a counter to the loop and when it hits the desired number call
>>qemu_drive_mirror_monitor() and reset the counter, then also call it after
>>the loop.)
>>Come to think of it, _finish() might be a better name than _monitor() for
>>that function.

>>If you think a job count makes more sense then clone_disk() would not need
>>changing (and then patch 3 would be fine as it is now).

>>I do find, however, that clone_disk() should return whether it actually used
>>qemu_drive_mirror() as that is not always the case and I don't like that the
>>user has to know to check both $running and $snapshot outside after using it.
>>(Despite the fact that the _monitor() function is a no-op if no job is
>>running, it just feels like a weird API.)

No objection, I'll adapt my patches.

I'll try to update them for tomorrow.


Thanks for the review !


----- Mail original -----
De: "Wolfgang Bumiller" <w.bumiller at proxmox.com>
À: "Alexandre Derumier" <aderumier at odiso.com>
Cc: "pve-devel" <pve-devel at pve.proxmox.com>
Envoyé: Jeudi 20 Octobre 2016 12:45:15
Objet: Re: [pve-devel] [PATCH 2/7] use qemu_drive_mirror_monitor in live full clone

> On October 20, 2016 at 11:03 AM Alexandre DERUMIER <aderumier at odiso.com> wrote: 
> 
> 
> >>I wonder if we should give the clone_disk() - which is called above in a 
> >>loop - a parameter telling it to do this. Not only would save us a 
> >>query-block-job call in the cases where we qemu_drive_mirror() isn't 
> >>called, but since this is the $running case, I'm worried about too many 
> >>parallel mirror jobs hitting a networking bottle-neck and failing to 
> >>complete where a series of single-disk mirrors would succeed. 
> 
> So, begin to mirror first disk, wait until it reach 100% but don't block-job-complete, 
> then mirror second disk, ... ? 
> 
> I don't have checked network bandwith when multiple job are running in parallel. 
> Not sure if we should add an option to define number of parallel job ? 

That was what the code did previously. In cose of clone_vm I'm not sure how 
much sense that made before. A job count should be easy to implement though? 
(Add a counter to the loop and when it hits the desired number call 
qemu_drive_mirror_monitor() and reset the counter, then also call it after 
the loop.) 
Come to think of it, _finish() might be a better name than _monitor() for 
that function. 

If you think a job count makes more sense then clone_disk() would not need 
changing (and then patch 3 would be fine as it is now). 

I do find, however, that clone_disk() should return whether it actually used 
qemu_drive_mirror() as that is not always the case and I don't like that the 
user has to know to check both $running and $snapshot outside after using it. 
(Despite the fact that the _monitor() function is a no-op if no job is 
running, it just feels like a weird API.)