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

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

On 2021/8/19 上午7:00, David Sterba wrote:
> On Tue, Aug 17, 2021 at 04:10:43PM +0800, Qu Wenruo wrote:
>> On 2021/8/17 下午3:55, Nikolay Borisov wrote:
>>> On 17.08.21 г. 2:55, Qu Wenruo wrote:
>>>> @@ -665,7 +665,18 @@ blk_status_t btrfs_csum_one_bio(struct btrfs_inode *inode, struct bio *bio,
>>>>
>>>>    		if (!ordered) {
>>>>    			ordered = btrfs_lookup_ordered_extent(inode, offset);
>>>> -			BUG_ON(!ordered); /* Logic error */
>>>> +			/*
>>>> +			 * The bio range is not covered by any ordered extent,
>>>> +			 * must be a code logic error.
>>>> +			 */
>>>> +			if (unlikely(!ordered)) {
>>>> +				WARN(1, KERN_WARNING
>>>> +		"no ordered extent for root %llu ino %llu offset %llu\n",
>>>> +				     inode->root->root_key.objectid,
>>>> +				     btrfs_ino(inode), offset);
>>>> +				kvfree(sums);
>>>> +				return BLK_STS_IOERR;
>>>> +			}
>>>
>>> nit: How about :
>>>
>>> if (WARN_ON(!ordered)  {
>>
>> I still remember that if (WARN_ON()) usage is not recommended by David.
>>
>> Is that still the case?
>
> Quick grep shows there are many if (WARN_ON(...)) so as long as it's a
> simple "if (WARN_ON(condition))" and the code is readable I won't
> object.
>
> The problematic one is "if (!WARN_ON(condition))", because it warns when
> condition is true, but the if does not continue and that breaks the
> reading flow. The acceptable pattern read like "if condition and warn
> eventually and continue".
>

I want to play safe by never bothering the WARN_ON() in if () condition
anymore.

The fact that some if (WARN_ON()) is acceptable and some is not is
already worrying.

Can we just stick to no-WARN_ON-in-if policy?

Thanks,
Qu