Re: [PATCH v2 06/18] monitor: Stop removing non-duplicated fds

[Peter Xu <peterx@redhat.com>]

On Thu, May 23, 2024 at 04:05:36PM -0300, Fabiano Rosas wrote:
> We've been up until now cleaning up any file descriptors that have
> been passed into QEMU and never duplicated[1,2]. A file descriptor
> without duplicates indicates that no part of QEMU has made use of
> it. This approach is starting to show some cracks now that we're
> starting to consume fds from the migration code:
> 
> - Doing cleanup every time the last monitor connection closes works to
>   reap unused fds, but also has the side effect of forcing the
>   management layer to pass the file descriptors again in case of a
>   disconnect/re-connect, if that happened to be the only monitor
>   connection.
> 
>   Another side effect is that removing an fd with qmp_remove_fd() is
>   effectively delayed until the last monitor connection closes.
> 
>   The reliance on mon_refcount is also problematic because it's racy.
> 
> - Checking runstate_is_running() skips the cleanup unless the VM is
>   running and avoids premature cleanup of the fds, but also has the
>   side effect of blocking the legitimate removal of an fd via
>   qmp_remove_fd() if the VM happens to be in another state.
> 
>   This affects qmp_remove_fd() and qmp_query_fdsets() in particular
>   because requesting a removal at a bad time (guest stopped) might
>   cause an fd to never be removed, or to be removed at a much later
>   point in time, causing the query command to continue showing the
>   supposedly removed fd/fdset.
> 
> Note that file descriptors that *have* been duplicated are owned by
> the code that uses them and will be removed after qemu_close() is
> called. Therefore we've decided that the best course of action to
> avoid the undesired side-effects is to stop managing non-duplicated
> file descriptors.
> 
> 1- efb87c1697 ("monitor: Clean up fd sets on monitor disconnect")
> 2- ebe52b592d ("monitor: Prevent removing fd from set during init")
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>  monitor/fds.c              | 15 ++++++++-------
>  monitor/hmp.c              |  2 --
>  monitor/monitor-internal.h |  1 -
>  monitor/qmp.c              |  2 --
>  4 files changed, 8 insertions(+), 12 deletions(-)
> 
> diff --git a/monitor/fds.c b/monitor/fds.c
> index 101e21720d..f7b98814fa 100644
> --- a/monitor/fds.c
> +++ b/monitor/fds.c
> @@ -169,6 +169,11 @@ int monitor_get_fd(Monitor *mon, const char *fdname, Error **errp)
>  
>  static void monitor_fdset_free(MonFdset *mon_fdset)
>  {
> +    /*
> +     * Only remove an empty fdset. The fds are owned by the user and
> +     * should have been removed with qmp_remove_fd(). The dup_fds are
> +     * owned by QEMU and should have been removed with qemu_close().
> +     */
>      if (QLIST_EMPTY(&mon_fdset->fds) && QLIST_EMPTY(&mon_fdset->dup_fds)) {
>          QLIST_REMOVE(mon_fdset, next);
>          g_free(mon_fdset);
> @@ -189,9 +194,7 @@ static void monitor_fdset_cleanup(MonFdset *mon_fdset)
>      MonFdsetFd *mon_fdset_fd_next;
>  
>      QLIST_FOREACH_SAFE(mon_fdset_fd, &mon_fdset->fds, next, mon_fdset_fd_next) {
> -        if ((mon_fdset_fd->removed ||
> -                (QLIST_EMPTY(&mon_fdset->dup_fds) && mon_refcount == 0)) &&
> -                runstate_is_running()) {
> +        if (mon_fdset_fd->removed) {

I don't know the code well, but I like it.

>              monitor_fdset_fd_free(mon_fdset_fd);
>          }
>      }
> @@ -206,7 +209,7 @@ void monitor_fdsets_cleanup(void)
>  
>      QEMU_LOCK_GUARD(&mon_fdsets_lock);
>      QLIST_FOREACH_SAFE(mon_fdset, &mon_fdsets, next, mon_fdset_next) {
> -        monitor_fdset_cleanup(mon_fdset);
> +        monitor_fdset_free(mon_fdset);
>      }
>  }
>  
> @@ -479,9 +482,7 @@ void monitor_fdset_dup_fd_remove(int dup_fd)
>              if (mon_fdset_fd_dup->fd == dup_fd) {
>                  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);
> -                }
> +                monitor_fdset_free(mon_fdset);

This and above changes are not crystal clear to me where the _cleanup()
does extra check "removed" and clean those fds.

I think it'll make it easier for me to understand if this one and the next
are squashed together.  But maybe it's simply because I didn't fully
understand.

>                  return;
>              }
>          }
> diff --git a/monitor/hmp.c b/monitor/hmp.c
> index 69c1b7e98a..460e8832f6 100644
> --- a/monitor/hmp.c
> +++ b/monitor/hmp.c
> @@ -1437,11 +1437,9 @@ static void monitor_event(void *opaque, QEMUChrEvent event)
>              monitor_resume(mon);
>          }
>          qemu_mutex_unlock(&mon->mon_lock);
> -        mon_refcount++;
>          break;
>  
>      case CHR_EVENT_CLOSED:
> -        mon_refcount--;
>          monitor_fdsets_cleanup();
>          break;
>  
> diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h
> index 252de85681..cb628f681d 100644
> --- a/monitor/monitor-internal.h
> +++ b/monitor/monitor-internal.h
> @@ -168,7 +168,6 @@ extern bool qmp_dispatcher_co_shutdown;
>  extern QmpCommandList qmp_commands, qmp_cap_negotiation_commands;
>  extern QemuMutex monitor_lock;
>  extern MonitorList mon_list;
> -extern int mon_refcount;
>  
>  extern HMPCommand hmp_cmds[];
>  
> diff --git a/monitor/qmp.c b/monitor/qmp.c
> index a239945e8d..5e538f34c0 100644
> --- a/monitor/qmp.c
> +++ b/monitor/qmp.c
> @@ -466,7 +466,6 @@ static void monitor_qmp_event(void *opaque, QEMUChrEvent event)
>          data = qmp_greeting(mon);
>          qmp_send_response(mon, data);
>          qobject_unref(data);
> -        mon_refcount++;
>          break;
>      case CHR_EVENT_CLOSED:
>          /*
> @@ -479,7 +478,6 @@ static void monitor_qmp_event(void *opaque, QEMUChrEvent event)
>          json_message_parser_destroy(&mon->parser);
>          json_message_parser_init(&mon->parser, handle_qmp_command,
>                                   mon, NULL);
> -        mon_refcount--;
>          monitor_fdsets_cleanup();
>          break;
>      case CHR_EVENT_BREAK:

I like this too when mon_refcount can be dropped.  It turns out I like this
patch and the next a lot, and I hope nothing will break.

Aside, you seem to have forgot removal of the "int mon_refcount" in
monitor.c.

-- 
Peter Xu