* [PATCH v2 1/5] btrfs: enhance ASSERT() to take optional format string
2025-04-17 9:16 [PATCH v2 0/5] Assertion and debugging helpers David Sterba
@ 2025-04-17 9:16 ` David Sterba
2025-04-17 9:17 ` [PATCH v2 2/5] btrfs: example use of enhanced ASSERT() in volumes.c David Sterba
` (4 subsequent siblings)
5 siblings, 0 replies; 8+ messages in thread
From: David Sterba @ 2025-04-17 9:16 UTC (permalink / raw)
To: linux-btrfs; +Cc: 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 something 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. With some preprocessor magic and compile-time
optimizations we can enhance ASSERT to work like that as well:
ASSERT(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
- the optional format is verified at compile-time
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 format has been slightly updated to also print the result of the
evaluation of the condition, appended to the stringified condition as
"condition :: <value>".
Signed-off-by: David Sterba <dsterba@suse.com>
---
fs/btrfs/messages.h | 52 +++++++++++++++++++++++++++++++++++++++------
1 file changed, 45 insertions(+), 7 deletions(-)
diff --git a/fs/btrfs/messages.h b/fs/btrfs/messages.h
index 08a9272399d26f..c9031fee7169eb 100644
--- a/fs/btrfs/messages.h
+++ b/fs/btrfs/messages.h
@@ -170,15 +170,53 @@ do { \
#ifdef CONFIG_BTRFS_ASSERT
-#define btrfs_assertfail(expr, file, line) ({ \
- pr_err("assertion failed: %s, in %s:%d\n", (expr), (file), (line)); \
- BUG(); \
-})
+__printf(1, 2)
+static inline void verify_assert_printk_format(const char *fmt, ...) {
+ /* Stub to verify the assertion format string. */
+}
+
+/* Take the first token if any. */
+#define __FIRST_ARG(_, ...) _
+/* Skip the first token and return the rest, if it's empty the comma is dropped. */
+#define __REST_ARGS(_, ...) __VA_OPT__(,) __VA_ARGS__
+
+/*
+ * Assertion with optional printk() format.
+ *
+ * Accepted syntax:
+ * ASSERT(condition);
+ * ASSERT(condition, "string");
+ * ASSERT(condition, "variable=%d", variable);
+ *
+ * How it works:
+ * - if there's no format string, ""[0] evaluates at compile time to 0 and the
+ * first branch is executed
+ * - any non-empty format string with the "" prefix evaluates to != 0 at
+ * compile time and the second branch is executed
+ * - stringified condition is printed as %s so we don't accidentally mix format
+ * strings (the % operator)
+ * - there can be only one printk() call, so the format strings and arguments are
+ * spliced together:
+ * DEFAULT_FMT USER_FMT, DEFAULT_ARGS [,] USER_ARGS
+ * - comma between DEFAULT_ARGS and USER_ARGS is handled by preprocessor
+ */
+#define ASSERT(cond, args...) \
+do { \
+ verify_assert_printk_format("not empty" args); \
+ if (!likely(cond)) { \
+ if (("" __FIRST_ARG(args) [0]) == 0) { \
+ pr_err("assertion failed: %s :: %ld, in %s:%d\n", \
+ #cond, (long)(cond), __FILE__, __LINE__); \
+ } else { \
+ pr_err("assertion failed: %s :: %ld, in %s:%d (" __FIRST_ARG(args) ")\n", \
+ #cond, (long)(cond), __FILE__, __LINE__ __REST_ARGS(args)); \
+ } \
+ BUG(); \
+ } \
+} while(0)
-#define ASSERT(expr) \
- (likely(expr) ? (void)0 : btrfs_assertfail(#expr, __FILE__, __LINE__))
#else
-#define ASSERT(expr) (void)(expr)
+#define ASSERT(cond, args...) (void)(cond)
#endif
__printf(5, 6)
--
2.49.0
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCH v2 2/5] btrfs: example use of enhanced ASSERT() in volumes.c
2025-04-17 9:16 [PATCH v2 0/5] Assertion and debugging helpers David Sterba
2025-04-17 9:16 ` [PATCH v2 1/5] btrfs: enhance ASSERT() to take optional format string David Sterba
@ 2025-04-17 9:17 ` David Sterba
2025-04-17 9:17 ` [PATCH v2 3/5] btrfs: add debug build only WARN David Sterba
` (3 subsequent siblings)
5 siblings, 0 replies; 8+ messages in thread
From: David Sterba @ 2025-04-17 9:17 UTC (permalink / raw)
To: linux-btrfs; +Cc: David Sterba
The file volumes.c has about 40 assertions and half of them are suitable
for ASSERT() with additional data.
Signed-off-by: David Sterba <dsterba@suse.com>
---
fs/btrfs/volumes.c | 55 +++++++++++++++++++++++++++++-----------------
1 file changed, 35 insertions(+), 20 deletions(-)
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 4d5c59083003ff..934006af4b4254 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));
+ ASSERT(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);
+ ASSERT(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);
+ ASSERT(num_devices > 1, "num_devices=%llu", num_devices);
num_devices--;
}
up_read(&fs_info->dev_replace.rwsem);
@@ -2408,7 +2411,7 @@ 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);
+ ASSERT(cur_devices->opened == 1, "opened=%d", cur_devices->opened);
cur_devices->opened--;
free_fs_devices(cur_devices);
}
@@ -4753,7 +4756,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);
+ ASSERT(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 +5440,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);
+ ASSERT(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 +5455,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);
+ ASSERT(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 +6063,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)));
+ ASSERT((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 +6365,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);
+ ASSERT(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 +6376,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);
+ ASSERT(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 +6404,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);
+ ASSERT(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 +6423,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);
+ ASSERT(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);
+ ASSERT(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 +6594,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);
+ ASSERT(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 +7939,7 @@ void btrfs_commit_device_sizes(struct btrfs_transaction *trans)
{
struct btrfs_device *curr, *next;
- ASSERT(trans->state == TRANS_STATE_COMMIT_DOING);
+ ASSERT(trans->state == TRANS_STATE_COMMIT_DOING, "state=%d" , trans->state);
if (list_empty(&trans->dev_update_list))
return;
@@ -8299,7 +8313,7 @@ static void map_raid56_repair_block(struct btrfs_io_context *bioc,
logical < stripe_start + BTRFS_STRIPE_LEN)
break;
}
- ASSERT(i < data_stripes);
+ ASSERT(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 +8342,7 @@ int btrfs_map_repair_block(struct btrfs_fs_info *fs_info,
int mirror_ret = mirror_num;
int ret;
- ASSERT(mirror_num > 0);
+ ASSERT(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 +8350,7 @@ 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);
+ ASSERT(map_length >= length, "map_length=%llu length=%u", map_length, length);
/* Already mapped to single stripe. */
if (!bioc)
@@ -8348,7 +8362,8 @@ int btrfs_map_repair_block(struct btrfs_fs_info *fs_info,
goto out;
}
- ASSERT(mirror_num <= bioc->num_stripes);
+ ASSERT(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
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCH v2 3/5] btrfs: add debug build only WARN
2025-04-17 9:16 [PATCH v2 0/5] Assertion and debugging helpers David Sterba
2025-04-17 9:16 ` [PATCH v2 1/5] btrfs: enhance ASSERT() to take optional format string David Sterba
2025-04-17 9:17 ` [PATCH v2 2/5] btrfs: example use of enhanced ASSERT() in volumes.c David Sterba
@ 2025-04-17 9:17 ` David Sterba
2025-04-17 9:17 ` [PATCH v2 4/5] btrfs: convert WARN_ON(IS_ENABLED(CONFIG_BTRFS_DEBUG)) to DEBUG_WARN David Sterba
` (2 subsequent siblings)
5 siblings, 0 replies; 8+ messages in thread
From: David Sterba @ 2025-04-17 9:17 UTC (permalink / raw)
To: linux-btrfs; +Cc: David Sterba
Add conditional WARN() wrapper that's enabled only in debug build. It
should be used for unexpected conditions 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 c9031fee7169eb..6089bab3aca179 100644
--- a/fs/btrfs/messages.h
+++ b/fs/btrfs/messages.h
@@ -219,6 +219,13 @@ do { \
#define ASSERT(cond, args...) (void)(cond)
#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
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCH v2 4/5] btrfs: convert WARN_ON(IS_ENABLED(CONFIG_BTRFS_DEBUG)) to DEBUG_WARN
2025-04-17 9:16 [PATCH v2 0/5] Assertion and debugging helpers David Sterba
` (2 preceding siblings ...)
2025-04-17 9:17 ` [PATCH v2 3/5] btrfs: add debug build only WARN David Sterba
@ 2025-04-17 9:17 ` David Sterba
2025-04-17 9:17 ` [PATCH v2 5/5] btrfs: convert ASSERT(0) to DEBUG_WARN() David Sterba
2025-04-17 12:51 ` [PATCH v2 0/5] Assertion and debugging helpers Josef Bacik
5 siblings, 0 replies; 8+ messages in thread
From: David Sterba @ 2025-04-17 9:17 UTC (permalink / raw)
To: linux-btrfs; +Cc: 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
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCH v2 5/5] btrfs: convert ASSERT(0) to DEBUG_WARN()
2025-04-17 9:16 [PATCH v2 0/5] Assertion and debugging helpers David Sterba
` (3 preceding siblings ...)
2025-04-17 9:17 ` [PATCH v2 4/5] btrfs: convert WARN_ON(IS_ENABLED(CONFIG_BTRFS_DEBUG)) to DEBUG_WARN David Sterba
@ 2025-04-17 9:17 ` David Sterba
2025-04-22 12:06 ` David Sterba
2025-04-17 12:51 ` [PATCH v2 0/5] Assertion and debugging helpers Josef Bacik
5 siblings, 1 reply; 8+ messages in thread
From: David Sterba @ 2025-04-17 9:17 UTC (permalink / raw)
To: linux-btrfs; +Cc: 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 934006af4b4254..140fb51bd73ca9 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -3341,7 +3341,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);
}
@@ -5664,7 +5665,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);
}
@@ -5676,7 +5677,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
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH v2 5/5] btrfs: convert ASSERT(0) to DEBUG_WARN()
2025-04-17 9:17 ` [PATCH v2 5/5] btrfs: convert ASSERT(0) to DEBUG_WARN() David Sterba
@ 2025-04-22 12:06 ` David Sterba
0 siblings, 0 replies; 8+ messages in thread
From: David Sterba @ 2025-04-22 12:06 UTC (permalink / raw)
To: David Sterba; +Cc: linux-btrfs
On Thu, Apr 17, 2025 at 11:17:03AM +0200, David Sterba wrote:
> 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()
I've kept only the cases that do ASSERT(0) and error handling, there are
remaining cases for the impossible conditions like unkonwn delayed ref
actions etc, this will need the conversion to the error handling first.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 0/5] Assertion and debugging helpers
2025-04-17 9:16 [PATCH v2 0/5] Assertion and debugging helpers David Sterba
` (4 preceding siblings ...)
2025-04-17 9:17 ` [PATCH v2 5/5] btrfs: convert ASSERT(0) to DEBUG_WARN() David Sterba
@ 2025-04-17 12:51 ` Josef Bacik
5 siblings, 0 replies; 8+ messages in thread
From: Josef Bacik @ 2025-04-17 12:51 UTC (permalink / raw)
To: David Sterba; +Cc: linux-btrfs
On Thu, Apr 17, 2025 at 11:16:58AM +0200, David Sterba wrote:
> v2:
> - add the functionality of VASSERT to ASSERT proper
>
> 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) Update 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
>
> ASSERT(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.
>
I love it
Reviewed-by: Josef Bacik <josef@toxicpanda.com>
Thanks,
Josef
^ permalink raw reply [flat|nested] 8+ messages in thread