Re: [PATCH v2] btrfs: tests: add selftests for RAID stripe-tree

[Filipe Manana]

On Tue, Oct 1, 2024 at 7:59 AM Johannes Thumshirn <jth@xxxxxxxxxx> wrote:
>
> From: Johannes Thumshirn <johannes.thumshirn@xxxxxxx>
>
> Add first stash of very basic self tests for the RAID stripe-tree.
>
> More test cases will follow exercising the tree.
>
> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@xxxxxxx>
> ---
> Changes to v1:
> * Fix build errors with CONFIG_BTRFS_FS_RUN_SANITY_TESTS=n
> * Document expectations from the test cases
>
> Link to v1:
> https://lore.kernel.org/linux-btrfs/20240930104054.12290-1-jth@xxxxxxxxxx
>
>  fs/btrfs/Makefile                       |   3 +-
>  fs/btrfs/raid-stripe-tree.c             |   5 +-
>  fs/btrfs/raid-stripe-tree.h             |   5 +
>  fs/btrfs/tests/btrfs-tests.c            |   3 +
>  fs/btrfs/tests/btrfs-tests.h            |   1 +
>  fs/btrfs/tests/raid-stripe-tree-tests.c | 285 ++++++++++++++++++++++++
>  fs/btrfs/volumes.c                      |   6 +-
>  fs/btrfs/volumes.h                      |   5 +
>  8 files changed, 307 insertions(+), 6 deletions(-)
>  create mode 100644 fs/btrfs/tests/raid-stripe-tree-tests.c
>
> diff --git a/fs/btrfs/Makefile b/fs/btrfs/Makefile
> index 87617f2968bc..3cfc440c636c 100644
> --- a/fs/btrfs/Makefile
> +++ b/fs/btrfs/Makefile
> @@ -43,4 +43,5 @@ btrfs-$(CONFIG_FS_VERITY) += verity.o
>  btrfs-$(CONFIG_BTRFS_FS_RUN_SANITY_TESTS) += tests/free-space-tests.o \
>         tests/extent-buffer-tests.o tests/btrfs-tests.o \
>         tests/extent-io-tests.o tests/inode-tests.o tests/qgroup-tests.o \
> -       tests/free-space-tree-tests.o tests/extent-map-tests.o
> +       tests/free-space-tree-tests.o tests/extent-map-tests.o \
> +       tests/raid-stripe-tree-tests.o
> diff --git a/fs/btrfs/raid-stripe-tree.c b/fs/btrfs/raid-stripe-tree.c
> index 4c859b550f6c..b7787a8e4af2 100644
> --- a/fs/btrfs/raid-stripe-tree.c
> +++ b/fs/btrfs/raid-stripe-tree.c
> @@ -108,8 +108,9 @@ static int update_raid_extent_item(struct btrfs_trans_handle *trans,
>         return ret;
>  }
>
> -static int btrfs_insert_one_raid_extent(struct btrfs_trans_handle *trans,
> -                                       struct btrfs_io_context *bioc)
> +EXPORT_FOR_TESTS
> +int btrfs_insert_one_raid_extent(struct btrfs_trans_handle *trans,
> +                                struct btrfs_io_context *bioc)
>  {
>         struct btrfs_fs_info *fs_info = trans->fs_info;
>         struct btrfs_key stripe_key;
> diff --git a/fs/btrfs/raid-stripe-tree.h b/fs/btrfs/raid-stripe-tree.h
> index 1ac1c21aac2f..541836421778 100644
> --- a/fs/btrfs/raid-stripe-tree.h
> +++ b/fs/btrfs/raid-stripe-tree.h
> @@ -28,6 +28,11 @@ int btrfs_get_raid_extent_offset(struct btrfs_fs_info *fs_info,
>  int btrfs_insert_raid_extent(struct btrfs_trans_handle *trans,
>                              struct btrfs_ordered_extent *ordered_extent);
>
> +#ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS
> +int btrfs_insert_one_raid_extent(struct btrfs_trans_handle *trans,
> +                                struct btrfs_io_context *bioc);
> +#endif
> +
>  static inline bool btrfs_need_stripe_tree_update(struct btrfs_fs_info *fs_info,
>                                                  u64 map_type)
>  {
> diff --git a/fs/btrfs/tests/btrfs-tests.c b/fs/btrfs/tests/btrfs-tests.c
> index ce50847e1e01..18e1ab4a0914 100644
> --- a/fs/btrfs/tests/btrfs-tests.c
> +++ b/fs/btrfs/tests/btrfs-tests.c
> @@ -291,6 +291,9 @@ int btrfs_run_sanity_tests(void)
>                         ret = btrfs_test_free_space_tree(sectorsize, nodesize);
>                         if (ret)
>                                 goto out;
> +                       ret = btrfs_test_raid_stripe_tree(sectorsize, nodesize);
> +                       if (ret)
> +                               goto out;
>                 }
>         }
>         ret = btrfs_test_extent_map();
> diff --git a/fs/btrfs/tests/btrfs-tests.h b/fs/btrfs/tests/btrfs-tests.h
> index dc2f2ab15fa5..61bcadaf2036 100644
> --- a/fs/btrfs/tests/btrfs-tests.h
> +++ b/fs/btrfs/tests/btrfs-tests.h
> @@ -37,6 +37,7 @@ int btrfs_test_extent_io(u32 sectorsize, u32 nodesize);
>  int btrfs_test_inodes(u32 sectorsize, u32 nodesize);
>  int btrfs_test_qgroups(u32 sectorsize, u32 nodesize);
>  int btrfs_test_free_space_tree(u32 sectorsize, u32 nodesize);
> +int btrfs_test_raid_stripe_tree(u32 sectorsize, u32 nodesize);
>  int btrfs_test_extent_map(void);
>  struct inode *btrfs_new_test_inode(void);
>  struct btrfs_fs_info *btrfs_alloc_dummy_fs_info(u32 nodesize, u32 sectorsize);
> diff --git a/fs/btrfs/tests/raid-stripe-tree-tests.c b/fs/btrfs/tests/raid-stripe-tree-tests.c
> new file mode 100644
> index 000000000000..7b6380d3a44c
> --- /dev/null
> +++ b/fs/btrfs/tests/raid-stripe-tree-tests.c
> @@ -0,0 +1,285 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2024 Western Digital Corporation or its affiliates.
> + */
> +
> +#include <linux/sizes.h>
> +#include "../fs.h"
> +#include "../disk-io.h"
> +#include "../transaction.h"
> +#include "../volumes.h"
> +#include "../raid-stripe-tree.h"
> +#include "btrfs-tests.h"
> +
> +#define RST_TEST_NUM_DEVICES   2
> +#define RST_TEST_RAID1_TYPE    (BTRFS_BLOCK_GROUP_DATA | BTRFS_BLOCK_GROUP_RAID1)
> +
> +typedef int (*test_func_t)(struct btrfs_trans_handle *trans);
> +
> +static struct btrfs_device *btrfs_device_by_devid(struct btrfs_fs_devices *fs_devices,
> +                                                 u64 devid)
> +{
> +       struct btrfs_device *dev;
> +
> +       list_for_each_entry(dev, &fs_devices->devices, dev_list) {
> +               if (dev->devid == devid)
> +                       return dev;
> +       }
> +
> +       return NULL;
> +}
> +
> +/*
> + * Test a 64k RST write on a 2 disk RAID1 at a logical address of 1M and then
> + * overwrite the whole range giving it new physical address at an offset of 1G.
> + * The intent of this test is to exercise the 'update_raid_extent_item()'
> + * function called be btrfs_insert_one_raid_extent().
> + */
> +static int test_create_update_delete(struct btrfs_trans_handle *trans)
> +{
> +       struct btrfs_fs_info *fs_info = trans->fs_info;
> +       struct btrfs_io_context *bioc;
> +       struct btrfs_io_stripe io_stripe = { };

Just to be consistent with most of the codebase, the style { 0 } is
more dominant and preferred IIRC.
Same applies to every other instance in the tests below.

> +       u64 map_type = RST_TEST_RAID1_TYPE;
> +       u64 logical = SZ_1M;
> +       u64 len = SZ_64K;
> +       int ret;
> +
> +       bioc = alloc_btrfs_io_context(fs_info, logical, RST_TEST_NUM_DEVICES);
> +       if (!bioc) {
> +               ret = -ENOMEM;

So generally in tests we add some messages for error branches, like
this from extent-map-tests.c for example:

test_std_err(TEST_ALLOC_EXTENT_MAP);

Same applies to other tests below.

> +               goto out;
> +       }
> +
> +       io_stripe.dev = btrfs_device_by_devid(fs_info->fs_devices, 0);
> +
> +       for (int i = 0; i < RST_TEST_NUM_DEVICES; i++) {
> +               struct btrfs_io_stripe *stripe = &bioc->stripes[i];
> +               struct btrfs_device *dev;
> +
> +               dev = btrfs_device_by_devid(fs_info->fs_devices, i);
> +               if (!dev) {
> +                       ret = -EINVAL;

Add some error message with test_err(), just like what we generally do
in other tests.
This comment applies to every error branch, as many of them don't have
any, making it difficult to debug when we get failures.

> +                       goto out;
> +               }
> +
> +               stripe->dev = dev;

The dev variable is unnecessary, we can directly assign the result of
btrfs_device_by_devid(fs_info->fs_devices, i).

> +               stripe->physical = logical + i * SZ_1G;
> +       }
> +
> +       ret = btrfs_insert_one_raid_extent(trans, bioc);
> +       if (ret)
> +               goto out;
> +
> +       io_stripe.dev = btrfs_device_by_devid(fs_info->fs_devices, 0);
> +       if (!io_stripe.dev) {
> +               ret = -EINVAL;
> +               goto out;
> +       }
> +
> +       ret = btrfs_get_raid_extent_offset(fs_info, logical, &len, map_type, 0,
> +                                          &io_stripe);
> +       if (ret)
> +               goto out;
> +
> +       if (io_stripe.physical != logical) {
> +               test_err("invalid physical address, expected %llu, got %llu",
> +                        logical, io_stripe.physical);
> +               ret = -EINVAL;
> +               goto out;
> +       }
> +
> +       if (len != SZ_64K) {
> +               test_err("invalid stripe length, expected %llu, got %llu",
> +                        (u64)SZ_64K, len);
> +               ret = -EINVAL;
> +               goto out;
> +       }
> +
> +       for (int i = 0; i < RST_TEST_NUM_DEVICES; i++) {
> +               struct btrfs_io_stripe *stripe = &bioc->stripes[i];
> +               struct btrfs_device *dev;
> +
> +               dev = btrfs_device_by_devid(fs_info->fs_devices, i);
> +               if (!dev) {
> +                       ret = -EINVAL;
> +                       goto out;
> +               }
> +
> +               stripe->dev = dev;

Same here, 'dev' is unnecessary, we can directly assign the result of
the function call to stripe->dev.
And the same for several other similar cases below.

> +               stripe->physical = SZ_1G + logical + i * SZ_1G;
> +       }
> +
> +       ret = btrfs_insert_one_raid_extent(trans, bioc);
> +       if (ret)
> +               goto out;
> +       if (io_stripe.physical != logical + SZ_1G) {
> +               test_err("invalid physical address, expected %llu, got %llu",
> +                        logical + SZ_1G, io_stripe.physical);
> +               ret = -EINVAL;
> +               goto out;
> +       }
> +
> +       if (len != SZ_64K) {
> +               test_err("invalid stripe length, expected %llu, got %llu",
> +                        (u64)SZ_64K, len);
> +               ret = -EINVAL;
> +               goto out;
> +       }
> +
> +       ret = btrfs_delete_raid_extent(trans, logical, len);

Can we also add an explicit error message here in case this fails?

> +
> +out:
> +       btrfs_put_bioc(bioc);
> +       return ret;
> +}
> +
> +/*
> + * Test a simple 64k RST write on a 2 disk RAID1 at a logical address of 1M.
> + * The "physical" copy on device 0 is at 1M, on device 1 it is at 1G+1M.
> + */
> +static int test_simple_create_delete(struct btrfs_trans_handle *trans)
> +{
> +       struct btrfs_fs_info *fs_info = trans->fs_info;
> +       struct btrfs_io_context *bioc;
> +       struct btrfs_io_stripe io_stripe = { };
> +       u64 map_type = RST_TEST_RAID1_TYPE;
> +       u64 logical = SZ_1M;
> +       u64 len = SZ_64K;
> +       int ret;
> +
> +       bioc = alloc_btrfs_io_context(fs_info, logical, RST_TEST_NUM_DEVICES);
> +       if (!bioc) {
> +               ret = -ENOMEM;
> +               goto out;
> +       }
> +
> +       bioc->map_type = map_type;
> +       bioc->size = SZ_64K;
> +
> +       for (int i = 0; i < RST_TEST_NUM_DEVICES; i++) {
> +               struct btrfs_io_stripe *stripe = &bioc->stripes[i];
> +               struct btrfs_device *dev;
> +
> +               dev = btrfs_device_by_devid(fs_info->fs_devices, i);
> +               if (!dev) {
> +                       ret = -EINVAL;
> +                       goto out;
> +               }
> +
> +               stripe->dev = dev;
> +               stripe->physical = logical + i * SZ_1G;
> +       }
> +
> +       ret = btrfs_insert_one_raid_extent(trans, bioc);
> +       if (ret)
> +               goto out;
> +
> +       io_stripe.dev = btrfs_device_by_devid(fs_info->fs_devices, 0);
> +       if (!io_stripe.dev) {
> +               ret = -EINVAL;
> +               goto out;
> +       }
> +
> +       ret = btrfs_get_raid_extent_offset(fs_info, logical, &len, map_type, 0,
> +                                          &io_stripe);
> +       if (ret)
> +               goto out;
> +
> +       if (io_stripe.physical != logical) {
> +               test_err("invalid physical address, expected %llu, got %llu",
> +                        logical, io_stripe.physical);
> +               ret = -EINVAL;
> +               goto out;
> +       }
> +
> +       if (len != SZ_64K) {
> +               test_err("invalid stripe length, expected %llu, got %llu",
> +                        (u64)SZ_64K, len);
> +               ret = -EINVAL;
> +               goto out;
> +       }
> +
> +       ret = btrfs_delete_raid_extent(trans, logical, len);

Same here, in case it fails, have an explicit error message.

> +
> +out:
> +       btrfs_put_bioc(bioc);
> +       return ret;
> +}
> +
> +test_func_t tests[] = {
> +       test_simple_create_delete,
> +       test_create_update_delete,
> +};
> +
> +static int run_test(test_func_t test, u32 sectorsize, u32 nodesize)
> +{
> +       struct btrfs_trans_handle trans;
> +       struct btrfs_fs_info *fs_info;
> +       struct btrfs_root *root = NULL;
> +       int ret;
> +
> +       fs_info = btrfs_alloc_dummy_fs_info(sectorsize, nodesize);
> +       if (!fs_info) {
> +               test_std_err(TEST_ALLOC_FS_INFO);
> +               ret = -ENOMEM;
> +               goto out;
> +       }
> +
> +       root = btrfs_alloc_dummy_root(fs_info);
> +       if (IS_ERR(root)) {
> +               test_std_err(TEST_ALLOC_ROOT);
> +               ret = PTR_ERR(root);
> +               goto out;
> +       }
> +       btrfs_set_super_compat_ro_flags(root->fs_info->super_copy,
> +               BTRFS_FEATURE_INCOMPAT_RAID_STRIPE_TREE);
> +       root->root_key.objectid = BTRFS_RAID_STRIPE_TREE_OBJECTID;
> +       root->root_key.type = BTRFS_ROOT_ITEM_KEY;
> +       root->root_key.offset = 0;
> +       fs_info->stripe_root = root;
> +       root->fs_info->tree_root = root;

Why root->fs_info->tree_root and not just fs_info->tree_root? It's the
same fs_info, right?


> +
> +       root->node = alloc_test_extent_buffer(root->fs_info, nodesize);
> +       if (IS_ERR(root->node)) {
> +               test_std_err(TEST_ALLOC_EXTENT_BUFFER);
> +               ret = PTR_ERR(root->node);
> +               goto out;
> +       }
> +       btrfs_set_header_level(root->node, 0);
> +       btrfs_set_header_nritems(root->node, 0);
> +       root->alloc_bytenr += 2 * nodesize;
> +
> +       for (int i = 0; i < RST_TEST_NUM_DEVICES; i++) {
> +               struct btrfs_device *dev;
> +
> +               dev = btrfs_alloc_dummy_device(fs_info);

Need to check if dev is an ERR_PTR().

Everything else looks fine to me.
With those minor fixes:

Reviewed-by: Filipe Manana <fdmanana@xxxxxxxx>

Thanks.

> +               dev->devid = i;
> +       }
> +
> +       btrfs_init_dummy_trans(&trans, root->fs_info);
> +       ret = test(&trans);
> +       if (ret)
> +               goto out;
> +
> +out:
> +       btrfs_free_dummy_root(root);
> +       btrfs_free_dummy_fs_info(fs_info);
> +
> +       return ret;
> +}
> +
> +int btrfs_test_raid_stripe_tree(u32 sectorsize, u32 nodesize)
> +{
> +       int ret = 0;
> +
> +       test_msg("running RAID stripe-tree tests");
> +       for (int i = 0; i < ARRAY_SIZE(tests); i++) {
> +               ret = run_test(tests[i], sectorsize, nodesize);
> +               if (ret)
> +                       goto out;
> +       }
> +
> +out:
> +       return ret;
> +}
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 668138451f7c..6ff64a30349f 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -6022,9 +6022,9 @@ static int find_live_mirror(struct btrfs_fs_info *fs_info,
>         return preferred_mirror;
>  }
>
> -static struct btrfs_io_context *alloc_btrfs_io_context(struct btrfs_fs_info *fs_info,
> -                                                      u64 logical,
> -                                                      u16 total_stripes)
> +EXPORT_FOR_TESTS
> +struct btrfs_io_context *alloc_btrfs_io_context(struct btrfs_fs_info *fs_info,
> +                                               u64 logical, u16 total_stripes)
>  {
>         struct btrfs_io_context *bioc;
>
> diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
> index 26e35fc1c8fd..3ebb3c2732b0 100644
> --- a/fs/btrfs/volumes.h
> +++ b/fs/btrfs/volumes.h
> @@ -840,4 +840,9 @@ bool btrfs_repair_one_zone(struct btrfs_fs_info *fs_info, u64 logical);
>  bool btrfs_pinned_by_swapfile(struct btrfs_fs_info *fs_info, void *ptr);
>  const u8 *btrfs_sb_fsid_ptr(const struct btrfs_super_block *sb);
>
> +#ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS
> +struct btrfs_io_context *alloc_btrfs_io_context(struct btrfs_fs_info *fs_info,
> +                                               u64 logical, u16 total_stripes);
> +#endif
> +
>  #endif
> --
> 2.43.0
>
>