Re: [PATCh v2 5/9] btrfs: tree-checker: Verify dev item

[Qu Wenruo <wqu@suse.de>]

On 2019/3/20 下午7:51, Johannes Thumshirn wrote:
> On 20/03/2019 07:37, Qu Wenruo wrote:
> [...]
> 
>> +static int check_dev_item(struct btrfs_fs_info *fs_info,
>> +			  struct extent_buffer *leaf,
>> +			  struct btrfs_key *key, int slot)
>> +{
>> +	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(fs_info, leaf, slot,
>> +			     "invalid objectid: has=%llu expect=%llu",
>> +			     key->objectid, BTRFS_DEV_ITEMS_OBJECTID);
>> +		goto error;
>> +	}
>> +	if (key->offset > max_devid) {
>> +		dev_item_err(fs_info, leaf, slot,
>> +			     "invalid devid: has=%llu expect=[0, %llu]",
>> +			     key->offset, max_devid);
>> +		goto error;
>> +	}
>> +	ditem = btrfs_item_ptr(leaf, slot, struct btrfs_dev_item);
>> +	if (btrfs_device_id(leaf, ditem) != key->offset) {
>> +		dev_item_err(fs_info, leaf, slot,
>> +			     "devid mismatch: key has=%llu item has=%llu",
>> +			     key->offset, btrfs_device_id(leaf, ditem));
>> +		goto error;
>> +	}
>> +
>> +	/*
>> +	 * Since btrfs device add doesn't check device size at all, we could
>> +	 * have device item whose size is smaller than 1M which is useless, but
>> +	 * still valid.
>> +	 * So here we can only check the obviously wrong case.
>> +	 */
>> +	if (btrfs_device_total_bytes(leaf, ditem) == 0) {
>> +		dev_item_err(fs_info, leaf, slot,
>> +			     "invalid total bytes: have 0");
>> +		goto error;
>> +	}
>> +	if (btrfs_device_bytes_used(leaf, ditem) >
>> +	    btrfs_device_total_bytes(leaf, ditem)) {
>> +		dev_item_err(fs_info, leaf, slot,
>> +			     "invalid bytes used: have %llu expect [0, %llu]",
>> +			     btrfs_device_bytes_used(leaf, ditem),
>> +			     btrfs_device_total_bytes(leaf, ditem));
>> +		goto error;
>> +	}
>> +	/*
>> +	 * Remaining members like io_align/type/gen/dev_group aren't really
>> +	 * utilized.
>> +	 * Skip them to make later usage of them easier.
>> +	 */
>> +	return 0;
>> +error:
>> +	return -EUCLEAN;
>> +}
>> +
> 
> Why aren't you directly returning -EUCLEAN instead of the gotos? There's
> no cleanup pending so the additional jump label is unnecessary.

Just a coding preference.

Will it impact the performance or compiler is clever enough to change
the goto line to return -EUCLEAN?

Thanks,
Qu