Re: [PATCH 2/9] migration: Fix file migration with fdset

[Fabiano Rosas <farosas@suse.de>]

Peter Xu <peterx@redhat.com> writes:

> On Fri, Apr 26, 2024 at 11:20:35AM -0300, Fabiano Rosas wrote:
>> When the migration using the "file:" URI was implemented, I don't
>> think any of us noticed that if you pass in a file name with the
>> format "/dev/fdset/N", this allows a file descriptor to be passed in
>> to QEMU and that behaves just like the "fd:" URI. So the "file:"
>> support has been added without regard for the fdset part and we got
>> some things wrong.
>> 
>> The first issue is that we should not truncate the migration file if
>> we're allowing an fd + offset. We need to leave the file contents
>> untouched.
>
> I'm wondering whether we can use fallocate() instead on the ranges so that
> we always don't open() with O_TRUNC.  Before that..  could you remind me
> why do we need to truncate in the first place?  I definitely missed
> something else here too.

AFAIK, just to avoid any issues if the file is pre-existing. I don't see
the difference between O_TRUNC and fallocate in this case.

>
>> 
>> The second issue is that there's an expectation that QEMU removes the
>> fd after the migration has finished. That's what the "fd:" code
>> does. Otherwise a second migration on the same VM could attempt to
>> provide an fdset with the same name and QEMU would reject it.
>
> Let me check what we do when with "fd:" and when migration completes or
> cancels.
>
> IIUC it's qio_channel_file_close() that does the final cleanup work on
> e.g. to_dst_file, right?  Then there's qemu_close(), and it has:
>
>     /* Close fd that was dup'd from an fdset */
>     fdset_id = monitor_fdset_dup_fd_find(fd);
>     if (fdset_id != -1) {
>         int ret;
>
>         ret = close(fd);
>         if (ret == 0) {
>             monitor_fdset_dup_fd_remove(fd);
>         }
>
>         return ret;
>     }
>
> Shouldn't this done the work already?

That removes the mon_fdset_fd_dup->fd, we want to remove the
mon_fdset_fd->fd.

>
> Off topic: I think this code is over complicated too, maybe I missed
> something, but afaict we don't need monitor_fdset_dup_fd_find at all.. we
> simply walk the list and remove stuff..  I attach a patch at the end that I
> tried to clean that up, just in case there's early comments.  But we can
> ignore that so we don't get side-tracked, and focus on the direct-io
> issues.

Well, I'm not confident touching this code. This is more than a decade
old, I have no idea what the original motivations were. The possible
interactions with the user via command-line (-add-fd), QMP (add-fd) and
the monitor lifetime make me confused. Not to mention the fdset part
being plumbed into the guts of a widely used qemu_open_internal() that
very misleadingly presents itself as just a wrapper for open().

