On 2021/8/19 上午7:00, David Sterba wrote:

On Tue, Aug 17, 2021 at 04:10:43PM +0800, Qu Wenruo wrote:

quoted

On 2021/8/17 下午3:55, Nikolay Borisov wrote:

quoted

On 17.08.21 г. 2:55, Qu Wenruo wrote:

quoted

@@ -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