[PATCH 0/5] Assertion and debugging helpers

[PATCH 0/5] Assertion and debugging helpers

[David Sterba]

Hi,

this is a RFC series. We need to improve debugging and logging helpers
so there's no ASSERT(0) or the convoluted
WARN_ON(IS_DEFINED(CONFIG_BTRFS_DEBUG)). This was mentioned in past
discussions so here's my proposal.

The series is only build tested, I'd like to hear some feedback if this
is going the right direction or if there are suggestions for fine
tuning.

1) Add verbose ASSERT macro, so we can print additional information when
it triggers, namely printing the values of the assertion expression.
More details in the first patch, basic pattern is something like

    VASSERT(value > limit, "value=%llu limit=%llu", value, limit);

The second patch shows it's application in volumes.c, converting about
half where it's relevant. There are about 800 assertions in fs/btrfs/
and we don't need to convert them all. This can be done incrementally
and as needed.

The verbose version is another macro, although with some preprocessor
magic it should be possible to make ASSERT take variable number of
arguments. Does not seem worth though.

2) Wrap WARN_ON(IS_DEFINED(CONFIG_BTRFS_DEBUG)) to DEBUG_WARN() with
optional message with printk format. This is used to replace the
WARN_ON(...) above and also the ASSERT(0).

The ultimate goal for me is to get rid of all ASSERT(0), it's not used
consistently and looks like it's a note to the code author. There may be
several reasons for it's use and although I've converted almost all to
DEBUG_WARN it may miss the intentions.

In some cases it may be better to add proper error handling, print a
message or warn and exit with error. Possibly the are cases where the
code cannot continue, meaning it should be a BUG_ON but this is also
something we want to convert to proper error handling.

David Sterba (5):
  btrfs: add verbose version of ASSERT
  btrfs: example use of VASSERT() in volumes.c
  btrfs: add debug build only WARN
  btrfs: convert WARN_ON(IS_ENABLED(CONFIG_BTRFS_DEBUG)) to DEBUG_WARN
  btrfs: convert ASSERT(0) to DEBUG_WARN()

 fs/btrfs/backref.c         |  4 +--
 fs/btrfs/delayed-ref.c     |  2 +-
 fs/btrfs/dev-replace.c     |  2 +-
 fs/btrfs/disk-io.c         |  2 +-
 fs/btrfs/extent-tree.c     |  2 +-
 fs/btrfs/extent_io.c       |  4 +--
 fs/btrfs/extent_map.c      |  2 +-
 fs/btrfs/free-space-tree.c | 27 +++++++++-------
 fs/btrfs/inode.c           |  6 ++--
 fs/btrfs/messages.h        | 30 ++++++++++++++++++
 fs/btrfs/qgroup.c          |  6 ++--
 fs/btrfs/relocation.c      |  4 +--
 fs/btrfs/send.c            |  4 +--
 fs/btrfs/space-info.c      |  2 +-
 fs/btrfs/tree-checker.c    |  8 ++---
 fs/btrfs/volumes.c         | 64 ++++++++++++++++++++++++--------------
 fs/btrfs/zoned.c           |  2 +-
 17 files changed, 109 insertions(+), 62 deletions(-)

-- 
2.49.0

[PATCH 1/5] btrfs: add verbose version of ASSERT

[David Sterba]

Currently ASSERT prints the stringified condition and without macro
expansions so simple constants like BTRFS_MAX_METADATA_BLOCKSIZE remain
readable in the output.

There are expressions where we'd like to see the exact values but all we
get is someting like:

  assertion failed: em->start <= start && start < extent_map_end(em), in fs/btrfs/extent_map.c:613

It would be nice to be able to print any additional information to help
understand the problem. For that there's now VASSERT and can be used
like:

  VASSERT(value > limit, "value=%llu limit=%llu", value, limit);

with free-form printk arguments that will be part of the assertion
message.

Pros:
- helps debugging and understanding reported problems

Cons:
- increases the .ko size
- writing the message is repetitive (condition, format, values)
- format and variable type must match (extra lookup)

Recommended use is for non-trivial expressions, so basic ASSERT(value) can be
used for pointers or sometimes integers.

The helper btrfs_assertfail_verbose() is inlined because it generates a
bit better assembly (avoids a function call).

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/messages.h | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/fs/btrfs/messages.h b/fs/btrfs/messages.h
index 08a9272399d26f..8cca9d834b274d 100644
--- a/fs/btrfs/messages.h
+++ b/fs/btrfs/messages.h
@@ -175,10 +175,33 @@ do {								\
 	BUG();								\
 })
 
