3 ms·
No, but I may have been missing some debugging CONFIGs. I was just using the stock Arch kconfig. I did submit a patch to add a WARN_ON() for this case: https:
by tavianator 3y ago
No, but I may have been missing some debugging CONFIGs. I was just using the stock Arch kconfig.
I did submit a patch to add a WARN_ON() for this case: https://lore.kernel.org/linux-btrfs/d4a055317bdb8ecbd7e6d9bdb5ebb074fa93f7f8.1710769876.git.tavianator@tavianator.com/ https://lore.kernel.org/linux-btrfs/d4a055317bdb8ecbd7e6d9bd...
But to catch the general class of bugs you'd need something like KCSAN. I did try that but the kernel is not KCSAN-clean so it was hard to know if any reports were relevant.
- jcalvinowens 3y agoThe WARN is great. Something as general as KCSAN isn't necessary: it's a classic ABA problem, double checking the ->bflags value on transitions is sufficient to catch it. Like a lockdep-style thing where *_bit() are optionally replaced by helpers that check the current value and WARN if the transition is unexpected. Using the event numbering from your patch description, such a thing would have flagged seeing UPTODATE at (2). But the space of invalid transitions is is much larger than the space of valid ones, which is why I think it might help catch other future bugs sooner. Dunno if it's actually worth it, but I definitely recall bugs of this flavor in the past. It'll take some work to unearth them all, alas...
- tavianator 3y agoI like that idea, but it isn't really compatible with my fix. `bflags` still makes the same transitions, I just skip the actual read in the `UPTODATE | READING` case.
- jcalvinowens 3y agoI think after your fix the set of valid states in that spot would just include UPTODATE. But this is all very poorly thought out on my part...