Re: [PATCH v3 3/9] btrfs: hunt down the BUG_ON()s inside btrfs_submit_compressed_read()

[Qu Wenruo <wqu@suse.com>]

On 2021/6/16 下午10:03, David Sterba wrote:
> On Tue, Jun 15, 2021 at 08:18:30PM +0800, Qu Wenruo wrote:
>> There are quite some BUG_ON()s inside btrfs_submit_compressed_read(),
>> namingly all errors inside the for() loop relies on BUG_ON() to handle
>> -ENOMEM.
>>
>> Hunt down these BUG_ON()s properly by:
>>
>> - Introduce compressed_bio::pending_bios_wait
>>    This allows us to wait for any submitted bio to finish, while still
>>    keeps the compressed_bio from being freed, as we should have
>>    compressed_bio::io_sectors not zero.
>>
>> - Introduce finish_compressed_bio_read() to finish the compressed_bio
>>
>> - Properly end the bio and finish compressed_bio when error happens
>>
>> Now in btrfs_submit_compressed_read() even when the bio submission
>> failed, we can properly handle the error without triggering BUG_ON().
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
> 
> Please change the subject to something like "btrfs: do proper error
> handling in btrfs_submit_compressed_read", same for the other patch.

Got it, will change the naming scheme for future patches too.
> 
[...]
>>   
>> +	/* To wait for any submitted bio, used in error handling */
>> +	wait_queue_head_t pending_bio_wait;
> 
> This adds 24 bytes to the structure and it's only used for error
> handling, so that does not seem justified enough.

And after more consideration, in fact we can just remove 
compressed_bio::pending_bios completely, using io_sectors to replace it.

For error handling case, finally we will have things like 
@cur_disk_bytenr to trace our submitted compressed_bio progress, and we 
can use io_sectors to wait.

I'll go this direction in next update so that no size bump at all for 
compressed_bio structure.

Thanks,
Qu

> 
> There are system-wide wait queues, shared with other subsystems but it
> looks like a better fit for the exceptional case of errors. See commit
> 6b2bb7265f0b62605 for more details, the change is otherwise trivial and
> the api functions are wait_var_event(&variable, condition) and
> wake_up_var(&variable), where the variable is a unique key which would
> be the compressed_bio.
> 
>> +
>>   	/* Number of compressed pages in the array */
>>   	unsigned int nr_pages;
>>   
>> -- 
>> 2.32.0
>