Re: [Qemu-devel] [PATCH REPOST 0/2] Add basic "detach" support for dump-guest-memory

[Peter Xu <peterx@redhat.com>]

On 11/25/2015 10:46 AM, Fam Zheng wrote:
> On Tue, 11/24 06:49, Eric Blake wrote:
>> On 11/24/2015 04:37 AM, Fam Zheng wrote:
>>
>>>> I think the patch should be dropped, and periodic progress reports
>>>> should be emitted from within the dump loops that do the heavy lifting.
>>>>
>>>> For the ELF format dumps, that loop appears to reside in dump_iterate()
>>>> [dump.c].
>>>>
>>>> For the compressed format dumps, the loop seems to live in
>>>> write_dump_pages() [dump.c].
>>>
>>> This is a good idea!
>>>
>>> What I'm not sure is where to report the progress. Can it be the monitor where
>>> the dump-guest-memory command was issued? In other words, do we support raising
>>> events before the previous command returns? If yes, can libvirt handle this
>>> correctly? (But the worst case is using another channel to communicate the
>>> progress, it is ad-hocery but it must be better than all the risk and effort to
>>> enable multi-threaded dump.)
>>>
>>> Eric, Markus, have any idea with the progress reporting?
>>
>> I'm fairly certain we support raising events prior to completion of a
>> synchronous command; what I'm not sure of is whether the event hits the
>> wire right away or whether it piles up waiting for the next synchronous
>> command completion.  If the latter, then we need to rework it (since the
>> whole point of this exercise is that we are trying to give progress of a
>> long-running synchronous command that hasn't completed yet).
> 
> So in that case we may want some "flush" operation of events. That sounds OK to
> me.
> 
>> But we
>> only have the one monitor connection for libvirt - the only way to pass
>> events through a second channel is to open a second monitor connection,
>> but that feels wrong to make libvirt have to track two monitors.
> 
> OK, that's a fair point, but FWIW I was thinking about adding an optional
> argument:
> 
>     "*progress": "fd:dump-progress"
> 
> into which dump.c talks in a mini-protocol, to send progress information. It's
> just an crazily hacky idea, not anything I'm advocating.

If query status is necessary, what about adding one command:
"query-dump"? Which could be a simplified version of "query-migration":

1. before first dump:

-> { "execute": "query-dump" }
<- { "return": {} }

2. one background dump in progress:

-> { "execute": "query-dump" }
<- {
      "return":{
         "status":"active",
         "percentage": {0..99},
      }
   }

3. after first dump, and not running background dump (substraction
   of case 1 and 2)

-> { "execute": "query-dump" }
<- {
     "return": {
        "status": "completed|failed",
     }
   }

All these would be based on the fact that this patch might not be
dropped though. :)

Thanks.
Peter

> 
> Thanks,
> 
> Fam
>