[PATCH v3 16/16] tests/qtest/migration: Add a test for mapped-ram with passing of fds

Fabiano Rosas posted 16 patches 5 months, 1 week ago
Maintainers: Peter Xu <peterx@redhat.com>, Fabiano Rosas <farosas@suse.de>, "Daniel P. Berrangé" <berrange@redhat.com>, "Dr. David Alan Gilbert" <dave@treblig.org>, Markus Armbruster <armbru@redhat.com>, Eric Blake <eblake@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>, Thomas Huth <thuth@redhat.com>, Laurent Vivier <lvivier@redhat.com>
[PATCH v3 16/16] tests/qtest/migration: Add a test for mapped-ram with passing of fds
Posted by Fabiano Rosas 5 months, 1 week ago
Add a multifd test for mapped-ram with passing of fds into QEMU. This
is how libvirt will consume the feature.

There are a couple of details to the fdset mechanism:

- multifd needs two distinct file descriptors (not duplicated with
  dup()) so it can enable O_DIRECT only on the channels that do
  aligned IO. The dup() system call creates file descriptors that
  share status flags, of which O_DIRECT is one.

- the open() access mode flags used for the fds passed into QEMU need
  to match the flags QEMU uses to open the file. Currently O_WRONLY
  for src and O_RDONLY for dst.

Note that fdset code goes under _WIN32 because fd passing is not
supported on Windows.

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
- dropped Peter's r-b

- stopped removing the fdset at the end of the tests. The migration
code should always cleanup after itself.
---
 tests/qtest/migration-test.c | 98 ++++++++++++++++++++++++++++++++++--
 1 file changed, 95 insertions(+), 3 deletions(-)

diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 928f0ca6ce..85a21ff5e9 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -2024,11 +2024,18 @@ static void test_precopy_file(void)
 
 #ifndef _WIN32
 static void fdset_add_fds(QTestState *qts, const char *file, int flags,
-                          int num_fds)
+                          int num_fds, bool direct_io)
 {
     for (int i = 0; i < num_fds; i++) {
         int fd;
 
+#ifdef O_DIRECT
+        /* only secondary channels can use direct-io */
+        if (direct_io && i != 0) {
+            flags |= O_DIRECT;
+        }
+#endif
+
         fd = open(file, flags, 0660);
         assert(fd != -1);
 
@@ -2042,8 +2049,8 @@ static void *file_offset_fdset_start_hook(QTestState *from, QTestState *to)
 {
     g_autofree char *file = g_strdup_printf("%s/%s", tmpfs, FILE_TEST_FILENAME);
 
-    fdset_add_fds(from, file, O_WRONLY, 1);
-    fdset_add_fds(to, file, O_RDONLY, 1);
+    fdset_add_fds(from, file, O_WRONLY, 1, false);
+    fdset_add_fds(to, file, O_RDONLY, 1, false);
 
     return NULL;
 }
@@ -2215,6 +2222,84 @@ static void test_multifd_file_mapped_ram_dio(void)
     test_file_common(&args, true);
 }
 
+#ifndef _WIN32
+static void multifd_mapped_ram_fdset_end(QTestState *from, QTestState *to,
+                                         void *opaque)
+{
+    QDict *resp;
+    QList *fdsets;
+
+    /*
+     * Make sure no fdsets are left after migration, otherwise a
+     * second migration would fail due fdset reuse.
+     */
+    resp = qtest_qmp(from, "{'execute': 'query-fdsets', "
+                     "'arguments': {}}");
+    g_assert(qdict_haskey(resp, "return"));
+    fdsets = qdict_get_qlist(resp, "return");
+    g_assert(fdsets && qlist_empty(fdsets));
+}
+
+static void *multifd_mapped_ram_fdset_dio(QTestState *from, QTestState *to)
+{
+    g_autofree char *file = g_strdup_printf("%s/%s", tmpfs, FILE_TEST_FILENAME);
+
+    fdset_add_fds(from, file, O_WRONLY, 2, true);
+    fdset_add_fds(to, file, O_RDONLY, 2, true);
+
+    migrate_multifd_mapped_ram_start(from, to);
+    migrate_set_parameter_bool(from, "direct-io", true);
+    migrate_set_parameter_bool(to, "direct-io", true);
+
+    return NULL;
+}
+
+static void *multifd_mapped_ram_fdset(QTestState *from, QTestState *to)
+{
+    g_autofree char *file = g_strdup_printf("%s/%s", tmpfs, FILE_TEST_FILENAME);
+
+    fdset_add_fds(from, file, O_WRONLY, 2, false);
+    fdset_add_fds(to, file, O_RDONLY, 2, false);
+
+    migrate_multifd_mapped_ram_start(from, to);
+
+    return NULL;
+}
+
+static void test_multifd_file_mapped_ram_fdset(void)
+{
+    g_autofree char *uri = g_strdup_printf("file:/dev/fdset/1,offset=%d",
+                                           FILE_TEST_OFFSET);
+    MigrateCommon args = {
+        .connect_uri = uri,
+        .listen_uri = "defer",
+        .start_hook = multifd_mapped_ram_fdset,
+        .finish_hook = multifd_mapped_ram_fdset_end,
+    };
+
+    test_file_common(&args, true);
+}
+
+static void test_multifd_file_mapped_ram_fdset_dio(void)
+{
+    g_autofree char *uri = g_strdup_printf("file:/dev/fdset/1,offset=%d",
+                                           FILE_TEST_OFFSET);
+    MigrateCommon args = {
+        .connect_uri = uri,
+        .listen_uri = "defer",
+        .start_hook = multifd_mapped_ram_fdset_dio,
+        .finish_hook = multifd_mapped_ram_fdset_end,
+    };
+
+    if (!probe_o_direct_support(tmpfs)) {
+        g_test_skip("Filesystem does not support O_DIRECT");
+        return;
+    }
+
+    test_file_common(&args, true);
+}
+#endif /* !_WIN32 */
+
 static void test_precopy_tcp_plain(void)
 {
     MigrateCommon args = {
@@ -3654,6 +3739,13 @@ int main(int argc, char **argv)
     migration_test_add("/migration/multifd/file/mapped-ram/dio",
                        test_multifd_file_mapped_ram_dio);
 
+#ifndef _WIN32
+    migration_test_add("/migration/multifd/file/mapped-ram/fdset",
+                       test_multifd_file_mapped_ram_fdset);
+    migration_test_add("/migration/multifd/file/mapped-ram/fdset/dio",
+                       test_multifd_file_mapped_ram_fdset_dio);
+#endif
+
 #ifdef CONFIG_GNUTLS
     migration_test_add("/migration/precopy/unix/tls/psk",
                        test_precopy_unix_tls_psk);
-- 
2.35.3
Re: [PATCH v3 16/16] tests/qtest/migration: Add a test for mapped-ram with passing of fds
Posted by Peter Xu 5 months, 1 week ago
On Mon, Jun 17, 2024 at 03:57:31PM -0300, Fabiano Rosas wrote:
> Add a multifd test for mapped-ram with passing of fds into QEMU. This
> is how libvirt will consume the feature.
> 
> There are a couple of details to the fdset mechanism:
> 
> - multifd needs two distinct file descriptors (not duplicated with
>   dup()) so it can enable O_DIRECT only on the channels that do
>   aligned IO. The dup() system call creates file descriptors that
>   share status flags, of which O_DIRECT is one.
> 
> - the open() access mode flags used for the fds passed into QEMU need
>   to match the flags QEMU uses to open the file. Currently O_WRONLY
>   for src and O_RDONLY for dst.
> 
> Note that fdset code goes under _WIN32 because fd passing is not
> supported on Windows.
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
> - dropped Peter's r-b
> 
> - stopped removing the fdset at the end of the tests. The migration
> code should always cleanup after itself.

Ah, that looks also ok.

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu
Re: [PATCH v3 16/16] tests/qtest/migration: Add a test for mapped-ram with passing of fds
Posted by Fabiano Rosas 5 months ago
Peter Xu <peterx@redhat.com> writes:

> On Mon, Jun 17, 2024 at 03:57:31PM -0300, Fabiano Rosas wrote:
>> Add a multifd test for mapped-ram with passing of fds into QEMU. This
>> is how libvirt will consume the feature.
>> 
>> There are a couple of details to the fdset mechanism:
>> 
>> - multifd needs two distinct file descriptors (not duplicated with
>>   dup()) so it can enable O_DIRECT only on the channels that do
>>   aligned IO. The dup() system call creates file descriptors that
>>   share status flags, of which O_DIRECT is one.
>> 
>> - the open() access mode flags used for the fds passed into QEMU need
>>   to match the flags QEMU uses to open the file. Currently O_WRONLY
>>   for src and O_RDONLY for dst.
>> 
>> Note that fdset code goes under _WIN32 because fd passing is not
>> supported on Windows.
>> 
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> ---
>> - dropped Peter's r-b
>> 
>> - stopped removing the fdset at the end of the tests. The migration
>> code should always cleanup after itself.
>
> Ah, that looks also ok.

I made a mistake here. We still need to require that the management
layer explicitly removes the fds they added by calling qmp_remove_fd().

The reason I thought it was ok to not remove the fds was that after your
suggestion to use monitor_fdset_free_if_empty() in patch 7, I mistakenly
put monitor_fdset_free() instead. So the qmp_remove_fd() was not finding
any fdsets and I thought that was QEMU removing everything. Which is
silly, because the whole purpose of the patch is to not do that.

I think I'll just fix this in the migration tree. It's just a revert to
v2 of this patch and the correction to patch 7.
Re: [PATCH v3 16/16] tests/qtest/migration: Add a test for mapped-ram with passing of fds
Posted by Peter Xu 5 months ago
On Fri, Jun 21, 2024 at 09:33:48AM -0300, Fabiano Rosas wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > On Mon, Jun 17, 2024 at 03:57:31PM -0300, Fabiano Rosas wrote:
> >> Add a multifd test for mapped-ram with passing of fds into QEMU. This
> >> is how libvirt will consume the feature.
> >> 
> >> There are a couple of details to the fdset mechanism:
> >> 
> >> - multifd needs two distinct file descriptors (not duplicated with
> >>   dup()) so it can enable O_DIRECT only on the channels that do
> >>   aligned IO. The dup() system call creates file descriptors that
> >>   share status flags, of which O_DIRECT is one.
> >> 
> >> - the open() access mode flags used for the fds passed into QEMU need
> >>   to match the flags QEMU uses to open the file. Currently O_WRONLY
> >>   for src and O_RDONLY for dst.
> >> 
> >> Note that fdset code goes under _WIN32 because fd passing is not
> >> supported on Windows.
> >> 
> >> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> >> ---
> >> - dropped Peter's r-b
> >> 
> >> - stopped removing the fdset at the end of the tests. The migration
> >> code should always cleanup after itself.
> >
> > Ah, that looks also ok.
> 
> I made a mistake here. We still need to require that the management
> layer explicitly removes the fds they added by calling qmp_remove_fd().
> 
> The reason I thought it was ok to not remove the fds was that after your
> suggestion to use monitor_fdset_free_if_empty() in patch 7, I mistakenly
> put monitor_fdset_free() instead. So the qmp_remove_fd() was not finding
> any fdsets and I thought that was QEMU removing everything. Which is
> silly, because the whole purpose of the patch is to not do that.
> 
> I think I'll just fix this in the migration tree. It's just a revert to
> v2 of this patch and the correction to patch 7.

Ah OK.  I thought you were talking about when QEMU exit()s, which should
cleanup everything too.

Personally I guess I kind of ignored that remove-fd api anyway being there
or not, as it seems nobody understand why it needs to exist in the first
place..

-- 
Peter Xu