[Qemu-devel] [PATCH] migrate_fd_close: delete associated io-handler before closing the fd

[Qemu-devel] [PATCH] migrate_fd_close: delete associated io-handler before closing the fd

[Uri Lublin]

It may happen that the io-handler is still registered. That causes
select() to return with EBADF, not calling handlers for other fds.

The io-handler would be registered when (on the source) the whole state
was written but not yet flushed. For example when using QEMUFileBuffered,
(tcp-migration) there may be data left in a buffer waiting to be transferred.
In such a case buffered_close() calls buffered_flush() which calls
migrate_fd_put_buffer, which may, upon EAGAIN, register migrate_fd_put_notify
as a handler.

Signed-off-by: Uri Lublin <uril@redhat.com>
---
 migration.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/migration.c b/migration.c
index 401383c..57f2a52 100644
--- a/migration.c
+++ b/migration.c
@@ -301,5 +301,7 @@ void migrate_fd_wait_for_unfreeze(void *opaque)
 int migrate_fd_close(void *opaque)
 {
     FdMigrationState *s = opaque;
+
+    qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
     return s->close(s);
 }
-- 
1.6.2.2

Re: [Qemu-devel] [PATCH] migrate_fd_close: delete associated io-handler before closing the fd

[Mark McLoughlin]

On Mon, 2009-06-08 at 14:28 +0300, Uri Lublin wrote:
> It may happen that the io-handler is still registered. That causes
> select() to return with EBADF, not calling handlers for other fds.
> 
> The io-handler would be registered when (on the source) the whole state
> was written but not yet flushed. For example when using QEMUFileBuffered,
> (tcp-migration) there may be data left in a buffer waiting to be transferred.
> In such a case buffered_close() calls buffered_flush() which calls
> migrate_fd_put_buffer, which may, upon EAGAIN, register migrate_fd_put_notify
> as a handler.
> 
> Signed-off-by: Uri Lublin <uril@redhat.com>
> ---
>  migration.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/migration.c b/migration.c
> index 401383c..57f2a52 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -301,5 +301,7 @@ void migrate_fd_wait_for_unfreeze(void *opaque)
>  int migrate_fd_close(void *opaque)
>  {
>      FdMigrationState *s = opaque;
> +
> +    qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);

Looks good, but perhaps a comment explaining how the I/O handler could
possibly be registered here would be useful - at first glance, it seemed
to me that the I/O handler should always be de-registered in
migrate_fd_cleanup() before getting here.

The key to understanding the problem is that qemu_fclose() on a buffered
file can cause I/O to be flushed.

Reviewed-by: Mark McLoughlin <markmc@redhat.com>

Cheers,
Mark.

Re: [Qemu-devel] [PATCH] migrate_fd_close: delete associated io-handler before closing the fd

[Uri Lublin]

