1. Patchew
  2. QEMU
  3. [PATCH v5 00/23] migration: File based migration with multifd and mapped-ram
  4. View patch

[PATCH v5 17/23] migration/multifd: Add outgoing QIOChannelFile support

Fabiano Rosas posted 23 patches 9 months ago
Maintainers: Paolo Bonzini <pbonzini@redhat.com>, Peter Xu <peterx@redhat.com>, David Hildenbrand <david@redhat.com>, "Philippe Mathieu-Daudé" <philmd@linaro.org>, "Daniel P. Berrangé" <berrange@redhat.com>, Fabiano Rosas <farosas@suse.de>, Eric Blake <eblake@redhat.com>, Markus Armbruster <armbru@redhat.com>, Thomas Huth <thuth@redhat.com>, Laurent Vivier <lvivier@redhat.com>
There is a newer version of this series
[PATCH v5 17/23] migration/multifd: Add outgoing QIOChannelFile support
Posted by Fabiano Rosas 9 months ago 
Allow multifd to open file-backed channels. This will be used when
enabling the mapped-ram migration stream format which expects a
seekable transport.

The QIOChannel read and write methods will use the preadv/pwritev
versions which don't update the file offset at each call so we can
reuse the fd without re-opening for every channel.

Contrary to the socket migration, the file migration doesn't need an
asynchronous channel creation process, so expose
multifd_channel_connect() and call it directly.

Note that this is just setup code and multifd cannot yet make use of
the file channels.

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
- moved flags change to another patch
- removed channels_created assert
---
 migration/file.c    | 41 +++++++++++++++++++++++++++++++++++++++--
 migration/file.h    |  4 ++++
 migration/multifd.c | 18 +++++++++++++++---
 migration/multifd.h |  1 +
 4 files changed, 59 insertions(+), 5 deletions(-)

diff --git a/migration/file.c b/migration/file.c
index 22d052a71f..83328a7a1b 100644
--- a/migration/file.c
+++ b/migration/file.c
@@ -12,12 +12,17 @@
 #include "channel.h"
 #include "file.h"
 #include "migration.h"
+#include "multifd.h"
 #include "io/channel-file.h"
 #include "io/channel-util.h"
 #include "trace.h"
 
 #define OFFSET_OPTION ",offset="
 
+static struct FileOutgoingArgs {
+    char *fname;
+} outgoing_args;
+
 /* Remove the offset option from @filespec and return it in @offsetp. */
 
 int file_parse_offset(char *filespec, uint64_t *offsetp, Error **errp)
@@ -37,6 +42,36 @@ int file_parse_offset(char *filespec, uint64_t *offsetp, Error **errp)
     return 0;
 }
 