>
> Thanks,
>
> =======
>
> From 2f6b6d1224486d8ee830a7afe34738a07003b863 Mon Sep 17 00:00:00 2001
> From: Peter Xu <peterx@redhat.com>
> Date: Fri, 3 May 2024 11:27:20 -0400
> Subject: [PATCH] monitor: Drop monitor_fdset_dup_fd_add()
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
>
> This function is not needed, one remove function should already work.
> Clean it up.
>
> Here the code doesn't really care about whether we need to keep that dupfd
> around if close() failed: when that happens something got very wrong,
> keeping the dup_fd around the fdsets may not help that situation so far.
>
> Cc: Dr. David Alan Gilbert <dave@treblig.org>
> Cc: Markus Armbruster <armbru@redhat.com>
> Cc: Philippe Mathieu-Daudé <philmd@linaro.org>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Daniel P. Berrangé <berrange@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  include/monitor/monitor.h |  1 -
>  monitor/fds.c             | 27 +++++----------------------
>  stubs/fdset.c             |  5 -----
>  util/osdep.c              | 15 +--------------
>  4 files changed, 6 insertions(+), 42 deletions(-)
>
> diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
> index 965f5d5450..fd9b3f538c 100644
> --- a/include/monitor/monitor.h
> +++ b/include/monitor/monitor.h
> @@ -53,7 +53,6 @@ AddfdInfo *monitor_fdset_add_fd(int fd, bool has_fdset_id, int64_t fdset_id,
>                                  const char *opaque, Error **errp);
>  int monitor_fdset_dup_fd_add(int64_t fdset_id, int flags);
>  void monitor_fdset_dup_fd_remove(int dup_fd);
> -int64_t monitor_fdset_dup_fd_find(int dup_fd);
>  
>  void monitor_register_hmp(const char *name, bool info,
>                            void (*cmd)(Monitor *mon, const QDict *qdict));
> diff --git a/monitor/fds.c b/monitor/fds.c
> index d86c2c674c..d5aecfb70e 100644
> --- a/monitor/fds.c
> +++ b/monitor/fds.c
> @@ -458,7 +458,7 @@ int monitor_fdset_dup_fd_add(int64_t fdset_id, int flags)
>  #endif
>  }
>  
> -static int64_t monitor_fdset_dup_fd_find_remove(int dup_fd, bool remove)
> +void monitor_fdset_dup_fd_remove(int dup_fd)
>  {
>      MonFdset *mon_fdset;
>      MonFdsetFd *mon_fdset_fd_dup;
> @@ -467,31 +467,14 @@ static int64_t monitor_fdset_dup_fd_find_remove(int dup_fd, bool remove)
>      QLIST_FOREACH(mon_fdset, &mon_fdsets, next) {
>          QLIST_FOREACH(mon_fdset_fd_dup, &mon_fdset->dup_fds, next) {
>              if (mon_fdset_fd_dup->fd == dup_fd) {
> -                if (remove) {
> -                    QLIST_REMOVE(mon_fdset_fd_dup, next);
> -                    g_free(mon_fdset_fd_dup);
> -                    if (QLIST_EMPTY(&mon_fdset->dup_fds)) {
> -                        monitor_fdset_cleanup(mon_fdset);
> -                    }
> -                    return -1;
> -                } else {
> -                    return mon_fdset->id;
> +                QLIST_REMOVE(mon_fdset_fd_dup, next);
> +                g_free(mon_fdset_fd_dup);
> +                if (QLIST_EMPTY(&mon_fdset->dup_fds)) {
> +                    monitor_fdset_cleanup(mon_fdset);
>                  }
>              }
>          }
>      }
> -
> -    return -1;
> -}
> -
> -int64_t monitor_fdset_dup_fd_find(int dup_fd)
> -{
> -    return monitor_fdset_dup_fd_find_remove(dup_fd, false);
> -}
> -
> -void monitor_fdset_dup_fd_remove(int dup_fd)
> -{
> -    monitor_fdset_dup_fd_find_remove(dup_fd, true);
>  }
>  
>  int monitor_fd_param(Monitor *mon, const char *fdname, Error **errp)
> diff --git a/stubs/fdset.c b/stubs/fdset.c
> index d7c39a28ac..389e368a29 100644
> --- a/stubs/fdset.c
> +++ b/stubs/fdset.c
> @@ -9,11 +9,6 @@ int monitor_fdset_dup_fd_add(int64_t fdset_id, int flags)
>      return -1;
>  }
>  
> -int64_t monitor_fdset_dup_fd_find(int dup_fd)
> -{
> -    return -1;
> -}
> -
>  void monitor_fdset_dup_fd_remove(int dupfd)
>  {
>  }
> diff --git a/util/osdep.c b/util/osdep.c
> index e996c4744a..2d9749d060 100644
> --- a/util/osdep.c
> +++ b/util/osdep.c
> @@ -393,21 +393,8 @@ int qemu_open_old(const char *name, int flags, ...)
>  
>  int qemu_close(int fd)
>  {
> -    int64_t fdset_id;
> -
>      /* Close fd that was dup'd from an fdset */
> -    fdset_id = monitor_fdset_dup_fd_find(fd);
> -    if (fdset_id != -1) {
> -        int ret;
> -
> -        ret = close(fd);
> -        if (ret == 0) {
> -            monitor_fdset_dup_fd_remove(fd);
> -        }
> -
> -        return ret;
> -    }
> -
> +    monitor_fdset_dup_fd_remove(fd);
>      return close(fd);
>  }
>  
> -- 
> 2.44.0