qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH] do not call monitor_resume() from migrate_fd_pu


From: Jan Kiszka
Subject: Re: [Qemu-devel] [PATCH] do not call monitor_resume() from migrate_fd_put_buffer() error path
Date: Fri, 05 Aug 2011 09:11:33 +0200
User-agent: Mozilla/5.0 (X11; U; Linux i686 (x86_64); de; rv:1.8.1.12) Gecko/20080226 SUSE/2.0.0.12-1.1 Thunderbird/2.0.0.12 Mnenhy/0.7.5.666

On 2011-08-05 08:51, Michael Tokarev wrote:
> 05.08.2011 02:19, Jan Kiszka wrote:
> []
>> Resume on error in migrate_fd_put_buffer raced with migrate_fd_cleanup
>> triggered via migrate_fd_put_ready called from migrate_fd_connect.
>>
>> This migration code is a horrible maze. Patch below tries to move
>> monitor_resume a bit out of this. Please check if it works for you, then
>> I'll send it out properly.
> 
> Yes it's a maze.
> 
> The patch was mime-damaged, I had to apply it manually, but it
> wasn't difficult (since it's understandable what it does etc).

Sorry, likely forgot to disable line-wrapping before hitting send.

> 
> And now I can't trigger the original problem anymore, with any
> of my variants.
> 
> Here we go:
> (qemu) migrate "exec:foo"
> sh: foo: not found
> (qemu) migrate "exec:foo"
> sh: foo: not found
> 
> (qemu) migrate "exec:foo"
> sh: foo: not found
> 
> (qemu) migrate "exec:foo"
> sh: foo: not found
> 
> (qemu) migrate "exec:foo"
> sh: foo: not found
> (qemu) migrate "exec:foo"
> sh: foo: not found
> 
> (qemu) migrate "exec:foo"
> sh: foo: not found
> 
> (qemu) migrate "exec:foo"
> sh: foo: not found
> 
> (qemu) migrate "exec:foo"
> sh: foo: not found
> 
> (qemu) migrate "exec:foo"
> sh: foo: not found
> (qemu) migrate "exec:foo"
> sh: foo: not found
> (qemu) migrate "exec:foo"
> sh: foo: not found
> (qemu) migrate "exec:foo"
> sh: foo: not found
> 
> (qemu) migrate "exec:foo"
> sh: foo: not found
> (qemu) migrate "exec:foo"
> sh: foo: not found
> (qemu) migrate "exec:foo"
> sh: foo: not found
> (qemu) migrate "exec:foo"
> sh: foo: not found
> (qemu) migrate "exec:foo"
> sh: foo: not found
> 
> (qemu) migrate "exec:foo"
> sh: foo: not found
> (qemu) migrate "exec:foo"
> sh: foo: not found
> 
> (qemu) migrate "exec:foo"
> sh: foo: not found
> 
> (qemu) migrate "exec:foo"
> sh: foo: not found
> (qemu) migrate "exec:foo"
> sh: foo: not found
> (qemu) migrate "exec:foo"
> sh: foo: not found
> 
> (qemu) migrate "exec:foo"
> sh: foo: not found
> (qemu) migrate "exec:foo"
> sh: foo: not found
> 
> (qemu) migrate "exec:foo"
> sh: foo: not found
> (qemu) migrate "exec:foo"
> sh: foo: not found
> (qemu) migrate "exec:foo"
> sh: foo: not found
> 
> (qemu) migrate "exec:foo"
> sh: foo: not found
> (qemu) migrate "exec:foo"
> sh: foo: not found
> 
> (qemu) migrate "exec:foo"
> sh: foo: not found
> 
> (qemu) migrate "exec:foo"
> sh: foo: not found
> 
> (qemu) migrate "exec:foo"
> sh: foo: not found
> 
> (qemu) migrate "exec:foo"
> sh: foo: not found
> (qemu) migrate "exec:foo"
> sh: foo: not found
> (qemu) migrate "exec:foo"
> sh: foo: not found
> (qemu) migrate "exec:foo"
> sh: foo: not found
> (qemu) migrate "exec:foo"
> sh: foo: not found
> (qemu) migrate "exec:foo"
> sh: foo: not found
> (qemu) migrate "exec:foo"
> sh: foo: not found
> 
> (qemu) migrate "exec:foo"
> sh: foo: not found
> (qemu) migrate "exec:foo"
> sh: foo: not found
> (qemu) migrate "exec:foo"
> sh: foo: not found
> 
> (qemu) migrate "exec:foo"
> sh: foo: not found
> (qemu) migrate "exec:foo"
> sh: foo: not found
> 
> (qemu) migrate "exec:foo"
> sh: foo: not found
> 
> (qemu) migrate "exec:foo"
> sh: foo: not found
> (qemu) migrate "exec:foo"
> sh: foo: not found
> (qemu) migrate "exec:foo"
> sh: foo: not found
> (qemu) migrate "exec:foo"
> sh: foo: not found
> 
> (qemu) migrate "exec:foo"
> sh: foo: not found
> 
> (qemu) migrate "exec:foo"
> sh: foo: not found
> 
> (qemu) migrate "exec:foo"
> sh: foo: not found
> (qemu) migrate "exec:foo"
> sh: foo: not found
> (qemu) migrate "exec:cat > /tmp/foo"
> (qemu) migrate "exec:cat > /tmp/foo"
> (qemu) migrate "exec:foo"
> sh: foo: not found
> (qemu) migrate "exec:foo"
> sh: foo: not found
> 
> (qemu) q
> 
> As you can see, I can hit either of the two cases - with
> and without extra newline, and in both cases (and in
> successful case too) it works correctly.
> 
> There's a difference still between the two - namely that
> extra newline - but that's something else and merely
> cosmetic.
> 
> I also verified that -incoming migration works as expected,
> in both failure and success cases - and indeed it works.
> 
> Speaking of the patch: shouldn't migrate_fd_close() be
> called from migrate_fd_cleanup(),

Maybe. But it requires careful review what the migration close callbacks
are doing and if that is always equivalent (or harmless).

> or vise versa, or both
> be combined into one?  In migrate_fd_cleanup(), the new
> logic is not clear still.  New version of it:
> 
> int migrate_fd_cleanup(FdMigrationState *s)
> {
>     int ret = 0;
> 
>     qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
> 
>     if (s->file) {
>         DPRINTF("closing file\n");
>         if (qemu_fclose(s->file) != 0) {
>             ret = -1;
>         }
>         s->file = NULL;
>     } else {
>         if (s->mon) {
>             monitor_resume(s->mon);
>         }
>     }
> 
>     if (s->fd != -1) {
>         close(s->fd);
>         s->fd = -1;
>     }
> 
>     return ret;
> }
> 
> Why it's EITHER s->file OR s->mon, but not both?  Shouldn't
> the s->mon thing be unconditional?

See description of the patch I just sent out.

> 
> And the whole thing is waiting coroutine conversion badly,
> since the logic is indeed a maze ;)

Yeah, though I think this will not magically remove all windings here.

Jan

Attachment: signature.asc
Description: OpenPGP digital signature


reply via email to

[Prev in Thread] Current Thread [Next in Thread]