Re: [PATCH 6/9] tests/qtest/migration: Add tests for file migration with direct-io

[Fabiano Rosas <farosas@suse.de>]

Peter Xu <peterx@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 <farosas@suse.de>
>
> Mostly:
>
> Reviewed-by: Peter Xu <peterx@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/devel@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
>>