Re: [PATCH 09/10] btrfs: refactor btrfs_map_bio

[Christoph Hellwig <hch@lst.de>]

On Mon, Apr 25, 2022 at 04:56:21PM +0800, Qu Wenruo wrote:
>> +	if (!dev || !dev->bdev ||
>> +	    test_bit(BTRFS_DEV_STATE_MISSING, &dev->dev_state) ||
>> +	    (btrfs_op(orig_bio) == BTRFS_MAP_WRITE &&
>> +	     !test_bit(BTRFS_DEV_STATE_WRITEABLE, &dev->dev_state))) {
>> +		atomic_inc(&bioc->error);
>> +		if (atomic_dec_and_test(&bioc->stripes_pending))
>> +			btrfs_end_bioc(bioc, false);
>
> The bioc is allocated by btrfs_map_block(), but freed inside a helper.
>
> This makes the allocation and free happening at different levels, not sure 
> if it's a good idea.

It is always freed by the end_io handler, this helper just decrements
the pending count and potentially invokes the orig bio end I/O handling
if we never made it to a bio submission for the pending mirror.

> I doubt this fallback would improve the readability.
>
> But you're also right, the original check condition for the RAID56 branch 
> is also not ideal.

I think it helps.  But the next series will do away with this anyway.

>>   	}
>>   -	for (dev_nr = 0; dev_nr < total_devs; dev_nr++) {
>> -		dev = bioc->stripes[dev_nr].dev;
>> -		if (!dev || !dev->bdev || test_bit(BTRFS_DEV_STATE_MISSING,
>> -						   &dev->dev_state) ||
>> -		    (btrfs_op(first_bio) == BTRFS_MAP_WRITE &&
>> -		    !test_bit(BTRFS_DEV_STATE_WRITEABLE, &dev->dev_state))) {
>
> Maybe just make the complex if () condition into a helper?
>
> In fact I see some other locations uses similar complex expressions to 
> check it's a missing device.
>
> Thus it should help a lot of call sites.

I'll see if a helper could be useful here.