On 2019/8/28 下午2:27, Nikolay Borisov wrote:
>
>
> 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?
Sure, already crafting.
Thanks,
Qu
>
>>
>> [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.jain@xxxxxxxxxx>
>> Fixes: ab4ba2e13346 ("btrfs: tree-checker: Verify dev item")
>> Signed-off-by: Qu Wenruo <wqu@xxxxxxxx>
>> ---
>> 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).
>>
