On Wed, Oct 21, 2020 at 09:32:24AM -0400, Josef Bacik wrote: > Hello, > > Nikolay discovered a regression with generic/371 with my delayed refs patches > applied. This isn't actually a regression caused by those patches, but rather > those patches uncovered a problem that has existed forever, we just have > papered > over it with our aggressive delayed ref flushing. Enter these two patches. > > The first patch is more of a refactoring and a simplification. We've been > passing in a &old_refs and a &new_refs into the delayed ref code, and > duplicating the > > if (old_refs >= 0 && new_refs < 0) > ->total_bytes_pinned += bytes; > else if (old_refs < 0 && new_refs >= 0) > ->total_bytes_pinned -= bytes; > > logic for data and metadata. This was made even more confusing by the fact > that > we clean up this accounting when we drop the delayed ref, but also include it > when we actually pin the extents down properly. It took me quite a while to > realize that we weren't mis-counting ->total_bytes_pinned because of how > weirdly > complicated the accounting was. > > I've refactored this code to make the handling distinct. We modify it in the > delayed refs code itself, which allows us to clean up a bunch of function > arguments and duplicated code. It also unifies how we handle the delayed ref > side of the ->total_bytes_pinned modification. Now it's a little easier to > see > who is responsible for the modification and where. > > The second patch is the actual fix for the problem. Previously we had simply > been assuming that ->total_ref_mod < 0 meant that we were freeing stuff. > However if we allocate and free in the same transaction, ->total_ref_mod == 0 > also means we're freeing. Adding that case is what fixes the problem Nikolay > was seeing. Thanks, > > Josef > > > Josef Bacik (2): > btrfs: handle ->total_bytes_pinned inside the delayed ref itself > btrfs: account for new extents being deleted in total_bytes_pinned
The 2nd patch did not get any review (1st one has from Nikolay), the patchset hasn't been in any topic branch or for-next. As we're going to have more space related changes in 5.12 cycle I'd like to get this one in too, so either please resend or get the remaining review. Thanks.