+void file_cleanup_outgoing_migration(void)
+{
+    g_free(outgoing_args.fname);
+    outgoing_args.fname = NULL;
+}
+
+bool file_send_channel_create(gpointer opaque, Error **errp)
+{
+    QIOChannelFile *ioc;
+    int flags = O_WRONLY;
+    bool ret = true;
+
+    ioc = qio_channel_file_new_path(outgoing_args.fname, flags, 0, errp);
+    if (!ioc) {
+        ret = false;
+        goto out;
+    }
+
+    multifd_channel_connect(opaque, QIO_CHANNEL(ioc));
+
+out:
+    /*
+     * File channel creation is synchronous. However posting this
+     * semaphore here is simpler than adding a special case.
+     */
+    multifd_send_channel_created();
+
+    return ret;
+}
+
 void file_start_outgoing_migration(MigrationState *s,
                                    FileMigrationArgs *file_args, Error **errp)
 {
@@ -47,12 +82,14 @@ void file_start_outgoing_migration(MigrationState *s,
 
     trace_migration_file_outgoing(filename);
 
-    fioc = qio_channel_file_new_path(filename, O_CREAT | O_WRONLY | O_TRUNC,
-                                     0600, errp);
+    fioc = qio_channel_file_new_path(filename, O_CREAT | O_TRUNC | O_WRONLY,
+                                     0660, errp);
     if (!fioc) {
         return;
     }
 
+    outgoing_args.fname = g_strdup(filename);
+
     ioc = QIO_CHANNEL(fioc);
     if (offset && qio_channel_io_seek(ioc, offset, SEEK_SET, errp) < 0) {
         return;
diff --git a/migration/file.h b/migration/file.h
index 37d6a08bfc..4577f9efdd 100644
--- a/migration/file.h
+++ b/migration/file.h
@@ -9,10 +9,14 @@
 #define QEMU_MIGRATION_FILE_H
 
 #include "qapi/qapi-types-migration.h"
+#include "io/task.h"
+#include "channel.h"
 
 void file_start_incoming_migration(FileMigrationArgs *file_args, Error **errp);
 
 void file_start_outgoing_migration(MigrationState *s,
                                    FileMigrationArgs *file_args, Error **errp);
 int file_parse_offset(char *filespec, uint64_t *offsetp, Error **errp);
+void file_cleanup_outgoing_migration(void);
+bool file_send_channel_create(gpointer opaque, Error **errp);
 #endif
diff --git a/migration/multifd.c b/migration/multifd.c
index 3574fd3953..f155223303 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -17,6 +17,7 @@
 #include "exec/ramblock.h"
 #include "qemu/error-report.h"
 #include "qapi/error.h"
+#include "file.h"
 #include "ram.h"
 #include "migration.h"
 #include "migration-stats.h"
@@ -28,6 +29,7 @@
 #include "threadinfo.h"
 #include "options.h"
 #include "qemu/yank.h"
+#include "io/channel-file.h"
 #include "io/channel-socket.h"
 #include "yank_functions.h"
 
@@ -694,6 +696,7 @@ static bool multifd_send_cleanup_channel(MultiFDSendParams *p, Error **errp)
 {
     if (p->c) {
         migration_ioc_unregister_yank(p->c);
+        qio_channel_close(p->c, NULL);
         object_unref(OBJECT(p->c));
         p->c = NULL;
     }
@@ -715,6 +718,7 @@ static bool multifd_send_cleanup_channel(MultiFDSendParams *p, Error **errp)
 
 static void multifd_send_cleanup_state(void)
 {
+    file_cleanup_outgoing_migration();
     socket_cleanup_outgoing_migration();
     qemu_sem_destroy(&multifd_send_state->channels_created);
     qemu_sem_destroy(&multifd_send_state->channels_ready);
@@ -977,7 +981,7 @@ static bool multifd_tls_channel_connect(MultiFDSendParams *p,
     return true;
 }
 
-static void multifd_channel_connect(MultiFDSendParams *p, QIOChannel *ioc)
+void multifd_channel_connect(MultiFDSendParams *p, QIOChannel *ioc)
 {
     qio_channel_set_delay(ioc, false);
 
@@ -1045,9 +1049,14 @@ out:
     error_free(local_err);
 }
 
-static void multifd_new_send_channel_create(gpointer opaque)
+static bool multifd_new_send_channel_create(gpointer opaque, Error **errp)
 {
+    if (!multifd_use_packets()) {
+        return file_send_channel_create(opaque, errp);
+    }
+
     socket_send_channel_create(multifd_new_send_channel_async, opaque);
+    return true;
 }
 
 bool multifd_send_setup(void)
@@ -1096,7 +1105,10 @@ bool multifd_send_setup(void)
         p->page_size = qemu_target_page_size();
         p->page_count = page_count;
         p->write_flags = 0;
-        multifd_new_send_channel_create(p);
+
+        if (!multifd_new_send_channel_create(p, &local_err)) {
+            return -1;
+        }
     }
 
     /*
diff --git a/migration/multifd.h b/migration/multifd.h
index 1d8bbaf96b..db8887f088 100644
--- a/migration/multifd.h
+++ b/migration/multifd.h
@@ -227,5 +227,6 @@ static inline void multifd_send_prepare_header(MultiFDSendParams *p)
     p->iovs_num++;
 }
 
+void multifd_channel_connect(MultiFDSendParams *p, QIOChannel *ioc);
 
 #endif
-- 
2.35.3
Re: [PATCH v5 17/23] migration/multifd: Add outgoing QIOChannelFile support
Posted by Peter Xu 9 months ago 
On Wed, Feb 28, 2024 at 12:21:21PM -0300, Fabiano Rosas wrote:
> Allow multifd to open file-backed channels. This will be used when
> enabling the mapped-ram migration stream format which expects a
> seekable transport.
> 
> The QIOChannel read and write methods will use the preadv/pwritev
> versions which don't update the file offset at each call so we can
> reuse the fd without re-opening for every channel.
> 
> Contrary to the socket migration, the file migration doesn't need an
> asynchronous channel creation process, so expose
> multifd_channel_connect() and call it directly.
> 
> Note that this is just setup code and multifd cannot yet make use of
> the file channels.
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
> - moved flags change to another patch
> - removed channels_created assert
> ---
>  migration/file.c    | 41 +++++++++++++++++++++++++++++++++++++++--
>  migration/file.h    |  4 ++++
>  migration/multifd.c | 18 +++++++++++++++---
>  migration/multifd.h |  1 +
>  4 files changed, 59 insertions(+), 5 deletions(-)
> 
> diff --git a/migration/file.c b/migration/file.c
> index 22d052a71f..83328a7a1b 100644
> --- a/migration/file.c
> +++ b/migration/file.c
> @@ -12,12 +12,17 @@
>  #include "channel.h"
>  #include "file.h"
>  #include "migration.h"
> +#include "multifd.h"
>  #include "io/channel-file.h"
>  #include "io/channel-util.h"
>  #include "trace.h"
>  
>  #define OFFSET_OPTION ",offset="
>  
> +static struct FileOutgoingArgs {
> +    char *fname;
> +} outgoing_args;
> +
>  /* Remove the offset option from @filespec and return it in @offsetp. */
>  
>  int file_parse_offset(char *filespec, uint64_t *offsetp, Error **errp)
> @@ -37,6 +42,36 @@ int file_parse_offset(char *filespec, uint64_t *offsetp, Error **errp)
>      return 0;
>  }
>  
> +void file_cleanup_outgoing_migration(void)
> +{
> +    g_free(outgoing_args.fname);
> +    outgoing_args.fname = NULL;
> +}
> +
> +bool file_send_channel_create(gpointer opaque, Error **errp)
> +{
> +    QIOChannelFile *ioc;
> +    int flags = O_WRONLY;
> +    bool ret = true;
> +
> +    ioc = qio_channel_file_new_path(outgoing_args.fname, flags, 0, errp);
> +    if (!ioc) {
> +        ret = false;
> +        goto out;
> +    }
> +
> +    multifd_channel_connect(opaque, QIO_CHANNEL(ioc));
> +
> +out:
> +    /*
> +     * File channel creation is synchronous. However posting this
> +     * semaphore here is simpler than adding a special case.
> +     */
> +    multifd_send_channel_created();
> +
> +    return ret;
> +}
> +
>  void file_start_outgoing_migration(MigrationState *s,
>                                     FileMigrationArgs *file_args, Error **errp)
>  {
> @@ -47,12 +82,14 @@ void file_start_outgoing_migration(MigrationState *s,
>  
>      trace_migration_file_outgoing(filename);
>  
> -    fioc = qio_channel_file_new_path(filename, O_CREAT | O_WRONLY | O_TRUNC,
> -                                     0600, errp);
> +    fioc = qio_channel_file_new_path(filename, O_CREAT | O_TRUNC | O_WRONLY,
> +                                     0660, errp);

It seems this is still leftover?

>      if (!fioc) {
>          return;
>      }
>  
> +    outgoing_args.fname = g_strdup(filename);
> +
>      ioc = QIO_CHANNEL(fioc);
>      if (offset && qio_channel_io_seek(ioc, offset, SEEK_SET, errp) < 0) {
>          return;
> diff --git a/migration/file.h b/migration/file.h
> index 37d6a08bfc..4577f9efdd 100644
> --- a/migration/file.h
> +++ b/migration/file.h
> @@ -9,10 +9,14 @@
>  #define QEMU_MIGRATION_FILE_H
>  
>  #include "qapi/qapi-types-migration.h"
> +#include "io/task.h"
> +#include "channel.h"
>  
>  void file_start_incoming_migration(FileMigrationArgs *file_args, Error **errp);
>  
>  void file_start_outgoing_migration(MigrationState *s,
>                                     FileMigrationArgs *file_args, Error **errp);
>  int file_parse_offset(char *filespec, uint64_t *offsetp, Error **errp);
> +void file_cleanup_outgoing_migration(void);
> +bool file_send_channel_create(gpointer opaque, Error **errp);
>  #endif
> diff --git a/migration/multifd.c b/migration/multifd.c
> index 3574fd3953..f155223303 100644
> --- a/migration/multifd.c
> +++ b/migration/multifd.c
> @@ -17,6 +17,7 @@
>  #include "exec/ramblock.h"
>  #include "qemu/error-report.h"
>  #include "qapi/error.h"
> +#include "file.h"
>  #include "ram.h"
>  #include "migration.h"
>  #include "migration-stats.h"
> @@ -28,6 +29,7 @@
>  #include "threadinfo.h"
>  #include "options.h"
>  #include "qemu/yank.h"
> +#include "io/channel-file.h"
>  #include "io/channel-socket.h"
>  #include "yank_functions.h"
>  
> @@ -694,6 +696,7 @@ static bool multifd_send_cleanup_channel(MultiFDSendParams *p, Error **errp)
>  {
>      if (p->c) {
>          migration_ioc_unregister_yank(p->c);
> +        qio_channel_close(p->c, NULL);

s/NULL/&error_abort/?

>          object_unref(OBJECT(p->c));
>          p->c = NULL;
>      }
> @@ -715,6 +718,7 @@ static bool multifd_send_cleanup_channel(MultiFDSendParams *p, Error **errp)
>  
>  static void multifd_send_cleanup_state(void)
>  {
> +    file_cleanup_outgoing_migration();
>      socket_cleanup_outgoing_migration();
>      qemu_sem_destroy(&multifd_send_state->channels_created);
>      qemu_sem_destroy(&multifd_send_state->channels_ready);
> @@ -977,7 +981,7 @@ static bool multifd_tls_channel_connect(MultiFDSendParams *p,
>      return true;
>  }
>  
> -static void multifd_channel_connect(MultiFDSendParams *p, QIOChannel *ioc)
> +void multifd_channel_connect(MultiFDSendParams *p, QIOChannel *ioc)
>  {
>      qio_channel_set_delay(ioc, false);
>  
> @@ -1045,9 +1049,14 @@ out:
>      error_free(local_err);
>  }
>  
> -static void multifd_new_send_channel_create(gpointer opaque)
> +static bool multifd_new_send_channel_create(gpointer opaque, Error **errp)
>  {
> +    if (!multifd_use_packets()) {
> +        return file_send_channel_create(opaque, errp);
> +    }
> +
>      socket_send_channel_create(multifd_new_send_channel_async, opaque);
> +    return true;
>  }
>  
>  bool multifd_send_setup(void)
> @@ -1096,7 +1105,10 @@ bool multifd_send_setup(void)
>          p->page_size = qemu_target_page_size();
>          p->page_count = page_count;
>          p->write_flags = 0;
> -        multifd_new_send_channel_create(p);
> +
> +        if (!multifd_new_send_channel_create(p, &local_err)) {
> +            return -1;

"-1" is unfortunately a "true"!..

> +        }
>      }
>  
>      /*
> diff --git a/migration/multifd.h b/migration/multifd.h
> index 1d8bbaf96b..db8887f088 100644
> --- a/migration/multifd.h
> +++ b/migration/multifd.h
> @@ -227,5 +227,6 @@ static inline void multifd_send_prepare_header(MultiFDSendParams *p)
>      p->iovs_num++;
>  }
>  
> +void multifd_channel_connect(MultiFDSendParams *p, QIOChannel *ioc);
>  
>  #endif
> -- 
> 2.35.3
> 

-- 
Peter Xu
Re: [PATCH v5 17/23] migration/multifd: Add outgoing QIOChannelFile support
Posted by Peter Xu 9 months ago 
On Thu, Feb 29, 2024 at 10:44:21AM +0800, Peter Xu wrote:
> On Wed, Feb 28, 2024 at 12:21:21PM -0300, Fabiano Rosas wrote:
> > Allow multifd to open file-backed channels. This will be used when
> > enabling the mapped-ram migration stream format which expects a
> > seekable transport.
> > 
> > The QIOChannel read and write methods will use the preadv/pwritev
> > versions which don't update the file offset at each call so we can
> > reuse the fd without re-opening for every channel.
> > 
> > Contrary to the socket migration, the file migration doesn't need an
> > asynchronous channel creation process, so expose
> > multifd_channel_connect() and call it directly.
> > 
> > Note that this is just setup code and multifd cannot yet make use of
> > the file channels.
> > 
> > Signed-off-by: Fabiano Rosas <farosas@suse.de>
> > ---
> > - moved flags change to another patch
> > - removed channels_created assert
> > ---
> >  migration/file.c    | 41 +++++++++++++++++++++++++++++++++++++++--
> >  migration/file.h    |  4 ++++
> >  migration/multifd.c | 18 +++++++++++++++---
> >  migration/multifd.h |  1 +
> >  4 files changed, 59 insertions(+), 5 deletions(-)
> > 
> > diff --git a/migration/file.c b/migration/file.c
> > index 22d052a71f..83328a7a1b 100644
> > --- a/migration/file.c
> > +++ b/migration/file.c
> > @@ -12,12 +12,17 @@
> >  #include "channel.h"
> >  #include "file.h"
> >  #include "migration.h"
> > +#include "multifd.h"
> >  #include "io/channel-file.h"
> >  #include "io/channel-util.h"
> >  #include "trace.h"
> >  
> >  #define OFFSET_OPTION ",offset="
> >  
> > +static struct FileOutgoingArgs {
> > +    char *fname;
> > +} outgoing_args;
> > +
> >  /* Remove the offset option from @filespec and return it in @offsetp. */
> >  
> >  int file_parse_offset(char *filespec, uint64_t *offsetp, Error **errp)
> > @@ -37,6 +42,36 @@ int file_parse_offset(char *filespec, uint64_t *offsetp, Error **errp)
> >      return 0;
> >  }
> >  
> > +void file_cleanup_outgoing_migration(void)
> > +{
> > +    g_free(outgoing_args.fname);
> > +    outgoing_args.fname = NULL;
> > +}
> > +
> > +bool file_send_channel_create(gpointer opaque, Error **errp)
> > +{
> > +    QIOChannelFile *ioc;
> > +    int flags = O_WRONLY;
> > +    bool ret = true;
> > +
> > +    ioc = qio_channel_file_new_path(outgoing_args.fname, flags, 0, errp);
> > +    if (!ioc) {
> > +        ret = false;
> > +        goto out;
> > +    }
> > +
> > +    multifd_channel_connect(opaque, QIO_CHANNEL(ioc));
> > +
> > +out:
> > +    /*
> > +     * File channel creation is synchronous. However posting this
> > +     * semaphore here is simpler than adding a special case.
> > +     */
> > +    multifd_send_channel_created();
> > +
> > +    return ret;
> > +}
> > +
> >  void file_start_outgoing_migration(MigrationState *s,
> >                                     FileMigrationArgs *file_args, Error **errp)
> >  {
> > @@ -47,12 +82,14 @@ void file_start_outgoing_migration(MigrationState *s,
> >  
> >      trace_migration_file_outgoing(filename);
> >  
> > -    fioc = qio_channel_file_new_path(filename, O_CREAT | O_WRONLY | O_TRUNC,
> > -                                     0600, errp);
> > +    fioc = qio_channel_file_new_path(filename, O_CREAT | O_TRUNC | O_WRONLY,
> > +                                     0660, errp);
> 
> It seems this is still leftover?
> 
> >      if (!fioc) {
> >          return;
> >      }
> >  
> > +    outgoing_args.fname = g_strdup(filename);
> > +
> >      ioc = QIO_CHANNEL(fioc);
> >      if (offset && qio_channel_io_seek(ioc, offset, SEEK_SET, errp) < 0) {
> >          return;
> > diff --git a/migration/file.h b/migration/file.h
> > index 37d6a08bfc..4577f9efdd 100644
> > --- a/migration/file.h
> > +++ b/migration/file.h
> > @@ -9,10 +9,14 @@
> >  #define QEMU_MIGRATION_FILE_H
> >  
> >  #include "qapi/qapi-types-migration.h"
> > +#include "io/task.h"
> > +#include "channel.h"
> >  
> >  void file_start_incoming_migration(FileMigrationArgs *file_args, Error **errp);
> >  
> >  void file_start_outgoing_migration(MigrationState *s,
> >                                     FileMigrationArgs *file_args, Error **errp);
> >  int file_parse_offset(char *filespec, uint64_t *offsetp, Error **errp);
> > +void file_cleanup_outgoing_migration(void);
> > +bool file_send_channel_create(gpointer opaque, Error **errp);
> >  #endif
> > diff --git a/migration/multifd.c b/migration/multifd.c
> > index 3574fd3953..f155223303 100644
> > --- a/migration/multifd.c
> > +++ b/migration/multifd.c
> > @@ -17,6 +17,7 @@
> >  #include "exec/ramblock.h"
> >  #include "qemu/error-report.h"
> >  #include "qapi/error.h"
> > +#include "file.h"
> >  #include "ram.h"
> >  #include "migration.h"
> >  #include "migration-stats.h"
> > @@ -28,6 +29,7 @@
> >  #include "threadinfo.h"
> >  #include "options.h"
> >  #include "qemu/yank.h"
> > +#include "io/channel-file.h"
> >  #include "io/channel-socket.h"
> >  #include "yank_functions.h"
> >  
> > @@ -694,6 +696,7 @@ static bool multifd_send_cleanup_channel(MultiFDSendParams *p, Error **errp)
> >  {
> >      if (p->c) {
> >          migration_ioc_unregister_yank(p->c);
> > +        qio_channel_close(p->c, NULL);
> 
> s/NULL/&error_abort/?

Or we can drop this line?  IIUC iochannel finalize() will always close it,
or it could be a separate bug.

> 
> >          object_unref(OBJECT(p->c));
> >          p->c = NULL;
> >      }
> > @@ -715,6 +718,7 @@ static bool multifd_send_cleanup_channel(MultiFDSendParams *p, Error **errp)
> >  
> >  static void multifd_send_cleanup_state(void)
> >  {
> > +    file_cleanup_outgoing_migration();
> >      socket_cleanup_outgoing_migration();
> >      qemu_sem_destroy(&multifd_send_state->channels_created);
> >      qemu_sem_destroy(&multifd_send_state->channels_ready);
> > @@ -977,7 +981,7 @@ static bool multifd_tls_channel_connect(MultiFDSendParams *p,
> >      return true;
> >  }
> >  
> > -static void multifd_channel_connect(MultiFDSendParams *p, QIOChannel *ioc)
> > +void multifd_channel_connect(MultiFDSendParams *p, QIOChannel *ioc)
> >  {
> >      qio_channel_set_delay(ioc, false);
> >  
> > @@ -1045,9 +1049,14 @@ out:
> >      error_free(local_err);
> >  }
> >  
> > -static void multifd_new_send_channel_create(gpointer opaque)
> > +static bool multifd_new_send_channel_create(gpointer opaque, Error **errp)
> >  {
> > +    if (!multifd_use_packets()) {
> > +        return file_send_channel_create(opaque, errp);
> > +    }
> > +
> >      socket_send_channel_create(multifd_new_send_channel_async, opaque);
> > +    return true;
> >  }
> >  
> >  bool multifd_send_setup(void)
> > @@ -1096,7 +1105,10 @@ bool multifd_send_setup(void)
> >          p->page_size = qemu_target_page_size();
> >          p->page_count = page_count;
> >          p->write_flags = 0;
> > -        multifd_new_send_channel_create(p);
> > +
> > +        if (!multifd_new_send_channel_create(p, &local_err)) {
> > +            return -1;
> 
> "-1" is unfortunately a "true"!..
> 
> > +        }
> >      }
> >  
> >      /*
> > diff --git a/migration/multifd.h b/migration/multifd.h
> > index 1d8bbaf96b..db8887f088 100644
> > --- a/migration/multifd.h
> > +++ b/migration/multifd.h
> > @@ -227,5 +227,6 @@ static inline void multifd_send_prepare_header(MultiFDSendParams *p)
> >      p->iovs_num++;
> >  }
> >  
> > +void multifd_channel_connect(MultiFDSendParams *p, QIOChannel *ioc);
> >  
> >  #endif
> > -- 
> > 2.35.3
> > 
> 
> -- 
> Peter Xu

-- 
Peter Xu
Re: [PATCH v5 17/23] migration/multifd: Add outgoing QIOChannelFile support
Posted by Fabiano Rosas 9 months ago 
Peter Xu <peterx@redhat.com> writes:

> On Thu, Feb 29, 2024 at 10:44:21AM +0800, Peter Xu wrote:
>> On Wed, Feb 28, 2024 at 12:21:21PM -0300, Fabiano Rosas wrote:
>> > Allow multifd to open file-backed channels. This will be used when
>> > enabling the mapped-ram migration stream format which expects a
>> > seekable transport.
>> > 
>> > The QIOChannel read and write methods will use the preadv/pwritev
>> > versions which don't update the file offset at each call so we can
>> > reuse the fd without re-opening for every channel.
>> > 
>> > Contrary to the socket migration, the file migration doesn't need an
>> > asynchronous channel creation process, so expose
>> > multifd_channel_connect() and call it directly.
>> > 
>> > Note that this is just setup code and multifd cannot yet make use of
>> > the file channels.
>> > 
>> > Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> > ---
>> > - moved flags change to another patch
>> > - removed channels_created assert
>> > ---
>> >  migration/file.c    | 41 +++++++++++++++++++++++++++++++++++++++--
>> >  migration/file.h    |  4 ++++
>> >  migration/multifd.c | 18 +++++++++++++++---
>> >  migration/multifd.h |  1 +
>> >  4 files changed, 59 insertions(+), 5 deletions(-)
>> > 
>> > diff --git a/migration/file.c b/migration/file.c
>> > index 22d052a71f..83328a7a1b 100644
>> > --- a/migration/file.c
>> > +++ b/migration/file.c
>> > @@ -12,12 +12,17 @@
>> >  #include "channel.h"
>> >  #include "file.h"
>> >  #include "migration.h"
>> > +#include "multifd.h"
>> >  #include "io/channel-file.h"
>> >  #include "io/channel-util.h"
>> >  #include "trace.h"
>> >  
>> >  #define OFFSET_OPTION ",offset="
>> >  
>> > +static struct FileOutgoingArgs {
>> > +    char *fname;
>> > +} outgoing_args;
>> > +
>> >  /* Remove the offset option from @filespec and return it in @offsetp. */
>> >  
>> >  int file_parse_offset(char *filespec, uint64_t *offsetp, Error **errp)
>> > @@ -37,6 +42,36 @@ int file_parse_offset(char *filespec, uint64_t *offsetp, Error **errp)
>> >      return 0;
>> >  }
>> >  
>> > +void file_cleanup_outgoing_migration(void)
>> > +{
>> > +    g_free(outgoing_args.fname);
>> > +    outgoing_args.fname = NULL;
>> > +}
>> > +
>> > +bool file_send_channel_create(gpointer opaque, Error **errp)
>> > +{
>> > +    QIOChannelFile *ioc;
>> > +    int flags = O_WRONLY;
>> > +    bool ret = true;
>> > +
>> > +    ioc = qio_channel_file_new_path(outgoing_args.fname, flags, 0, errp);
>> > +    if (!ioc) {
>> > +        ret = false;
>> > +        goto out;
>> > +    }
>> > +
>> > +    multifd_channel_connect(opaque, QIO_CHANNEL(ioc));
>> > +
>> > +out:
>> > +    /*
>> > +     * File channel creation is synchronous. However posting this
>> > +     * semaphore here is simpler than adding a special case.
>> > +     */
>> > +    multifd_send_channel_created();
>> > +
>> > +    return ret;
>> > +}
>> > +
>> >  void file_start_outgoing_migration(MigrationState *s,
>> >                                     FileMigrationArgs *file_args, Error **errp)
>> >  {
>> > @@ -47,12 +82,14 @@ void file_start_outgoing_migration(MigrationState *s,
>> >  
>> >      trace_migration_file_outgoing(filename);
>> >  
>> > -    fioc = qio_channel_file_new_path(filename, O_CREAT | O_WRONLY | O_TRUNC,
>> > -                                     0600, errp);
>> > +    fioc = qio_channel_file_new_path(filename, O_CREAT | O_TRUNC | O_WRONLY,
>> > +                                     0660, errp);
>> 
>> It seems this is still leftover?
>> 
>> >      if (!fioc) {
>> >          return;
>> >      }
>> >  
>> > +    outgoing_args.fname = g_strdup(filename);
>> > +
>> >      ioc = QIO_CHANNEL(fioc);
>> >      if (offset && qio_channel_io_seek(ioc, offset, SEEK_SET, errp) < 0) {
>> >          return;
>> > diff --git a/migration/file.h b/migration/file.h
>> > index 37d6a08bfc..4577f9efdd 100644
>> > --- a/migration/file.h
>> > +++ b/migration/file.h
>> > @@ -9,10 +9,14 @@
>> >  #define QEMU_MIGRATION_FILE_H
>> >  
>> >  #include "qapi/qapi-types-migration.h"
>> > +#include "io/task.h"
>> > +#include "channel.h"
>> >  
>> >  void file_start_incoming_migration(FileMigrationArgs *file_args, Error **errp);
>> >  
>> >  void file_start_outgoing_migration(MigrationState *s,
>> >                                     FileMigrationArgs *file_args, Error **errp);
>> >  int file_parse_offset(char *filespec, uint64_t *offsetp, Error **errp);
>> > +void file_cleanup_outgoing_migration(void);
>> > +bool file_send_channel_create(gpointer opaque, Error **errp);
>> >  #endif
>> > diff --git a/migration/multifd.c b/migration/multifd.c
>> > index 3574fd3953..f155223303 100644
>> > --- a/migration/multifd.c
>> > +++ b/migration/multifd.c
>> > @@ -17,6 +17,7 @@
>> >  #include "exec/ramblock.h"
>> >  #include "qemu/error-report.h"
>> >  #include "qapi/error.h"
>> > +#include "file.h"
>> >  #include "ram.h"
>> >  #include "migration.h"
>> >  #include "migration-stats.h"
>> > @@ -28,6 +29,7 @@
>> >  #include "threadinfo.h"
>> >  #include "options.h"
>> >  #include "qemu/yank.h"
>> > +#include "io/channel-file.h"
>> >  #include "io/channel-socket.h"
>> >  #include "yank_functions.h"
>> >  
>> > @@ -694,6 +696,7 @@ static bool multifd_send_cleanup_channel(MultiFDSendParams *p, Error **errp)
>> >  {
>> >      if (p->c) {
>> >          migration_ioc_unregister_yank(p->c);
>> > +        qio_channel_close(p->c, NULL);
>> 
>> s/NULL/&error_abort/?
>
> Or we can drop this line?  IIUC iochannel finalize() will always close it,
> or it could be a separate bug.
>

We need it so the fsync happens. The finalize() will be a noop because
the qio_channel_file_close() will clear the fd. Not the cleanest, but it
works.

>> 
>> >          object_unref(OBJECT(p->c));
>> >          p->c = NULL;
>> >      }
>> > @@ -715,6 +718,7 @@ static bool multifd_send_cleanup_channel(MultiFDSendParams *p, Error **errp)
>> >  
>> >  static void multifd_send_cleanup_state(void)
>> >  {
>> > +    file_cleanup_outgoing_migration();
>> >      socket_cleanup_outgoing_migration();
>> >      qemu_sem_destroy(&multifd_send_state->channels_created);
>> >      qemu_sem_destroy(&multifd_send_state->channels_ready);
>> > @@ -977,7 +981,7 @@ static bool multifd_tls_channel_connect(MultiFDSendParams *p,
>> >      return true;
>> >  }
>> >  
>> > -static void multifd_channel_connect(MultiFDSendParams *p, QIOChannel *ioc)
>> > +void multifd_channel_connect(MultiFDSendParams *p, QIOChannel *ioc)
>> >  {
>> >      qio_channel_set_delay(ioc, false);
>> >  
>> > @@ -1045,9 +1049,14 @@ out:
>> >      error_free(local_err);
>> >  }
>> >  
>> > -static void multifd_new_send_channel_create(gpointer opaque)
>> > +static bool multifd_new_send_channel_create(gpointer opaque, Error **errp)
>> >  {
>> > +    if (!multifd_use_packets()) {
>> > +        return file_send_channel_create(opaque, errp);
>> > +    }
>> > +
>> >      socket_send_channel_create(multifd_new_send_channel_async, opaque);
>> > +    return true;
>> >  }
>> >  
>> >  bool multifd_send_setup(void)
>> > @@ -1096,7 +1105,10 @@ bool multifd_send_setup(void)
>> >          p->page_size = qemu_target_page_size();
>> >          p->page_count = page_count;
>> >          p->write_flags = 0;
>> > -        multifd_new_send_channel_create(p);
>> > +
>> > +        if (!multifd_new_send_channel_create(p, &local_err)) {
>> > +            return -1;
>> 
>> "-1" is unfortunately a "true"!..
>> 
>> > +        }
>> >      }
>> >  
>> >      /*
>> > diff --git a/migration/multifd.h b/migration/multifd.h
>> > index 1d8bbaf96b..db8887f088 100644
>> > --- a/migration/multifd.h
>> > +++ b/migration/multifd.h
>> > @@ -227,5 +227,6 @@ static inline void multifd_send_prepare_header(MultiFDSendParams *p)
>> >      p->iovs_num++;
>> >  }
>> >  
>> > +void multifd_channel_connect(MultiFDSendParams *p, QIOChannel *ioc);
>> >  
>> >  #endif
>> > -- 
>> > 2.35.3
>> > 
>> 
>> -- 
>> Peter Xu
Re: [PATCH v5 17/23] migration/multifd: Add outgoing QIOChannelFile support
Posted by Daniel P. Berrangé 9 months ago 
On Thu, Feb 29, 2024 at 11:27:44AM -0300, Fabiano Rosas wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > On Thu, Feb 29, 2024 at 10:44:21AM +0800, Peter Xu wrote:
> >> On Wed, Feb 28, 2024 at 12:21:21PM -0300, Fabiano Rosas wrote:
> >> > Allow multifd to open file-backed channels. This will be used when
> >> > enabling the mapped-ram migration stream format which expects a
> >> > seekable transport.
> >> > 
> >> > The QIOChannel read and write methods will use the preadv/pwritev
> >> > versions which don't update the file offset at each call so we can
> >> > reuse the fd without re-opening for every channel.
> >> > 
> >> > Contrary to the socket migration, the file migration doesn't need an
> >> > asynchronous channel creation process, so expose
> >> > multifd_channel_connect() and call it directly.
> >> > 
> >> > Note that this is just setup code and multifd cannot yet make use of
> >> > the file channels.
> >> > 
> >> > Signed-off-by: Fabiano Rosas <farosas@suse.de>
> >> > ---
> >> > - moved flags change to another patch
> >> > - removed channels_created assert
> >> > ---
> >> >  migration/file.c    | 41 +++++++++++++++++++++++++++++++++++++++--
> >> >  migration/file.h    |  4 ++++
> >> >  migration/multifd.c | 18 +++++++++++++++---
> >> >  migration/multifd.h |  1 +
> >> >  4 files changed, 59 insertions(+), 5 deletions(-)
> >> > 
> >> > diff --git a/migration/file.c b/migration/file.c
> >> > index 22d052a71f..83328a7a1b 100644
> >> > --- a/migration/file.c
> >> > +++ b/migration/file.c
> >> > @@ -12,12 +12,17 @@
> >> >  #include "channel.h"
> >> >  #include "file.h"
> >> >  #include "migration.h"
> >> > +#include "multifd.h"
> >> >  #include "io/channel-file.h"
> >> >  #include "io/channel-util.h"
> >> >  #include "trace.h"
> >> >  
> >> >  #define OFFSET_OPTION ",offset="
> >> >  
> >> > +static struct FileOutgoingArgs {
> >> > +    char *fname;
> >> > +} outgoing_args;
> >> > +
> >> >  /* Remove the offset option from @filespec and return it in @offsetp. */
> >> >  
> >> >  int file_parse_offset(char *filespec, uint64_t *offsetp, Error **errp)
> >> > @@ -37,6 +42,36 @@ int file_parse_offset(char *filespec, uint64_t *offsetp, Error **errp)
> >> >      return 0;
> >> >  }
> >> >  
> >> > +void file_cleanup_outgoing_migration(void)
> >> > +{
> >> > +    g_free(outgoing_args.fname);
> >> > +    outgoing_args.fname = NULL;
> >> > +}
> >> > +
> >> > +bool file_send_channel_create(gpointer opaque, Error **errp)
> >> > +{
> >> > +    QIOChannelFile *ioc;
> >> > +    int flags = O_WRONLY;
> >> > +    bool ret = true;
> >> > +
> >> > +    ioc = qio_channel_file_new_path(outgoing_args.fname, flags, 0, errp);
> >> > +    if (!ioc) {
> >> > +        ret = false;
> >> > +        goto out;
> >> > +    }
> >> > +
> >> > +    multifd_channel_connect(opaque, QIO_CHANNEL(ioc));
> >> > +
> >> > +out:
> >> > +    /*
> >> > +     * File channel creation is synchronous. However posting this
> >> > +     * semaphore here is simpler than adding a special case.
> >> > +     */
> >> > +    multifd_send_channel_created();
> >> > +
> >> > +    return ret;
> >> > +}
> >> > +
> >> >  void file_start_outgoing_migration(MigrationState *s,
> >> >                                     FileMigrationArgs *file_args, Error **errp)
> >> >  {
> >> > @@ -47,12 +82,14 @@ void file_start_outgoing_migration(MigrationState *s,
> >> >  
> >> >      trace_migration_file_outgoing(filename);
> >> >  
> >> > -    fioc = qio_channel_file_new_path(filename, O_CREAT | O_WRONLY | O_TRUNC,
> >> > -                                     0600, errp);
> >> > +    fioc = qio_channel_file_new_path(filename, O_CREAT | O_TRUNC | O_WRONLY,
> >> > +                                     0660, errp);
> >> 
> >> It seems this is still leftover?
> >> 
> >> >      if (!fioc) {
> >> >          return;
> >> >      }
> >> >  
> >> > +    outgoing_args.fname = g_strdup(filename);
> >> > +
> >> >      ioc = QIO_CHANNEL(fioc);
> >> >      if (offset && qio_channel_io_seek(ioc, offset, SEEK_SET, errp) < 0) {
> >> >          return;
> >> > diff --git a/migration/file.h b/migration/file.h
> >> > index 37d6a08bfc..4577f9efdd 100644
> >> > --- a/migration/file.h
> >> > +++ b/migration/file.h
> >> > @@ -9,10 +9,14 @@
> >> >  #define QEMU_MIGRATION_FILE_H
> >> >  
> >> >  #include "qapi/qapi-types-migration.h"
> >> > +#include "io/task.h"
> >> > +#include "channel.h"
> >> >  
> >> >  void file_start_incoming_migration(FileMigrationArgs *file_args, Error **errp);
> >> >  
> >> >  void file_start_outgoing_migration(MigrationState *s,
> >> >                                     FileMigrationArgs *file_args, Error **errp);
> >> >  int file_parse_offset(char *filespec, uint64_t *offsetp, Error **errp);
> >> > +void file_cleanup_outgoing_migration(void);
> >> > +bool file_send_channel_create(gpointer opaque, Error **errp);
> >> >  #endif
> >> > diff --git a/migration/multifd.c b/migration/multifd.c
> >> > index 3574fd3953..f155223303 100644
> >> > --- a/migration/multifd.c
> >> > +++ b/migration/multifd.c
> >> > @@ -17,6 +17,7 @@
> >> >  #include "exec/ramblock.h"
> >> >  #include "qemu/error-report.h"
> >> >  #include "qapi/error.h"
> >> > +#include "file.h"
> >> >  #include "ram.h"
> >> >  #include "migration.h"
> >> >  #include "migration-stats.h"
> >> > @@ -28,6 +29,7 @@
> >> >  #include "threadinfo.h"
> >> >  #include "options.h"
> >> >  #include "qemu/yank.h"
> >> > +#include "io/channel-file.h"
> >> >  #include "io/channel-socket.h"
> >> >  #include "yank_functions.h"
> >> >  
> >> > @@ -694,6 +696,7 @@ static bool multifd_send_cleanup_channel(MultiFDSendParams *p, Error **errp)
> >> >  {
> >> >      if (p->c) {
> >> >          migration_ioc_unregister_yank(p->c);
> >> > +        qio_channel_close(p->c, NULL);
> >> 
> >> s/NULL/&error_abort/?
> >
> > Or we can drop this line?  IIUC iochannel finalize() will always close it,
> > or it could be a separate bug.
> >
> 
> We need it so the fsync happens. The finalize() will be a noop because
> the qio_channel_file_close() will clear the fd. Not the cleanest, but it
> works.

It is always wise to explicitly call 'close'.

If something still has a GSource watch registered against
the QIOChannel, that GSource will be holding a reference
on the QIOChannel and will thus prevent finalize() ever
running.

By calling close() you guarantee the channel is closed,
even if you've mistakenly leaked a GSource somewhere.
Finalize still won't run in that case, but at least the
FD is gone, and the HUP might cause the GSource callback
to trigger correct cleanup;

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
Re: [PATCH v5 17/23] migration/multifd: Add outgoing QIOChannelFile support
Posted by Peter Xu 9 months ago 
On Thu, Feb 29, 2024 at 02:34:13PM +0000, Daniel P. Berrangé wrote:
> On Thu, Feb 29, 2024 at 11:27:44AM -0300, Fabiano Rosas wrote:
> > Peter Xu <peterx@redhat.com> writes:
> > 
> > > On Thu, Feb 29, 2024 at 10:44:21AM +0800, Peter Xu wrote:
> > >> On Wed, Feb 28, 2024 at 12:21:21PM -0300, Fabiano Rosas wrote:
> > >> > Allow multifd to open file-backed channels. This will be used when
> > >> > enabling the mapped-ram migration stream format which expects a
> > >> > seekable transport.
> > >> > 
> > >> > The QIOChannel read and write methods will use the preadv/pwritev
> > >> > versions which don't update the file offset at each call so we can
> > >> > reuse the fd without re-opening for every channel.
> > >> > 
> > >> > Contrary to the socket migration, the file migration doesn't need an
> > >> > asynchronous channel creation process, so expose
> > >> > multifd_channel_connect() and call it directly.
> > >> > 
> > >> > Note that this is just setup code and multifd cannot yet make use of
> > >> > the file channels.
> > >> > 
> > >> > Signed-off-by: Fabiano Rosas <farosas@suse.de>
> > >> > ---
> > >> > - moved flags change to another patch
> > >> > - removed channels_created assert
> > >> > ---
> > >> >  migration/file.c    | 41 +++++++++++++++++++++++++++++++++++++++--
> > >> >  migration/file.h    |  4 ++++
> > >> >  migration/multifd.c | 18 +++++++++++++++---
> > >> >  migration/multifd.h |  1 +
> > >> >  4 files changed, 59 insertions(+), 5 deletions(-)
> > >> > 
> > >> > diff --git a/migration/file.c b/migration/file.c
> > >> > index 22d052a71f..83328a7a1b 100644
> > >> > --- a/migration/file.c
> > >> > +++ b/migration/file.c
> > >> > @@ -12,12 +12,17 @@
> > >> >  #include "channel.h"
> > >> >  #include "file.h"
> > >> >  #include "migration.h"
> > >> > +#include "multifd.h"
> > >> >  #include "io/channel-file.h"
> > >> >  #include "io/channel-util.h"
> > >> >  #include "trace.h"
> > >> >  
> > >> >  #define OFFSET_OPTION ",offset="
> > >> >  
> > >> > +static struct FileOutgoingArgs {
> > >> > +    char *fname;
> > >> > +} outgoing_args;
> > >> > +
> > >> >  /* Remove the offset option from @filespec and return it in @offsetp. */
> > >> >  
> > >> >  int file_parse_offset(char *filespec, uint64_t *offsetp, Error **errp)
> > >> > @@ -37,6 +42,36 @@ int file_parse_offset(char *filespec, uint64_t *offsetp, Error **errp)
> > >> >      return 0;
> > >> >  }
> > >> >  
> > >> > +void file_cleanup_outgoing_migration(void)
> > >> > +{
> > >> > +    g_free(outgoing_args.fname);
> > >> > +    outgoing_args.fname = NULL;
> > >> > +}
> > >> > +
> > >> > +bool file_send_channel_create(gpointer opaque, Error **errp)
> > >> > +{
> > >> > +    QIOChannelFile *ioc;
> > >> > +    int flags = O_WRONLY;
> > >> > +    bool ret = true;
> > >> > +
> > >> > +    ioc = qio_channel_file_new_path(outgoing_args.fname, flags, 0, errp);
> > >> > +    if (!ioc) {
> > >> > +        ret = false;
> > >> > +        goto out;
> > >> > +    }
> > >> > +
> > >> > +    multifd_channel_connect(opaque, QIO_CHANNEL(ioc));
> > >> > +
> > >> > +out:
> > >> > +    /*
> > >> > +     * File channel creation is synchronous. However posting this
> > >> > +     * semaphore here is simpler than adding a special case.
> > >> > +     */
> > >> > +    multifd_send_channel_created();
> > >> > +
> > >> > +    return ret;
> > >> > +}
> > >> > +
> > >> >  void file_start_outgoing_migration(MigrationState *s,
> > >> >                                     FileMigrationArgs *file_args, Error **errp)
> > >> >  {
> > >> > @@ -47,12 +82,14 @@ void file_start_outgoing_migration(MigrationState *s,
> > >> >  
> > >> >      trace_migration_file_outgoing(filename);
> > >> >  
> > >> > -    fioc = qio_channel_file_new_path(filename, O_CREAT | O_WRONLY | O_TRUNC,
> > >> > -                                     0600, errp);
> > >> > +    fioc = qio_channel_file_new_path(filename, O_CREAT | O_TRUNC | O_WRONLY,
> > >> > +                                     0660, errp);
> > >> 
> > >> It seems this is still leftover?
> > >> 
> > >> >      if (!fioc) {
> > >> >          return;
> > >> >      }
> > >> >  
> > >> > +    outgoing_args.fname = g_strdup(filename);
> > >> > +
> > >> >      ioc = QIO_CHANNEL(fioc);
> > >> >      if (offset && qio_channel_io_seek(ioc, offset, SEEK_SET, errp) < 0) {
> > >> >          return;
> > >> > diff --git a/migration/file.h b/migration/file.h
> > >> > index 37d6a08bfc..4577f9efdd 100644
> > >> > --- a/migration/file.h
> > >> > +++ b/migration/file.h
> > >> > @@ -9,10 +9,14 @@
> > >> >  #define QEMU_MIGRATION_FILE_H
> > >> >  
> > >> >  #include "qapi/qapi-types-migration.h"
> > >> > +#include "io/task.h"
> > >> > +#include "channel.h"
> > >> >  
> > >> >  void file_start_incoming_migration(FileMigrationArgs *file_args, Error **errp);
> > >> >  
> > >> >  void file_start_outgoing_migration(MigrationState *s,
> > >> >                                     FileMigrationArgs *file_args, Error **errp);
> > >> >  int file_parse_offset(char *filespec, uint64_t *offsetp, Error **errp);
> > >> > +void file_cleanup_outgoing_migration(void);
> > >> > +bool file_send_channel_create(gpointer opaque, Error **errp);
> > >> >  #endif
> > >> > diff --git a/migration/multifd.c b/migration/multifd.c
> > >> > index 3574fd3953..f155223303 100644
> > >> > --- a/migration/multifd.c
> > >> > +++ b/migration/multifd.c
> > >> > @@ -17,6 +17,7 @@
> > >> >  #include "exec/ramblock.h"
> > >> >  #include "qemu/error-report.h"
> > >> >  #include "qapi/error.h"
> > >> > +#include "file.h"
> > >> >  #include "ram.h"
> > >> >  #include "migration.h"
> > >> >  #include "migration-stats.h"
> > >> > @@ -28,6 +29,7 @@
> > >> >  #include "threadinfo.h"
> > >> >  #include "options.h"
> > >> >  #include "qemu/yank.h"
> > >> > +#include "io/channel-file.h"
> > >> >  #include "io/channel-socket.h"
> > >> >  #include "yank_functions.h"
> > >> >  
> > >> > @@ -694,6 +696,7 @@ static bool multifd_send_cleanup_channel(MultiFDSendParams *p, Error **errp)
> > >> >  {
> > >> >      if (p->c) {
> > >> >          migration_ioc_unregister_yank(p->c);
> > >> > +        qio_channel_close(p->c, NULL);
> > >> 
> > >> s/NULL/&error_abort/?
> > >
> > > Or we can drop this line?  IIUC iochannel finalize() will always close it,
> > > or it could be a separate bug.
> > >
> > 
> > We need it so the fsync happens. The finalize() will be a noop because
> > the qio_channel_file_close() will clear the fd. Not the cleanest, but it
> > works.
> 
> It is always wise to explicitly call 'close'.
> 
> If something still has a GSource watch registered against
> the QIOChannel, that GSource will be holding a reference
> on the QIOChannel and will thus prevent finalize() ever
> running.
> 
> By calling close() you guarantee the channel is closed,
> even if you've mistakenly leaked a GSource somewhere.
> Finalize still won't run in that case, but at least the
> FD is gone, and the HUP might cause the GSource callback
> to trigger correct cleanup;

I see.  Let's add a comment to explain why we do this explicit close(),
then?  It wasn't that clear, and we also don't do that on recv side.  It
seems also only useful for "file:", we can mention that in the comment if
so.

Thanks,

-- 
Peter Xu

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.