Re: [PATCH 5/8] btrfs: cleanup scrub_rbio

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

On 2022/12/13 21:24, Christoph Hellwig wrote:
> On Tue, Dec 13, 2022 at 04:53:33PM +0800, Qu Wenruo wrote:
>>>      	ret = scrub_assemble_read_bios(rbio, &bio_list);
>>>    	if (ret < 0)
>>> -		goto cleanup;
>>> +		return ret;
>>>      	submit_read_bios(rbio, &bio_list);
>>>    	wait_event(rbio->io_wait, atomic_read(&rbio->stripes_pending) == 0);
>>>      	/* We may have some failures, recover the failed sectors first. */
>>>    	ret = recover_scrub_rbio(rbio);
>>> -	if (ret < 0)
>>> -		goto cleanup;
>>> +	if (ret < 0) {
>>> +		while ((bio = bio_list_pop(&bio_list)))
>>> +			bio_put(bio);
>>> +		return ret;
>>> +	}
>>
>> Do we still need the cleanup? IIRC after submit_read_bios() (or be more
>> safe, after wait_event()), we should no longer touch @bio_list anymore.
> 
> Oh, true.  submit_read_bios does the list_pop, so we don't need
> this here, and in recover_rbio either.  And looking at it a little more
> I think the are could use even more cleanup by:
> 
>   - moving the wait_event for stripes_pending into submit_read_bios.
>   - moving the submit_read_bios *_assemble_read_bios and stop passing
>     the bio_list entirely, which removes all the confusion about
>     who cleans it up.
> 
> What do you think of this series (still needs testing before I can
> post it):
> 
> http://git.infradead.org/users/hch/misc.git/shortlog/refs/heads/btrfs-raid56-cleanups

The code change all looks good, I'd just prefer some naming changes.

One personal thing is, I'd prefer "_wait_" for functions that may wait 
for IOs. Thus submit_read_bios() may be better renamed to 
submit_read_wait_bios().
(bios means it still directly handle a bio list, or just 
submit_read_wait_bio_list()?)

Another changes is, since there is no bio_list out of the 
<work>_read_bios(), the "bios" naming can be removed.
Something like <work>_read_wait() may be a little better.

Thanks,
Qu