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

[David Sterba <dsterba@suse.cz>]

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".