Re: [PATCH 2/2] btrfs: replace BUG_ON() in btrfs_csum_one_bio() with proper error handling

[David Sterba <dsterba@suse.cz>]

On Mon, Aug 02, 2021 at 11:49:38AM +0300, Nikolay Borisov wrote:
> 
> 
> On 2.08.21 г. 11:03, Qu Wenruo wrote:
> > 
> > 
> > On 2021/8/2 下午3:53, Nikolay Borisov wrote:
> >>
> >>
> >> On 2.08.21 г. 9:54, Qu Wenruo wrote:
> >>> The BUG_ON() in btrfs_csum_one_bio() means we're trying to submit a bio
> >>> while we don't have ordered extent for it at all.
> >>>
> >>> Normally this won't happen and is indeed a code logical error.
> >>>
> >>> But previous fix has already shown another possibility that, some call
> >>> sites don't handle error properly and submit the write bio after its
> >>> ordered extent has already been cleaned up.
> >>>
> >>> This patch will add an extra safe net by replacing the BUG_ON() to
> >>> proper error handling.
> >>>
> >>> And even if some day we hit a regression that we're submitting bio
> >>> without an ordered extent, we will return error and the pages will be
> >>> marked Error, and being caught properly.
> >>
> >> Would this hamper debugability? I.e it will result in some writes
> >> failing with an error, right?
> > 
> > Yes, it will make such corner case way more silent than before.
> > 
> > But IMHO the existing BUG_ON() is also overkilled.
> > 
> > Maybe converting it to WARN_ON() would be a good middle land?
> 
> If this can occur only due to code bugs I'd prefer to leave it as a
> BUG_ON. Ideally this should only trigger on developer machines when
> testing code changes.

I'd rather see a WARN_ON + the error handling code, a BUG_ON will shoot
down the whole machine. In this case it's probably serious enough but
in the long run we want to get rid of BUG_ONs that can be reasonably
handled.