On 06/08/2009 06:49 PM, Mark McLoughlin wrote:
> On Mon, 2009-06-08 at 14:28 +0300, Uri Lublin wrote:
>> It may happen that the io-handler is still registered. That causes
>> select() to return with EBADF, not calling handlers for other fds.
>>
>> The io-handler would be registered when (on the source) the whole state
>> was written but not yet flushed. For example when using QEMUFileBuffered,
>> (tcp-migration) there may be data left in a buffer waiting to be transferred.
>> In such a case buffered_close() calls buffered_flush() which calls
>> migrate_fd_put_buffer, which may, upon EAGAIN, register migrate_fd_put_notify
>> as a handler.
>>
>> Signed-off-by: Uri Lublin<uril@redhat.com>
>> ---
>>   migration.c |    2 ++
>>   1 files changed, 2 insertions(+), 0 deletions(-)
>>
>> diff --git a/migration.c b/migration.c
>> index 401383c..57f2a52 100644
>> --- a/migration.c
>> +++ b/migration.c
>> @@ -301,5 +301,7 @@ void migrate_fd_wait_for_unfreeze(void *opaque)
>>   int migrate_fd_close(void *opaque)
>>   {
>>       FdMigrationState *s = opaque;
>> +
>> +    qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
>
> Looks good, but perhaps a comment explaining how the I/O handler could
> possibly be registered here would be useful - at first glance, it seemed
> to me that the I/O handler should always be de-registered in
> migrate_fd_cleanup() before getting here.
>
> The key to understanding the problem is that qemu_fclose() on a buffered
> file can cause I/O to be flushed.

Do you mean in addition to the log-message (copy part of the log message as a 
comment in the code) ?

>
> Reviewed-by: Mark McLoughlin<markmc@redhat.com>

Thanks for the review,
     Uri.

Re: [Qemu-devel] [PATCH] migrate_fd_close: delete associated io-handler before closing the fd

[Mark McLoughlin]

On Mon, 2009-06-08 at 18:56 +0300, Uri Lublin wrote:
> >> +
> >> +    qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
> >
> > Looks good, but perhaps a comment explaining how the I/O handler could
> > possibly be registered here would be useful - at first glance, it seemed
> > to me that the I/O handler should always be de-registered in
> > migrate_fd_cleanup() before getting here.
> >
> > The key to understanding the problem is that qemu_fclose() on a buffered
> > file can cause I/O to be flushed.
> 
> Do you mean in addition to the log-message (copy part of the log message as a 
> comment in the code) ?

It's subtle, so yeah - I think it deserves a comment.

Would the below patch working equally well? But then again, we should
really remove the I/O handler before closing the fd.

(The close(s->fd) in migration_fd_cleanup() looks like it can never
happen - perhaps we should remove it)

Cheers,
Mark.

diff --git a/migration.c b/migration.c
index 401383c..078967f 100644
--- a/migration.c
+++ b/migration.c
@@ -154,13 +154,21 @@ void migrate_fd_error(FdMigrationState *s)
 
 void migrate_fd_cleanup(FdMigrationState *s)
 {
-    qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
+    int fd;
+
+    /* qemu_fclose() can cause I/O to be flushed (see buffered_close())
+     * which, in turn, can cause an I/O handler to be registered. We
+     * need to delay removing the I/O handler until after qemu_fclose().
+     */
+    fd = s->fd;
 
     if (s->file) {
         dprintf("closing file\n");
         qemu_fclose(s->file);
     }
 
+    qemu_set_fd_handler2(fd, NULL, NULL, NULL, NULL);
+
     if (s->fd != -1)
         close(s->fd);
 

Re: [Qemu-devel] [PATCH] migrate_fd_close: delete associated io-handler before closing the fd

[Uri Lublin]

On 06/08/2009 07:38 PM, Mark McLoughlin wrote:
>
> Would the below patch working equally well? But then again, we should
> really remove the I/O handler before closing the fd.

I think it should work too.
My patch does remove the I/O handler before closing the fd.

>
> (The close(s->fd) in migration_fd_cleanup() looks like it can never
> happen - perhaps we should remove it)

Perhaps we should. I think it's like a "plan b" in case qemu_fclose did not 
actually closed the file descriptor (which, as you mentioned, currently, can 
never happen).

> diff --git a/migration.c b/migration.c
> index 401383c..078967f 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -154,13 +154,21 @@ void migrate_fd_error(FdMigrationState *s)
>
>   void migrate_fd_cleanup(FdMigrationState *s)
>   {
> -    qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);

Would it be safer to keep the I/O handler deletion before the call to 
qemu_fclose (in addition to adding the one below), or it does not matter.


> +    int fd;
> +
> +    /* qemu_fclose() can cause I/O to be flushed (see buffered_close())
> +     * which, in turn, can cause an I/O handler to be registered. We
> +     * need to delay removing the I/O handler until after qemu_fclose().
> +     */
> +    fd = s->fd;
>
>       if (s->file) {
>           dprintf("closing file\n");
>           qemu_fclose(s->file);
>       }
>
> +    qemu_set_fd_handler2(fd, NULL, NULL, NULL, NULL);
> +
>       if (s->fd != -1)
>           close(s->fd);
>
>

Regards,
     Uri.