Re: [RFC PATCH v3 3/3] migration: Add fd to FileMigrationArgs

[Daniel P . Berrangé]

On Tue, Mar 19, 2024 at 03:25:18PM -0400, Peter Xu wrote:
> On Tue, Mar 19, 2024 at 04:25:32PM +0000, Daniel P. Berrangé wrote:
> > On Fri, Mar 15, 2024 at 04:54:27PM -0400, Peter Xu wrote:
> > > On Fri, Mar 15, 2024 at 03:01:09PM -0300, Fabiano Rosas wrote:
> > > > Peter Xu <peterx@redhat.com> writes:
> > > > 
> > > > > [I queued patch 1-2 into -stable, leaving this patch for further
> > > > >  discussions]
> > > > >
> > > > > On Fri, Mar 15, 2024 at 08:55:42AM +0000, Daniel P. Berrangé wrote:
> > > > >> The 'file:' protocol eventually calls into qemu_open, and this
> > > > >> transparently allows for FD passing using /dev/fdset/NNN syntax
> > > > >> to pass in FDs. 
> > > > >
> > > > > If it always use /dev/fdsets for files, does it mean that the newly 
> > > > > added
> > > > > SOCKET_ADDRESS_TYPE_FD support on mapped-ram will never be used (so 
> > > > > we can
> > > > > drop them)?
> > > > 
> > > > We already have SOCKET_ADDRESS_TYPE_FD + file since 8.2 when the
> > > > MigrationAddress was added. So this:
> > > > 
> > > > 'channels': [ { 'channel-type': 'main',
> > > >                 'addr': { 'transport': 'socket',
> > > >                 'type': 'fd',
> > > >                 'str': 'fdname' } } ]
> > > > 
> > > > works without multifd and without mapped-ram if the fd is a file or
> > > > socket.
> > > > 
> > > > So yes, you're correct, but given we already have this^ it would be
> > > > perhaps more confusing for users to allow it, but not allow the very
> > > > same JSON when multifd=true, mapped-ram=true.
> > > 
> > > I don't think the fd: protocol (no matter the old "fd:", or the new JSON
> > > format) is trivial to use. If libvirt didn't use it I won't be surprised 
> > > to
> > > see nobody using it.  I want us to avoid working on things that nobody is
> > > using, or has a better replacement.
> > > 
> > > So even if Libvirt supports both, I'm wondering whether /dev/fdset/ works
> > > for all the cases that libvirt needs.  I am aware that the old getfd has
> > > the monitor limitation so that if the QMP disconnected and reconnect, the
> > > fd can be gone.  However I'm not sure whether that's the only reason to
> > > have add-fd, and also not sure whether it means add-fd is always 
> > > preferred,
> > > so that maybe we can consider obsolete getfd?
> > 
> > Historically libvirt primariily uses the 'fd:' protocol, with a
> > socket FD. It never gives QEMU a plain file FD, since it has
> > always added its "iohelper" as a MITM, in order to add O_DIRECT
> > on top.
> > 
> > The 'getfd' command is something that is needed when talking to
> > QEMU for any API that involves a "SocketAddress" QAPI type,
> > which is applicable for migration.
> > 
> > With the introduction of 'MigrationAddress', the 'socket' protocol
> > is backed by 'SocketAddress' and thus supports FD passing for
> > sockets (or potentally pipes too), in combination with 'getfd'.
> > 
> > With the 'file' protocol in 'MigrationAddress', since it gets
> > backed by qemu_open(), then /dev/fdset/NN and 'add-fd' provide
> > passing for plain files.
> 
> I see.  I assume it means we still have multiple users of getfd so it's
> still in use where add-fd is not yet avaiable.
> 
> But then, SOCKET_ADDRESS_TYPE_FD is then not used for libvirt in the whole
> mapped-ram effort, neither do we need any support on file migrations over
> "fd", e.g. fd_start_incoming_migration() for files. So we can drop these
> parts, am I right?

Correct, libvirt hasn't got any impl for 'mapped-ram' yet, at least
not something merged.

Since this is new functionality, libvirt could go straight for the
'file' protocol / address type.

At some point I think we can stop using 'fd' for traditional migration
too and pass the socket address to QEMU and let QEMU open the socket.

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