Re: [PATCH v3 2/2] btrfs: tree-checker: Fix wrong check on max devid

[WenRuo Qu <wqu@suse.com>]

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@oracle.com>
>> Fixes: ab4ba2e13346 ("btrfs: tree-checker: Verify dev item")
>> Signed-off-by: Qu Wenruo <wqu@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).
>>