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

[Daniel P . Berrangé]

On Wed, Oct 25, 2023 at 11:12:38AM -0300, Fabiano Rosas wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > 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);
> 
> By the way, we're experimenting with add-fd to flesh out the interface
> with libvirt and I see that the flags here can conflict with the flags
> set on the fd passed through `virsh --pass-fd ...` due to this at
> monitor_fdset_dup_fd_add():
> 
>     if ((flags & O_ACCMODE) == (mon_fd_flags & O_ACCMODE)) {
>         fd = mon_fdset_fd->fd;
>         break;
>     }
> 
> We're requiring the O_RDONLY, O_WRONLY, O_RDWR flags defined here to
> match the fdset passed into QEMU. Should we just sync the code of the
> two projects to use the same flags? That feels a little clumsy to me.

Is there a reason for libvirt to have set O_RDONLY for a file used
for outgoing migration ?  I can't recall off-hand.

If we document the required O_ACCMODE against the 'file' address
schema in QAPI, I don't think it'd be too painful for mgmt apps.


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