Re: [PATCH v8 03/11] btrfs: add support for inserting raid stripe extents

[Johannes Thumshirn]

On 14.09.23 11:25, Qu Wenruo wrote:
>> +static int btrfs_insert_one_raid_extent(struct btrfs_trans_handle *trans,
>> +				 int num_stripes,
>> +				 struct btrfs_io_context *bioc)
>> +{
>> +	struct btrfs_fs_info *fs_info = trans->fs_info;
>> +	struct btrfs_key stripe_key;
>> +	struct btrfs_root *stripe_root = btrfs_stripe_tree_root(fs_info);
>> +	u8 encoding = btrfs_bg_type_to_raid_encoding(bioc->map_type);
>> +	struct btrfs_stripe_extent *stripe_extent;
>> +	size_t item_size;
>> +	int ret;
>> +
>> +	item_size = struct_size(stripe_extent, strides, num_stripes);
> 
> I guess David has already pointed out this can be done at initialization
> and make it const.

Will do

> 
>> +
>> +	stripe_extent = kzalloc(item_size, GFP_NOFS);
>> +	if (!stripe_extent) {
>> +		btrfs_abort_transaction(trans, -ENOMEM);
>> +		btrfs_end_transaction(trans);
>> +		return -ENOMEM;
>> +	}
>> +
>> +	btrfs_set_stack_stripe_extent_encoding(stripe_extent, encoding);
>> +	for (int i = 0; i < num_stripes; i++) {
>> +		u64 devid = bioc->stripes[i].dev->devid;
>> +		u64 physical = bioc->stripes[i].physical;
>> +		u64 length = bioc->stripes[i].length;
>> +		struct btrfs_raid_stride *raid_stride =
>> +						&stripe_extent->strides[i];
>> +
>> +		if (length == 0)
>> +			length = bioc->size;
>> +
>> +		btrfs_set_stack_raid_stride_devid(raid_stride, devid);
>> +		btrfs_set_stack_raid_stride_physical(raid_stride, physical);
>> +		btrfs_set_stack_raid_stride_length(raid_stride, length);
>> +	}
>> +
>> +	stripe_key.objectid = bioc->logical;
>> +	stripe_key.type = BTRFS_RAID_STRIPE_KEY;
>> +	stripe_key.offset = bioc->size;
>> +
>> +	ret = btrfs_insert_item(trans, stripe_root, &stripe_key, stripe_extent,
>> +				item_size);
> 
> Have you tested in near-real-world on how continous the RST items could
> be for RAID0/RAID10?
> 
> My concern here is, we may want to try our best to reduce the size of
> RST, due to the 64K BTRFS_STRIPE_LEN.
> 

There are two things I can do for it. First is trying to merge contiguus 
RST items and second make BTRFS_STRIPE_LEN a mkfs time constant instead 
of a compile time constant.

But both can be done in a second step.

>> +	switch (map_type & BTRFS_BLOCK_GROUP_PROFILE_MASK) {
>> +	case BTRFS_BLOCK_GROUP_DUP:
>> +	case BTRFS_BLOCK_GROUP_RAID1:
>> +	case BTRFS_BLOCK_GROUP_RAID1C3:
>> +	case BTRFS_BLOCK_GROUP_RAID1C4:
>> +		ret = btrfs_insert_mirrored_raid_extents(trans, ordered_extent,
>> +							 map_type);
>> +		break;
>> +	case BTRFS_BLOCK_GROUP_RAID0:
>> +		ret = btrfs_insert_striped_raid_extents(trans, ordered_extent,
>> +							map_type);
>> +		break;
>> +	case BTRFS_BLOCK_GROUP_RAID10:
>> +		ret = btrfs_insert_striped_mirrored_raid_extents(trans, ordered_extent, map_type);
>> +		break;
>> +	default:
>> +		ret = -EINVAL;
> 
> Maybe we want to be a little more noisy?

OK.