Re: [PATCH v3 03/12] btrfs: introduce a new helper to submit write bio for scrub

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

On 2023/3/21 08:14, David Sterba wrote:
> On Mon, Mar 20, 2023 at 10:12:49AM +0800, Qu Wenruo wrote:
>> diff --git a/fs/btrfs/bio.c b/fs/btrfs/bio.c
>> index b96f40160b08..633447b6ba44 100644
>> --- a/fs/btrfs/bio.c
>> +++ b/fs/btrfs/bio.c
>> +	/* Map the RAID56 multi-stripe writes to a single one. */
>> +	if (bioc->map_type & BTRFS_BLOCK_GROUP_RAID56_MASK) {
>> +		int data_stripes = bioc->map_type & BTRFS_BLOCK_GROUP_RAID5 ?
>> +				   bioc->num_stripes - 1 : bioc->num_stripes - 2;
> 
> When ternary operator is used in expression, please put ( ) around it so
> it's clear where it starts.
> 
>> +		int i;
>> +
>> +		/* This special write only works for data stripes. */
>> +		ASSERT(mirror_num == 1);
>> +		for (i = 0; i < data_stripes; i++) {
> 
> 		for (int i = 0; ...)
> 
> We can now use the iterator value defined inside for (), it's relatively
> new due to bumped minimum compiler version so I'd like to see it used
> where possible.

Just one question.

There are quite some for loops in the last few patches.

In that case, should we still go the "for (int i = 0;...)" way?
Or it's better to declare "i" as the old way?

Thanks,
Qu

> 
>> +			u64 stripe_start = bioc->full_stripe_logical +
>> +					   (i << BTRFS_STRIPE_LEN_SHIFT);
>> +
>> +			if (logical >= stripe_start &&
>> +			    logical < stripe_start + BTRFS_STRIPE_LEN)
>> +				break;
>> +		}
>> +		ASSERT(i < data_stripes);
>> +		smap.dev = bioc->stripes[i].dev;
>> +		smap.physical = bioc->stripes[i].physical +
>> +				((logical - bioc->full_stripe_logical) &
>> +				 BTRFS_STRIPE_LEN_MASK);
>> +		goto submit;
>> +	}
>> +	ASSERT(mirror_num <= bioc->num_stripes);
>> +	smap.dev = bioc->stripes[mirror_num - 1].dev;
>> +	smap.physical = bioc->stripes[mirror_num - 1].physical;
>> +submit:
>> +	ASSERT(smap.dev);
>> +	btrfs_put_bioc(bioc);
>> +	bioc = NULL;
>> +	if (dev_replace) {
>> +		ASSERT(smap.dev == fs_info->dev_replace.srcdev);
>> +		smap.dev = fs_info->dev_replace.tgtdev;
>> +	}
>> +	__btrfs_submit_bio(&bbio->bio, bioc, &smap, mirror_num);
>> +	return;
>> +
>> +fail:
>> +	btrfs_bio_counter_dec(fs_info);
>> +	btrfs_bio_end_io(orig_bbio, ret);
>> +}