4 ms·

Do they run btrfs on top of dm-crypt? I suspect it's impossible to reproduce on a regular block device.

jcalvinowens 3y ago
Yes, most of them do, precisely because I'm trying to catch more bugs :)
jcalvinowens 3y ago
Did this ever trip any of the debugging ASSERT stuff for you? I'm really curious if some more generic debugging instrument might be able to flag this failure mode more explicitly, it's far from the first ABA problem with ->bflags. Also, if you don't mind sharing your kconfig that would be interesting to see too.
- 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 ago
    The 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 ago
      I 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 ago
        I 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...

Yes, most of them do, precisely because I'm trying to catch more bugs :)

Did this ever trip any of the debugging ASSERT stuff for you? I'm really curious if some more generic debugging instrument might be able to flag this failure mode more explicitly, it's far from the first ABA problem with ->bflags. Also, if you don't mind sharing your kconfig that would be interesting to see too.

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 ago
  The 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 ago
    I 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 ago
      I 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...