5 ms·

This bug is very very rare in practice: all my dev and testing machines run btrfs, and I haven't hit it once in 100+ machine-hours of running on 6.8-rc. The ac

This bug is very very rare in practice: all my dev and testing machines run btrfs, and I haven't hit it once in 100+ machine-hours of running on 6.8-rc. The actual patch is buried at the end the article: https://lore.kernel.org/linux-btrfs/1ca6e688950ee82b1526bb3098852af99b75e6ba.1710551459.git.tavianator@tavianator.com/ https://lore.kernel.org/linux-btrfs/1ca6e688950ee82b1526bb30...

tavianator 3y ago
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...
lxgr 3y ago
100 error-free machine hours isn’t exactly evidence of anything when it comes to FS bugs, though.
- jcalvinowens 3y ago
  Of course it is: it's evidence the average person will never hit this bug. Statistics, and all that. Having worked on huge deployments of Linux servers before, I can tell you that modelling race condition bugs as having a uniform random chance of happening per unit time is shockingly predictive. But it's not proof, obviously.
  - lxgr 3y ago
    > modelling race condition bugs as having a uniform random chance of happening per unit time is shockingly predictive I don't generally disagree with that methodology, but 100 hours is just not a lot of time. If you have a condition that takes, on average, 1000 hours to occur, you have a 9 in 10 chance of missing it based on 100 error-free hours observed, and yet it will still affect nearly 100% of all of your users after a bit more than a month! For file systems, the aim should be (much more than) five nines, not nine fives.
    - jcalvinowens 3y ago
      > If you have a condition that takes, on average, 1000 hours to occur, you have a 9 in 10 chance of missing it based on 100 error-free hours observed Yes. Which means 9/10 users who used their machines 100 or fewer hours on the new kernel will never hit the hypothetical bug. Thank you for proving my point! I'm not a filesystem developer, I'm a user: as a user, I don't care about the long tail, I only care about the average case as it relates to my deployment size. As you correctly point out, my deployment is of negligible size, and the long tail is far far beyond my reach. Aside: your hypothetical event has a 0.1% chance of happening each hour. That means it has a 99.9% chance of not happening each hour. The odds it doesn't happen after 100 hours is 0.999^100, or 90.5%. I think you know that, I just don't want a casual reader to infer it's 90% because 1-(100/1000) is 0.9.

tavianator 3y ago

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...

lxgr 3y ago

100 error-free machine hours isn’t exactly evidence of anything when it comes to FS bugs, though.

jcalvinowens 3y ago
Of course it is: it's evidence the average person will never hit this bug. Statistics, and all that. Having worked on huge deployments of Linux servers before, I can tell you that modelling race condition bugs as having a uniform random chance of happening per unit time is shockingly predictive. But it's not proof, obviously.
- lxgr 3y ago
  > modelling race condition bugs as having a uniform random chance of happening per unit time is shockingly predictive I don't generally disagree with that methodology, but 100 hours is just not a lot of time. If you have a condition that takes, on average, 1000 hours to occur, you have a 9 in 10 chance of missing it based on 100 error-free hours observed, and yet it will still affect nearly 100% of all of your users after a bit more than a month! For file systems, the aim should be (much more than) five nines, not nine fives.
  - jcalvinowens 3y ago
    > If you have a condition that takes, on average, 1000 hours to occur, you have a 9 in 10 chance of missing it based on 100 error-free hours observed Yes. Which means 9/10 users who used their machines 100 or fewer hours on the new kernel will never hit the hypothetical bug. Thank you for proving my point! I'm not a filesystem developer, I'm a user: as a user, I don't care about the long tail, I only care about the average case as it relates to my deployment size. As you correctly point out, my deployment is of negligible size, and the long tail is far far beyond my reach. Aside: your hypothetical event has a 0.1% chance of happening each hour. That means it has a 99.9% chance of not happening each hour. The odds it doesn't happen after 100 hours is 0.999^100, or 90.5%. I think you know that, I just don't want a casual reader to infer it's 90% because 1-(100/1000) is 0.9.