+__printf(4, 5)
+__cold
+static inline void btrfs_assertfail_verbose(const char *str_expr,
+					    const char *file, int line,
+					    const char *fmt, ...) {
+	struct va_format vaf;
+	va_list args;
+
+	va_start(args, fmt);
+	vaf.fmt = fmt;
+	vaf.va = &args;
+
+	pr_err("assertion failed: %s, in %s:%d (%pV)\n", str_expr, file, line, &vaf);
+	va_end(args);
+	BUG();
+}
+
 #define ASSERT(expr)						\
 	(likely(expr) ? (void)0 : btrfs_assertfail(#expr, __FILE__, __LINE__))
+
+/* Verbose assert, use to print any relevant values of the condition. */
+#define VASSERT(expr, fmt, ...)					\
+	(likely(expr) ? (void)0 : btrfs_assertfail_verbose(#expr, __FILE__, __LINE__, \
+							   fmt, __VA_ARGS__))
 #else
 #define ASSERT(expr)	(void)(expr)
+#define VASSERT(expr, fmt, ...)		(void)(expr)
 #endif
 
 __printf(5, 6)
-- 
2.49.0

[PATCH 2/5] btrfs: example use of VASSERT() in volumes.c

[David Sterba]

The file volumes.c has about 40 assertions and half of them are suitable
for VASSERT.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/volumes.c | 57 ++++++++++++++++++++++++++++++----------------
 1 file changed, 37 insertions(+), 20 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 4d5c59083003ff..e8b944aeb2e189 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1640,7 +1640,8 @@ static bool dev_extent_hole_check_zoned(struct btrfs_device *device,
 	int ret;
 	bool changed = false;
 
-	ASSERT(IS_ALIGNED(*hole_start, zone_size));
+	VASSERT(IS_ALIGNED(*hole_start, zone_size),
+		"hole_start=%llu zone_size=%llu", *hole_start, zone_size);
 
 	while (*hole_size > 0) {
 		pos = btrfs_find_allocatable_zones(device, *hole_start,
@@ -1891,7 +1892,9 @@ static int find_free_dev_extent(struct btrfs_device *device, u64 num_bytes,
 	else
 		ret = 0;
 
-	ASSERT(max_hole_start + max_hole_size <= search_end);
+	VASSERT(max_hole_start + max_hole_size <= search_end,
+		"max_hole_start=%llu max_hole_size=%llu search_end=%llu",
+		max_hole_start, max_hole_size, search_end);
 out:
 	btrfs_free_path(path);
 	*start = max_hole_start;
@@ -2204,7 +2207,7 @@ static u64 btrfs_num_devices(struct btrfs_fs_info *fs_info)
 
 	down_read(&fs_info->dev_replace.rwsem);
 	if (btrfs_dev_replace_is_ongoing(&fs_info->dev_replace)) {
-		ASSERT(num_devices > 1);
+		VASSERT(num_devices > 1, "num_devices=%llu", num_devices);
 		num_devices--;
 	}
 	up_read(&fs_info->dev_replace.rwsem);
@@ -2408,7 +2411,8 @@ int btrfs_rm_device(struct btrfs_fs_info *fs_info,
 	 */
 	if (cur_devices->num_devices == 0) {
 		list_del_init(&cur_devices->seed_list);
-		ASSERT(cur_devices->opened == 1);
+		VASSERT(cur_devices->opened == 1,
+			"opened=%d", cur_devices->opened);
 		cur_devices->opened--;
 		free_fs_devices(cur_devices);
 	}
@@ -4753,7 +4757,8 @@ int btrfs_resume_balance_async(struct btrfs_fs_info *fs_info)
 	}
 
 	spin_lock(&fs_info->super_lock);
-	ASSERT(fs_info->exclusive_operation == BTRFS_EXCLOP_BALANCE_PAUSED);
+	VASSERT(fs_info->exclusive_operation == BTRFS_EXCLOP_BALANCE_PAUSED,
+		"exclusive_operation=%d", fs_info->exclusive_operation);
 	fs_info->exclusive_operation = BTRFS_EXCLOP_BALANCE;
 	spin_unlock(&fs_info->super_lock);
 	/*
@@ -5436,7 +5441,9 @@ static int decide_stripe_size_zoned(struct alloc_chunk_ctl *ctl,
 	 * It should hold because:
 	 *    dev_extent_min == dev_extent_want == zone_size * dev_stripes
 	 */
-	ASSERT(devices_info[ctl->ndevs - 1].max_avail == ctl->dev_extent_min);
+	VASSERT(devices_info[ctl->ndevs - 1].max_avail == ctl->dev_extent_min,
+		"ndevs=%d max_avail=%llu dev_extent_min=%llu", ctl->ndevs,
+		devices_info[ctl->ndevs - 1].max_avail, ctl->dev_extent_min);
 
 	ctl->stripe_size = zone_size;
 	ctl->num_stripes = ctl->ndevs * ctl->dev_stripes;
@@ -5449,7 +5456,9 @@ static int decide_stripe_size_zoned(struct alloc_chunk_ctl *ctl,
 				     ctl->dev_stripes);
 		ctl->num_stripes = ctl->ndevs * ctl->dev_stripes;
 		data_stripes = (ctl->num_stripes - ctl->nparity) / ctl->ncopies;
-		ASSERT(ctl->stripe_size * data_stripes <= ctl->max_chunk_size);
+		VASSERT(ctl->stripe_size * data_stripes <= ctl->max_chunk_size,
+			"stripe_size=%llu data_stripes=%d max_chunk_size=%llu",
+			ctl->stripe_size, data_stripes, ctl->max_chunk_size);
 	}
 
 	ctl->chunk_size = ctl->stripe_size * data_stripes;
@@ -6055,8 +6064,8 @@ static int find_live_mirror(struct btrfs_fs_info *fs_info,
 	int tolerance;
 	struct btrfs_device *srcdev;
 
-	ASSERT((map->type &
-		 (BTRFS_BLOCK_GROUP_RAID1_MASK | BTRFS_BLOCK_GROUP_RAID10)));
+	VASSERT((map->type & (BTRFS_BLOCK_GROUP_RAID1_MASK | BTRFS_BLOCK_GROUP_RAID10)),
+		"type=%llu", map->type);
 
 	if (map->type & BTRFS_BLOCK_GROUP_RAID10)
 		num_stripes = map->sub_stripes;
@@ -6357,7 +6366,7 @@ static void handle_ops_on_dev_replace(struct btrfs_io_context *bioc,
 	}
 
 	/* We can only have at most 2 extra nr_stripes (for DUP). */
-	ASSERT(nr_extra_stripes <= 2);
+	VASSERT(nr_extra_stripes <= 2, "nr_extra_stripes=%d", nr_extra_stripes);
 	/*
 	 * For GET_READ_MIRRORS, we can only return at most 1 extra stripe for
 	 * replace.
@@ -6368,7 +6377,8 @@ static void handle_ops_on_dev_replace(struct btrfs_io_context *bioc,
 		struct btrfs_io_stripe *second = &bioc->stripes[num_stripes + 1];
 
 		/* Only DUP can have two extra stripes. */
-		ASSERT(bioc->map_type & BTRFS_BLOCK_GROUP_DUP);
+		VASSERT(bioc->map_type & BTRFS_BLOCK_GROUP_DUP,
+			"map_type=%llu", bioc->map_type);
 
 		/*
 		 * Swap the last stripe stripes and reduce @nr_extra_stripes.
@@ -6395,7 +6405,8 @@ static u64 btrfs_max_io_len(struct btrfs_chunk_map *map, u64 offset,
 	 */
 	io_geom->stripe_offset = offset & BTRFS_STRIPE_LEN_MASK;
 	io_geom->stripe_nr = offset >> BTRFS_STRIPE_LEN_SHIFT;
-	ASSERT(io_geom->stripe_offset < U32_MAX);
+	VASSERT(io_geom->stripe_offset < U32_MAX,
+		"stripe_offset=%llu", io_geom->stripe_offset);
 
 	if (map->type & BTRFS_BLOCK_GROUP_RAID56_MASK) {
 		unsigned long full_stripe_len =
@@ -6413,8 +6424,12 @@ static u64 btrfs_max_io_len(struct btrfs_chunk_map *map, u64 offset,
 		io_geom->raid56_full_stripe_start = btrfs_stripe_nr_to_offset(
 			rounddown(io_geom->stripe_nr, nr_data_stripes(map)));
 
-		ASSERT(io_geom->raid56_full_stripe_start + full_stripe_len > offset);
-		ASSERT(io_geom->raid56_full_stripe_start <= offset);
+		VASSERT(io_geom->raid56_full_stripe_start + full_stripe_len > offset,
+			"raid56_full_stripe_start=%llu full_stripe_len=%lu offset=%llu",
+			io_geom->raid56_full_stripe_start, full_stripe_len, offset);
+		VASSERT(io_geom->raid56_full_stripe_start <= offset,
+			"raid56_full_stripe_start=%llu offset=%llu",
+			io_geom->raid56_full_stripe_start, offset);
 		/*
 		 * For writes to RAID56, allow to write a full stripe set, but
 		 * no straddling of stripe sets.
@@ -6580,7 +6595,7 @@ static void map_blocks_raid56_read(struct btrfs_chunk_map *map,
 {
 	int data_stripes = nr_data_stripes(map);
 
-	ASSERT(io_geom->mirror_num <= 1);
+	VASSERT(io_geom->mirror_num <= 1, "mirror_num=%d", io_geom->mirror_num);
 	/* Just grab the data stripe directly. */
 	io_geom->stripe_index = io_geom->stripe_nr % data_stripes;
 	io_geom->stripe_nr /= data_stripes;
@@ -7925,7 +7940,7 @@ void btrfs_commit_device_sizes(struct btrfs_transaction *trans)
 {
 	struct btrfs_device *curr, *next;
 
-	ASSERT(trans->state == TRANS_STATE_COMMIT_DOING);
+	VASSERT(trans->state == TRANS_STATE_COMMIT_DOING, "state=%d" , trans->state);
 
 	if (list_empty(&trans->dev_update_list))
 		return;
@@ -8299,7 +8314,7 @@ static void map_raid56_repair_block(struct btrfs_io_context *bioc,
 		    logical < stripe_start + BTRFS_STRIPE_LEN)
 			break;
 	}
-	ASSERT(i < data_stripes);
+	VASSERT(i < data_stripes, "i=%d data_stripes=%d", i, data_stripes);
 	smap->dev = bioc->stripes[i].dev;
 	smap->physical = bioc->stripes[i].physical +
 			((logical - bioc->full_stripe_logical) &
@@ -8328,7 +8343,7 @@ int btrfs_map_repair_block(struct btrfs_fs_info *fs_info,
 	int mirror_ret = mirror_num;
 	int ret;
 
-	ASSERT(mirror_num > 0);
+	VASSERT(mirror_num > 0, "mirror_num=%d", mirror_num);
 
 	ret = btrfs_map_block(fs_info, BTRFS_MAP_WRITE, logical, &map_length,
 			      &bioc, smap, &mirror_ret);
@@ -8336,7 +8351,8 @@ int btrfs_map_repair_block(struct btrfs_fs_info *fs_info,
 		return ret;
 
 	/* The map range should not cross stripe boundary. */
-	ASSERT(map_length >= length);
+	VASSERT(map_length >= length,
+		"map_length=%llu length=%u", map_length, length);
 
 	/* Already mapped to single stripe. */
 	if (!bioc)
@@ -8348,7 +8364,8 @@ int btrfs_map_repair_block(struct btrfs_fs_info *fs_info,
 		goto out;
 	}
 
-	ASSERT(mirror_num <= bioc->num_stripes);
+	VASSERT(mirror_num <= bioc->num_stripes,
+		"mirror_num=%d num_stripes=%d", mirror_num,  bioc->num_stripes);
 	smap->dev = bioc->stripes[mirror_num - 1].dev;
 	smap->physical = bioc->stripes[mirror_num - 1].physical;
 out:
-- 
2.49.0

[PATCH 3/5] btrfs: add debug build only WARN

[David Sterba]

Add conditional WARN() wrapper that's enabled only in debug build. It
should be used for unexpected condtions that should be noisy.  Use it
instead of ASSERT(0). As it will not lead to BUG() make sure that
continuing is still possible, e.g. the error is handled anyway.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/messages.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/fs/btrfs/messages.h b/fs/btrfs/messages.h
index 8cca9d834b274d..31f9a2597a1c96 100644
--- a/fs/btrfs/messages.h
+++ b/fs/btrfs/messages.h
@@ -204,6 +204,13 @@ static inline void btrfs_assertfail_verbose(const char *str_expr,
 #define VASSERT(expr, fmt, ...)		(void)(expr)
 #endif
 
+#ifdef CONFIG_BTRFS_DEBUG
+/* Verbose warning only under debug build. */
+#define DEBUG_WARN(args...)	WARN(1, KERN_ERR args)
+#else
+#define DEBUG_WARN(...)		do {} while(0)
+#endif
+
 __printf(5, 6)
 __cold
 void __btrfs_handle_fs_error(struct btrfs_fs_info *fs_info, const char *function,
-- 
2.49.0

[PATCH 4/5] btrfs: convert WARN_ON(IS_ENABLED(CONFIG_BTRFS_DEBUG)) to DEBUG_WARN

[David Sterba]

Use the conditional warning instead of typing the whole condition.
Optional message is printed where it seems clear what could be the
problem.

Conversion is left out in btree_csum_one_bio() because of the additional
condition.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/backref.c      | 2 +-
 fs/btrfs/disk-io.c      | 2 +-
 fs/btrfs/extent-tree.c  | 2 +-
 fs/btrfs/extent_io.c    | 2 +-
 fs/btrfs/inode.c        | 6 ++----
 fs/btrfs/qgroup.c       | 6 +++---
 fs/btrfs/tree-checker.c | 8 +++-----
 7 files changed, 12 insertions(+), 16 deletions(-)

diff --git a/fs/btrfs/backref.c b/fs/btrfs/backref.c
index 5936cff80ff3d3..e76e1845cfce14 100644
--- a/fs/btrfs/backref.c
+++ b/fs/btrfs/backref.c
@@ -2877,7 +2877,7 @@ int btrfs_backref_iter_start(struct btrfs_backref_iter *iter, u64 bytenr)
 		goto release;
 	}
 	if (path->slots[0] == 0) {
-		WARN_ON(IS_ENABLED(CONFIG_BTRFS_DEBUG));
+		DEBUG_WARN();
 		ret = -EUCLEAN;
 		goto release;
 	}
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 59da809b7d57ab..041d5477647639 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -4435,7 +4435,7 @@ void __cold close_ctree(struct btrfs_fs_info *fs_info)
 	set_bit(BTRFS_FS_CLOSING_DONE, &fs_info->flags);
 
 	if (btrfs_check_quota_leak(fs_info)) {
-		WARN_ON(IS_ENABLED(CONFIG_BTRFS_DEBUG));
+		DEBUG_WARN("qgroup reserved space leaked");
 		btrfs_err(fs_info, "qgroup reserved space leaked");
 	}
 
diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index 8e3c6739d45e82..ad3a891874440b 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -6425,7 +6425,7 @@ static int btrfs_trim_free_extents(struct btrfs_device *device, u64 *trimmed,
 
 		/* Check if there are any CHUNK_* bits left */
 		if (start > device->total_bytes) {
-			WARN_ON(IS_ENABLED(CONFIG_BTRFS_DEBUG));
+			DEBUG_WARN();
 			btrfs_warn_in_rcu(fs_info,
 "ignoring attempt to trim beyond device size: offset %llu length %llu device %s device size %llu",
 					  start, end - start + 1,
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index c5aba14ee53dc0..fcd830a0bce20c 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -3681,7 +3681,7 @@ static bool report_eb_range(const struct extent_buffer *eb, unsigned long start,
 	btrfs_warn(eb->fs_info,
 		"access to eb bytenr %llu len %u out of range start %lu len %lu",
 		eb->start, eb->len, start, len);
-	WARN_ON(IS_ENABLED(CONFIG_BTRFS_DEBUG));
+	DEBUG_WARN();
 
 	return true;
 }
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index b75d4a035da024..17db9a870b37db 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -777,9 +777,7 @@ static inline int inode_need_compress(struct btrfs_inode *inode, u64 start,
 	struct btrfs_fs_info *fs_info = inode->root->fs_info;
 
 	if (!btrfs_inode_can_compress(inode)) {
-		WARN(IS_ENABLED(CONFIG_BTRFS_DEBUG),
-			KERN_ERR "BTRFS: unexpected compression for ino %llu\n",
-			btrfs_ino(inode));
+		DEBUG_WARN("BTRFS: unexpected compression for ino %llu", btrfs_ino(inode));
 		return 0;
 	}
 
@@ -2873,7 +2871,7 @@ int btrfs_writepage_cow_fixup(struct folio *folio)
 	 * We should not hit such out-of-band dirty folios anymore.
 	 */
 	if (IS_ENABLED(CONFIG_BTRFS_EXPERIMENTAL)) {
-		WARN_ON(IS_ENABLED(CONFIG_BTRFS_DEBUG));
+		DEBUG_WARN();
 		btrfs_err_rl(fs_info,
 	"root %lld ino %llu folio %llu is marked dirty without notifying the fs",
 			     BTRFS_I(inode)->root->root_key.objectid,
diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index 944c207cca87ff..b3176edbde82a0 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -1823,7 +1823,7 @@ int btrfs_remove_qgroup(struct btrfs_trans_handle *trans, u64 qgroupid)
 	if (qgroup->rsv.values[BTRFS_QGROUP_RSV_DATA] ||
 	    qgroup->rsv.values[BTRFS_QGROUP_RSV_META_PREALLOC] ||
 	    qgroup->rsv.values[BTRFS_QGROUP_RSV_META_PERTRANS]) {
-		WARN_ON(IS_ENABLED(CONFIG_BTRFS_DEBUG));
+		DEBUG_WARN();
 		btrfs_warn_rl(fs_info,
 "to be deleted qgroup %u/%llu has non-zero numbers, data %llu meta prealloc %llu meta pertrans %llu",
 			      btrfs_qgroup_level(qgroup->qgroupid),
@@ -1843,7 +1843,7 @@ int btrfs_remove_qgroup(struct btrfs_trans_handle *trans, u64 qgroupid)
 	    !(fs_info->qgroup_flags & BTRFS_QGROUP_STATUS_FLAG_INCONSISTENT)) {
 		if (qgroup->rfer || qgroup->excl ||
 		    qgroup->rfer_cmpr || qgroup->excl_cmpr) {
-			WARN_ON(IS_ENABLED(CONFIG_BTRFS_DEBUG));
+			DEBUG_WARN();
 			btrfs_warn_rl(fs_info,
 "to be deleted qgroup %u/%llu has non-zero numbers, rfer %llu rfer_cmpr %llu excl %llu excl_cmpr %llu",
 				      btrfs_qgroup_level(qgroup->qgroupid),
@@ -4767,7 +4767,7 @@ int btrfs_qgroup_add_swapped_blocks(struct btrfs_root *subvol_root,
 				 * Marking qgroup inconsistent should be enough
 				 * for end users.
 				 */
-				WARN_ON(IS_ENABLED(CONFIG_BTRFS_DEBUG));
+				DEBUG_WARN("duplicated but mismatched entry found");
 				ret = -EEXIST;
 			}
 			kfree(block);
diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c
index 6ca3c09514e7b5..8f4703b488b71d 100644
--- a/fs/btrfs/tree-checker.c
+++ b/fs/btrfs/tree-checker.c
@@ -2230,8 +2230,7 @@ int btrfs_verify_level_key(struct extent_buffer *eb,
 
 	found_level = btrfs_header_level(eb);
 	if (unlikely(found_level != check->level)) {
-		WARN(IS_ENABLED(CONFIG_BTRFS_DEBUG),
-		     KERN_ERR "BTRFS: tree level check failed\n");
+		DEBUG_WARN();
 		btrfs_err(fs_info,
 "tree level mismatch detected, bytenr=%llu level expected=%u has=%u",
 			  eb->start, check->level, found_level);
@@ -2255,7 +2254,7 @@ int btrfs_verify_level_key(struct extent_buffer *eb,
 		btrfs_err(fs_info,
 		"invalid tree nritems, bytenr=%llu nritems=0 expect >0",
 			  eb->start);
-		WARN_ON(IS_ENABLED(CONFIG_BTRFS_DEBUG));
+		DEBUG_WARN();
 		return -EUCLEAN;
 	}
 
@@ -2266,8 +2265,7 @@ int btrfs_verify_level_key(struct extent_buffer *eb,
 
 	ret = btrfs_comp_cpu_keys(&check->first_key, &found_key);
 	if (unlikely(ret)) {
-		WARN(IS_ENABLED(CONFIG_BTRFS_DEBUG),
-		     KERN_ERR "BTRFS: tree first key check failed\n");
+		DEBUG_WARN();
 		btrfs_err(fs_info,
 "tree first key mismatch detected, bytenr=%llu parent_transid=%llu key expected=(%llu,%u,%llu) has=(%llu,%u,%llu)",
 			  eb->start, check->transid, check->first_key.objectid,
-- 
2.49.0

[PATCH 5/5] btrfs: convert ASSERT(0) to DEBUG_WARN()

[David Sterba]

The use of ASSERT(0) is maybe useful for some cases but more like a
notice for developers. Assertions can be compiled in independently so
convert it to a debugging helper.

The difference is that it's just a warning and will not end up in BUG().
All the cases need a review and possibly be modified:

- delete it completely if the purpose is not clear
- replace/update by proper error handling
- replace by verbose error and BUG()/transaction abort if continuation
  is not possible at all
- use DEBUG_WARN()

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/backref.c         |  2 +-
 fs/btrfs/delayed-ref.c     |  2 +-
 fs/btrfs/dev-replace.c     |  2 +-
 fs/btrfs/extent_io.c       |  2 +-
 fs/btrfs/extent_map.c      |  2 +-
 fs/btrfs/free-space-tree.c | 27 +++++++++++++++------------
 fs/btrfs/relocation.c      |  4 ++--
 fs/btrfs/send.c            |  4 ++--
 fs/btrfs/space-info.c      |  2 +-
 fs/btrfs/volumes.c         |  7 ++++---
 fs/btrfs/zoned.c           |  2 +-
 11 files changed, 30 insertions(+), 26 deletions(-)

diff --git a/fs/btrfs/backref.c b/fs/btrfs/backref.c
index e76e1845cfce14..6ee0259048cd3e 100644
--- a/fs/btrfs/backref.c
+++ b/fs/btrfs/backref.c
@@ -3617,7 +3617,7 @@ int btrfs_backref_finish_upper_links(struct btrfs_backref_cache *cache,
 
 		/* Sanity check, we shouldn't have any unchecked nodes */
 		if (!upper->checked) {
-			ASSERT(0);
+			DEBUG_WARN("we should not have any unchecked nodes");
 			return -EUCLEAN;
 		}
 
diff --git a/fs/btrfs/delayed-ref.c b/fs/btrfs/delayed-ref.c
index 739c9e29aaa389..2009eb7cce334c 100644
--- a/fs/btrfs/delayed-ref.c
+++ b/fs/btrfs/delayed-ref.c
@@ -613,7 +613,7 @@ static bool insert_delayed_ref(struct btrfs_trans_handle *trans,
 				ASSERT(!list_empty(&exist->add_list));
 				list_del_init(&exist->add_list);
 			} else {
-				ASSERT(0);
+				DEBUG_WARN("unknown ref->action=%d", ref->action);
 			}
 		} else
 			mod = -ref->ref_mod;
diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
index 483e71e09181ff..5c26f0fcf0d500 100644
--- a/fs/btrfs/dev-replace.c
+++ b/fs/btrfs/dev-replace.c
@@ -637,7 +637,7 @@ static int btrfs_dev_replace_start(struct btrfs_fs_info *fs_info,
 		break;
 	case BTRFS_IOCTL_DEV_REPLACE_STATE_STARTED:
 	case BTRFS_IOCTL_DEV_REPLACE_STATE_SUSPENDED:
-		ASSERT(0);
+		DEBUG_WARN("unexpected STARTED ot SUSPENDED dev-replace state");
 		ret = BTRFS_IOCTL_DEV_REPLACE_RESULT_ALREADY_STARTED;
 		up_write(&dev_replace->rwsem);
 		goto leave;
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index fcd830a0bce20c..03d9bc1fcb51fd 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -3258,7 +3258,7 @@ struct extent_buffer *alloc_extent_buffer(struct btrfs_fs_info *fs_info,
 		 * using 0-order folios.
 		 */
 		if (unlikely(ret == -EAGAIN)) {
-			ASSERT(0);
+			DEBUG_WARN("folio order mismatch between new eb and filemap");
 			goto reallocate;
 		}
 		attached++;
diff --git a/fs/btrfs/extent_map.c b/fs/btrfs/extent_map.c
index 02bfdb976e40ce..d9b3282b1af2bc 100644
--- a/fs/btrfs/extent_map.c
+++ b/fs/btrfs/extent_map.c
@@ -313,7 +313,7 @@ static void dump_extent_map(struct btrfs_fs_info *fs_info, const char *prefix,
 "%s, start=%llu len=%llu disk_bytenr=%llu disk_num_bytes=%llu ram_bytes=%llu offset=%llu flags=0x%x",
 		prefix, em->start, em->len, em->disk_bytenr, em->disk_num_bytes,
 		em->ram_bytes, em->offset, em->flags);
-	ASSERT(0);
+	BUG();
 }
 
 /* Internal sanity checks for btrfs debug builds. */
diff --git a/fs/btrfs/free-space-tree.c b/fs/btrfs/free-space-tree.c
index 39c6b96a4c25a8..6886992f24d1e3 100644
--- a/fs/btrfs/free-space-tree.c
+++ b/fs/btrfs/free-space-tree.c
@@ -117,7 +117,7 @@ struct btrfs_free_space_info *search_free_space_info(
 	if (ret != 0) {
 		btrfs_warn(fs_info, "missing free space info for %llu",
 			   block_group->start);
-		ASSERT(0);
+		DEBUG_WARN();
 		return ERR_PTR(-ENOENT);
 	}
 
@@ -141,12 +141,12 @@ static int btrfs_search_prev_slot(struct btrfs_trans_handle *trans,
 		return ret;
 
 	if (ret == 0) {
-		ASSERT(0);
+		DEBUG_WARN();
 		return -EIO;
 	}
 
 	if (p->slots[0] == 0) {
-		ASSERT(0);
+		DEBUG_WARN("no previous slot found");
 		return -EIO;
 	}
 	p->slots[0]--;
@@ -266,7 +266,8 @@ int convert_free_space_to_bitmaps(struct btrfs_trans_handle *trans,
 				nr++;
 				path->slots[0]--;
 			} else {
-				ASSERT(0);
+				DEBUG_WARN("unexpected free space extent key type %d found",
+					   found_key.type);
 			}
 		}
 
@@ -293,7 +294,7 @@ int convert_free_space_to_bitmaps(struct btrfs_trans_handle *trans,
 			  "incorrect extent count for %llu; counted %u, expected %u",
 			  block_group->start, extent_count,
 			  expected_extent_count);
-		ASSERT(0);
+		DEBUG_WARN();
 		ret = -EIO;
 		goto out;
 	}
@@ -407,7 +408,8 @@ int convert_free_space_to_extents(struct btrfs_trans_handle *trans,
 				nr++;
 				path->slots[0]--;
 			} else {
-				ASSERT(0);
+				DEBUG_WARN("unexpected free space bitmap key type %d found",
+					   found_key.type);
 			}
 		}
 
@@ -455,7 +457,7 @@ int convert_free_space_to_extents(struct btrfs_trans_handle *trans,
 			  "incorrect extent count for %llu; counted %u, expected %u",
 			  block_group->start, extent_count,
 			  expected_extent_count);
-		ASSERT(0);
+		DEBUG_WARN();
 		ret = -EIO;
 		goto out;
 	}
@@ -843,7 +845,7 @@ int remove_from_free_space_tree(struct btrfs_trans_handle *trans,
 
 	block_group = btrfs_lookup_block_group(trans->fs_info, start);
 	if (!block_group) {
-		ASSERT(0);
+		DEBUG_WARN("no block group found for start=%llu", start);
 		ret = -ENOENT;
 		goto out;
 	}
@@ -1036,7 +1038,7 @@ int add_to_free_space_tree(struct btrfs_trans_handle *trans,
 
 	block_group = btrfs_lookup_block_group(trans->fs_info, start);
 	if (!block_group) {
-		ASSERT(0);
+		DEBUG_WARN("no block group found for start=%llu", start);
 		ret = -ENOENT;
 		goto out;
 	}
@@ -1463,7 +1465,8 @@ int remove_block_group_free_space(struct btrfs_trans_handle *trans,
 				nr++;
 				path->slots[0]--;
 			} else {
-				ASSERT(0);
+				DEBUG_WARN("unexpected free space key type %d found",
+					   found_key.type);
 			}
 		}
 
@@ -1555,7 +1558,7 @@ static int load_free_space_bitmaps(struct btrfs_caching_control *caching_ctl,
 			  "incorrect extent count for %llu; counted %u, expected %u",
 			  block_group->start, extent_count,
 			  expected_extent_count);
-		ASSERT(0);
+		DEBUG_WARN();
 		ret = -EIO;
 		goto out;
 	}
@@ -1619,7 +1622,7 @@ static int load_free_space_extents(struct btrfs_caching_control *caching_ctl,
 			  "incorrect extent count for %llu; counted %u, expected %u",
 			  block_group->start, extent_count,
 			  expected_extent_count);
-		ASSERT(0);
+		DEBUG_WARN();
 		ret = -EIO;
 		goto out;
 	}
diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index 19acdd9ede7964..27fdfeb7d04326 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -1931,11 +1931,11 @@ static int record_reloc_root_in_trans(struct btrfs_trans_handle *trans,
 	 * reloc root without a corresponding root this could return ENOENT.
 	 */
 	if (IS_ERR(root)) {
-		ASSERT(0);
+		DEBUG_WARN("error %ld reading root for reloc root", PTR_ERR(root));
 		return PTR_ERR(root);
 	}
 	if (root->reloc_root != reloc_root) {
-		ASSERT(0);
+		DEBUG_WARN("unexpected reloc root found");
 		btrfs_err(fs_info,
 			  "root %llu has two reloc roots associated with it",
 			  reloc_root->root_key.offset);
diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
index f0e49a813ccbbf..0315fd56242667 100644
--- a/fs/btrfs/send.c
+++ b/fs/btrfs/send.c
@@ -383,11 +383,11 @@ static void inconsistent_snapshot_error(struct send_ctx *sctx,
 		result_string = "updated";
 		break;
 	case BTRFS_COMPARE_TREE_SAME:
-		ASSERT(0);
+		DEBUG_WARN("no change between trees");
 		result_string = "unchanged";
 		break;
 	default:
-		ASSERT(0);
+		DEBUG_WARN("unexpected comparison result %d", result);
 		result_string = "unexpected";
 	}
 
diff --git a/fs/btrfs/space-info.c b/fs/btrfs/space-info.c
index 77cc5d4a5a4774..434df9aa9b7123 100644
--- a/fs/btrfs/space-info.c
+++ b/fs/btrfs/space-info.c
@@ -1584,7 +1584,7 @@ static int handle_reserve_ticket(struct btrfs_fs_info *fs_info,
 		priority_reclaim_data_space(fs_info, space_info, ticket);
 		break;
 	default:
-		ASSERT(0);
+		DEBUG_WARN("unknown flush state %d", flush);
 		break;
 	}
 
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index e8b944aeb2e189..284bac76dbef8c 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -3342,7 +3342,8 @@ int btrfs_remove_chunk(struct btrfs_trans_handle *trans, u64 chunk_offset)
 		 * user having built with ASSERT enabled, so if ASSERT doesn't
 		 * do anything we still error out.
 		 */
-		ASSERT(0);
+		DEBUG_WARN("errr %ld reading chunk map at offset %llu",
+			   PTR_ERR(map), chunk_offset);
 		return PTR_ERR(map);
 	}
 
@@ -5665,7 +5666,7 @@ struct btrfs_block_group *btrfs_create_chunk(struct btrfs_trans_handle *trans,
 	lockdep_assert_held(&info->chunk_mutex);
 
 	if (!alloc_profile_is_valid(type, 0)) {
-		ASSERT(0);
+		DEBUG_WARN("invalid alloc profile for type %llu", type);
 		return ERR_PTR(-EINVAL);
 	}
 
@@ -5677,7 +5678,7 @@ struct btrfs_block_group *btrfs_create_chunk(struct btrfs_trans_handle *trans,
 
 	if (!(type & BTRFS_BLOCK_GROUP_TYPE_MASK)) {
 		btrfs_err(info, "invalid chunk type 0x%llx requested", type);
-		ASSERT(0);
+		DEBUG_WARN();
 		return ERR_PTR(-EINVAL);
 	}
 
diff --git a/fs/btrfs/zoned.c b/fs/btrfs/zoned.c
index 7b30700ec9304f..7fc2f73dc8d8a6 100644
--- a/fs/btrfs/zoned.c
+++ b/fs/btrfs/zoned.c
@@ -989,7 +989,7 @@ int btrfs_advance_sb_log(struct btrfs_device *device, int mirror)
 	}
 
 	/* All the zones are FULL. Should not reach here. */
-	ASSERT(0);
+	DEBUG_WARN("unexpected state, all zones full");
 	return -EIO;
 }
 
-- 
2.49.0

Re: [PATCH 0/5] Assertion and debugging helpers

[Qu Wenruo]

在 2025/4/16 18:38, David Sterba 写道:
> Hi,
> 
> this is a RFC series. We need to improve debugging and logging helpers
> so there's no ASSERT(0) or the convoluted
> WARN_ON(IS_DEFINED(CONFIG_BTRFS_DEBUG)). This was mentioned in past
> discussions so here's my proposal.
> 
> The series is only build tested, I'd like to hear some feedback if this
> is going the right direction or if there are suggestions for fine
> tuning.
> 
> 1) Add verbose ASSERT macro, so we can print additional information when
> it triggers, namely printing the values of the assertion expression.
> More details in the first patch, basic pattern is something like
> 
>      VASSERT(value > limit, "value=%llu limit=%llu", value, limit);

I think the idea is pretty good for debug.

I have hit too many cases where outputting the values will immediately 
help me pinning down the cause, other than adding extra outputs and then 
curse myself being too stupid.


But the concern is, we already have too many different debugging tools 
just inside btrfs:

- btrfs_warn()/btrfs_err()/btrfs_crit()
   These are the most sane ones so far, and they saves us a lot of time
   debugging things like memory bitflip in tree-checkers.

- btrfs_debug()
   Looks like the least used ones, if someone is actively utilizing it,
   please let me know.

- WARN_ON()

- ASSERT()
   I'm definitely the abuser, almost all my patches have utilized one at
   least.

Now we will have another one, and we will need another set of rules for 
the newer one.

I know everyone loves new things, but I think we should sometimes to get 
it more consistent.

So, if we're pushing towards VASSERT(), then it should replace all 
ASSERT() eventually. At least mark the ASSERT() macro deprecated and 
stop new usages.

> 
> The second patch shows it's application in volumes.c, converting about
> half where it's relevant. There are about 800 assertions in fs/btrfs/
> and we don't need to convert them all. This can be done incrementally
> and as needed.
> 
> The verbose version is another macro, although with some preprocessor
> magic it should be possible to make ASSERT take variable number of
> arguments. Does not seem worth though.
> 
> 2) Wrap WARN_ON(IS_DEFINED(CONFIG_BTRFS_DEBUG)) to DEBUG_WARN() with
> optional message with printk format. This is used to replace the
> WARN_ON(...) above and also the ASSERT(0).

This looks very fine to me, independent from the VASSERT() part.

Thanks,
Qu>
> The ultimate goal for me is to get rid of all ASSERT(0), it's not used
> consistently and looks like it's a note to the code author. There may be
> several reasons for it's use and although I've converted almost all to
> DEBUG_WARN it may miss the intentions.
> 
> In some cases it may be better to add proper error handling, print a
> message or warn and exit with error. Possibly the are cases where the
> code cannot continue, meaning it should be a BUG_ON but this is also
> something we want to convert to proper error handling.
> 
> David Sterba (5):
>    btrfs: add verbose version of ASSERT
>    btrfs: example use of VASSERT() in volumes.c
>    btrfs: add debug build only WARN
>    btrfs: convert WARN_ON(IS_ENABLED(CONFIG_BTRFS_DEBUG)) to DEBUG_WARN
>    btrfs: convert ASSERT(0) to DEBUG_WARN()
> 
>   fs/btrfs/backref.c         |  4 +--
>   fs/btrfs/delayed-ref.c     |  2 +-
>   fs/btrfs/dev-replace.c     |  2 +-
>   fs/btrfs/disk-io.c         |  2 +-
>   fs/btrfs/extent-tree.c     |  2 +-
>   fs/btrfs/extent_io.c       |  4 +--
>   fs/btrfs/extent_map.c      |  2 +-
>   fs/btrfs/free-space-tree.c | 27 +++++++++-------
>   fs/btrfs/inode.c           |  6 ++--
>   fs/btrfs/messages.h        | 30 ++++++++++++++++++
>   fs/btrfs/qgroup.c          |  6 ++--
>   fs/btrfs/relocation.c      |  4 +--
>   fs/btrfs/send.c            |  4 +--
>   fs/btrfs/space-info.c      |  2 +-
>   fs/btrfs/tree-checker.c    |  8 ++---
>   fs/btrfs/volumes.c         | 64 ++++++++++++++++++++++++--------------
>   fs/btrfs/zoned.c           |  2 +-
>   17 files changed, 109 insertions(+), 62 deletions(-)
> 

Re: [PATCH 1/5] btrfs: add verbose version of ASSERT

[Johannes Thumshirn]

On 16.04.25 11:09, David Sterba wrote:
> +
> +/* Verbose assert, use to print any relevant values of the condition. */
> +#define VASSERT(expr, fmt, ...)					\
> +	(likely(expr) ? (void)0 : btrfs_assertfail_verbose(#expr, __FILE__, __LINE__, \
> +							   fmt, __VA_ARGS__))
>   #else
>   #define ASSERT(expr)	(void)(expr)
> +#define VASSERT(expr, fmt, ...)		(void)(expr)
>   #endif

Ahm stupid question (applies for ASSERT() as well), doesn't that 
generate code as well when CONFIG_BTRFS_ASSERT=n? So we're doing a lot 
of potentially unneeded tests?

Re: [PATCH 1/5] btrfs: add verbose version of ASSERT

[David Sterba]

On Wed, Apr 16, 2025 at 03:03:18PM +0000, Johannes Thumshirn wrote:
> On 16.04.25 11:09, David Sterba wrote:
> > +
> > +/* Verbose assert, use to print any relevant values of the condition. */
> > +#define VASSERT(expr, fmt, ...)					\
> > +	(likely(expr) ? (void)0 : btrfs_assertfail_verbose(#expr, __FILE__, __LINE__, \
> > +							   fmt, __VA_ARGS__))
> >   #else
> >   #define ASSERT(expr)	(void)(expr)
> > +#define VASSERT(expr, fmt, ...)		(void)(expr)
> >   #endif
> 
> Ahm stupid question (applies for ASSERT() as well), doesn't that 
> generate code as well when CONFIG_BTRFS_ASSERT=n? So we're doing a lot 
> of potentially unneeded tests?

It should not generate any code. It's parsed, syntax-checked and as the
result is cast to void it's thrown out and optimized out (-O2).

Re: [PATCH 0/5] Assertion and debugging helpers

[David Sterba]

On Wed, Apr 16, 2025 at 08:25:56PM +0930, Qu Wenruo wrote:
> 
> 
> 在 2025/4/16 18:38, David Sterba 写道:
> > Hi,
> > 
> > this is a RFC series. We need to improve debugging and logging helpers
> > so there's no ASSERT(0) or the convoluted
> > WARN_ON(IS_DEFINED(CONFIG_BTRFS_DEBUG)). This was mentioned in past
> > discussions so here's my proposal.
> > 
> > The series is only build tested, I'd like to hear some feedback if this
> > is going the right direction or if there are suggestions for fine
> > tuning.
> > 
> > 1) Add verbose ASSERT macro, so we can print additional information when
> > it triggers, namely printing the values of the assertion expression.
> > More details in the first patch, basic pattern is something like
> > 
> >      VASSERT(value > limit, "value=%llu limit=%llu", value, limit);
> 
> I think the idea is pretty good for debug.
> 
> I have hit too many cases where outputting the values will immediately 
> help me pinning down the cause, other than adding extra outputs and then 
> curse myself being too stupid.
> 
> 
> But the concern is, we already have too many different debugging tools 
> just inside btrfs:
> 
> - btrfs_warn()/btrfs_err()/btrfs_crit()
>    These are the most sane ones so far, and they saves us a lot of time
>    debugging things like memory bitflip in tree-checkers.

Neither of that is mean for debugging, this is for users and for system
logs. The rules are described at https://btrfs.readthedocs.io/en/latest/dev/Development-notes.html#message-level

Though now that I see it, it does not mention btrfs_crit(), we have
about 60 of them and I think some of them could be turned to
btrfs_err().

> - btrfs_debug()
>    Looks like the least used ones, if someone is actively utilizing it,
>    please let me know.

Yeah, I think they've been there historically but I don't see any
consistent use. We may start deleting them as well or turning to trace
points if it's some notable event like repairing a block. Where it may
make sense is some internal debugging infrastructure like the leak
checkers to do a report before umount.

We can still keep the macros and helpers if anybody needs to write
custom debugging messages, I do that rather with btrfs_err with
searchable prefix so it's logged by default.

> - WARN_ON()

This is probably the most disordered way of debugging also present in
the release builds. The known problem is also that some systems are
configured to panic on warn so even a harmless warning has a bad impact.
I'd like to have them all audited and removed or replaced by something
more specific with an error so we don't have to guess the intentions for
the warnings. But this needs to be done case by case, there are about
300 of them.

> - ASSERT()
>    I'm definitely the abuser, almost all my patches have utilized one at
>    least.

This is actually good, please add more. The best time to add assertions
is when the code is written because the invariants and assumptions are
known.

We have the documentation at
https://btrfs.readthedocs.io/en/latest/dev/Development-notes.html#handling-unexpected-conditions
it may need some updates to give better guidelines.

> Now we will have another one, and we will need another set of rules for 
> the newer one.
> 
> I know everyone loves new things, but I think we should sometimes to get 
> it more consistent.

I'd like to unify and eventually reduce the number of the logging or
debugging primitives we use.

> So, if we're pushing towards VASSERT(), then it should replace all 
> ASSERT() eventually. At least mark the ASSERT() macro deprecated and 
> stop new usages.

You can consider VASSERT equivalent to ASSERT, the only reason it's a
different macro now is because I'd have to implement the variable number
of arguments and printk. But I can look into that, I agree that having
just ASSERT would be best in the long term.

Re: [PATCH 0/5] Assertion and debugging helpers

[David Sterba]

On Wed, Apr 16, 2025 at 10:20:56PM +0200, David Sterba wrote:
> > So, if we're pushing towards VASSERT(), then it should replace all 
> > ASSERT() eventually. At least mark the ASSERT() macro deprecated and 
> > stop new usages.
> 
> You can consider VASSERT equivalent to ASSERT, the only reason it's a
> different macro now is because I'd have to implement the variable number
> of arguments and printk. But I can look into that, I agree that having
> just ASSERT would be best in the long term.

I have something, so the following will work:

ASSERT(condition);
ASSERT(condition, "string");
ASSERT(condition, "string=%d", variable);

Re: [PATCH 0/5] Assertion and debugging helpers

[Qu Wenruo]

在 2025/4/17 08:00, David Sterba 写道:
> On Wed, Apr 16, 2025 at 10:20:56PM +0200, David Sterba wrote:
>>> So, if we're pushing towards VASSERT(), then it should replace all
>>> ASSERT() eventually. At least mark the ASSERT() macro deprecated and
>>> stop new usages.
>>
>> You can consider VASSERT equivalent to ASSERT, the only reason it's a
>> different macro now is because I'd have to implement the variable number
>> of arguments and printk. But I can look into that, I agree that having
>> just ASSERT would be best in the long term.
> 
> I have something, so the following will work:
> 
> ASSERT(condition);
> ASSERT(condition, "string");
> ASSERT(condition, "string=%d", variable);

That will be great, we can keep the single ASSERT() debug type, with all 
the new extra output.

Thanks,
Qu