Re: [PATCH v2 19/29] migration/multifd: Add outgoing QIOChannelFile support

["Daniel P. Berrangé" <berrange@redhat.com>]

On Mon, Oct 23, 2023 at 05:35:58PM -0300, Fabiano Rosas wrote:
> Allow multifd to open file-backed channels. This will be used when
> enabling the fixed-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.
> 
> 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>
> ---
>  migration/file.c      | 64 +++++++++++++++++++++++++++++++++++++++++--
>  migration/file.h      | 10 +++++--
>  migration/migration.c |  2 +-
>  migration/multifd.c   | 14 ++++++++--
>  migration/options.c   |  7 +++++
>  migration/options.h   |  1 +
>  6 files changed, 90 insertions(+), 8 deletions(-)
> 
> diff --git a/migration/file.c b/migration/file.c
> index cf5b1bf365..93b9b7bf5d 100644
> --- a/migration/file.c
> +++ b/migration/file.c
> @@ -17,6 +17,12 @@

> +void file_send_channel_create(QIOTaskFunc f, void *data)
> +{
> +    QIOChannelFile *ioc;
> +    QIOTask *task;
> +    Error *errp = NULL;
> +
> +    ioc = qio_channel_file_new_path(outgoing_args.fname,
> +                                    outgoing_args.flags,
> +                                    outgoing_args.mode, &errp);
> +    if (!ioc) {
> +        file_migration_cancel(errp);
> +        return;
> +    }
> +
> +    task = qio_task_new(OBJECT(ioc), f, (gpointer)data, NULL);
> +    qio_task_run_in_thread(task, qio_channel_file_connect_worker,
> +                           (gpointer)data, NULL, NULL);
> +}
> +
>  void file_start_outgoing_migration(MigrationState *s, const char *filespec,
>                                     Error **errp)
>  {
> -    g_autofree char *filename = g_strdup(filespec);
>      g_autoptr(QIOChannelFile) fioc = NULL;
> +    g_autofree char *filename = g_strdup(filespec);
>      uint64_t offset = 0;
>      QIOChannel *ioc;
> +    int flags = O_CREAT | O_TRUNC | O_WRONLY;
> +    mode_t mode = 0660;
>  
>      trace_migration_file_outgoing(filename);
>  
> @@ -50,12 +105,15 @@ void file_start_outgoing_migration(MigrationState *s, const char *filespec,
>          return;
>      }
>  
> -    fioc = qio_channel_file_new_path(filename, O_CREAT | O_WRONLY | O_TRUNC,
> -                                     0600, errp);
> +    fioc = qio_channel_file_new_path(filename, flags, mode, errp);

So this initially opens the file with O_CREAT|O_TRUNC which
makes sense.

>      if (!fioc) {
>          return;
>      }
>  
> +    outgoing_args.fname = g_strdup(filename);
> +    outgoing_args.flags = flags;
> +    outgoing_args.mode = mode;

We're passing on O_CREAT|O_TRUNC to all the multifd threads too. This
doesn't make sense to me - the file should already exist and be truncated
by the time the threads open it. I would think they should only be using
O_WRONLY and no mode at all.


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 :|