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

[Peter Xu]

Hi, all,

CC'ing all the reviewers.

Sorry to not respond one by one, just trying to avoid spliting into
several emails and people spend extra time reading them, especially
if this patch is to be dropped.

Please check bottom reply.

On 11/24/2015 01:57 AM, Andrew Jones wrote:
> On Mon, Nov 23, 2015 at 05:22:29PM +0100, Laszlo Ersek wrote:
>> On 11/23/15 11:07, Peter Xu wrote:
>>> Currently, dump-guest-memory supports synchronous operation only. This patch
>>> sets are adding "detach" support for it (just like "migrate -d" for
>>> migration). When "-d" is provided, dump-guest-memory command will return
>>> immediately without hanging user. This should be useful when the backend
>>> storage for the dump file is very slow.
>>>
>>> Peter Xu (2):
>>>   dump-guest-memory: add "detach" flag for QMP/HMP interfaces
>>>   dump-guest-memory: add basic "detach" support.
>>>
>>>  dump.c                | 62 
>>> ++++++++++++++++++++++++++++++++++++++++++++++-----
>>>  hmp-commands.hx       |  5 +++--
>>>  hmp.c                 |  3 ++-
>>>  include/sysemu/dump.h |  4 ++++
>>>  qapi-schema.json      |  3 ++-
>>>  qmp-commands.hx       |  4 ++--
>>>  qmp.c                 |  9 ++++++++
>>>  vl.c                  |  3 +++
>>>  8 files changed, 81 insertions(+), 12 deletions(-)
>>>
>>
>> I'm not seeing anything that would prevent races between the new thread
>> and any other existing threads that manipulate the MemoryRegion objects
>> (in response to guest actions), or the guest RAM contents (by way of
>> executing guest code).
>>
>> The dump_init() function has
>>
>>     if (runstate_is_running()) {
>>         vm_stop(RUN_STATE_SAVE_VM);
>>         s->resume = true;
>>     } else {
>>         s->resume = false;
>>     }
>>
>> Whereas dump_cleanup() has:
>>
>>     if (s->resume) {
>>         vm_start();
>>     }
>>
>> If you return control to the QEMU monitor's user before the dump
>> completes, they could issue the "cont" command, and unleash the VCPU
>> threads again. (Of course, this is just one example where things could
>> go wrong.)

Yes, I added the global flag "dump_in_progress_p" to do this. For
now, what I found might be affected was "dump-guest-memory" itself,
and "cont". Please check patch 2/2 modification for qmp_cont(). I
failed to find any other place that might be influenced by this
asynchronous operation (you are right, maybe it still exists, and it
might introduce extra bugs, actually that's what I was looking for
to see whether I missed something in the review session).

>>
>> Also, the live migration analogy is not a good one IMO. For live
>> migration, a whole infrastructure exists for tracking asynchronous guest
>> state changes (dirty bitmap, locking, whatever).
>>
>> The good analogy with live migration would be continuous streaming of
>> guest memory changes into the dump file, until it converges, or a cutoff
>> is reached (at which point the guest would be frozen, same as now). Of
>> course, such streaming could generate huge amounts of traffic and
>> entirely defeat the original purpose.

Yes, I see that migration is much more complex scenario, so that's
why I am trying to add "basic detach" support, just as I mentioned
in the patch title. :)

Before doing anything like that complex, I will send a mail asking
about it, to first know whether we need to do that.

>>
>> In total, I don't think this is a good idea. I find it possible that
>> this would lead to QEMU crashes, and/or wildly inconsistent guest memory
>> images.
> 
> Despite having already run through both patches giving review comments,
> I agree with Laszlo. At first blush it seems like a good idea, but I
> can't think of how it would be safe. Also, an admin can always background
> the task that invokes the dump if they need that particular terminal
> back. So, this looks more like a management tool problem to solve, if
> anything.
> 
>>
>> As for the goal itself... People also tend to cope with *kdump* being
>> slow on physical machines.
>>
>> My recommendation would be to use the dump compression feature
>> (especially lzo and snappy). In my experience, even when people are
>> aware of their existence, they don't always realize that shrinking the
>> dump file size by a given factor also shrinks the dumping *time* by the
>> same factor, provided that the dumping process remains IO-bound even in
>> the compressed case.
>>
>> Which it should, assuming a "very slow storage" -- lzo and snappy are
>> very CPU-efficient.
> 
> This has been my experience, i.e. using lzo or snappy tends to be much,
> much faster.

Sorry that I am not the daily user of dump-guest-memory, so I may
have not tried to compare how time would save when compression
techniques are used. Thanks (Drew & Laszlo) to let me know this.
Actually, what I am coping with is the bz:

https://bugzilla.redhat.com/show_bug.cgi?id=1193826

I just feel like it would be nice to offer something extra, when
people are using the stdio monitor, they could have another choice
when dump. Also, this is my first patch to QEMU. That's all I
thought about.

Thanks you all (especially Drew and Laszlo) for leaving mass review
comments. After knowing that more than one of you would suggest not
taking the risk comparing to the feature it brings, I'd totally
agree to drop this patch.

Thanks.
Peter

> 
> Thanks,
> drew
>