Re: [PATCH v2 1/3] btrfs: avoid double clean up when submit_one_bio() failed

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

On 2022/4/15 15:14, Qu Wenruo wrote:
>
>
> On 2022/4/15 15:12, Christoph Hellwig wrote:
>> On Fri, Apr 15, 2022 at 03:02:41PM +0800, Qu Wenruo wrote:
>>> But this can not be said to btrfs_submit_compressed_read(), which has
>>> the same problem and can be triggered by EIO error easily.
>>>
>>> Do you want to give it a try? Or mind to me fix it?
>>
>> I don't really know the btrfs writeback and compression code very well,
>> so if you can tackle it that would be great.  I'll review it and will
>> ask lots of stupid question in exchange :)
>
> No stupid questions at all.
>
> Great we have some extra reviewing eyes!
>
> Thanks for your review in advance.
> Qu

OK, it turns out things are better than we thought.

For btrfs_submit_compressed_read() and btrfs_submit_compressed_write(),
we have a different (and correct) way handling the endio.

Let's look at btrfs_submit_compressed_read() as an example.

If functions like btrfs_lookup_bio_sums() failed, although we go
finish_cb: label, our @cur_disk_byte is already updated.

Then under finish_cb: label, we first finish the current @comp_bio,
which may:

1. Decrease cb::pending_bytes and it reached 0.
    Call finish_compressed_bio_read() as we're the last pending bytes.

2. Just decrease cb::pending_bytes
    There are still other pending ios.

For case 1, our @cur_disk_bytnr has already reached our range end, thus
we won't do anything but exit early, without manually calling
finish_compressed_bio_read().

For case 2, we continue to wait_var_event() first, to wait all bios
on-the-fly to finish.
As since the pending_sectors will never reach 0, no one is going to
finish the @cb.

Then we're safe to call finish_compressed_bio_read() then.


In fact, those are exact what I fixed in commit 6853c64a6e76 ("btrfs:
handle errors properly inside btrfs_submit_compressed_write()") and
86ccbb4d2a2a ("btrfs: handle errors properly inside
btrfs_submit_compressed_read()").

In fact, when I saw the overkilled usage of ASSERT()s, I should know
it's myself...

So in fact we're safe since v5.16.

Thanks,
Qu