Re: [Qemu-devel] [PATCH] do not call monitor_resume() from migrate_fd_put_buffer() error path

[Jan Kiszka <jan.kiszka@web.de>]

[-- Attachment #1: Type: text/plain, Size: 5529 bytes --]

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 #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]