Re: [PATCH v5] Prevent vhost-user-blk-test hang

[Stefan Hajnoczi]

On Mon, Sep 27, 2021 at 05:17:01PM +0000, Raphael Norwitz wrote:
> In the vhost-user-blk-test, as of now there is nothing stoping
> vhost-user-blk in QEMU writing to the socket right after forking off the
> storage daemon before it has a chance to come up properly, leaving the
> test hanging forever. This intermittently hanging test has caused QEMU
> automation failures reported multiple times on the mailing list [1].
> 
> This change makes the storage-daemon notify the vhost-user-blk-test
> that it is fully initialized and ready to handle client connections by
> creating a pidfile on initialiation. This ensures that the storage-daemon
> backend won't miss vhost-user messages and thereby resolves the hang.
> 
> [1] 
> https://lore.kernel.org/qemu-devel/CAFEAcA8kYpz9LiPNxnWJAPSjc=nv532bEdyfynaBeMeohqBp3A@mail.gmail.com/

Hi Raphael,
I would like to understand the issue that is being worked around in the
patch.

QEMU should be okay with listen fd passing. The qemu-storage-daemon
documentation even contains example code for this
(docs/tools/qemu-storage-daemon.rst) and that may need to be updated if
listen fd passing is fundamentally broken.

Can you share more details about the problem?

Does "writing to the socket" mean writing vhost-user protocol messages
or does it mean connect(2)?

Could the problem be that vhost-user-blk-test.c creates the listen fds
and does not close them? This means the host network stack doesn't
consider the socket closed after QEMU terminates and therefore the test
process hangs after QEMU is gone? In that case vhost-user-blk-test needs
to close the fds after spawning qemu-storage-daemon.

Stefan

> 
> Signed-off-by: Raphael Norwitz <raphael.norwitz@nutanix.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  tests/qtest/vhost-user-blk-test.c | 26 +++++++++++++++++++++++++-
>  1 file changed, 25 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/qtest/vhost-user-blk-test.c 
> b/tests/qtest/vhost-user-blk-test.c
> index 6f108a1b62..5fed262da1 100644
> --- a/tests/qtest/vhost-user-blk-test.c
> +++ b/tests/qtest/vhost-user-blk-test.c
> @@ -24,6 +24,7 @@
>  #define TEST_IMAGE_SIZE         (64 * 1024 * 1024)
>  #define QVIRTIO_BLK_TIMEOUT_US  (30 * 1000 * 1000)
>  #define PCI_SLOT_HP             0x06
> +#define PIDFILE_RETRIES         5
>  
>  typedef struct {
>      pid_t pid;
> @@ -885,7 +886,8 @@ static void start_vhost_user_blk(GString *cmd_line, int 
> vus_instances,
>                                   int num_queues)
>  {
>      const char *vhost_user_blk_bin = qtest_qemu_storage_daemon_binary();
> -    int i;
> +    int i, retries;
> +    char *daemon_pidfile_path;
>      gchar *img_path;
>      GString *storage_daemon_command = g_string_new(NULL);
>      QemuStorageDaemonState *qsd;
> @@ -898,6 +900,8 @@ static void start_vhost_user_blk(GString *cmd_line, int 
> vus_instances,
>              " -object memory-backend-memfd,id=mem,size=256M,share=on "
>              " -M memory-backend=mem -m 256M ");
>  
> +    daemon_pidfile_path = g_strdup_printf("/tmp/daemon-%d", getpid());
> +
>      for (i = 0; i < vus_instances; i++) {
>          int fd;
>          char *sock_path = create_listen_socket(&fd);
> @@ -914,6 +918,9 @@ static void start_vhost_user_blk(GString *cmd_line, int 
> vus_instances,
>                                 i + 1, sock_path);
>      }
>  
> +    g_string_append_printf(storage_daemon_command, "--pidfile %s ",
> +                           daemon_pidfile_path);
> +
>      g_test_message("starting vhost-user backend: %s",
>                     storage_daemon_command->str);
>      pid_t pid = fork();
> @@ -930,7 +937,24 @@ static void start_vhost_user_blk(GString *cmd_line, int 
> vus_instances,
>          execlp("/bin/sh", "sh", "-c", storage_daemon_command->str, NULL);
>          exit(1);
>      }
> +
> +    /*
> +     * Ensure the storage-daemon has come up properly before allowing the
> +     * test to proceed.
> +     */
> +    retries = 0;
> +    while (access(daemon_pidfile_path, F_OK) != 0) {
> +        g_assert_cmpint(retries, <, PIDFILE_RETRIES);
> +
> +        retries++;
> +        g_usleep(1000);
> +    }
> +
>      g_string_free(storage_daemon_command, true);
> +    if (access(daemon_pidfile_path, F_OK) == 0) {
> +        unlink(daemon_pidfile_path);
> +    }
> +    g_free(daemon_pidfile_path);
>  
>      qsd = g_new(QemuStorageDaemonState, 1);
>      qsd->pid = pid;
> -- 
> 2.20.1
>

signature.asc
Description: PGP signature