On 2018/9/17 下午3:28, Su Yue wrote:
> In check_fs_roots_lowmem(), we do search and follow the resulted path
> to call check_fs_root(), then call btrfs_next_item() to check next
> root.
> However, if repair is enabled, the root tree can be cowed, the
> existed path can cause strange errors.
>
> Solution:
> If repair, save the key before calling check_fs_root,
> search the saved key again before checking next root.
>
> Signed-off-by: Su Yue <suy.fnst@xxxxxxxxxxxxxx>
The idea is pretty valid, however I'm not pretty sure about one practice
below.
> ---
> check/mode-lowmem.c | 22 ++++++++++++++++++++++
> 1 file changed, 22 insertions(+)
>
> diff --git a/check/mode-lowmem.c b/check/mode-lowmem.c
> index 4db12cc7f9fe..db44456fd85b 100644
> --- a/check/mode-lowmem.c
> +++ b/check/mode-lowmem.c
> @@ -4967,9 +4967,13 @@ int check_fs_roots_lowmem(struct btrfs_fs_info *fs_info)
> }
>
> while (1) {
> + struct btrfs_key saved_key;
> +
> node = path.nodes[0];
> slot = path.slots[0];
> btrfs_item_key_to_cpu(node, &key, slot);
> + if (repair)
> + saved_key = key;
Is this OK? Shouldn't it be something like memcpy(&saved_key, &key,
sizeof(key));
Or some new C standard make it work?
Although compiler doesn't give any warning on this.
Thanks,
Qu
> if (key.objectid > BTRFS_LAST_FREE_OBJECTID)
> goto out;
> if (key.type == BTRFS_ROOT_ITEM_KEY &&
> @@ -5000,6 +5004,24 @@ int check_fs_roots_lowmem(struct btrfs_fs_info *fs_info)
> err |= ret;
> }
> next:
> + /*
> + * Since root tree can be cowed during repair,
> + * here search the saved key again.
> + */
> + if (repair) {
> + btrfs_release_path(&path);
> + ret = btrfs_search_slot(NULL, fs_info->tree_root,
> + &saved_key, &path, 0, 0);
> + /* Repair never deletes trees, search must succeed. */
> + if (ret > 0)
> + ret = -ENOENT;
> + if (ret) {
> + error(
> + "search root key[%llu %u %llu] failed after repair: %s",
> + saved_key.objectid, saved_key.type,
> + saved_key.offset, strerror(-ret));
> + }
> + }
> ret = btrfs_next_item(tree_root, &path);
> if (ret > 0)
> goto out;
>
