Re: [PATCH 3/3] btrfs: Do super block verification before writing it to disk

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3871 bytes --]



On 2018年04月19日 18:16, David Sterba wrote:
> Looks good, some minor comments below. I'm wondering how to test that.

Currently I'm using the most stupid way to test it, insert code randomly
modifies super blocks.

> We'd have to inject either the corruption or to provide a way to
> forcibly fail the test. For the latter a debugfs should do, I'll send
> something for comments.

What about a sysfs interface to modify super blocks?
Since we already have sectorsize and nodessize, allow it to be writeable
for CONFIG_BTRFS_DEBUG looks pretty good.

Thanks,
Qu

> 
> On Thu, Apr 19, 2018 at 05:38:16PM +0800, Qu Wenruo wrote:
>> @@ -3563,6 +3565,12 @@ int write_all_supers(struct btrfs_fs_info *fs_info, int max_mirrors)
>>  	sb = fs_info->super_for_commit;
>>  	dev_item = &sb->dev_item;
>>  
>> +	if (btrfs_check_super_valid(fs_info, sb, -1)) {
> 
> A comment that this is skipping the bytenr check would be good.
> 
>> +		btrfs_err(fs_info,
>> +		"superblock corruption detected before transaction commitment");
> 
>                                                                    commit
> 
> 
>> +		return -EUCLEAN;
>> +	}
>> +
>>  	mutex_lock(&fs_info->fs_devices->device_list_mutex);
>>  	head = &fs_info->fs_devices->devices;
>>  	max_errors = btrfs_super_num_devices(fs_info->super_copy) - 1;
>> @@ -3974,9 +3982,18 @@ int btrfs_read_buffer(struct extent_buffer *buf, u64 parent_transid, int level,
>>  					      level, first_key);
>>  }
>>  
>> -static int btrfs_check_super_valid(struct btrfs_fs_info *fs_info)
>> +/*
>> + * Check the validation of btrfs super block.
>> + *
>> + * @sb:			super block to check
>> + * @super_mirror:	the super block number to check its bytenr.
>> + * 			0 means the primary (1st) sb, 1 and 2 means 2nd and
>> + * 			3rd backup sb, while -1 means to skip bytenr check.
>> + */
>> +static int btrfs_check_super_valid(struct btrfs_fs_info *fs_info,
>> +				   struct btrfs_super_block *sb,
>> +				   int super_mirror)
>>  {
>> -	struct btrfs_super_block *sb = fs_info->super_copy;
>>  	u64 nodesize = btrfs_super_nodesize(sb);
>>  	u64 sectorsize = btrfs_super_sectorsize(sb);
>>  	int ret = 0;
>> @@ -4019,7 +4036,7 @@ static int btrfs_check_super_valid(struct btrfs_fs_info *fs_info)
>>  	 * Check sectorsize and nodesize first, other check will need it.
>>  	 * Check all possible sectorsize(4K, 8K, 16K, 32K, 64K) here.
>>  	 */
>> -	if (!is_power_of_2(sectorsize) || sectorsize < 4096 ||
>> +	if (!is_power_of_2(sectorsize) || sectorsize < SZ_4K ||
> 
> No unrelated changes please. There are some remaining raw values, send a
> separate patch if you want to convert them.
> 
>>  	    sectorsize > BTRFS_MAX_METADATA_BLOCKSIZE) {
>>  		btrfs_err(fs_info, "invalid sectorsize %llu", sectorsize);
>>  		ret = -EINVAL;
>> @@ -4088,9 +4105,10 @@ static int btrfs_check_super_valid(struct btrfs_fs_info *fs_info)
>>  		ret = -EINVAL;
>>  	}
>>  
>> -	if (btrfs_super_bytenr(sb) != BTRFS_SUPER_INFO_OFFSET) {
>> -		btrfs_err(fs_info, "super offset mismatch %llu != %u",
>> -			  btrfs_super_bytenr(sb), BTRFS_SUPER_INFO_OFFSET);
>> +	if (super_mirror >= 0 && btrfs_super_bytenr(sb) !=
>> +	    btrfs_sb_offset(super_mirror)) {
>> +		btrfs_err(fs_info, "super offset mismatch %llu != %llu",
>> +			btrfs_super_bytenr(sb), btrfs_sb_offset(super_mirror));
>>  		ret = -EINVAL;
>>  	}
>>  
>> -- 
>> 2.17.0
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]