Re: [PATCH] btrfs: separate single stripe optimization into a dedicate branch

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

On 2023/1/24 16:43, Johannes Thumshirn wrote:
> On 24.01.23 09:00, Qu Wenruo wrote:
>> +static int patch_single_stripe_for_replace(struct btrfs_fs_info *fs_info,
>> +					   struct btrfs_io_stripe *smap,
>> +					   int mirror_num, int ncopies)
>> +{
>> +	if (mirror_num > ncopies) {
>> +		if (mirror_num == ncopies + 1 &&
>> +		    btrfs_dev_replace_is_ongoing(&fs_info->dev_replace) &&
>> +		    fs_info->dev_replace.srcdev == smap->dev &&
>> +		    fs_info->dev_replace.tgtdev)
>> +			smap->dev = fs_info->dev_replace.tgtdev;
>> +		else
>> +			return -EINVAL;
>> +	}
> 
> If you'd reverse the above if statement and return early, you can save one
> level of indentation.

The problem is, the above one is way too complex.
Yes, we can go if (!(xxxx)), but I 'm not sure if that's any reader 
friendly...

> 
>> +	return 0;
>> +}
>> +
>> +static int map_single_stripe(struct btrfs_fs_info *fs_info,
>> +			     struct btrfs_io_stripe *smap,
>> +			     struct map_lookup *map,
>> +			     struct btrfs_io_geometry *geom,
>> +			     enum btrfs_map_op op,
>> +			     int mirror_num)
>> +{
>> +	enum btrfs_raid_types raid_index = btrfs_bg_flags_to_raid_index(map->type);
>> +	bool is_raid56 = map->type & BTRFS_BLOCK_GROUP_RAID56_MASK;
>> +	int data_stripes = nr_data_stripes(map);
>> +	int ncopies;
>> +	int target;
>> +	int rot;
>> +
>> +	/* For non-RAID56, just select the target stripe.*/
> 
> Why not have a RAID56 function and a non-RAID56 version?

That's a good advice!

Thanks,
Qu
> 
>> +	if (!is_raid56) {
>> +		/*
>> +		 * After BTRFS_STRIPE_LEN bytes, we need to forward @stripe_inc
>> +		 * stripes, and increase physical by (stripe_nr / @stripe_div) *
>> +		 * BTRFS_STRIPE_LEN bytes.
>> +		 *
>> +		 * The default values would handle SINGLE/DUP/RAID1*.
>> +		 * Only need to update to handle RAID0 and RAID10.
>> +		 */
>