Linux Btrfs filesystem development
 help / color / mirror / Atom feed
From: Josef Bacik <josef@toxicpanda.com>
To: linux-btrfs@vger.kernel.org, kernel-team@fb.com
Subject: [PATCH v2 0/2] ->total_bytes_pinned fixes for early ENOSPC issues
Date: Wed, 16 Dec 2020 11:46:52 -0500	[thread overview]
Message-ID: <cover.1608137123.git.josef@toxicpanda.com> (raw)

v1->v2:
- Rebase onto latest misc-next.
- Added Nikolay's reviewed-by for the first patch.

--- Original email ---
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

 fs/btrfs/block-group.c |  11 ++--
 fs/btrfs/delayed-ref.c |  60 ++++++++++++-------
 fs/btrfs/delayed-ref.h |  16 +++--
 fs/btrfs/extent-tree.c | 129 ++++++++++-------------------------------
 fs/btrfs/space-info.h  |  17 ++++++
 5 files changed, 103 insertions(+), 130 deletions(-)

-- 
2.26.2


             reply	other threads:[~2020-12-16 16:47 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-12-16 16:46 Josef Bacik [this message]
2020-12-16 16:46 ` [PATCH v2 1/2] btrfs: handle ->total_bytes_pinned inside the delayed ref itself Josef Bacik
2020-12-16 16:46 ` [PATCH v2 2/2] btrfs: account for new extents being deleted in total_bytes_pinned Josef Bacik
2020-12-17 12:54   ` Nikolay Borisov
2020-12-17 16:41   ` David Sterba

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=cover.1608137123.git.josef@toxicpanda.com \
    --to=josef@toxicpanda.com \
    --cc=kernel-team@fb.com \
    --cc=linux-btrfs@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox