| Message ID | 20240617185731.9725-17-farosas@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | migration/mapped-ram: Add direct-io support | expand |
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 <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.
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..
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);
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(-)