Peter Xu <pet...@redhat.com> writes:
> On Fri, Apr 26, 2024 at 11:20:39AM -0300, Fabiano Rosas wrote:
>> The tests are only allowed to run in systems that know about the
>> O_DIRECT flag and in filesystems which support it.
>>
>> Signed-off-by: Fabiano Rosas <faro...@suse.de>
>
> Mostly:
>
> Reviewed-by: Peter Xu <pet...@redhat.com>
>
> Two trivial comments below.
>
>> ---
>> tests/qtest/migration-helpers.c | 42 +++++++++++++++++++++++++++++++++
>> tests/qtest/migration-helpers.h | 1 +
>> tests/qtest/migration-test.c | 42 +++++++++++++++++++++++++++++++++
>> 3 files changed, 85 insertions(+)
>>
>> diff --git a/tests/qtest/migration-helpers.c
>> b/tests/qtest/migration-helpers.c
>> index ce6d6615b5..356cd4fa8c 100644
>> --- a/tests/qtest/migration-helpers.c
>> +++ b/tests/qtest/migration-helpers.c
>> @@ -473,3 +473,45 @@ void migration_test_add(const char *path, void
>> (*fn)(void))
>> qtest_add_data_func_full(path, test, migration_test_wrapper,
>> migration_test_destroy);
>> }
>> +
>> +#ifdef O_DIRECT
>> +/*
>> + * Probe for O_DIRECT support on the filesystem. Since this is used
>> + * for tests, be conservative, if anything fails, assume it's
>> + * unsupported.
>> + */
>> +bool probe_o_direct_support(const char *tmpfs)
>> +{
>> + g_autofree char *filename = g_strdup_printf("%s/probe-o-direct", tmpfs);
>> + int fd, flags = O_CREAT | O_RDWR | O_TRUNC | O_DIRECT;
>> + void *buf;
>> + ssize_t ret, len;
>> + uint64_t offset;
>> +
>> + fd = open(filename, flags, 0660);
>> + if (fd < 0) {
>> + unlink(filename);
>> + return false;
>> + }
>> +
>> + /*
>> + * Assuming 4k should be enough to satisfy O_DIRECT alignment
>> + * requirements. The migration code uses 1M to be conservative.
>> + */
>> + len = 0x100000;
>> + offset = 0x100000;
>> +
>> + buf = aligned_alloc(len, len);
>
> This is the first usage of aligned_alloc() in qemu. IIUC it's just a newer
> posix_memalign(), which QEMU has one use of, and it's protected with:
>
> #if defined(CONFIG_POSIX_MEMALIGN)
> int ret;
> ret = posix_memalign(&ptr, alignment, size);
> ...
> #endif
>
> Didn't check deeper. Just keep this in mind if you see any compilation
> issues in future CIs, or simply switch to similar pattern.
>
>> + g_assert(buf);
>> +
>> + ret = pwrite(fd, buf, len, offset);
>> + unlink(filename);
>> + g_free(buf);
>> +
>> + if (ret < 0) {
>> + return false;
>> + }
>> +
>> + return true;
>> +}
>> +#endif
>> diff --git a/tests/qtest/migration-helpers.h
>> b/tests/qtest/migration-helpers.h
>> index 1339835698..d827e16145 100644
>> --- a/tests/qtest/migration-helpers.h
>> +++ b/tests/qtest/migration-helpers.h
>> @@ -54,5 +54,6 @@ char *find_common_machine_version(const char *mtype, const
>> char *var1,
>> const char *var2);
>> char *resolve_machine_version(const char *alias, const char *var1,
>> const char *var2);
>> +bool probe_o_direct_support(const char *tmpfs);
>> void migration_test_add(const char *path, void (*fn)(void));
>> #endif /* MIGRATION_HELPERS_H */
>> diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
>> index 7b177686b4..512b7ede8b 100644
>> --- a/tests/qtest/migration-test.c
>> +++ b/tests/qtest/migration-test.c
>> @@ -2295,6 +2295,43 @@ static void test_multifd_file_mapped_ram(void)
>> test_file_common(&args, true);
>> }
>>
>> +#ifdef O_DIRECT
>> +static void *migrate_mapped_ram_dio_start(QTestState *from,
>> + QTestState *to)
>> +{
>> + migrate_mapped_ram_start(from, to);
>
> This line seems redundant, migrate_multifd_mapped_ram_start() should cover
> that.
>
This is an artifact of another patch that adds direct-io + mapped-ram
without multifd. I'm bringing that back on v2. We were having a
discussion[1] about it in the libvirt mailing list. Having direct-io
even without multifd might still be useful for libvirt.
1-
https://lists.libvirt.org/archives/list/de...@lists.libvirt.org/thread/K4BDDJDMJ22XMJEFAUE323H5S5E47VQX/
>> + migrate_set_parameter_bool(from, "direct-io", true);
>> + migrate_set_parameter_bool(to, "direct-io", true);
>> +
>> + return NULL;
>> +}
>> +
>> +static void *migrate_multifd_mapped_ram_dio_start(QTestState *from,
>> + QTestState *to)
>> +{
>> + migrate_multifd_mapped_ram_start(from, to);
>> + return migrate_mapped_ram_dio_start(from, to);
>> +}
>> +
>> +static void test_multifd_file_mapped_ram_dio(void)
>> +{
>> + g_autofree char *uri = g_strdup_printf("file:%s/%s", tmpfs,
>> + FILE_TEST_FILENAME);
>> + MigrateCommon args = {
>> + .connect_uri = uri,
>> + .listen_uri = "defer",
>> + .start_hook = migrate_multifd_mapped_ram_dio_start,
>> + };
>> +
>> + if (!probe_o_direct_support(tmpfs)) {
>> + g_test_skip("Filesystem does not support O_DIRECT");
>> + return;
>> + }
>> +
>> + test_file_common(&args, true);
>> +}
>> +
>> +#endif /* O_DIRECT */
>>
>> static void test_precopy_tcp_plain(void)
>> {
>> @@ -3719,6 +3756,11 @@ int main(int argc, char **argv)
>> migration_test_add("/migration/multifd/file/mapped-ram/live",
>> test_multifd_file_mapped_ram_live);
>>
>> +#ifdef O_DIRECT
>> + migration_test_add("/migration/multifd/file/mapped-ram/dio",
>> + test_multifd_file_mapped_ram_dio);
>> +#endif
>> +
>> #ifdef CONFIG_GNUTLS
>> migration_test_add("/migration/precopy/unix/tls/psk",
>> test_precopy_unix_tls_psk);
>> --
>> 2.35.3
>>
