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

[Peter Xu <peterx@redhat.com>]

On Tue, Mar 19, 2024 at 07:52:47PM +0000, Daniel P. Berrangé wrote:
> 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.

Thanks for confirming this, that sounds good.  I quickly discussed this
with Fabiano just now, I think there's a plan we start to mark fd migration
deprecated for the next release (9.1), then there is a chance we drop it in
migration for 9.3.  That'll copy libvirt list so we can re-check there.

Fabiano will then prepare patches to remove the "fd:" support on file
migrations; that will be for 9.0.

Thanks,

-- 
Peter Xu