Re: [Qemu-devel] [PATCH 2/2] main: switch qemu_set_fd_handler to g_io_add_watch

[Anthony Liguori <anthony@codemonkey.ws>]

On 09/04/2011 09:03 AM, Avi Kivity wrote:
> On 08/22/2011 04:12 PM, Anthony Liguori wrote:
>> This patch changes qemu_set_fd_handler to be implemented in terms of
>> g_io_add_watch(). The semantics are a bit different so some glue is
>> required.
>>
>> qemu_set_fd_handler2 is much harder to convert because of its use of
>> polling.
>>
>> The glib main loop has the major of advantage of having a proven
>> thread safe
>> architecture. By using the glib main loop instead of our own, it will
>> allow us
>> to eventually introduce multiple I/O threads.
>>
>> I'm pretty sure that this will work on Win32, but I would appreciate
>> some help
>> testing. I think the semantics of g_io_channel_unix_new() are really
>> just tied
>> to the notion of a "unix fd" and not necessarily unix itself.
>
> 'git bisect' fingered this as responsible for breaking
> qcow2+cache=unsafe. I think there's an off-by-one here and the guilty
> patch is the one that switches the main loop, but that's just a guess.

Semantically, this patch changes when a file descriptor callback is issued.

Does the following make a difference:

diff --git a/vl.c b/vl.c
index 5ba9b35..02f694d 100644
--- a/vl.c
+++ b/vl.c
@@ -1432,9 +1432,9 @@ int main_loop_wait(int nonblocking)
          qemu_mutex_lock_iothread();
      }

+    glib_select_poll(&rfds, &wfds, &xfds, (ret < 0));
      qemu_iohandler_poll(&rfds, &wfds, &xfds, ret);
      slirp_select_poll(&rfds, &wfds, &xfds, (ret < 0));
-    glib_select_poll(&rfds, &wfds, &xfds, (ret < 0));

      qemu_run_all_timers();

What type of guest install is it that fails for you and is this 
qemu-kvm.git or qemu.git?

Regards,

Anthony Liguori

>
> The symptoms are that a guest that is restarted (new qemu process) after
> install doesn't make it through grub - some image data didn't make it do
> disk. With qcow2 and cache=unsafe that can easily happen through exit
> notifiers not being run and the entire qcow2 metadata being thrown out
> the window. Running with raw+cache=unsafe works.
>
>
>>
>> diff --git a/iohandler.c b/iohandler.c
>> index 4deae1e..5ef66fb 100644
>> --- a/iohandler.c
>> +++ b/iohandler.c
>> @@ -80,12 +80,67 @@ int qemu_set_fd_handler2(int fd,
>> return 0;
>> }
>>
>> +typedef struct IOTrampoline
>> +{
>> + GIOChannel *chan;
>> + IOHandler *fd_read;
>> + IOHandler *fd_write;
>> + void *opaque;
>> + guint tag;
>> +} IOTrampoline;
>> +
>> +static gboolean fd_trampoline(GIOChannel *chan, GIOCondition cond,
>> gpointer opaque)
>> +{
>> + IOTrampoline *tramp = opaque;
>> +
>> + if (tramp->opaque == NULL) {
>> + return FALSE;
>> + }
>> +
>> + if ((cond& G_IO_IN)&& tramp->fd_read) {
>> + tramp->fd_read(tramp->opaque);
>> + }
>> +
>> + if ((cond& G_IO_OUT)&& tramp->fd_write) {
>> + tramp->fd_write(tramp->opaque);
>> + }
>> +
>> + return TRUE;
>> +}
>> +
>> int qemu_set_fd_handler(int fd,
>> IOHandler *fd_read,
>> IOHandler *fd_write,
>> void *opaque)
>> {
>> - return qemu_set_fd_handler2(fd, NULL, fd_read, fd_write, opaque);
>> + static IOTrampoline fd_trampolines[FD_SETSIZE];
>> + IOTrampoline *tramp =&fd_trampolines[fd];
>> +
>> + if (tramp->tag != 0) {
>> + g_io_channel_unref(tramp->chan);
>> + g_source_remove(tramp->tag);
>> + }
>> +
>> + if (opaque) {
>> + GIOCondition cond = 0;
>> +
>> + tramp->fd_read = fd_read;
>> + tramp->fd_write = fd_write;
>> + tramp->opaque = opaque;
>> +
>> + if (fd_read) {
>> + cond |= G_IO_IN | G_IO_ERR;
>> + }
>> +
>> + if (fd_write) {
>> + cond |= G_IO_OUT | G_IO_ERR;
>> + }
>> +
>> + tramp->chan = g_io_channel_unix_new(fd);
>> + tramp->tag = g_io_add_watch(tramp->chan, cond, fd_trampoline, tramp);
>> + }
>> +
>> + return 0;
>> }
>>
>> void qemu_iohandler_fill(int *pnfds, fd_set *readfds, fd_set
>> *writefds, fd_set *xfds)
>
>