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

[Fabiano Rosas <farosas@suse.de>]

Peter Xu <peterx@redhat.com> writes:

> On Fri, May 03, 2024 at 04:56:08PM -0300, Fabiano Rosas wrote:
>> 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.
>
> Then, shall we avoid truncations at all, leaving all the feasibility to
> user (also errors prone to make)?
>

Is this a big deal? I'd rather close that possible gap and avoid the bug
reports.

>> 
>> >
>> >> 
>> >> 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.
>
> What I read so far is when we are removing the dup-fds, we'll do one more
> thing:
>
> monitor_fdset_dup_fd_find_remove():
>                     if (QLIST_EMPTY(&mon_fdset->dup_fds)) {
>                         monitor_fdset_cleanup(mon_fdset);
>                     }
>
> It means if we removed all the dup-fds correctly, we should also remove the
> whole fdset, which includes the ->fds, IIUC.
>

Since mon_fdset_fd->removed == false, we hit the runstate_is_running()
problem. I'm not sure, but probably mon_refcount > 0 as well. So the fd
would not be removed.

But I'll retest this on Monday just be sure, it's been a while since I
wrote some parts of this.

>> 
>> >
>> > 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().
>
> If to make QEMU long live, we'll probably need to touch it at some
> point.. or at least discuss about it and figure things out. We pay tech
> debts like this when there's no good comment / docs to refer in this case,
> then the earlier, perhaps also the better.. to try taking the stab, imho.
>
> Definitely not a request to clean everything up. :) Let's see whether
> others can chim in with better knowledge of the history.
>
>> 
>> >
>> > 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
>>