| Message ID | cover.1744881159.git.dsterba@suse.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Assertion and debugging helpers | expand |
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
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. David Sterba (5): btrfs: enhance ASSERT() to take optional format string btrfs: example use of enhanced ASSERT() 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 | 59 +++++++++++++++++++++++++++++++----- 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 | 62 ++++++++++++++++++++++++-------------- fs/btrfs/zoned.c | 2 +- 17 files changed, 129 insertions(+), 69 deletions(-)