On 28.08.19 г. 5:33 ч., Qu Wenruo wrote:
> [BUG]
> The following script will cause false alert on devid check.
> #!/bin/bash
>
> dev1=/dev/test/test
> dev2=/dev/test/scratch1
> mnt=/mnt/btrfs
>
> umount $dev1 &> /dev/null
> umount $dev2 &> /dev/null
> umount $mnt &> /dev/null
>
> mkfs.btrfs -f $dev1
>
> mount $dev1 $mnt
>
> _fail()
> {
> echo "!!! FAILED !!!"
> exit 1
> }
>
> for ((i = 0; i < 4096; i++)); do
> btrfs dev add -f $dev2 $mnt || _fail
> btrfs dev del $dev1 $mnt || _fail
> dev_tmp=$dev1
> dev1=$dev2
> dev2=$dev_tmp
> done
Instead of putting the script here, can't it be turned into a fstest to
ensure we don't regress in the future?
>
> [CAUSE]
> Tree-checker uses BTRFS_MAX_DEVS() and BTRFS_MAX_DEVS_SYS_CHUNK() as
> upper limit for devid.
> But we can have devid holes just like above script.
>
> So the check for devid is incorrect and could cause false alert.
>
> [FIX]
> Just remove the whole devid check.
> We don't have any hard requirement for devid assignment.
>
> Furthermore, even devid get corrupted by bitflip, we still have dev
> extents verification at mount time, so corrupted data won't sneak into
> kernel.
>
> Reported-by: Anand Jain <anand.j...@oracle.com>
> Fixes: ab4ba2e13346 ("btrfs: tree-checker: Verify dev item")
> Signed-off-by: Qu Wenruo <w...@suse.com>
> ---
> Changelog:
> v2:
> - Remove devid check completely
> As we already have verify_one_dev_extent().
> v3:
> - Unexport BTRFS_MAX_DEVS() and BTRFS_MAX_DEVS_SYS_CHUNK macros
> - Update commit message to include the reproducer.
> ---
> fs/btrfs/tree-checker.c | 8 --------
> fs/btrfs/volumes.c | 9 +++++++++
> fs/btrfs/volumes.h | 9 ---------
> 3 files changed, 9 insertions(+), 17 deletions(-)
>
> diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c
> index ccd5706199d7..15d1aa7cef1f 100644
> --- a/fs/btrfs/tree-checker.c
> +++ b/fs/btrfs/tree-checker.c
> @@ -686,9 +686,7 @@ static void dev_item_err(const struct extent_buffer *eb,
> int slot,
> static int check_dev_item(struct extent_buffer *leaf,
> struct btrfs_key *key, int slot)
> {
> - struct btrfs_fs_info *fs_info = leaf->fs_info;
> struct btrfs_dev_item *ditem;
> - u64 max_devid = max(BTRFS_MAX_DEVS(fs_info), BTRFS_MAX_DEVS_SYS_CHUNK);
>
> if (key->objectid != BTRFS_DEV_ITEMS_OBJECTID) {
> dev_item_err(leaf, slot,
> @@ -696,12 +694,6 @@ static int check_dev_item(struct extent_buffer *leaf,
> key->objectid, BTRFS_DEV_ITEMS_OBJECTID);
> return -EUCLEAN;
> }
> - if (key->offset > max_devid) {
> - dev_item_err(leaf, slot,
> - "invalid devid: has=%llu expect=[0, %llu]",
> - key->offset, max_devid);
> - return -EUCLEAN;
> - }
> ditem = btrfs_item_ptr(leaf, slot, struct btrfs_dev_item);
> if (btrfs_device_id(leaf, ditem) != key->offset) {
> dev_item_err(leaf, slot,
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 8b72d03738d9..56f751192a6c 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -4901,6 +4901,15 @@ static void check_raid56_incompat_flag(struct
> btrfs_fs_info *info, u64 type)
> btrfs_set_fs_incompat(info, RAID56);
> }
>
> +#define BTRFS_MAX_DEVS(info) ((BTRFS_MAX_ITEM_SIZE(info) \
> + - sizeof(struct btrfs_chunk)) \
> + / sizeof(struct btrfs_stripe) + 1)
> +
> +#define BTRFS_MAX_DEVS_SYS_CHUNK ((BTRFS_SYSTEM_CHUNK_ARRAY_SIZE \
> + - 2 * sizeof(struct btrfs_disk_key) \
> + - 2 * sizeof(struct btrfs_chunk)) \
> + / sizeof(struct btrfs_stripe) + 1)
> +
> static int __btrfs_alloc_chunk(struct btrfs_trans_handle *trans,
> u64 start, u64 type)
> {
> diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
> index 7f6aa1816409..789f983a98c5 100644
> --- a/fs/btrfs/volumes.h
> +++ b/fs/btrfs/volumes.h
> @@ -273,15 +273,6 @@ struct btrfs_fs_devices {
>
> #define BTRFS_BIO_INLINE_CSUM_SIZE 64
>
> -#define BTRFS_MAX_DEVS(info) ((BTRFS_MAX_ITEM_SIZE(info) \
> - - sizeof(struct btrfs_chunk)) \
> - / sizeof(struct btrfs_stripe) + 1)
> -
> -#define BTRFS_MAX_DEVS_SYS_CHUNK ((BTRFS_SYSTEM_CHUNK_ARRAY_SIZE \
> - - 2 * sizeof(struct btrfs_disk_key) \
> - - 2 * sizeof(struct btrfs_chunk)) \
> - / sizeof(struct btrfs_stripe) + 1)
> -
> /*
> * we need the mirror number and stripe index to be passed around
> * the call chain while we are processing end_io (especially errors).
>
