Re: [PATCH 8/9] migration: Add support for fdset with multifd + file

[Peter Xu <peterx@redhat.com>]

On Wed, May 08, 2024 at 09:53:48AM +0100, Daniel P. Berrangé wrote:
> On Fri, Apr 26, 2024 at 11:20:41AM -0300, Fabiano Rosas wrote:
> > Allow multifd to use an fdset when migrating to a file. This is useful
> > for the scenario where the management layer wants to have control over
> > the migration file.
> > 
> > By receiving the file descriptors directly, QEMU can delegate some
> > high level operating system operations to the management layer (such
> > as mandatory access control). The management layer might also want to
> > add its own headers before the migration stream.
> > 
> > Enable the "file:/dev/fdset/#" syntax for the multifd migration with
> > mapped-ram. The requirements for the fdset mechanism are:
> > 
> > On the migration source side:
> > 
> > - the fdset must contain two fds that are not duplicates between
> >   themselves;
> > - if direct-io is to be used, exactly one of the fds must have the
> >   O_DIRECT flag set;
> > - the file must be opened with WRONLY both times.
> > 
> > On the migration destination side:
> > 
> > - the fdset must contain one fd;
> > - the file must be opened with RDONLY.
> > 
> > Signed-off-by: Fabiano Rosas <farosas@suse.de>
> > ---
> >  docs/devel/migration/main.rst       | 18 ++++++++++++++
> >  docs/devel/migration/mapped-ram.rst |  6 ++++-
> >  migration/file.c                    | 38 ++++++++++++++++++++++++++++-
> >  3 files changed, 60 insertions(+), 2 deletions(-)
> > 
> > diff --git a/docs/devel/migration/main.rst b/docs/devel/migration/main.rst
> > index 54385a23e5..50f6096470 100644
> > --- a/docs/devel/migration/main.rst
> > +++ b/docs/devel/migration/main.rst
> > @@ -47,6 +47,24 @@ over any transport.
> >    QEMU interference. Note that QEMU does not flush cached file
> >    data/metadata at the end of migration.
> >  
> > +  The file migration also supports using a file that has already been
> > +  opened. A set of file descriptors is passed to QEMU via an "fdset"
> > +  (see add-fd QMP command documentation). This method allows a
> > +  management application to have control over the migration file
> > +  opening operation. There are, however, strict requirements to this
> > +  interface:
> > +
> > +  On the migration source side:
> > +    - if the multifd capability is to be used, the fdset must contain
> > +      two file descriptors that are not duplicates between themselves;
> > +    - if the direct-io capability is to be used, exactly one of the
> > +      file descriptors must have the O_DIRECT flag set;
> > +    - the file must be opened with WRONLY.
> > +
> > +  On the migration destination side:
> > +    - the fdset must contain one file descriptor;
> > +    - the file must be opened with RDONLY.
> > +
> >  In addition, support is included for migration using RDMA, which
> >  transports the page data using ``RDMA``, where the hardware takes care of
> >  transporting the pages, and the load on the CPU is much lower.  While the
> > diff --git a/docs/devel/migration/mapped-ram.rst b/docs/devel/migration/mapped-ram.rst
> > index fa4cefd9fc..e6505511f0 100644
> > --- a/docs/devel/migration/mapped-ram.rst
> > +++ b/docs/devel/migration/mapped-ram.rst
> > @@ -16,7 +16,7 @@ location in the file, rather than constantly being added to a
> >  sequential stream. Having the pages at fixed offsets also allows the
> >  usage of O_DIRECT for save/restore of the migration stream as the
> >  pages are ensured to be written respecting O_DIRECT alignment
> > -restrictions (direct-io support not yet implemented).
> > +restrictions.
> >  
> >  Usage
> >  -----
> > @@ -35,6 +35,10 @@ Use a ``file:`` URL for migration:
> >  Mapped-ram migration is best done non-live, i.e. by stopping the VM on
> >  the source side before migrating.
> >  
> > +For best performance enable the ``direct-io`` capability as well:
> > +
> > +    ``migrate_set_capability direct-io on``
> > +
> >  Use-cases
> >  ---------
> >  
> > diff --git a/migration/file.c b/migration/file.c
> > index b9265b14dd..3bc8bc7463 100644
> > --- a/migration/file.c
> > +++ b/migration/file.c
> > @@ -17,6 +17,7 @@
> >  #include "io/channel-file.h"
> >  #include "io/channel-socket.h"
> >  #include "io/channel-util.h"
> > +#include "monitor/monitor.h"
> >  #include "options.h"
> >  #include "trace.h"
> >  
> > @@ -54,10 +55,18 @@ static void file_remove_fdset(void)
> >      }
> >  }
> >  
> > +/*
> > + * With multifd, due to the behavior of the dup() system call, we need
> > + * the fdset to have two non-duplicate fds so we can enable direct IO
> > + * in the secondary channels without affecting the main channel.
> > + */
> >  static bool file_parse_fdset(const char *filename, int64_t *fdset_id,
> >                               Error **errp)
> >  {
> > +    FdsetInfoList *fds_info;
> > +    FdsetFdInfoList *fd_info;
> >      const char *fdset_id_str;
> > +    int nfds = 0;
> >  
> >      *fdset_id = -1;
> >  
> > @@ -71,6 +80,32 @@ static bool file_parse_fdset(const char *filename, int64_t *fdset_id,
> >          return false;
> >      }
> >  
> > +    if (!migrate_multifd() || !migrate_direct_io()) {
> > +        return true;
> > +    }
> > +
> > +    for (fds_info = qmp_query_fdsets(NULL); fds_info;
> > +         fds_info = fds_info->next) {
> > +
> > +        if (*fdset_id != fds_info->value->fdset_id) {
> > +            continue;
> > +        }
> > +
> > +        for (fd_info = fds_info->value->fds; fd_info; fd_info = fd_info->next) {
> > +            if (nfds++ > 2) {
> > +                break;
> > +            }
> > +        }
> > +    }
> > +
> > +    if (nfds != 2) {
> > +        error_setg(errp, "Outgoing migration needs two fds in the fdset, "
> > +                   "got %d", nfds);
> > +        qmp_remove_fd(*fdset_id, false, -1, NULL);
> > +        *fdset_id = -1;
> > +        return false;
> > +    }
> > +
> >      return true;
> >  }
> 
> Related to my thoughts in an earlier patch, where I say that use of fdsets
> ought to be transparent to QEMU code, I'm not a fan of having this logic
> in migration code.
> 
> IIUC, the migration code will call  qio_channel_file_new_path twice,
> once with O_DIRECT and once without. This should trigger two calls
> into monitor_fdset_dup_fd_add with different flags. If we're matching
> flags in that monitor_fdset_dup_fd_add(), then if only 1 FD was
> provided, are we not able to report an error there ?

Right, this sounds working.

For a real sanity check, we may want to somehow check the two fds returned
from qio_channel_file_new_path() to point to the same file underneath.

What mentioned in the other thread (kcmp with KCMP_FILE) might not work, as
the whole purpose of having two fds is to make sure they have different
struct file to back the fd (and only one of them has O_DIRECT).  fstat()
might work in this case over the st_ino field, etc. maybe fstatfs() too but
perhaps that's over cautious.  Just a pain to use two fds as a start..

Thanks,

-- 
Peter Xu