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

[David Sterba <dsterba@suse.cz>]

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.