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

[Qu Wenruo <wqu@suse.com>]

在 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(-)
>