Re: [Qemu-devel] [PATCH 07/46] Return path: Open a return path on QEMUFile for sockets

[Paolo Bonzini]

Il 16/07/2014 11:37, Dr. David Alan Gilbert ha scritto:
> > > +
> > > +    /* If it's already open, return it */
> > > +    if (qfs->file->return_path) {
> > > +        return qfs->file->return_path;
> >
> > Wouldn't this leave a dangling file descriptor if you call
> > socket_dup_return_path twice, and then close the original QEMUFile?
>
> Hmm - how?

The problem is that there is no reference count on QEMUFile, so if you do

  f1 = open_return_path(f0);
  f2 = open_return_path(f0);
  /* now f1 == f2 */
  qemu_fclose(f1);
  /* now f2 is dangling */

The remark about "closing the original QEMUFile" is also related to this 
part:

    if (result) {
        /* We are the reverse path of our reverse path (although I don't
           expect this to be used, it would stop another dup if it was /
        result->return_path = qfs->file;

which has a similar bug

  f1 = open_return_path(f0);
  f2 = open_return_path(f1);
  /* now f2 == f0 */
  qemu_fclose(f0);
  /* now f2 is dangling */

If this is correct, the simplest fix is to drop the optimization.

>   Source side
>      Forward path - written by migration thread
>             : It's OK for this to be blocking, but we end up with it being
>               non-blocking, and modify the socket code to emulate blocking.

This likely has a performance impact though.  The first migration thread 
code drop from Juan already improved throughput a lot, even if it kept 
the iothread all the time and only converted from nonblocking writes to 
blocking.

>      Return path  - opened by main thread, read by fd_handler on main thread
>             : Must be non-blocking so as not to block the main thread while
>               waiting for a partially sent command.

Why can't you handle this in the migration thread (or a new postcopy 
thread on the source side)?  Then it can stay blocking.

>   Destination side
>      Forward path - read by main thread

This must be nonblocking so that the monitor keeps responding.

>      Return path  - opened by main thread, written by main thread AND postcopy
>                     thread (protected by rp_mutex)

When does the main thread needs to write?

If it doesn't need that, you can just switch to blocking when you 
process the listen command (i.e. when the postcopy thread starts).

Paolo

>      I think I'm OK with both these being blocking.