[PATCH 0/2] btrfs: fix filesystem corruption caused by space cache race

[PATCH 0/2] btrfs: fix filesystem corruption caused by space cache race

[Omar Sandoval]

From: Omar Sandoval <osandov@fb.com>

Hello,

We recently deployed space_cache v2 on a large set of machines to do
some performance comparisons and found a nasty filesystem corruption bug
that was introduced by the fsync transaction optimization in 5.12. It's
much more likely to affect space_cache=v2 and nospace_cache, but since
space_cache=v1 effectively falls back to nospace_cache if there is a
free space inode generation mismatch, space_cache=v1 could also
theoretically be affected. discard/discard=sync also makes the bug much
easier to hit by making the race window larger.

Patch 1 is the fix itself with a lot more details. Patch 2 is a followup
cleanup.

I'm still working on a reproducer, but I wanted to get this fix out
ASAP.

Thanks!

Omar Sandoval (2):
  btrfs: fix space cache corruption and potential double allocations
  btrfs: get rid of block group caching progress logic

 fs/btrfs/block-group.c     | 49 +++++++++++---------------------------
 fs/btrfs/block-group.h     |  5 +---
 fs/btrfs/extent-tree.c     | 39 +++++++-----------------------
 fs/btrfs/free-space-tree.c |  8 -------
 fs/btrfs/transaction.c     | 41 -------------------------------
 fs/btrfs/zoned.c           |  1 -
 6 files changed, 23 insertions(+), 120 deletions(-)

-- 
2.37.2

[PATCH 1/2] btrfs: fix space cache corruption and potential double allocations

[Omar Sandoval]

From: Omar Sandoval <osandov@fb.com>

When testing space_cache v2 on a large set of machines, we encountered a
few symptoms:

1. "unable to add free space :-17" (EEXIST) errors.
2. Missing free space info items, sometimes caught with a "missing free
   space info for X" error.
3. Double-accounted space: ranges that were allocated in the extent tree
   and also marked as free in the free space tree, ranges that were
   marked as allocated twice in the extent tree, or ranges that were
   marked as free twice in the free space tree. If the latter made it
   onto disk, the next reboot would hit the BUG_ON() in
   add_new_free_space().
4. On some hosts with no on-disk corruption or error messages, the
   in-memory space cache (dumped with drgn) disagreed with the free
   space tree.

All of these symptoms have the same underlying cause: a race between
caching the free space for a block group and returning free space to the
in-memory space cache for pinned extents causes us to double-add a free
range to the space cache. This race exists when free space is cached
from the free space tree (space_cache=v2) or the extent tree
(nospace_cache, or space_cache=v1 if the cache needs to be regenerated).
struct btrfs_block_group::last_byte_to_unpin and struct
btrfs_block_group::progress are supposed to protect against this race,
but commit d0c2f4fa555e ("btrfs: make concurrent fsyncs wait less when
waiting for a transaction commit") subtly broke this by allowing
multiple transactions to be unpinning extents at the same time.

Specifically, the race is as follows:

1. An extent is deleted from an uncached block group in transaction A.
2. btrfs_commit_transaction() is called for transaction A.
3. btrfs_run_delayed_refs() -> __btrfs_free_extent() runs the delayed
   ref for the deleted extent.
4. __btrfs_free_extent() -> do_free_extent_accounting() ->
   add_to_free_space_tree() adds the deleted extent back to the free
   space tree.
5. do_free_extent_accounting() -> btrfs_update_block_group() ->
   btrfs_cache_block_group() queues up the block group to get cached.
   block_group->progress is set to block_group->start.
6. btrfs_commit_transaction() for transaction A calls
   switch_commit_roots(). It sets block_group->last_byte_to_unpin to
   block_group->progress, which is block_group->start because the block
   group hasn't been cached yet.
7. The caching thread gets to our block group. Since the commit roots
   were already switched, load_free_space_tree() sees the deleted extent
   as free and adds it to the space cache. It finishes caching and sets
   block_group->progress to U64_MAX.
8. btrfs_commit_transaction() advances transaction A to
   TRANS_STATE_SUPER_COMMITTED.
9. fsync calls btrfs_commit_transaction() for transaction B. Since
   transaction A is already in TRANS_STATE_SUPER_COMMITTED and the
   commit is for fsync, it advances.
10. btrfs_commit_transaction() for transaction B calls
    switch_commit_roots(). This time, the block group has already been
    cached, so it sets block_group->last_byte_to_unpin to U64_MAX.
11. btrfs_commit_transaction() for transaction A calls
    btrfs_finish_extent_commit(), which calls unpin_extent_range() for
    the deleted extent. It sees last_byte_to_unpin set to U64_MAX (by
    transaction B!), so it adds the deleted extent to the space cache
    again!

This explains all of our symptoms above:

* If the sequence of events is exactly as described above, when the free
  space is re-added in step 11, it will fail with EEXIST.
* If another thread reallocates the deleted extent in between steps 7
  and 11, then step 11 will silently re-add that space to the space
  cache as free even though it is actually allocated. Then, if that
  space is allocated *again*, the free space tree will be corrupted
 (namely, the wrong item will be deleted).
* If we don't catch this free space tree corruption, it will continue
  to get worse as extents are deleted and reallocated.

The v1 space_cache is synchronously loaded when an extent is deleted
(btrfs_update_block_group() with alloc=0 calls btrfs_cache_block_group()
with load_cache_only=1), so it is not normally affected by this bug.
However, as noted above, if we fail to load the space cache, we will
fall back to caching from the extent tree and may hit this bug.

The easiest fix for this race is to also make caching from the free
space tree or extent tree synchronous. Josef tested this and found no
performance regressions.

A few extra changes fall out of this change. Namely, this fix does the
following, with step 2 being the crucial fix:

1. Factor btrfs_caching_ctl_wait_done() out of
   btrfs_wait_block_group_cache_done() to allow waiting on a caching_ctl
   that we already hold a reference to.
2. Change the call in btrfs_cache_block_group() of
   btrfs_wait_space_cache_v1_finished() to
   btrfs_caching_ctl_wait_done(), which makes us wait regardless of the
   space_cache option.
3. Delete the now unused btrfs_wait_space_cache_v1_finished() and
   space_cache_v1_done().
4. Change btrfs_cache_block_group()'s `int load_cache_only` parameter to
   `bool wait` to more accurately describe its new meaning.
5. Change a few callers which had a separate call to
   btrfs_wait_block_group_cache_done() to use wait = true instead.
6. Make btrfs_wait_block_group_cache_done() static now that it's not
   used outside of block-group.c anymore.

Fixes: d0c2f4fa555e ("btrfs: make concurrent fsyncs wait less when waiting for a transaction commit")
Signed-off-by: Omar Sandoval <osandov@fb.com>
---
 fs/btrfs/block-group.c | 36 ++++++++++++++----------------------
 fs/btrfs/block-group.h |  3 +--
 fs/btrfs/extent-tree.c | 30 ++++++------------------------
 3 files changed, 21 insertions(+), 48 deletions(-)

diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
index c8162b8d85a2..1af6fc395a52 100644
--- a/fs/btrfs/block-group.c
+++ b/fs/btrfs/block-group.c
@@ -440,33 +440,26 @@ void btrfs_wait_block_group_cache_progress(struct btrfs_block_group *cache,
 	btrfs_put_caching_control(caching_ctl);
 }
 
-int btrfs_wait_block_group_cache_done(struct btrfs_block_group *cache)
+static int btrfs_caching_ctl_wait_done(struct btrfs_block_group *cache,
+				      struct btrfs_caching_control *caching_ctl)
+{
+	wait_event(caching_ctl->wait, btrfs_block_group_done(cache));
+	return cache->cached == BTRFS_CACHE_ERROR ? -EIO : 0;
+}
+
+static int btrfs_wait_block_group_cache_done(struct btrfs_block_group *cache)
 {
 	struct btrfs_caching_control *caching_ctl;
-	int ret = 0;
+	int ret;
 
 	caching_ctl = btrfs_get_caching_control(cache);
 	if (!caching_ctl)
 		return (cache->cached == BTRFS_CACHE_ERROR) ? -EIO : 0;
-
-	wait_event(caching_ctl->wait, btrfs_block_group_done(cache));
-	if (cache->cached == BTRFS_CACHE_ERROR)
-		ret = -EIO;
+	ret = btrfs_caching_ctl_wait_done(cache, caching_ctl);
 	btrfs_put_caching_control(caching_ctl);
 	return ret;
 }
 
-static bool space_cache_v1_done(struct btrfs_block_group *cache)
-{
-	bool ret;
-
-	spin_lock(&cache->lock);
-	ret = cache->cached != BTRFS_CACHE_FAST;
-	spin_unlock(&cache->lock);
-
-	return ret;
-}
-
 #ifdef CONFIG_BTRFS_DEBUG
 static void fragment_free_space(struct btrfs_block_group *block_group)
 {
@@ -744,9 +737,8 @@ static noinline void caching_thread(struct btrfs_work *work)
 	btrfs_put_block_group(block_group);
 }
 
-int btrfs_cache_block_group(struct btrfs_block_group *cache, int load_cache_only)
+int btrfs_cache_block_group(struct btrfs_block_group *cache, bool wait)
 {
-	DEFINE_WAIT(wait);
 	struct btrfs_fs_info *fs_info = cache->fs_info;
 	struct btrfs_caching_control *caching_ctl = NULL;
 	int ret = 0;
@@ -794,8 +786,8 @@ int btrfs_cache_block_group(struct btrfs_block_group *cache, int load_cache_only
 
 	btrfs_queue_work(fs_info->caching_workers, &caching_ctl->work);
 out:
-	if (load_cache_only && caching_ctl)
-		wait_event(caching_ctl->wait, space_cache_v1_done(cache));
+	if (wait && caching_ctl)
+		ret = btrfs_caching_ctl_wait_done(cache, caching_ctl);
 	if (caching_ctl)
 		btrfs_put_caching_control(caching_ctl);
 
@@ -3288,7 +3280,7 @@ int btrfs_update_block_group(struct btrfs_trans_handle *trans,
 		 * space back to the block group, otherwise we will leak space.
 		 */
 		if (!alloc && !btrfs_block_group_done(cache))
-			btrfs_cache_block_group(cache, 1);
+			btrfs_cache_block_group(cache, true);
 
 		byte_in_group = bytenr - cache->start;
 		WARN_ON(byte_in_group > cache->length);
diff --git a/fs/btrfs/block-group.h b/fs/btrfs/block-group.h
index b5c8425839dc..9dba28bb1806 100644
--- a/fs/btrfs/block-group.h
+++ b/fs/btrfs/block-group.h
@@ -267,9 +267,8 @@ void btrfs_dec_nocow_writers(struct btrfs_block_group *bg);
 void btrfs_wait_nocow_writers(struct btrfs_block_group *bg);
 void btrfs_wait_block_group_cache_progress(struct btrfs_block_group *cache,
 				           u64 num_bytes);
-int btrfs_wait_block_group_cache_done(struct btrfs_block_group *cache);
 int btrfs_cache_block_group(struct btrfs_block_group *cache,
-			    int load_cache_only);
+			    bool wait);
 void btrfs_put_caching_control(struct btrfs_caching_control *ctl);
 struct btrfs_caching_control *btrfs_get_caching_control(
 		struct btrfs_block_group *cache);
diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index 7bb05d49809b..86ac953c69ac 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -2551,17 +2551,10 @@ int btrfs_pin_extent_for_log_replay(struct btrfs_trans_handle *trans,
 		return -EINVAL;
 
 	/*
-	 * pull in the free space cache (if any) so that our pin
-	 * removes the free space from the cache.  We have load_only set
-	 * to one because the slow code to read in the free extents does check
-	 * the pinned extents.
+	 * Fully cache the free space first so that our pin removes the free space
+	 * from the cache.
 	 */
-	btrfs_cache_block_group(cache, 1);
-	/*
-	 * Make sure we wait until the cache is completely built in case it is
-	 * missing or is invalid and therefore needs to be rebuilt.
-	 */
-	ret = btrfs_wait_block_group_cache_done(cache);
+	ret = btrfs_cache_block_group(cache, true);
 	if (ret)
 		goto out;
 
@@ -2584,12 +2577,7 @@ static int __exclude_logged_extent(struct btrfs_fs_info *fs_info,
 	if (!block_group)
 		return -EINVAL;
 
-	btrfs_cache_block_group(block_group, 1);
-	/*
-	 * Make sure we wait until the cache is completely built in case it is
-	 * missing or is invalid and therefore needs to be rebuilt.
-	 */
-	ret = btrfs_wait_block_group_cache_done(block_group);
+	ret = btrfs_cache_block_group(block_group, true);
 	if (ret)
 		goto out;
 
@@ -4400,7 +4388,7 @@ static noinline int find_free_extent(struct btrfs_root *root,
 		ffe_ctl->cached = btrfs_block_group_done(block_group);
 		if (unlikely(!ffe_ctl->cached)) {
 			ffe_ctl->have_caching_bg = true;
-			ret = btrfs_cache_block_group(block_group, 0);
+			ret = btrfs_cache_block_group(block_group, false);
 
 			/*
 			 * If we get ENOMEM here or something else we want to
@@ -6170,13 +6158,7 @@ int btrfs_trim_fs(struct btrfs_fs_info *fs_info, struct fstrim_range *range)
 
 		if (end - start >= range->minlen) {
 			if (!btrfs_block_group_done(cache)) {
-				ret = btrfs_cache_block_group(cache, 0);
-				if (ret) {
-					bg_failed++;
-					bg_ret = ret;
-					continue;
-				}
-				ret = btrfs_wait_block_group_cache_done(cache);
+				ret = btrfs_cache_block_group(cache, true);
 				if (ret) {
 					bg_failed++;
 					bg_ret = ret;
-- 
2.37.2

Re: [PATCH 1/2] btrfs: fix space cache corruption and potential double allocations

[Andrea Gelmini]

Il giorno mer 17 ago 2022 alle ore 01:22 Omar Sandoval
<osandov@osandov.com> ha scritto:
> * If we don't catch this free space tree corruption, it will continue
>   to get worse as extents are deleted and reallocated.

Is it possible to detect this by scrub?
I have a few >100TB storage, and comparing file by file with backup
could take months.

Using compression and snapshots is worse/better, or  there are no difference?

Thanks a lot for your work,
Gelma

Re: [PATCH 1/2] btrfs: fix space cache corruption and potential double allocations

[Christoph Anton Mitterer]

Hey.


It would be indeed quite helpful if there was some more usable
information for end-users on how to detect any possible corruptions
from that issue.


From how I'd understand the commit message there are at least some ways
where one would not be able to notice that, and so the corruptions are
actually possibly silent?



Are there perhaps at least some usage patters that were not prone to 
the issue?
I have e.g. numerous filesystems were regular files were only ever
added, but never deleted (just moved perhaps within the filesystem).



Are there any recommendations for people what they should do once the
have the fix? Like invalidating the free space cache with btrfs check?



Is this going to be submitted to the stable kernels?



Also, I think, there should be some better means of communicating data
corruption issues, especially silent ones, to the users.
In the past there were several of them (like the issues with holes and
compression) that - just like this one now - people only ever noticed
merely by chance.

This however easily crushes any chances for people to actively search
for corrupted data (e.g. by manually comparing with backups) and
retrieving still good copies of damaged files (which possibly will
eventually be rotated out of existence).


Cheers,
Chris.

Re: [PATCH 1/2] btrfs: fix space cache corruption and potential double allocations

[Omar Sandoval]

On Wed, Aug 17, 2022 at 06:53:56PM +0200, Christoph Anton Mitterer wrote:
> Hey.
> 
> 
> It would be indeed quite helpful if there was some more usable
> information for end-users on how to detect any possible corruptions
> from that issue.

I'm working on a tool that can be run on a mounted filesystem to detect
most of the corruptions that could result from this bug. I'll share that
in the next couple of days.

> From how I'd understand the commit message there are at least some ways
> where one would not be able to notice that, and so the corruptions are
> actually possibly silent?
> 
> 
> 
> Are there perhaps at least some usage patters that were not prone to 
> the issue?
> I have e.g. numerous filesystems were regular files were only ever
> added, but never deleted (just moved perhaps within the filesystem).

From what I've found, it's much more likely to happen if you delete a
lot of data soon after boot with space_cache=v2/nospace_cache and
discard/discard=sync. I can't say that it'd never happen outside of
those conditions, but I suspect that it's much harder to hit otherwise.

> Are there any recommendations for people what they should do once the
> have the fix? Like invalidating the free space cache with btrfs check?

I'll try to have the check tool I'm working on print some
recommendations. We'll probably be able to avoid invalidating the free
space cache in the happy case that no corruption is detected.

> Is this going to be submitted to the stable kernels?

Yes.

> Also, I think, there should be some better means of communicating data
> corruption issues, especially silent ones, to the users.
> In the past there were several of them (like the issues with holes and
> compression) that - just like this one now - people only ever noticed
> merely by chance.
> 
> This however easily crushes any chances for people to actively search
> for corrupted data (e.g. by manually comparing with backups) and
> retrieving still good copies of damaged files (which possibly will
> eventually be rotated out of existence).

That's a fair point. We'll need to figure out a good way to do this. Do
you have any suggestions?

Thanks,
Omar

Re: [PATCH 1/2] btrfs: fix space cache corruption and potential double allocations

[Christoph Anton Mitterer]

On Wed, 2022-08-17 at 16:59 -0700, Omar Sandoval wrote:
> I'm working on a tool that can be run on a mounted filesystem to
> detect
> most of the corruptions that could result from this bug. I'll share
> that
> in the next couple of days.

That sounds quite good.

Maybe such thing should find it's way into btrfs-progs.

I remember back then whether there were some corruption issues that
occurred with holes in compressed files, there were also some ways to
search for files which may have been affected.

So I mean such things wouldn't be the day2day tools of btrfs-progs, but
could be helpful to those who'd like to investigate more... and having
it as part of btrfs-progrs would make them much more accessible to end-
users (no need to search for it, no need to compile it, no need to
trust their author (well at least not more than they anyway need to
trust btrfs-progs)).


> 
> From what I've found, it's much more likely to happen if you delete a
> lot of data soon after boot with space_cache=v2/nospace_cache and
> discard/discard=sync. I can't say that it'd never happen outside of
> those conditions, but I suspect that it's much harder to hit
> otherwise.

Okay, but deletion *is* necessary, right? So if one has never deleted
(regular) files on a btrfs in question, the thing shouldn't be able to
have happened.



> 
> That's a fair point. We'll need to figure out a good way to do this.
> Do
> you have any suggestions?

Well, I had already brought this up before (on some other corruption
issue)... where some people argued it wouldn't be needed.

What one could do...

- A separate linux-btrfs-announce mailing list, or so, where only
  developers can post, informing only about really important stuff
  Not sure whether new versions or features would be important
  enough, but even if eventually "more" stuff gets posted than just
  corruption issues, one could prefix the subject with easy to spot
  tags like [NEWS], [CORRUPTION], or so.
  
  Downside is of course that people would need to register there.


- One also could think this even further and team up with other major
  filesystems, to have some dedicated corruption warning list,
  where posts would have a subject like "[<filesystem-type>] issue" and
  just pointing to further information.
  That way interested people could easily filter on the filesystems
  they use, and have a rather easy chance of noticing that they may
  need to take action.


- Another way would be to add it to a NEWS file of btrfs-progs, but I'd
  say it's too easy there too simply get overlooked.
  Also, it doesn't really belong to btrfs-progs (unless that would
  itself cause the corruption).
  Also, it would require release every time a new serious issue is
  found, which is then not guaranteed to end up anytime soon in the
  distributions.

- Same speaks IMO against having this in some NEWS file or so in the
  Linux sources.

- IMO anything which is not actively pushed to people is ruled out,
  so having it in the btrfs wiki or so, is also not really a solution.


Cheers,
Chris.

Re: [PATCH 1/2] btrfs: fix space cache corruption and potential double allocations

[Omar Sandoval]

On Thu, Aug 18, 2022 at 02:22:00AM +0200, Christoph Anton Mitterer wrote:
> On Wed, 2022-08-17 at 16:59 -0700, Omar Sandoval wrote:
> > I'm working on a tool that can be run on a mounted filesystem to
> > detect
> > most of the corruptions that could result from this bug. I'll share
> > that
> > in the next couple of days.
> 
> That sounds quite good.
> 
> Maybe such thing should find it's way into btrfs-progs.
> 
> I remember back then whether there were some corruption issues that
> occurred with holes in compressed files, there were also some ways to
> search for files which may have been affected.
> 
> So I mean such things wouldn't be the day2day tools of btrfs-progs, but
> could be helpful to those who'd like to investigate more... and having
> it as part of btrfs-progrs would make them much more accessible to end-
> users (no need to search for it, no need to compile it, no need to
> trust their author (well at least not more than they anyway need to
> trust btrfs-progs)).
> 
> 
> > 
> > From what I've found, it's much more likely to happen if you delete a
> > lot of data soon after boot with space_cache=v2/nospace_cache and
> > discard/discard=sync. I can't say that it'd never happen outside of
> > those conditions, but I suspect that it's much harder to hit
> > otherwise.
> 
> Okay, but deletion *is* necessary, right?

Yes, but metadata deletions also count, so basically any modification
results in a deletion. We haven't seen this in practice, but I couldn't
find anything that would make it impossible.

In place modifications of files also result in COW and deletion of the
old data, so that also technically counts.

Re: [PATCH 1/2] btrfs: fix space cache corruption and potential double allocations

[Christoph Anton Mitterer]

Hey Omar.


I'd have some more questions, which I hope help others and myself to
better asses the impact of this issue:


On Wed, 2022-08-17 at 17:30 -0700, Omar Sandoval wrote:
> > 
> but metadata deletions also count, so basically any modification
> results in a deletion. We haven't seen this in practice, but I
> couldn't
> find anything that would make it impossible.
> 
> In place modifications of files also result in COW and deletion of
> the
> old data, so that also technically counts.


1) I thought the issue happens primarily with space-cache-v2, and there
only when extents are deleted, which is when e.g. deleting (or because
of CoW: modifying) regular files.

I assume also when doing balance or scrub (when the scrub causes a
repair)?


But now you wrote "metadata deletions also count"? Wouldn't that mean
that any other tree (csum, etc.) of the fs could be affected?

So can corruptions happen when moving/renaming files, writing any
metadata (either btrfs internal trees or also things like file
permissions bits, XATTRs, etc.) or creating snapshots or subvolumes (or
moving files therein)?


I may also happen with v1 (or no-space-cache), but only when that needs
to be regenerated (i.e. when one sees that cheksum error messages on
the v1 space cache), but less likely (because it already requires the
v1 cache to get corrupted somehow?




2) The silent data corruption itself happens by some range being sill
used by some extent... AND also added back to the free space cache.
So next time that range from the free space cache is assigned and
written to the precious data would be lost.

Or are there other ways that issue could strike?

Or is it just a corruption of the free space cache, and the actual data
in the extents is fine?




3) In your commit, you described a number of symptoms that were seen
when the issue occurred.
AFAIU, some of the cases were silent (data corruption), right?

Is the whole issue super rare (like back the corruption with
compression and holes only happened under some very awkward and rare
situations) for v2?

I just wonder a bit,... cause if you say it was introduced with 5.12,
and since 5.15 (IIRC) v2 is the default for btrfs-progs... why haven't
more people seen any corruptions?

Or is the silent corruption much less likely than the one giving EEXIST
- or vice versa?




4) The tool you're going to write - what will it be able to do?
[I guess it will do something check whether any extent ranges are
 allocated AND also in the free space cache (v1 or v2)?]

Will it just be able to tell whether one's current free space cache is
corrupted.
Or will it also be able to tell previously any actual data corruption
already happened (i.e. still used extents got reallocated and
overwritten)?




5) Are there ways for people to *definitely* rule out whether their
data OR any other part of the fs was corrupted?

Like I personally have SHA512 hashsums attached as XATTRS on all files
of some filesystems with just data.
So I can easily go over those and verify.

Would a scrub (csum verification) also tell any data corruption?
But I guess that would only tell it for extents... not if any other
metadata in btrfs could be affected, too?




6) What should people do to get back to a safe and sound state?
I mean if I'd now verify my hashsum XATTRS, even if they'd all be
valid, my kernel would be still unpatched.


But once the kernel is patched:

Will it not be necessary to somehow check the free space cache then?
Cause otherwise I could do my verification (and get an: all ok) but the
free space tree is still bogus and some time later my data gets
corrupted again - despite the fixed kernel.

Wouldn't it be better to simply re-create the cache then (that is:
after the kernel is patched BUT before doing the verification of data
with e.g. hash sums or scrub), using:
btrfs check --clear-space-cache v1|v2
?


When any other metadata could in principle have been affected by this
issue,... wouldn't it - for 100%-safety - be recommended to start with
creating a new filesystem (after having the fixed kernel) and recover
from backups?

(Right now this might be just quite some work... in 5 years or so
people might simply no longer have their old backups. So I better re-
create the fs now, than being sorry later.)




7) What are people advised to do until they receive a fixed kernel?

You said with nodiscard the race window is smaller. So HDDs should
already have this, but SSDs not (and people may want to set it).
What about btrfs e.g. on top of dm-crypt, which per default blocks
discard, would that count (even if not specifically set for btrfs)?

Should one switch from v2 to v1?




Thanks,
Chris.

Re: [PATCH 1/2] btrfs: fix space cache corruption and potential double allocations

[Christoph Anton Mitterer]

Hey.

Now that the first stable kernel with the fix is out,... is there going
to be any more information on how people can asses the impact of this
issue on their data integrity?

I had asked some questions below, but I guess for me, personally, it
would also be okay to just recreate the potentially affected
filesystems from backups, if it cannot be determined for sure whether
any corruptions have happened (especially in both data or metadata).


Also anything expectations on when the announced tool for this might
become available?

Thanks,
Chris.


On Fri, 2022-08-19 at 02:16 +0200, Christoph Anton Mitterer wrote:
> Hey Omar.
> 
> 
> I'd have some more questions, which I hope help others and myself to
> better asses the impact of this issue:
> 
> 
> On Wed, 2022-08-17 at 17:30 -0700, Omar Sandoval wrote:
> > > 
> > but metadata deletions also count, so basically any modification
> > results in a deletion. We haven't seen this in practice, but I
> > couldn't
> > find anything that would make it impossible.
> > 
> > In place modifications of files also result in COW and deletion of
> > the
> > old data, so that also technically counts.
> 
> 
> 1) I thought the issue happens primarily with space-cache-v2, and
> there
> only when extents are deleted, which is when e.g. deleting (or
> because
> of CoW: modifying) regular files.
> 
> I assume also when doing balance or scrub (when the scrub causes a
> repair)?
> 
> 
> But now you wrote "metadata deletions also count"? Wouldn't that mean
> that any other tree (csum, etc.) of the fs could be affected?
> 
> So can corruptions happen when moving/renaming files, writing any
> metadata (either btrfs internal trees or also things like file
> permissions bits, XATTRs, etc.) or creating snapshots or subvolumes
> (or
> moving files therein)?
> 
> 
> I may also happen with v1 (or no-space-cache), but only when that
> needs
> to be regenerated (i.e. when one sees that cheksum error messages on
> the v1 space cache), but less likely (because it already requires the
> v1 cache to get corrupted somehow?
> 
> 
> 
> 
> 2) The silent data corruption itself happens by some range being sill
> used by some extent... AND also added back to the free space cache.
> So next time that range from the free space cache is assigned and
> written to the precious data would be lost.
> 
> Or are there other ways that issue could strike?
> 
> Or is it just a corruption of the free space cache, and the actual
> data
> in the extents is fine?
> 
> 
> 
> 
> 3) In your commit, you described a number of symptoms that were seen
> when the issue occurred.
> AFAIU, some of the cases were silent (data corruption), right?
> 
> Is the whole issue super rare (like back the corruption with
> compression and holes only happened under some very awkward and rare
> situations) for v2?
> 
> I just wonder a bit,... cause if you say it was introduced with 5.12,
> and since 5.15 (IIRC) v2 is the default for btrfs-progs... why
> haven't
> more people seen any corruptions?
> 
> Or is the silent corruption much less likely than the one giving
> EEXIST
> - or vice versa?
> 
> 
> 
> 
> 4) The tool you're going to write - what will it be able to do?
> [I guess it will do something check whether any extent ranges are
>  allocated AND also in the free space cache (v1 or v2)?]
> 
> Will it just be able to tell whether one's current free space cache
> is
> corrupted.
> Or will it also be able to tell previously any actual data corruption
> already happened (i.e. still used extents got reallocated and
> overwritten)?
> 
> 
> 
> 
> 5) Are there ways for people to *definitely* rule out whether their
> data OR any other part of the fs was corrupted?
> 
> Like I personally have SHA512 hashsums attached as XATTRS on all
> files
> of some filesystems with just data.
> So I can easily go over those and verify.
> 
> Would a scrub (csum verification) also tell any data corruption?
> But I guess that would only tell it for extents... not if any other
> metadata in btrfs could be affected, too?
> 
> 
> 
> 
> 6) What should people do to get back to a safe and sound state?
> I mean if I'd now verify my hashsum XATTRS, even if they'd all be
> valid, my kernel would be still unpatched.
> 
> 
> But once the kernel is patched:
> 
> Will it not be necessary to somehow check the free space cache then?
> Cause otherwise I could do my verification (and get an: all ok) but
> the
> free space tree is still bogus and some time later my data gets
> corrupted again - despite the fixed kernel.
> 
> Wouldn't it be better to simply re-create the cache then (that is:
> after the kernel is patched BUT before doing the verification of data
> with e.g. hash sums or scrub), using:
> btrfs check --clear-space-cache v1|v2
> ?
> 
> 
> When any other metadata could in principle have been affected by this
> issue,... wouldn't it - for 100%-safety - be recommended to start
> with
> creating a new filesystem (after having the fixed kernel) and recover
> from backups?
> 
> (Right now this might be just quite some work... in 5 years or so
> people might simply no longer have their old backups. So I better re-
> create the fs now, than being sorry later.)
> 
> 
> 
> 
> 7) What are people advised to do until they receive a fixed kernel?
> 
> You said with nodiscard the race window is smaller. So HDDs should
> already have this, but SSDs not (and people may want to set it).
> What about btrfs e.g. on top of dm-crypt, which per default blocks
> discard, would that count (even if not specifically set for btrfs)?
> 
> Should one switch from v2 to v1?
> 
> 
> 
> 
> Thanks,
> Chris.

Re: [PATCH 1/2] btrfs: fix space cache corruption and potential double allocations

[Omar Sandoval]

On Thu, Sep 01, 2022 at 06:59:05PM +0200, Christoph Anton Mitterer wrote:
> Hey.
> 
> Now that the first stable kernel with the fix is out,... is there going
> to be any more information on how people can asses the impact of this
> issue on their data integrity?
> 
> I had asked some questions below, but I guess for me, personally, it
> would also be okay to just recreate the potentially affected
> filesystems from backups, if it cannot be determined for sure whether
> any corruptions have happened (especially in both data or metadata).
> 
> 
> Also anything expectations on when the announced tool for this might
> become available?
> 
> Thanks,
> Chris.

The tool is here for now:
https://github.com/osandov/osandov-linux/blob/master/scripts/btrfs_check_space_cache.c

Please try it out with:

git clone https://github.com/osandov/osandov-linux.git
cd osandov-linux/scripts
make btrfs_check_space_cache
sudo ./btrfs_check_space_cache $MOUNTED_FILESYSTEM_PATH

It'll print out a diagnosis and some advice. Please let me know what
output it gives you and any suggestions you have.

Re: [PATCH 1/2] btrfs: fix space cache corruption and potential double allocations

[Holger Hoffstätte]

On 2022-09-01 20:18, Omar Sandoval wrote:
> On Thu, Sep 01, 2022 at 06:59:05PM +0200, Christoph Anton Mitterer wrote:
>> Hey.
>>
>> Now that the first stable kernel with the fix is out,... is there going
>> to be any more information on how people can asses the impact of this
>> issue on their data integrity?
>>
>> I had asked some questions below, but I guess for me, personally, it
>> would also be okay to just recreate the potentially affected
>> filesystems from backups, if it cannot be determined for sure whether
>> any corruptions have happened (especially in both data or metadata).
>>
>>
>> Also anything expectations on when the announced tool for this might
>> become available?
>>
>> Thanks,
>> Chris.
> 
> The tool is here for now:
> https://github.com/osandov/osandov-linux/blob/master/scripts/btrfs_check_space_cache.c
> 
> Please try it out with:
> 
> git clone https://github.com/osandov/osandov-linux.git
> cd osandov-linux/scripts
> make btrfs_check_space_cache
> sudo ./btrfs_check_space_cache $MOUNTED_FILESYSTEM_PATH
> 
> It'll print out a diagnosis and some advice. Please let me know what
> output it gives you and any suggestions you have.
> 

Thank you! Got two warnings though, with both gcc-12 and clang-14:

gcc $(echo $CFLAGS) -Wall btrfs_check_space_cache.c -o btrfs_check_space_cache
btrfs_check_space_cache.c: In function 'check_free_space_tree':
btrfs_check_space_cache.c:346:58: warning: taking address of packed member of 'struct btrfs_free_space_info' may result in an unaligned pointer value [-Waddress-of-packed-member]
   346 |                         expected_extent_count = get_le32(&info->extent_count);
       |                                                          ^~~~~~~~~~~~~~~~~~~
btrfs_check_space_cache.c:347:45: warning: taking address of packed member of 'struct btrfs_free_space_info' may result in an unaligned pointer value [-Waddress-of-packed-member]
   347 |                         bitmaps = (get_le32(&info->flags)
       |                                             ^~~~~~~~~~~~

I know this will work on Intel but don't remember what might happen on other arches.
Anyway it seems to work fine:

--snip--
$./btrfs_check_space_cache /mnt/backup
Space cache checker 1.0
retries = 2, freeze = 0
Running on Linux 5.19.6 #1 SMP PREEMPT_DYNAMIC Mon Aug 29 13:34:50 CEST 2022 x86_64
Free space tree is enabled

No corruption detected :)

You should install a kernel with the fix as soon as possible and avoid
rebooting until then.

Once you are running a kernel with the fix:

1. Run this program again.
2. Run btrfs scrub.
--snip--

cheers
Holger

Re: [PATCH 1/2] btrfs: fix space cache corruption and potential double allocations

[Omar Sandoval]

On Thu, Sep 01, 2022 at 08:52:44PM +0200, Holger Hoffstätte wrote:
> On 2022-09-01 20:18, Omar Sandoval wrote:
> > On Thu, Sep 01, 2022 at 06:59:05PM +0200, Christoph Anton Mitterer wrote:
> > > Hey.
> > > 
> > > Now that the first stable kernel with the fix is out,... is there going
> > > to be any more information on how people can asses the impact of this
> > > issue on their data integrity?
> > > 
> > > I had asked some questions below, but I guess for me, personally, it
> > > would also be okay to just recreate the potentially affected
> > > filesystems from backups, if it cannot be determined for sure whether
> > > any corruptions have happened (especially in both data or metadata).
> > > 
> > > 
> > > Also anything expectations on when the announced tool for this might
> > > become available?
> > > 
> > > Thanks,
> > > Chris.
> > 
> > The tool is here for now:
> > https://github.com/osandov/osandov-linux/blob/master/scripts/btrfs_check_space_cache.c
> > 
> > Please try it out with:
> > 
> > git clone https://github.com/osandov/osandov-linux.git
> > cd osandov-linux/scripts
> > make btrfs_check_space_cache
> > sudo ./btrfs_check_space_cache $MOUNTED_FILESYSTEM_PATH
> > 
> > It'll print out a diagnosis and some advice. Please let me know what
> > output it gives you and any suggestions you have.
> > 
> 
> Thank you! Got two warnings though, with both gcc-12 and clang-14:
> 
> gcc $(echo $CFLAGS) -Wall btrfs_check_space_cache.c -o btrfs_check_space_cache
> btrfs_check_space_cache.c: In function 'check_free_space_tree':
> btrfs_check_space_cache.c:346:58: warning: taking address of packed member of 'struct btrfs_free_space_info' may result in an unaligned pointer value [-Waddress-of-packed-member]
>   346 |                         expected_extent_count = get_le32(&info->extent_count);
>       |                                                          ^~~~~~~~~~~~~~~~~~~
> btrfs_check_space_cache.c:347:45: warning: taking address of packed member of 'struct btrfs_free_space_info' may result in an unaligned pointer value [-Waddress-of-packed-member]
>   347 |                         bitmaps = (get_le32(&info->flags)
>       |                                             ^~~~~~~~~~~~
> 
> I know this will work on Intel but don't remember what might happen on other arches.

The warning is bogus; get_le32() handles unaligned pointer values. FWIW,
the Makefile I provided passes -Wno-address-of-packed-member.

> Anyway it seems to work fine:
> 
> --snip--
> $./btrfs_check_space_cache /mnt/backup
> Space cache checker 1.0
> retries = 2, freeze = 0
> Running on Linux 5.19.6 #1 SMP PREEMPT_DYNAMIC Mon Aug 29 13:34:50 CEST 2022 x86_64
> Free space tree is enabled
> 
> No corruption detected :)
> 
> You should install a kernel with the fix as soon as possible and avoid
> rebooting until then.
> 
> Once you are running a kernel with the fix:
> 
> 1. Run this program again.
> 2. Run btrfs scrub.
> --snip--
> 
> cheers
> Holger

Thanks for testing! Something like 0.01% of our machines running with
the free space tree hit the bug, and that was with a very specific
trigger, so I'll be surprised if many people hit it in practice.

Re: [PATCH 1/2] btrfs: fix space cache corruption and potential double allocations

[Christoph Anton Mitterer]

On Thu, 2022-09-01 at 11:18 -0700, Omar Sandoval wrote:
> The tool is here for now:
> https://github.com/osandov/osandov-linux/blob/master/scripts/btrfs_check_space_cache.c

Thanks.

> sudo ./btrfs_check_space_cache $MOUNTED_FILESYSTEM_PATH

I assume this would work on all data of the fs, even if that was
mounted on a subvolume?

And does it also check v1 space cache? And is it going to be part of
btrfs check?


> It'll print out a diagnosis and some advice. Please let me know what
> output it gives you and any suggestions you have.

I just ran it on the still unfixed kernel on several filesystems,...
some with v1 some with v2.

The output was e.g.
-----------------------------------------------------------------------

# ./btrfs_check_space_cache /data/a/1/
Space cache checker 1.0
retries = 2, freeze = 0
Running on Linux 5.18.0-4-amd64 #1 SMP PREEMPT_DYNAMIC Debian 5.18.16-1
(2022-08-10) x86_64
Free space tree is enabled

No corruption detected :)

You should install a kernel with the fix as soon as possible and avoid
rebooting until then.

Once you are running a kernel with the fix:

1. Run this program again.
2. Run btrfs scrub.
-----------------------------------------------------------------------


But to me it's still absolutely unclear what that really means now.

Is it just a: you're 99% safe... or 100%?

Were all trees/metadata checked? Would any corruption be detected, even
if it happened in the past and maybe the double allocation had been
cleared by chance in meantime?

Would the scrub detect any corruptions?

Basically it boils down to the questions I've had asked in:
https://lore.kernel.org/linux-btrfs/467e49af8348d085e21079e8969bedbe379b3145.camel@scientia.org/



But it seem right now, one cannot really give a definite answer, so I
probably have to recover everything from backups?!


Thanks,
Chris.

Re: [PATCH 1/2] btrfs: fix space cache corruption and potential double allocations

[Christoph Anton Mitterer]

Oh and one more

On Thu, 2022-09-01 at 11:18 -0700, Omar Sandoval wrote:
> and any suggestions you have.

> "If you want to be extra cautious, you can also clear the v1 space cache.\n"
> "There are two ways to do this. The first is:\n"
> "\n"
> "1. Add the clear_cache mount option to this filesystem in fstab.\n"
> "2. Unmount then mount the filesystem. Note that `mount -o remount` is\n"
> "   not sufficient; you need a full unmount/mount cycle. You can also\n"
> "   reboot instead.\n"
> "3. Remove the clear_cache mount option from fstab.\n"

Wasn't the clear_cache flag only clearing it for stuff that get's
actually written?
And if so, wouldn't that be *not* enough to do?


> "\n"
> "The second way to clear the space cache is:\n"
> "\n"
> "1. Unmount the filesystem.\n"
> "2. Run `btrfs check --clear-space-cache v1 <device>`.\n"
> "3. Mount the filesystem.\n");


Wouldn't it make sense to generally advise people to clear the v1 and
v2 space cache?


Cheers,
Chris.

Re: [PATCH 1/2] btrfs: fix space cache corruption and potential double allocations

[Andrea Gelmini]

Il giorno gio 18 ago 2022 alle ore 01:59 Omar Sandoval
<osandov@osandov.com> ha scritto:
> From what I've found, it's much more likely to happen if you delete a
> lot of data soon after boot with space_cache=v2/nospace_cache and
> discard/discard=sync. I can't say that it'd never happen outside of
> those conditions, but I suspect that it's much harder to hit otherwise.

Thanks a lot for details.

If "discard" is a necessary condition, does it mean that on HDD we
don't have the problem?

Do you think is enough for the moment to disable "discard" on nvme/ssd?

Ciao,
Gelma

Re: [PATCH 1/2] btrfs: fix space cache corruption and potential double allocations

[Omar Sandoval]

On Thu, Aug 18, 2022 at 08:21:30AM +0200, Andrea Gelmini wrote:
> Il giorno gio 18 ago 2022 alle ore 01:59 Omar Sandoval
> <osandov@osandov.com> ha scritto:
> > From what I've found, it's much more likely to happen if you delete a
> > lot of data soon after boot with space_cache=v2/nospace_cache and
> > discard/discard=sync. I can't say that it'd never happen outside of
> > those conditions, but I suspect that it's much harder to hit otherwise.
> 
> Thanks a lot for details.
> 
> If "discard" is a necessary condition, does it mean that on HDD we
> don't have the problem?
> 
> Do you think is enough for the moment to disable "discard" on nvme/ssd?

Discard is not necessary, but it does make the race window larger. So it
wouldn't hurt to disable it for now.

Re: [PATCH 1/2] btrfs: fix space cache corruption and potential double allocations

[Filipe Manana]

On Tue, Aug 16, 2022 at 04:12:15PM -0700, Omar Sandoval wrote:
> From: Omar Sandoval <osandov@fb.com>
> 
> When testing space_cache v2 on a large set of machines, we encountered a
> few symptoms:
> 
> 1. "unable to add free space :-17" (EEXIST) errors.
> 2. Missing free space info items, sometimes caught with a "missing free
>    space info for X" error.
> 3. Double-accounted space: ranges that were allocated in the extent tree
>    and also marked as free in the free space tree, ranges that were
>    marked as allocated twice in the extent tree, or ranges that were
>    marked as free twice in the free space tree. If the latter made it
>    onto disk, the next reboot would hit the BUG_ON() in
>    add_new_free_space().
> 4. On some hosts with no on-disk corruption or error messages, the
>    in-memory space cache (dumped with drgn) disagreed with the free
>    space tree.
> 
> All of these symptoms have the same underlying cause: a race between
> caching the free space for a block group and returning free space to the
> in-memory space cache for pinned extents causes us to double-add a free
> range to the space cache. This race exists when free space is cached
> from the free space tree (space_cache=v2) or the extent tree
> (nospace_cache, or space_cache=v1 if the cache needs to be regenerated).
> struct btrfs_block_group::last_byte_to_unpin and struct
> btrfs_block_group::progress are supposed to protect against this race,
> but commit d0c2f4fa555e ("btrfs: make concurrent fsyncs wait less when
> waiting for a transaction commit") subtly broke this by allowing
> multiple transactions to be unpinning extents at the same time.
> 
> Specifically, the race is as follows:
> 
> 1. An extent is deleted from an uncached block group in transaction A.
> 2. btrfs_commit_transaction() is called for transaction A.
> 3. btrfs_run_delayed_refs() -> __btrfs_free_extent() runs the delayed
>    ref for the deleted extent.
> 4. __btrfs_free_extent() -> do_free_extent_accounting() ->
>    add_to_free_space_tree() adds the deleted extent back to the free
>    space tree.
> 5. do_free_extent_accounting() -> btrfs_update_block_group() ->
>    btrfs_cache_block_group() queues up the block group to get cached.
>    block_group->progress is set to block_group->start.
> 6. btrfs_commit_transaction() for transaction A calls
>    switch_commit_roots(). It sets block_group->last_byte_to_unpin to
>    block_group->progress, which is block_group->start because the block
>    group hasn't been cached yet.
> 7. The caching thread gets to our block group. Since the commit roots
>    were already switched, load_free_space_tree() sees the deleted extent
>    as free and adds it to the space cache. It finishes caching and sets
>    block_group->progress to U64_MAX.
> 8. btrfs_commit_transaction() advances transaction A to
>    TRANS_STATE_SUPER_COMMITTED.
> 9. fsync calls btrfs_commit_transaction() for transaction B. Since
>    transaction A is already in TRANS_STATE_SUPER_COMMITTED and the
>    commit is for fsync, it advances.
> 10. btrfs_commit_transaction() for transaction B calls
>     switch_commit_roots(). This time, the block group has already been
>     cached, so it sets block_group->last_byte_to_unpin to U64_MAX.
> 11. btrfs_commit_transaction() for transaction A calls
>     btrfs_finish_extent_commit(), which calls unpin_extent_range() for
>     the deleted extent. It sees last_byte_to_unpin set to U64_MAX (by
>     transaction B!), so it adds the deleted extent to the space cache
>     again!
> 
> This explains all of our symptoms above:
> 
> * If the sequence of events is exactly as described above, when the free
>   space is re-added in step 11, it will fail with EEXIST.
> * If another thread reallocates the deleted extent in between steps 7
>   and 11, then step 11 will silently re-add that space to the space
>   cache as free even though it is actually allocated. Then, if that
>   space is allocated *again*, the free space tree will be corrupted
>  (namely, the wrong item will be deleted).
> * If we don't catch this free space tree corruption, it will continue
>   to get worse as extents are deleted and reallocated.
> 
> The v1 space_cache is synchronously loaded when an extent is deleted
> (btrfs_update_block_group() with alloc=0 calls btrfs_cache_block_group()
> with load_cache_only=1), so it is not normally affected by this bug.
> However, as noted above, if we fail to load the space cache, we will
> fall back to caching from the extent tree and may hit this bug.
> 
> The easiest fix for this race is to also make caching from the free
> space tree or extent tree synchronous. Josef tested this and found no
> performance regressions.
> 
> A few extra changes fall out of this change. Namely, this fix does the
> following, with step 2 being the crucial fix:
> 
> 1. Factor btrfs_caching_ctl_wait_done() out of
>    btrfs_wait_block_group_cache_done() to allow waiting on a caching_ctl
>    that we already hold a reference to.
> 2. Change the call in btrfs_cache_block_group() of
>    btrfs_wait_space_cache_v1_finished() to
>    btrfs_caching_ctl_wait_done(), which makes us wait regardless of the
>    space_cache option.
> 3. Delete the now unused btrfs_wait_space_cache_v1_finished() and
>    space_cache_v1_done().
> 4. Change btrfs_cache_block_group()'s `int load_cache_only` parameter to
>    `bool wait` to more accurately describe its new meaning.
> 5. Change a few callers which had a separate call to
>    btrfs_wait_block_group_cache_done() to use wait = true instead.
> 6. Make btrfs_wait_block_group_cache_done() static now that it's not
>    used outside of block-group.c anymore.
> 
> Fixes: d0c2f4fa555e ("btrfs: make concurrent fsyncs wait less when waiting for a transaction commit")
> Signed-off-by: Omar Sandoval <osandov@fb.com>

Looks good to me, thanks for fixing this and the very detailed analysis.
Only one comment inlined below, which is not critical, so:

Reviewed-by: Filipe Manana <fdmanana@suse.com>

> ---
>  fs/btrfs/block-group.c | 36 ++++++++++++++----------------------
>  fs/btrfs/block-group.h |  3 +--
>  fs/btrfs/extent-tree.c | 30 ++++++------------------------
>  3 files changed, 21 insertions(+), 48 deletions(-)
> 
> diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
> index c8162b8d85a2..1af6fc395a52 100644
> --- a/fs/btrfs/block-group.c
> +++ b/fs/btrfs/block-group.c
> @@ -440,33 +440,26 @@ void btrfs_wait_block_group_cache_progress(struct btrfs_block_group *cache,
>  	btrfs_put_caching_control(caching_ctl);
>  }
>  
> -int btrfs_wait_block_group_cache_done(struct btrfs_block_group *cache)
> +static int btrfs_caching_ctl_wait_done(struct btrfs_block_group *cache,
> +				      struct btrfs_caching_control *caching_ctl)
> +{
> +	wait_event(caching_ctl->wait, btrfs_block_group_done(cache));
> +	return cache->cached == BTRFS_CACHE_ERROR ? -EIO : 0;
> +}
> +
> +static int btrfs_wait_block_group_cache_done(struct btrfs_block_group *cache)
>  {
>  	struct btrfs_caching_control *caching_ctl;
> -	int ret = 0;
> +	int ret;
>  
>  	caching_ctl = btrfs_get_caching_control(cache);
>  	if (!caching_ctl)
>  		return (cache->cached == BTRFS_CACHE_ERROR) ? -EIO : 0;
> -
> -	wait_event(caching_ctl->wait, btrfs_block_group_done(cache));
> -	if (cache->cached == BTRFS_CACHE_ERROR)
> -		ret = -EIO;
> +	ret = btrfs_caching_ctl_wait_done(cache, caching_ctl);
>  	btrfs_put_caching_control(caching_ctl);
>  	return ret;
>  }
>  
> -static bool space_cache_v1_done(struct btrfs_block_group *cache)
> -{
> -	bool ret;
> -
> -	spin_lock(&cache->lock);
> -	ret = cache->cached != BTRFS_CACHE_FAST;

This BTRFS_CACHE_FAST state now becomes useless.
Any reason you didn't remove it?
Something like this on top of this patch:

diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
index c8162b8d85a2..9b2314f6ffed 100644
--- a/fs/btrfs/block-group.c
+++ b/fs/btrfs/block-group.c
@@ -779,10 +779,7 @@ int btrfs_cache_block_group(struct btrfs_block_group *cache, int load_cache_only
        }
        WARN_ON(cache->caching_ctl);
        cache->caching_ctl = caching_ctl;
-       if (btrfs_test_opt(fs_info, SPACE_CACHE))
-               cache->cached = BTRFS_CACHE_FAST;
-       else
-               cache->cached = BTRFS_CACHE_STARTED;
+       cache->cached = BTRFS_CACHE_STARTED;
        spin_unlock(&cache->lock);
 
        write_lock(&fs_info->block_group_cache_lock);
diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 51c480439263..0fe5f68aadc2 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -505,7 +505,6 @@ struct btrfs_free_cluster {
 enum btrfs_caching_type {
        BTRFS_CACHE_NO,
        BTRFS_CACHE_STARTED,
-       BTRFS_CACHE_FAST,
        BTRFS_CACHE_FINISHED,
        BTRFS_CACHE_ERROR,
 };


> -	spin_unlock(&cache->lock);
> -
> -	return ret;
> -}
> -
>  #ifdef CONFIG_BTRFS_DEBUG
>  static void fragment_free_space(struct btrfs_block_group *block_group)
>  {
> @@ -744,9 +737,8 @@ static noinline void caching_thread(struct btrfs_work *work)
>  	btrfs_put_block_group(block_group);
>  }
>  
> -int btrfs_cache_block_group(struct btrfs_block_group *cache, int load_cache_only)
> +int btrfs_cache_block_group(struct btrfs_block_group *cache, bool wait)
>  {
> -	DEFINE_WAIT(wait);
>  	struct btrfs_fs_info *fs_info = cache->fs_info;
>  	struct btrfs_caching_control *caching_ctl = NULL;
>  	int ret = 0;
> @@ -794,8 +786,8 @@ int btrfs_cache_block_group(struct btrfs_block_group *cache, int load_cache_only
>  
>  	btrfs_queue_work(fs_info->caching_workers, &caching_ctl->work);
>  out:
> -	if (load_cache_only && caching_ctl)
> -		wait_event(caching_ctl->wait, space_cache_v1_done(cache));
> +	if (wait && caching_ctl)
> +		ret = btrfs_caching_ctl_wait_done(cache, caching_ctl);
>  	if (caching_ctl)
>  		btrfs_put_caching_control(caching_ctl);
>  
> @@ -3288,7 +3280,7 @@ int btrfs_update_block_group(struct btrfs_trans_handle *trans,
>  		 * space back to the block group, otherwise we will leak space.
>  		 */
>  		if (!alloc && !btrfs_block_group_done(cache))
> -			btrfs_cache_block_group(cache, 1);
> +			btrfs_cache_block_group(cache, true);
>  
>  		byte_in_group = bytenr - cache->start;
>  		WARN_ON(byte_in_group > cache->length);
> diff --git a/fs/btrfs/block-group.h b/fs/btrfs/block-group.h
> index b5c8425839dc..9dba28bb1806 100644
> --- a/fs/btrfs/block-group.h
> +++ b/fs/btrfs/block-group.h
> @@ -267,9 +267,8 @@ void btrfs_dec_nocow_writers(struct btrfs_block_group *bg);
>  void btrfs_wait_nocow_writers(struct btrfs_block_group *bg);
>  void btrfs_wait_block_group_cache_progress(struct btrfs_block_group *cache,
>  				           u64 num_bytes);
> -int btrfs_wait_block_group_cache_done(struct btrfs_block_group *cache);
>  int btrfs_cache_block_group(struct btrfs_block_group *cache,
> -			    int load_cache_only);
> +			    bool wait);
>  void btrfs_put_caching_control(struct btrfs_caching_control *ctl);
>  struct btrfs_caching_control *btrfs_get_caching_control(
>  		struct btrfs_block_group *cache);
> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index 7bb05d49809b..86ac953c69ac 100644
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -2551,17 +2551,10 @@ int btrfs_pin_extent_for_log_replay(struct btrfs_trans_handle *trans,
>  		return -EINVAL;
>  
>  	/*
> -	 * pull in the free space cache (if any) so that our pin
> -	 * removes the free space from the cache.  We have load_only set
> -	 * to one because the slow code to read in the free extents does check
> -	 * the pinned extents.
> +	 * Fully cache the free space first so that our pin removes the free space
> +	 * from the cache.
>  	 */
> -	btrfs_cache_block_group(cache, 1);
> -	/*
> -	 * Make sure we wait until the cache is completely built in case it is
> -	 * missing or is invalid and therefore needs to be rebuilt.
> -	 */
> -	ret = btrfs_wait_block_group_cache_done(cache);
> +	ret = btrfs_cache_block_group(cache, true);
>  	if (ret)
>  		goto out;
>  
> @@ -2584,12 +2577,7 @@ static int __exclude_logged_extent(struct btrfs_fs_info *fs_info,
>  	if (!block_group)
>  		return -EINVAL;
>  
> -	btrfs_cache_block_group(block_group, 1);
> -	/*
> -	 * Make sure we wait until the cache is completely built in case it is
> -	 * missing or is invalid and therefore needs to be rebuilt.
> -	 */
> -	ret = btrfs_wait_block_group_cache_done(block_group);
> +	ret = btrfs_cache_block_group(block_group, true);
>  	if (ret)
>  		goto out;
>  
> @@ -4400,7 +4388,7 @@ static noinline int find_free_extent(struct btrfs_root *root,
>  		ffe_ctl->cached = btrfs_block_group_done(block_group);
>  		if (unlikely(!ffe_ctl->cached)) {
>  			ffe_ctl->have_caching_bg = true;
> -			ret = btrfs_cache_block_group(block_group, 0);
> +			ret = btrfs_cache_block_group(block_group, false);
>  
>  			/*
>  			 * If we get ENOMEM here or something else we want to
> @@ -6170,13 +6158,7 @@ int btrfs_trim_fs(struct btrfs_fs_info *fs_info, struct fstrim_range *range)
>  
>  		if (end - start >= range->minlen) {
>  			if (!btrfs_block_group_done(cache)) {
> -				ret = btrfs_cache_block_group(cache, 0);
> -				if (ret) {
> -					bg_failed++;
> -					bg_ret = ret;
> -					continue;
> -				}
> -				ret = btrfs_wait_block_group_cache_done(cache);
> +				ret = btrfs_cache_block_group(cache, true);
>  				if (ret) {
>  					bg_failed++;
>  					bg_ret = ret;
> -- 
> 2.37.2
> 

Re: [PATCH 1/2] btrfs: fix space cache corruption and potential double allocations

[Omar Sandoval]

On Wed, Aug 17, 2022 at 10:47:08AM +0100, Filipe Manana wrote:
> On Tue, Aug 16, 2022 at 04:12:15PM -0700, Omar Sandoval wrote:
> > From: Omar Sandoval <osandov@fb.com>
> > 
> > When testing space_cache v2 on a large set of machines, we encountered a
> > few symptoms:
> > 
> > 1. "unable to add free space :-17" (EEXIST) errors.
> > 2. Missing free space info items, sometimes caught with a "missing free
> >    space info for X" error.
> > 3. Double-accounted space: ranges that were allocated in the extent tree
> >    and also marked as free in the free space tree, ranges that were
> >    marked as allocated twice in the extent tree, or ranges that were
> >    marked as free twice in the free space tree. If the latter made it
> >    onto disk, the next reboot would hit the BUG_ON() in
> >    add_new_free_space().
> > 4. On some hosts with no on-disk corruption or error messages, the
> >    in-memory space cache (dumped with drgn) disagreed with the free
> >    space tree.
> > 
> > All of these symptoms have the same underlying cause: a race between
> > caching the free space for a block group and returning free space to the
> > in-memory space cache for pinned extents causes us to double-add a free
> > range to the space cache. This race exists when free space is cached
> > from the free space tree (space_cache=v2) or the extent tree
> > (nospace_cache, or space_cache=v1 if the cache needs to be regenerated).
> > struct btrfs_block_group::last_byte_to_unpin and struct
> > btrfs_block_group::progress are supposed to protect against this race,
> > but commit d0c2f4fa555e ("btrfs: make concurrent fsyncs wait less when
> > waiting for a transaction commit") subtly broke this by allowing
> > multiple transactions to be unpinning extents at the same time.
> > 
> > Specifically, the race is as follows:
> > 
> > 1. An extent is deleted from an uncached block group in transaction A.
> > 2. btrfs_commit_transaction() is called for transaction A.
> > 3. btrfs_run_delayed_refs() -> __btrfs_free_extent() runs the delayed
> >    ref for the deleted extent.
> > 4. __btrfs_free_extent() -> do_free_extent_accounting() ->
> >    add_to_free_space_tree() adds the deleted extent back to the free
> >    space tree.
> > 5. do_free_extent_accounting() -> btrfs_update_block_group() ->
> >    btrfs_cache_block_group() queues up the block group to get cached.
> >    block_group->progress is set to block_group->start.
> > 6. btrfs_commit_transaction() for transaction A calls
> >    switch_commit_roots(). It sets block_group->last_byte_to_unpin to
> >    block_group->progress, which is block_group->start because the block
> >    group hasn't been cached yet.
> > 7. The caching thread gets to our block group. Since the commit roots
> >    were already switched, load_free_space_tree() sees the deleted extent
> >    as free and adds it to the space cache. It finishes caching and sets
> >    block_group->progress to U64_MAX.
> > 8. btrfs_commit_transaction() advances transaction A to
> >    TRANS_STATE_SUPER_COMMITTED.
> > 9. fsync calls btrfs_commit_transaction() for transaction B. Since
> >    transaction A is already in TRANS_STATE_SUPER_COMMITTED and the
> >    commit is for fsync, it advances.
> > 10. btrfs_commit_transaction() for transaction B calls
> >     switch_commit_roots(). This time, the block group has already been
> >     cached, so it sets block_group->last_byte_to_unpin to U64_MAX.
> > 11. btrfs_commit_transaction() for transaction A calls
> >     btrfs_finish_extent_commit(), which calls unpin_extent_range() for
> >     the deleted extent. It sees last_byte_to_unpin set to U64_MAX (by
> >     transaction B!), so it adds the deleted extent to the space cache
> >     again!
> > 
> > This explains all of our symptoms above:
> > 
> > * If the sequence of events is exactly as described above, when the free
> >   space is re-added in step 11, it will fail with EEXIST.
> > * If another thread reallocates the deleted extent in between steps 7
> >   and 11, then step 11 will silently re-add that space to the space
> >   cache as free even though it is actually allocated. Then, if that
> >   space is allocated *again*, the free space tree will be corrupted
> >  (namely, the wrong item will be deleted).
> > * If we don't catch this free space tree corruption, it will continue
> >   to get worse as extents are deleted and reallocated.
> > 
> > The v1 space_cache is synchronously loaded when an extent is deleted
> > (btrfs_update_block_group() with alloc=0 calls btrfs_cache_block_group()
> > with load_cache_only=1), so it is not normally affected by this bug.
> > However, as noted above, if we fail to load the space cache, we will
> > fall back to caching from the extent tree and may hit this bug.
> > 
> > The easiest fix for this race is to also make caching from the free
> > space tree or extent tree synchronous. Josef tested this and found no
> > performance regressions.
> > 
> > A few extra changes fall out of this change. Namely, this fix does the
> > following, with step 2 being the crucial fix:
> > 
> > 1. Factor btrfs_caching_ctl_wait_done() out of
> >    btrfs_wait_block_group_cache_done() to allow waiting on a caching_ctl
> >    that we already hold a reference to.
> > 2. Change the call in btrfs_cache_block_group() of
> >    btrfs_wait_space_cache_v1_finished() to
> >    btrfs_caching_ctl_wait_done(), which makes us wait regardless of the
> >    space_cache option.
> > 3. Delete the now unused btrfs_wait_space_cache_v1_finished() and
> >    space_cache_v1_done().
> > 4. Change btrfs_cache_block_group()'s `int load_cache_only` parameter to
> >    `bool wait` to more accurately describe its new meaning.
> > 5. Change a few callers which had a separate call to
> >    btrfs_wait_block_group_cache_done() to use wait = true instead.
> > 6. Make btrfs_wait_block_group_cache_done() static now that it's not
> >    used outside of block-group.c anymore.
> > 
> > Fixes: d0c2f4fa555e ("btrfs: make concurrent fsyncs wait less when waiting for a transaction commit")
> > Signed-off-by: Omar Sandoval <osandov@fb.com>
> 
> Looks good to me, thanks for fixing this and the very detailed analysis.
> Only one comment inlined below, which is not critical, so:
> 
> Reviewed-by: Filipe Manana <fdmanana@suse.com>
> 
> > ---
> >  fs/btrfs/block-group.c | 36 ++++++++++++++----------------------
> >  fs/btrfs/block-group.h |  3 +--
> >  fs/btrfs/extent-tree.c | 30 ++++++------------------------
> >  3 files changed, 21 insertions(+), 48 deletions(-)
> > 
> > diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
> > index c8162b8d85a2..1af6fc395a52 100644
> > --- a/fs/btrfs/block-group.c
> > +++ b/fs/btrfs/block-group.c
> > @@ -440,33 +440,26 @@ void btrfs_wait_block_group_cache_progress(struct btrfs_block_group *cache,
> >  	btrfs_put_caching_control(caching_ctl);
> >  }
> >  
> > -int btrfs_wait_block_group_cache_done(struct btrfs_block_group *cache)
> > +static int btrfs_caching_ctl_wait_done(struct btrfs_block_group *cache,
> > +				      struct btrfs_caching_control *caching_ctl)
> > +{
> > +	wait_event(caching_ctl->wait, btrfs_block_group_done(cache));
> > +	return cache->cached == BTRFS_CACHE_ERROR ? -EIO : 0;
> > +}
> > +
> > +static int btrfs_wait_block_group_cache_done(struct btrfs_block_group *cache)
> >  {
> >  	struct btrfs_caching_control *caching_ctl;
> > -	int ret = 0;
> > +	int ret;
> >  
> >  	caching_ctl = btrfs_get_caching_control(cache);
> >  	if (!caching_ctl)
> >  		return (cache->cached == BTRFS_CACHE_ERROR) ? -EIO : 0;
> > -
> > -	wait_event(caching_ctl->wait, btrfs_block_group_done(cache));
> > -	if (cache->cached == BTRFS_CACHE_ERROR)
> > -		ret = -EIO;
> > +	ret = btrfs_caching_ctl_wait_done(cache, caching_ctl);
> >  	btrfs_put_caching_control(caching_ctl);
> >  	return ret;
> >  }
> >  
> > -static bool space_cache_v1_done(struct btrfs_block_group *cache)
> > -{
> > -	bool ret;
> > -
> > -	spin_lock(&cache->lock);
> > -	ret = cache->cached != BTRFS_CACHE_FAST;
> 
> This BTRFS_CACHE_FAST state now becomes useless.
> Any reason you didn't remove it?
> Something like this on top of this patch:
> 
> diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
> index c8162b8d85a2..9b2314f6ffed 100644
> --- a/fs/btrfs/block-group.c
> +++ b/fs/btrfs/block-group.c
> @@ -779,10 +779,7 @@ int btrfs_cache_block_group(struct btrfs_block_group *cache, int load_cache_only
>         }
>         WARN_ON(cache->caching_ctl);
>         cache->caching_ctl = caching_ctl;
> -       if (btrfs_test_opt(fs_info, SPACE_CACHE))
> -               cache->cached = BTRFS_CACHE_FAST;
> -       else
> -               cache->cached = BTRFS_CACHE_STARTED;
> +       cache->cached = BTRFS_CACHE_STARTED;
>         spin_unlock(&cache->lock);
>  
>         write_lock(&fs_info->block_group_cache_lock);
> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> index 51c480439263..0fe5f68aadc2 100644
> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -505,7 +505,6 @@ struct btrfs_free_cluster {
>  enum btrfs_caching_type {
>         BTRFS_CACHE_NO,
>         BTRFS_CACHE_STARTED,
> -       BTRFS_CACHE_FAST,
>         BTRFS_CACHE_FINISHED,
>         BTRFS_CACHE_ERROR,
>  };

You're right, I missed that. I can add that as another followup cleanup
patch unless you want to send it.

Re: [PATCH 1/2] btrfs: fix space cache corruption and potential double allocations

[Filipe Manana]

On Wed, Aug 17, 2022 at 4:32 PM Omar Sandoval <osandov@osandov.com> wrote:
>
> On Wed, Aug 17, 2022 at 10:47:08AM +0100, Filipe Manana wrote:
> > On Tue, Aug 16, 2022 at 04:12:15PM -0700, Omar Sandoval wrote:
> > > From: Omar Sandoval <osandov@fb.com>
> > >
> > > When testing space_cache v2 on a large set of machines, we encountered a
> > > few symptoms:
> > >
> > > 1. "unable to add free space :-17" (EEXIST) errors.
> > > 2. Missing free space info items, sometimes caught with a "missing free
> > >    space info for X" error.
> > > 3. Double-accounted space: ranges that were allocated in the extent tree
> > >    and also marked as free in the free space tree, ranges that were
> > >    marked as allocated twice in the extent tree, or ranges that were
> > >    marked as free twice in the free space tree. If the latter made it
> > >    onto disk, the next reboot would hit the BUG_ON() in
> > >    add_new_free_space().
> > > 4. On some hosts with no on-disk corruption or error messages, the
> > >    in-memory space cache (dumped with drgn) disagreed with the free
> > >    space tree.
> > >
> > > All of these symptoms have the same underlying cause: a race between
> > > caching the free space for a block group and returning free space to the
> > > in-memory space cache for pinned extents causes us to double-add a free
> > > range to the space cache. This race exists when free space is cached
> > > from the free space tree (space_cache=v2) or the extent tree
> > > (nospace_cache, or space_cache=v1 if the cache needs to be regenerated).
> > > struct btrfs_block_group::last_byte_to_unpin and struct
> > > btrfs_block_group::progress are supposed to protect against this race,
> > > but commit d0c2f4fa555e ("btrfs: make concurrent fsyncs wait less when
> > > waiting for a transaction commit") subtly broke this by allowing
> > > multiple transactions to be unpinning extents at the same time.
> > >
> > > Specifically, the race is as follows:
> > >
> > > 1. An extent is deleted from an uncached block group in transaction A.
> > > 2. btrfs_commit_transaction() is called for transaction A.
> > > 3. btrfs_run_delayed_refs() -> __btrfs_free_extent() runs the delayed
> > >    ref for the deleted extent.
> > > 4. __btrfs_free_extent() -> do_free_extent_accounting() ->
> > >    add_to_free_space_tree() adds the deleted extent back to the free
> > >    space tree.
> > > 5. do_free_extent_accounting() -> btrfs_update_block_group() ->
> > >    btrfs_cache_block_group() queues up the block group to get cached.
> > >    block_group->progress is set to block_group->start.
> > > 6. btrfs_commit_transaction() for transaction A calls
> > >    switch_commit_roots(). It sets block_group->last_byte_to_unpin to
> > >    block_group->progress, which is block_group->start because the block
> > >    group hasn't been cached yet.
> > > 7. The caching thread gets to our block group. Since the commit roots
> > >    were already switched, load_free_space_tree() sees the deleted extent
> > >    as free and adds it to the space cache. It finishes caching and sets
> > >    block_group->progress to U64_MAX.
> > > 8. btrfs_commit_transaction() advances transaction A to
> > >    TRANS_STATE_SUPER_COMMITTED.
> > > 9. fsync calls btrfs_commit_transaction() for transaction B. Since
> > >    transaction A is already in TRANS_STATE_SUPER_COMMITTED and the
> > >    commit is for fsync, it advances.
> > > 10. btrfs_commit_transaction() for transaction B calls
> > >     switch_commit_roots(). This time, the block group has already been
> > >     cached, so it sets block_group->last_byte_to_unpin to U64_MAX.
> > > 11. btrfs_commit_transaction() for transaction A calls
> > >     btrfs_finish_extent_commit(), which calls unpin_extent_range() for
> > >     the deleted extent. It sees last_byte_to_unpin set to U64_MAX (by
> > >     transaction B!), so it adds the deleted extent to the space cache
> > >     again!
> > >
> > > This explains all of our symptoms above:
> > >
> > > * If the sequence of events is exactly as described above, when the free
> > >   space is re-added in step 11, it will fail with EEXIST.
> > > * If another thread reallocates the deleted extent in between steps 7
> > >   and 11, then step 11 will silently re-add that space to the space
> > >   cache as free even though it is actually allocated. Then, if that
> > >   space is allocated *again*, the free space tree will be corrupted
> > >  (namely, the wrong item will be deleted).
> > > * If we don't catch this free space tree corruption, it will continue
> > >   to get worse as extents are deleted and reallocated.
> > >
> > > The v1 space_cache is synchronously loaded when an extent is deleted
> > > (btrfs_update_block_group() with alloc=0 calls btrfs_cache_block_group()
> > > with load_cache_only=1), so it is not normally affected by this bug.
> > > However, as noted above, if we fail to load the space cache, we will
> > > fall back to caching from the extent tree and may hit this bug.
> > >
> > > The easiest fix for this race is to also make caching from the free
> > > space tree or extent tree synchronous. Josef tested this and found no
> > > performance regressions.
> > >
> > > A few extra changes fall out of this change. Namely, this fix does the
> > > following, with step 2 being the crucial fix:
> > >
> > > 1. Factor btrfs_caching_ctl_wait_done() out of
> > >    btrfs_wait_block_group_cache_done() to allow waiting on a caching_ctl
> > >    that we already hold a reference to.
> > > 2. Change the call in btrfs_cache_block_group() of
> > >    btrfs_wait_space_cache_v1_finished() to
> > >    btrfs_caching_ctl_wait_done(), which makes us wait regardless of the
> > >    space_cache option.
> > > 3. Delete the now unused btrfs_wait_space_cache_v1_finished() and
> > >    space_cache_v1_done().
> > > 4. Change btrfs_cache_block_group()'s `int load_cache_only` parameter to
> > >    `bool wait` to more accurately describe its new meaning.
> > > 5. Change a few callers which had a separate call to
> > >    btrfs_wait_block_group_cache_done() to use wait = true instead.
> > > 6. Make btrfs_wait_block_group_cache_done() static now that it's not
> > >    used outside of block-group.c anymore.
> > >
> > > Fixes: d0c2f4fa555e ("btrfs: make concurrent fsyncs wait less when waiting for a transaction commit")
> > > Signed-off-by: Omar Sandoval <osandov@fb.com>
> >
> > Looks good to me, thanks for fixing this and the very detailed analysis.
> > Only one comment inlined below, which is not critical, so:
> >
> > Reviewed-by: Filipe Manana <fdmanana@suse.com>
> >
> > > ---
> > >  fs/btrfs/block-group.c | 36 ++++++++++++++----------------------
> > >  fs/btrfs/block-group.h |  3 +--
> > >  fs/btrfs/extent-tree.c | 30 ++++++------------------------
> > >  3 files changed, 21 insertions(+), 48 deletions(-)
> > >
> > > diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
> > > index c8162b8d85a2..1af6fc395a52 100644
> > > --- a/fs/btrfs/block-group.c
> > > +++ b/fs/btrfs/block-group.c
> > > @@ -440,33 +440,26 @@ void btrfs_wait_block_group_cache_progress(struct btrfs_block_group *cache,
> > >     btrfs_put_caching_control(caching_ctl);
> > >  }
> > >
> > > -int btrfs_wait_block_group_cache_done(struct btrfs_block_group *cache)
> > > +static int btrfs_caching_ctl_wait_done(struct btrfs_block_group *cache,
> > > +                                 struct btrfs_caching_control *caching_ctl)
> > > +{
> > > +   wait_event(caching_ctl->wait, btrfs_block_group_done(cache));
> > > +   return cache->cached == BTRFS_CACHE_ERROR ? -EIO : 0;
> > > +}
> > > +
> > > +static int btrfs_wait_block_group_cache_done(struct btrfs_block_group *cache)
> > >  {
> > >     struct btrfs_caching_control *caching_ctl;
> > > -   int ret = 0;
> > > +   int ret;
> > >
> > >     caching_ctl = btrfs_get_caching_control(cache);
> > >     if (!caching_ctl)
> > >             return (cache->cached == BTRFS_CACHE_ERROR) ? -EIO : 0;
> > > -
> > > -   wait_event(caching_ctl->wait, btrfs_block_group_done(cache));
> > > -   if (cache->cached == BTRFS_CACHE_ERROR)
> > > -           ret = -EIO;
> > > +   ret = btrfs_caching_ctl_wait_done(cache, caching_ctl);
> > >     btrfs_put_caching_control(caching_ctl);
> > >     return ret;
> > >  }
> > >
> > > -static bool space_cache_v1_done(struct btrfs_block_group *cache)
> > > -{
> > > -   bool ret;
> > > -
> > > -   spin_lock(&cache->lock);
> > > -   ret = cache->cached != BTRFS_CACHE_FAST;
> >
> > This BTRFS_CACHE_FAST state now becomes useless.
> > Any reason you didn't remove it?
> > Something like this on top of this patch:
> >
> > diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
> > index c8162b8d85a2..9b2314f6ffed 100644
> > --- a/fs/btrfs/block-group.c
> > +++ b/fs/btrfs/block-group.c
> > @@ -779,10 +779,7 @@ int btrfs_cache_block_group(struct btrfs_block_group *cache, int load_cache_only
> >         }
> >         WARN_ON(cache->caching_ctl);
> >         cache->caching_ctl = caching_ctl;
> > -       if (btrfs_test_opt(fs_info, SPACE_CACHE))
> > -               cache->cached = BTRFS_CACHE_FAST;
> > -       else
> > -               cache->cached = BTRFS_CACHE_STARTED;
> > +       cache->cached = BTRFS_CACHE_STARTED;
> >         spin_unlock(&cache->lock);
> >
> >         write_lock(&fs_info->block_group_cache_lock);
> > diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> > index 51c480439263..0fe5f68aadc2 100644
> > --- a/fs/btrfs/ctree.h
> > +++ b/fs/btrfs/ctree.h
> > @@ -505,7 +505,6 @@ struct btrfs_free_cluster {
> >  enum btrfs_caching_type {
> >         BTRFS_CACHE_NO,
> >         BTRFS_CACHE_STARTED,
> > -       BTRFS_CACHE_FAST,
> >         BTRFS_CACHE_FINISHED,
> >         BTRFS_CACHE_ERROR,
> >  };
>
> You're right, I missed that. I can add that as another followup cleanup
> patch unless you want to send it.

Feel free to add it. I think it makes more sense to squash into this
patch rather
than add it as a separate patch, but I don't disagree with doing it that way.

Thanks.

Re: [PATCH 1/2] btrfs: fix space cache corruption and potential double allocations

[David Sterba]

On Wed, Aug 17, 2022 at 04:46:01PM +0100, Filipe Manana wrote:
> On Wed, Aug 17, 2022 at 4:32 PM Omar Sandoval <osandov@osandov.com> wrote:
> > > --- a/fs/btrfs/ctree.h
> > > +++ b/fs/btrfs/ctree.h
> > > @@ -505,7 +505,6 @@ struct btrfs_free_cluster {
> > >  enum btrfs_caching_type {
> > >         BTRFS_CACHE_NO,
> > >         BTRFS_CACHE_STARTED,
> > > -       BTRFS_CACHE_FAST,
> > >         BTRFS_CACHE_FINISHED,
> > >         BTRFS_CACHE_ERROR,
> > >  };
> >
> > You're right, I missed that. I can add that as another followup cleanup
> > patch unless you want to send it.
> 
> Feel free to add it. I think it makes more sense to squash into this
> patch rather
> than add it as a separate patch, but I don't disagree with doing it that way.

Folded to the patch, thanks.

[PATCH 2/2] btrfs: get rid of block group caching progress logic

[Omar Sandoval]

From: Omar Sandoval <osandov@fb.com>

struct btrfs_caching_ctl::progress and struct
btrfs_block_group::last_byte_to_unpin were previously needed to ensure
that unpin_extent_range() didn't return a range to the free space cache
before the caching thread had a chance to cache that range. However, the
previous commit made it so that we always synchronously cache the block
group at the time that we pin the extent, so this machinery is no longer
necessary.

Signed-off-by: Omar Sandoval <osandov@fb.com>
---
 fs/btrfs/block-group.c     | 13 ------------
 fs/btrfs/block-group.h     |  2 --
 fs/btrfs/extent-tree.c     |  9 ++-------
 fs/btrfs/free-space-tree.c |  8 --------
 fs/btrfs/transaction.c     | 41 --------------------------------------
 fs/btrfs/zoned.c           |  1 -
 6 files changed, 2 insertions(+), 72 deletions(-)

diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
index 1af6fc395a52..68992ad9ff2a 100644
--- a/fs/btrfs/block-group.c
+++ b/fs/btrfs/block-group.c
@@ -593,8 +593,6 @@ static int load_extent_tree_free(struct btrfs_caching_control *caching_ctl)
 
 			if (need_resched() ||
 			    rwsem_is_contended(&fs_info->commit_root_sem)) {
-				if (wakeup)
-					caching_ctl->progress = last;
 				btrfs_release_path(path);
 				up_read(&fs_info->commit_root_sem);
 				mutex_unlock(&caching_ctl->mutex);
@@ -618,9 +616,6 @@ static int load_extent_tree_free(struct btrfs_caching_control *caching_ctl)
 			key.objectid = last;
 			key.offset = 0;
 			key.type = BTRFS_EXTENT_ITEM_KEY;
-
-			if (wakeup)
-				caching_ctl->progress = last;
 			btrfs_release_path(path);
 			goto next;
 		}
@@ -655,7 +650,6 @@ static int load_extent_tree_free(struct btrfs_caching_control *caching_ctl)
 
 	total_found += add_new_free_space(block_group, last,
 				block_group->start + block_group->length);
-	caching_ctl->progress = (u64)-1;
 
 out:
 	btrfs_free_path(path);
@@ -725,8 +719,6 @@ static noinline void caching_thread(struct btrfs_work *work)
 	}
 #endif
 
-	caching_ctl->progress = (u64)-1;
-
 	up_read(&fs_info->commit_root_sem);
 	btrfs_free_excluded_extents(block_group);
 	mutex_unlock(&caching_ctl->mutex);
@@ -755,7 +747,6 @@ int btrfs_cache_block_group(struct btrfs_block_group *cache, bool wait)
 	mutex_init(&caching_ctl->mutex);
 	init_waitqueue_head(&caching_ctl->wait);
 	caching_ctl->block_group = cache;
-	caching_ctl->progress = cache->start;
 	refcount_set(&caching_ctl->count, 2);
 	btrfs_init_work(&caching_ctl->work, caching_thread, NULL, NULL);
 
@@ -2076,11 +2067,9 @@ static int read_one_block_group(struct btrfs_fs_info *info,
 		/* Should not have any excluded extents. Just in case, though. */
 		btrfs_free_excluded_extents(cache);
 	} else if (cache->length == cache->used) {
-		cache->last_byte_to_unpin = (u64)-1;
 		cache->cached = BTRFS_CACHE_FINISHED;
 		btrfs_free_excluded_extents(cache);
 	} else if (cache->used == 0) {
-		cache->last_byte_to_unpin = (u64)-1;
 		cache->cached = BTRFS_CACHE_FINISHED;
 		add_new_free_space(cache, cache->start,
 				   cache->start + cache->length);
@@ -2136,7 +2125,6 @@ static int fill_dummy_bgs(struct btrfs_fs_info *fs_info)
 		/* Fill dummy cache as FULL */
 		bg->length = em->len;
 		bg->flags = map->type;
-		bg->last_byte_to_unpin = (u64)-1;
 		bg->cached = BTRFS_CACHE_FINISHED;
 		bg->used = em->len;
 		bg->flags = map->type;
@@ -2482,7 +2470,6 @@ struct btrfs_block_group *btrfs_make_block_group(struct btrfs_trans_handle *tran
 	set_free_space_tree_thresholds(cache);
 	cache->used = bytes_used;
 	cache->flags = type;
-	cache->last_byte_to_unpin = (u64)-1;
 	cache->cached = BTRFS_CACHE_FINISHED;
 	cache->global_root_id = calculate_global_root_id(fs_info, cache->start);
 
diff --git a/fs/btrfs/block-group.h b/fs/btrfs/block-group.h
index 9dba28bb1806..59a86e82a28e 100644
--- a/fs/btrfs/block-group.h
+++ b/fs/btrfs/block-group.h
@@ -63,7 +63,6 @@ struct btrfs_caching_control {
 	wait_queue_head_t wait;
 	struct btrfs_work work;
 	struct btrfs_block_group *block_group;
-	u64 progress;
 	refcount_t count;
 };
 
@@ -115,7 +114,6 @@ struct btrfs_block_group {
 	/* Cache tracking stuff */
 	int cached;
 	struct btrfs_caching_control *caching_ctl;
-	u64 last_byte_to_unpin;
 
 	struct btrfs_space_info *space_info;
 
diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index 86ac953c69ac..bcd0e72cded3 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -2686,13 +2686,8 @@ static int unpin_extent_range(struct btrfs_fs_info *fs_info,
 		len = cache->start + cache->length - start;
 		len = min(len, end + 1 - start);
 
-		down_read(&fs_info->commit_root_sem);
-		if (start < cache->last_byte_to_unpin && return_free_space) {
-			u64 add_len = min(len, cache->last_byte_to_unpin - start);
-
-			btrfs_add_free_space(cache, start, add_len);
-		}
-		up_read(&fs_info->commit_root_sem);
+		if (return_free_space)
+			btrfs_add_free_space(cache, start, len);
 
 		start += len;
 		total_unpinned += len;
diff --git a/fs/btrfs/free-space-tree.c b/fs/btrfs/free-space-tree.c
index 1bf89aa67216..367bcfcf68f5 100644
--- a/fs/btrfs/free-space-tree.c
+++ b/fs/btrfs/free-space-tree.c
@@ -1453,8 +1453,6 @@ static int load_free_space_bitmaps(struct btrfs_caching_control *caching_ctl,
 		ASSERT(key.type == BTRFS_FREE_SPACE_BITMAP_KEY);
 		ASSERT(key.objectid < end && key.objectid + key.offset <= end);
 
-		caching_ctl->progress = key.objectid;
-
 		offset = key.objectid;
 		while (offset < key.objectid + key.offset) {
 			bit = free_space_test_bit(block_group, path, offset);
@@ -1490,8 +1488,6 @@ static int load_free_space_bitmaps(struct btrfs_caching_control *caching_ctl,
 		goto out;
 	}
 
-	caching_ctl->progress = (u64)-1;
-
 	ret = 0;
 out:
 	return ret;
@@ -1531,8 +1527,6 @@ static int load_free_space_extents(struct btrfs_caching_control *caching_ctl,
 		ASSERT(key.type == BTRFS_FREE_SPACE_EXTENT_KEY);
 		ASSERT(key.objectid < end && key.objectid + key.offset <= end);
 
-		caching_ctl->progress = key.objectid;
-
 		total_found += add_new_free_space(block_group, key.objectid,
 						  key.objectid + key.offset);
 		if (total_found > CACHING_CTL_WAKE_UP) {
@@ -1552,8 +1546,6 @@ static int load_free_space_extents(struct btrfs_caching_control *caching_ctl,
 		goto out;
 	}
 
-	caching_ctl->progress = (u64)-1;
-
 	ret = 0;
 out:
 	return ret;
diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
index 6e3b2cb6a04a..4c87bf2abc14 100644
--- a/fs/btrfs/transaction.c
+++ b/fs/btrfs/transaction.c
@@ -161,7 +161,6 @@ static noinline void switch_commit_roots(struct btrfs_trans_handle *trans)
 	struct btrfs_transaction *cur_trans = trans->transaction;
 	struct btrfs_fs_info *fs_info = trans->fs_info;
 	struct btrfs_root *root, *tmp;
-	struct btrfs_caching_control *caching_ctl, *next;
 
 	/*
 	 * At this point no one can be using this transaction to modify any tree
@@ -196,46 +195,6 @@ static noinline void switch_commit_roots(struct btrfs_trans_handle *trans)
 	}
 	spin_unlock(&cur_trans->dropped_roots_lock);
 
-	/*
-	 * We have to update the last_byte_to_unpin under the commit_root_sem,
-	 * at the same time we swap out the commit roots.
-	 *
-	 * This is because we must have a real view of the last spot the caching
-	 * kthreads were while caching.  Consider the following views of the
-	 * extent tree for a block group
-	 *
-	 * commit root
-	 * +----+----+----+----+----+----+----+
-	 * |\\\\|    |\\\\|\\\\|    |\\\\|\\\\|
-	 * +----+----+----+----+----+----+----+
-	 * 0    1    2    3    4    5    6    7
-	 *
-	 * new commit root
-	 * +----+----+----+----+----+----+----+
-	 * |    |    |    |\\\\|    |    |\\\\|
-	 * +----+----+----+----+----+----+----+
-	 * 0    1    2    3    4    5    6    7
-	 *
-	 * If the cache_ctl->progress was at 3, then we are only allowed to
-	 * unpin [0,1) and [2,3], because the caching thread has already
-	 * processed those extents.  We are not allowed to unpin [5,6), because
-	 * the caching thread will re-start it's search from 3, and thus find
-	 * the hole from [4,6) to add to the free space cache.
-	 */
-	write_lock(&fs_info->block_group_cache_lock);
-	list_for_each_entry_safe(caching_ctl, next,
-				 &fs_info->caching_block_groups, list) {
-		struct btrfs_block_group *cache = caching_ctl->block_group;
-
-		if (btrfs_block_group_done(cache)) {
-			cache->last_byte_to_unpin = (u64)-1;
-			list_del_init(&caching_ctl->list);
-			btrfs_put_caching_control(caching_ctl);
-		} else {
-			cache->last_byte_to_unpin = caching_ctl->progress;
-		}
-	}
-	write_unlock(&fs_info->block_group_cache_lock);
 	up_write(&fs_info->commit_root_sem);
 }
 
diff --git a/fs/btrfs/zoned.c b/fs/btrfs/zoned.c
index 61ae58c3a354..56a147a6e571 100644
--- a/fs/btrfs/zoned.c
+++ b/fs/btrfs/zoned.c
@@ -1558,7 +1558,6 @@ void btrfs_calc_zone_unusable(struct btrfs_block_group *cache)
 	free = cache->zone_capacity - cache->alloc_offset;
 
 	/* We only need ->free_space in ALLOC_SEQ block groups */
-	cache->last_byte_to_unpin = (u64)-1;
 	cache->cached = BTRFS_CACHE_FINISHED;
 	cache->free_space_ctl->free_space = free;
 	cache->zone_unusable = unusable;
-- 
2.37.2

Re: [PATCH 2/2] btrfs: get rid of block group caching progress logic

[Filipe Manana]

On Tue, Aug 16, 2022 at 04:12:16PM -0700, Omar Sandoval wrote:
> From: Omar Sandoval <osandov@fb.com>
> 
> struct btrfs_caching_ctl::progress and struct
> btrfs_block_group::last_byte_to_unpin were previously needed to ensure
> that unpin_extent_range() didn't return a range to the free space cache
> before the caching thread had a chance to cache that range. However, the
> previous commit made it so that we always synchronously cache the block
> group at the time that we pin the extent, so this machinery is no longer
> necessary.
> 
> Signed-off-by: Omar Sandoval <osandov@fb.com>

Reviewed-by: Filipe Manana <fdmanana@suse.com>

Looks good, thanks.

> ---
>  fs/btrfs/block-group.c     | 13 ------------
>  fs/btrfs/block-group.h     |  2 --
>  fs/btrfs/extent-tree.c     |  9 ++-------
>  fs/btrfs/free-space-tree.c |  8 --------
>  fs/btrfs/transaction.c     | 41 --------------------------------------
>  fs/btrfs/zoned.c           |  1 -
>  6 files changed, 2 insertions(+), 72 deletions(-)
> 
> diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
> index 1af6fc395a52..68992ad9ff2a 100644
> --- a/fs/btrfs/block-group.c
> +++ b/fs/btrfs/block-group.c
> @@ -593,8 +593,6 @@ static int load_extent_tree_free(struct btrfs_caching_control *caching_ctl)
>  
>  			if (need_resched() ||
>  			    rwsem_is_contended(&fs_info->commit_root_sem)) {
> -				if (wakeup)
> -					caching_ctl->progress = last;
>  				btrfs_release_path(path);
>  				up_read(&fs_info->commit_root_sem);
>  				mutex_unlock(&caching_ctl->mutex);
> @@ -618,9 +616,6 @@ static int load_extent_tree_free(struct btrfs_caching_control *caching_ctl)
>  			key.objectid = last;
>  			key.offset = 0;
>  			key.type = BTRFS_EXTENT_ITEM_KEY;
> -
> -			if (wakeup)
> -				caching_ctl->progress = last;
>  			btrfs_release_path(path);
>  			goto next;
>  		}
> @@ -655,7 +650,6 @@ static int load_extent_tree_free(struct btrfs_caching_control *caching_ctl)
>  
>  	total_found += add_new_free_space(block_group, last,
>  				block_group->start + block_group->length);
> -	caching_ctl->progress = (u64)-1;
>  
>  out:
>  	btrfs_free_path(path);
> @@ -725,8 +719,6 @@ static noinline void caching_thread(struct btrfs_work *work)
>  	}
>  #endif
>  
> -	caching_ctl->progress = (u64)-1;
> -
>  	up_read(&fs_info->commit_root_sem);
>  	btrfs_free_excluded_extents(block_group);
>  	mutex_unlock(&caching_ctl->mutex);
> @@ -755,7 +747,6 @@ int btrfs_cache_block_group(struct btrfs_block_group *cache, bool wait)
>  	mutex_init(&caching_ctl->mutex);
>  	init_waitqueue_head(&caching_ctl->wait);
>  	caching_ctl->block_group = cache;
> -	caching_ctl->progress = cache->start;
>  	refcount_set(&caching_ctl->count, 2);
>  	btrfs_init_work(&caching_ctl->work, caching_thread, NULL, NULL);
>  
> @@ -2076,11 +2067,9 @@ static int read_one_block_group(struct btrfs_fs_info *info,
>  		/* Should not have any excluded extents. Just in case, though. */
>  		btrfs_free_excluded_extents(cache);
>  	} else if (cache->length == cache->used) {
> -		cache->last_byte_to_unpin = (u64)-1;
>  		cache->cached = BTRFS_CACHE_FINISHED;
>  		btrfs_free_excluded_extents(cache);
>  	} else if (cache->used == 0) {
> -		cache->last_byte_to_unpin = (u64)-1;
>  		cache->cached = BTRFS_CACHE_FINISHED;
>  		add_new_free_space(cache, cache->start,
>  				   cache->start + cache->length);
> @@ -2136,7 +2125,6 @@ static int fill_dummy_bgs(struct btrfs_fs_info *fs_info)
>  		/* Fill dummy cache as FULL */
>  		bg->length = em->len;
>  		bg->flags = map->type;
> -		bg->last_byte_to_unpin = (u64)-1;
>  		bg->cached = BTRFS_CACHE_FINISHED;
>  		bg->used = em->len;
>  		bg->flags = map->type;
> @@ -2482,7 +2470,6 @@ struct btrfs_block_group *btrfs_make_block_group(struct btrfs_trans_handle *tran
>  	set_free_space_tree_thresholds(cache);
>  	cache->used = bytes_used;
>  	cache->flags = type;
> -	cache->last_byte_to_unpin = (u64)-1;
>  	cache->cached = BTRFS_CACHE_FINISHED;
>  	cache->global_root_id = calculate_global_root_id(fs_info, cache->start);
>  
> diff --git a/fs/btrfs/block-group.h b/fs/btrfs/block-group.h
> index 9dba28bb1806..59a86e82a28e 100644
> --- a/fs/btrfs/block-group.h
> +++ b/fs/btrfs/block-group.h
> @@ -63,7 +63,6 @@ struct btrfs_caching_control {
>  	wait_queue_head_t wait;
>  	struct btrfs_work work;
>  	struct btrfs_block_group *block_group;
> -	u64 progress;
>  	refcount_t count;
>  };
>  
> @@ -115,7 +114,6 @@ struct btrfs_block_group {
>  	/* Cache tracking stuff */
>  	int cached;
>  	struct btrfs_caching_control *caching_ctl;
> -	u64 last_byte_to_unpin;
>  
>  	struct btrfs_space_info *space_info;
>  
> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index 86ac953c69ac..bcd0e72cded3 100644
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -2686,13 +2686,8 @@ static int unpin_extent_range(struct btrfs_fs_info *fs_info,
>  		len = cache->start + cache->length - start;
>  		len = min(len, end + 1 - start);
>  
> -		down_read(&fs_info->commit_root_sem);
> -		if (start < cache->last_byte_to_unpin && return_free_space) {
> -			u64 add_len = min(len, cache->last_byte_to_unpin - start);
> -
> -			btrfs_add_free_space(cache, start, add_len);
> -		}
> -		up_read(&fs_info->commit_root_sem);
> +		if (return_free_space)
> +			btrfs_add_free_space(cache, start, len);
>  
>  		start += len;
>  		total_unpinned += len;
> diff --git a/fs/btrfs/free-space-tree.c b/fs/btrfs/free-space-tree.c
> index 1bf89aa67216..367bcfcf68f5 100644
> --- a/fs/btrfs/free-space-tree.c
> +++ b/fs/btrfs/free-space-tree.c
> @@ -1453,8 +1453,6 @@ static int load_free_space_bitmaps(struct btrfs_caching_control *caching_ctl,
>  		ASSERT(key.type == BTRFS_FREE_SPACE_BITMAP_KEY);
>  		ASSERT(key.objectid < end && key.objectid + key.offset <= end);
>  
> -		caching_ctl->progress = key.objectid;
> -
>  		offset = key.objectid;
>  		while (offset < key.objectid + key.offset) {
>  			bit = free_space_test_bit(block_group, path, offset);
> @@ -1490,8 +1488,6 @@ static int load_free_space_bitmaps(struct btrfs_caching_control *caching_ctl,
>  		goto out;
>  	}
>  
> -	caching_ctl->progress = (u64)-1;
> -
>  	ret = 0;
>  out:
>  	return ret;
> @@ -1531,8 +1527,6 @@ static int load_free_space_extents(struct btrfs_caching_control *caching_ctl,
>  		ASSERT(key.type == BTRFS_FREE_SPACE_EXTENT_KEY);
>  		ASSERT(key.objectid < end && key.objectid + key.offset <= end);
>  
> -		caching_ctl->progress = key.objectid;
> -
>  		total_found += add_new_free_space(block_group, key.objectid,
>  						  key.objectid + key.offset);
>  		if (total_found > CACHING_CTL_WAKE_UP) {
> @@ -1552,8 +1546,6 @@ static int load_free_space_extents(struct btrfs_caching_control *caching_ctl,
>  		goto out;
>  	}
>  
> -	caching_ctl->progress = (u64)-1;
> -
>  	ret = 0;
>  out:
>  	return ret;
> diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
> index 6e3b2cb6a04a..4c87bf2abc14 100644
> --- a/fs/btrfs/transaction.c
> +++ b/fs/btrfs/transaction.c
> @@ -161,7 +161,6 @@ static noinline void switch_commit_roots(struct btrfs_trans_handle *trans)
>  	struct btrfs_transaction *cur_trans = trans->transaction;
>  	struct btrfs_fs_info *fs_info = trans->fs_info;
>  	struct btrfs_root *root, *tmp;
> -	struct btrfs_caching_control *caching_ctl, *next;
>  
>  	/*
>  	 * At this point no one can be using this transaction to modify any tree
> @@ -196,46 +195,6 @@ static noinline void switch_commit_roots(struct btrfs_trans_handle *trans)
>  	}
>  	spin_unlock(&cur_trans->dropped_roots_lock);
>  
> -	/*
> -	 * We have to update the last_byte_to_unpin under the commit_root_sem,
> -	 * at the same time we swap out the commit roots.
> -	 *
> -	 * This is because we must have a real view of the last spot the caching
> -	 * kthreads were while caching.  Consider the following views of the
> -	 * extent tree for a block group
> -	 *
> -	 * commit root
> -	 * +----+----+----+----+----+----+----+
> -	 * |\\\\|    |\\\\|\\\\|    |\\\\|\\\\|
> -	 * +----+----+----+----+----+----+----+
> -	 * 0    1    2    3    4    5    6    7
> -	 *
> -	 * new commit root
> -	 * +----+----+----+----+----+----+----+
> -	 * |    |    |    |\\\\|    |    |\\\\|
> -	 * +----+----+----+----+----+----+----+
> -	 * 0    1    2    3    4    5    6    7
> -	 *
> -	 * If the cache_ctl->progress was at 3, then we are only allowed to
> -	 * unpin [0,1) and [2,3], because the caching thread has already
> -	 * processed those extents.  We are not allowed to unpin [5,6), because
> -	 * the caching thread will re-start it's search from 3, and thus find
> -	 * the hole from [4,6) to add to the free space cache.
> -	 */
> -	write_lock(&fs_info->block_group_cache_lock);
> -	list_for_each_entry_safe(caching_ctl, next,
> -				 &fs_info->caching_block_groups, list) {
> -		struct btrfs_block_group *cache = caching_ctl->block_group;
> -
> -		if (btrfs_block_group_done(cache)) {
> -			cache->last_byte_to_unpin = (u64)-1;
> -			list_del_init(&caching_ctl->list);
> -			btrfs_put_caching_control(caching_ctl);
> -		} else {
> -			cache->last_byte_to_unpin = caching_ctl->progress;
> -		}
> -	}
> -	write_unlock(&fs_info->block_group_cache_lock);
>  	up_write(&fs_info->commit_root_sem);
>  }
>  
> diff --git a/fs/btrfs/zoned.c b/fs/btrfs/zoned.c
> index 61ae58c3a354..56a147a6e571 100644
> --- a/fs/btrfs/zoned.c
> +++ b/fs/btrfs/zoned.c
> @@ -1558,7 +1558,6 @@ void btrfs_calc_zone_unusable(struct btrfs_block_group *cache)
>  	free = cache->zone_capacity - cache->alloc_offset;
>  
>  	/* We only need ->free_space in ALLOC_SEQ block groups */
> -	cache->last_byte_to_unpin = (u64)-1;
>  	cache->cached = BTRFS_CACHE_FINISHED;
>  	cache->free_space_ctl->free_space = free;
>  	cache->zone_unusable = unusable;
> -- 
> 2.37.2
> 

Re: [PATCH 2/2] btrfs: get rid of block group caching progress logic

[Alex Lyakas]

Hi Omar,

On Wed, Aug 17, 2022 at 2:13 AM Omar Sandoval <osandov@osandov.com> wrote:
>
> From: Omar Sandoval <osandov@fb.com>
>
> struct btrfs_caching_ctl::progress and struct
> btrfs_block_group::last_byte_to_unpin were previously needed to ensure
> that unpin_extent_range() didn't return a range to the free space cache
> before the caching thread had a chance to cache that range. However, the
> previous commit made it so that we always synchronously cache the block
> group at the time that we pin the extent, so this machinery is no longer
> necessary.
>
> Signed-off-by: Omar Sandoval <osandov@fb.com>
> ---
>  fs/btrfs/block-group.c     | 13 ------------
>  fs/btrfs/block-group.h     |  2 --
>  fs/btrfs/extent-tree.c     |  9 ++-------
>  fs/btrfs/free-space-tree.c |  8 --------
>  fs/btrfs/transaction.c     | 41 --------------------------------------
>  fs/btrfs/zoned.c           |  1 -
>  6 files changed, 2 insertions(+), 72 deletions(-)
>
> diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
> index 1af6fc395a52..68992ad9ff2a 100644
> --- a/fs/btrfs/block-group.c
> +++ b/fs/btrfs/block-group.c
> @@ -593,8 +593,6 @@ static int load_extent_tree_free(struct btrfs_caching_control *caching_ctl)
>
>                         if (need_resched() ||
>                             rwsem_is_contended(&fs_info->commit_root_sem)) {
> -                               if (wakeup)
> -                                       caching_ctl->progress = last;
>                                 btrfs_release_path(path);
>                                 up_read(&fs_info->commit_root_sem);
>                                 mutex_unlock(&caching_ctl->mutex);
> @@ -618,9 +616,6 @@ static int load_extent_tree_free(struct btrfs_caching_control *caching_ctl)
>                         key.objectid = last;
>                         key.offset = 0;
>                         key.type = BTRFS_EXTENT_ITEM_KEY;
> -
> -                       if (wakeup)
> -                               caching_ctl->progress = last;
>                         btrfs_release_path(path);
>                         goto next;
>                 }
> @@ -655,7 +650,6 @@ static int load_extent_tree_free(struct btrfs_caching_control *caching_ctl)
>
>         total_found += add_new_free_space(block_group, last,
>                                 block_group->start + block_group->length);
> -       caching_ctl->progress = (u64)-1;
>
>  out:
>         btrfs_free_path(path);
> @@ -725,8 +719,6 @@ static noinline void caching_thread(struct btrfs_work *work)
>         }
>  #endif
>
> -       caching_ctl->progress = (u64)-1;
> -
>         up_read(&fs_info->commit_root_sem);
>         btrfs_free_excluded_extents(block_group);
>         mutex_unlock(&caching_ctl->mutex);
> @@ -755,7 +747,6 @@ int btrfs_cache_block_group(struct btrfs_block_group *cache, bool wait)
>         mutex_init(&caching_ctl->mutex);
>         init_waitqueue_head(&caching_ctl->wait);
>         caching_ctl->block_group = cache;
> -       caching_ctl->progress = cache->start;
>         refcount_set(&caching_ctl->count, 2);
>         btrfs_init_work(&caching_ctl->work, caching_thread, NULL, NULL);
>
> @@ -2076,11 +2067,9 @@ static int read_one_block_group(struct btrfs_fs_info *info,
>                 /* Should not have any excluded extents. Just in case, though. */
>                 btrfs_free_excluded_extents(cache);
>         } else if (cache->length == cache->used) {
> -               cache->last_byte_to_unpin = (u64)-1;
>                 cache->cached = BTRFS_CACHE_FINISHED;
>                 btrfs_free_excluded_extents(cache);
>         } else if (cache->used == 0) {
> -               cache->last_byte_to_unpin = (u64)-1;
>                 cache->cached = BTRFS_CACHE_FINISHED;
>                 add_new_free_space(cache, cache->start,
>                                    cache->start + cache->length);
> @@ -2136,7 +2125,6 @@ static int fill_dummy_bgs(struct btrfs_fs_info *fs_info)
>                 /* Fill dummy cache as FULL */
>                 bg->length = em->len;
>                 bg->flags = map->type;
> -               bg->last_byte_to_unpin = (u64)-1;
>                 bg->cached = BTRFS_CACHE_FINISHED;
>                 bg->used = em->len;
>                 bg->flags = map->type;
> @@ -2482,7 +2470,6 @@ struct btrfs_block_group *btrfs_make_block_group(struct btrfs_trans_handle *tran
>         set_free_space_tree_thresholds(cache);
>         cache->used = bytes_used;
>         cache->flags = type;
> -       cache->last_byte_to_unpin = (u64)-1;
>         cache->cached = BTRFS_CACHE_FINISHED;
>         cache->global_root_id = calculate_global_root_id(fs_info, cache->start);
>
> diff --git a/fs/btrfs/block-group.h b/fs/btrfs/block-group.h
> index 9dba28bb1806..59a86e82a28e 100644
> --- a/fs/btrfs/block-group.h
> +++ b/fs/btrfs/block-group.h
> @@ -63,7 +63,6 @@ struct btrfs_caching_control {
>         wait_queue_head_t wait;
>         struct btrfs_work work;
>         struct btrfs_block_group *block_group;
> -       u64 progress;
>         refcount_t count;
>  };
>
> @@ -115,7 +114,6 @@ struct btrfs_block_group {
>         /* Cache tracking stuff */
>         int cached;
>         struct btrfs_caching_control *caching_ctl;
> -       u64 last_byte_to_unpin;
>
>         struct btrfs_space_info *space_info;
>
> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index 86ac953c69ac..bcd0e72cded3 100644
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -2686,13 +2686,8 @@ static int unpin_extent_range(struct btrfs_fs_info *fs_info,
>                 len = cache->start + cache->length - start;
>                 len = min(len, end + 1 - start);
>
> -               down_read(&fs_info->commit_root_sem);
> -               if (start < cache->last_byte_to_unpin && return_free_space) {
> -                       u64 add_len = min(len, cache->last_byte_to_unpin - start);
> -
> -                       btrfs_add_free_space(cache, start, add_len);
> -               }
> -               up_read(&fs_info->commit_root_sem);
> +               if (return_free_space)
> +                       btrfs_add_free_space(cache, start, len);
>
>                 start += len;
>                 total_unpinned += len;
> diff --git a/fs/btrfs/free-space-tree.c b/fs/btrfs/free-space-tree.c
> index 1bf89aa67216..367bcfcf68f5 100644
> --- a/fs/btrfs/free-space-tree.c
> +++ b/fs/btrfs/free-space-tree.c
> @@ -1453,8 +1453,6 @@ static int load_free_space_bitmaps(struct btrfs_caching_control *caching_ctl,
>                 ASSERT(key.type == BTRFS_FREE_SPACE_BITMAP_KEY);
>                 ASSERT(key.objectid < end && key.objectid + key.offset <= end);
>
> -               caching_ctl->progress = key.objectid;
> -
>                 offset = key.objectid;
>                 while (offset < key.objectid + key.offset) {
>                         bit = free_space_test_bit(block_group, path, offset);
> @@ -1490,8 +1488,6 @@ static int load_free_space_bitmaps(struct btrfs_caching_control *caching_ctl,
>                 goto out;
>         }
>
> -       caching_ctl->progress = (u64)-1;
> -
>         ret = 0;
>  out:
>         return ret;
> @@ -1531,8 +1527,6 @@ static int load_free_space_extents(struct btrfs_caching_control *caching_ctl,
>                 ASSERT(key.type == BTRFS_FREE_SPACE_EXTENT_KEY);
>                 ASSERT(key.objectid < end && key.objectid + key.offset <= end);
>
> -               caching_ctl->progress = key.objectid;
> -
>                 total_found += add_new_free_space(block_group, key.objectid,
>                                                   key.objectid + key.offset);
>                 if (total_found > CACHING_CTL_WAKE_UP) {
> @@ -1552,8 +1546,6 @@ static int load_free_space_extents(struct btrfs_caching_control *caching_ctl,
>                 goto out;
>         }
>
> -       caching_ctl->progress = (u64)-1;
> -
>         ret = 0;
>  out:
>         return ret;
> diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
> index 6e3b2cb6a04a..4c87bf2abc14 100644
> --- a/fs/btrfs/transaction.c
> +++ b/fs/btrfs/transaction.c
> @@ -161,7 +161,6 @@ static noinline void switch_commit_roots(struct btrfs_trans_handle *trans)
>         struct btrfs_transaction *cur_trans = trans->transaction;
>         struct btrfs_fs_info *fs_info = trans->fs_info;
>         struct btrfs_root *root, *tmp;
> -       struct btrfs_caching_control *caching_ctl, *next;
>
>         /*
>          * At this point no one can be using this transaction to modify any tree
> @@ -196,46 +195,6 @@ static noinline void switch_commit_roots(struct btrfs_trans_handle *trans)
>         }
>         spin_unlock(&cur_trans->dropped_roots_lock);
>
> -       /*
> -        * We have to update the last_byte_to_unpin under the commit_root_sem,
> -        * at the same time we swap out the commit roots.
> -        *
> -        * This is because we must have a real view of the last spot the caching
> -        * kthreads were while caching.  Consider the following views of the
> -        * extent tree for a block group
> -        *
> -        * commit root
> -        * +----+----+----+----+----+----+----+
> -        * |\\\\|    |\\\\|\\\\|    |\\\\|\\\\|
> -        * +----+----+----+----+----+----+----+
> -        * 0    1    2    3    4    5    6    7
> -        *
> -        * new commit root
> -        * +----+----+----+----+----+----+----+
> -        * |    |    |    |\\\\|    |    |\\\\|
> -        * +----+----+----+----+----+----+----+
> -        * 0    1    2    3    4    5    6    7
> -        *
> -        * If the cache_ctl->progress was at 3, then we are only allowed to
> -        * unpin [0,1) and [2,3], because the caching thread has already
> -        * processed those extents.  We are not allowed to unpin [5,6), because
> -        * the caching thread will re-start it's search from 3, and thus find
> -        * the hole from [4,6) to add to the free space cache.
> -        */
> -       write_lock(&fs_info->block_group_cache_lock);
> -       list_for_each_entry_safe(caching_ctl, next,
> -                                &fs_info->caching_block_groups, list) {
> -               struct btrfs_block_group *cache = caching_ctl->block_group;
> -
> -               if (btrfs_block_group_done(cache)) {
> -                       cache->last_byte_to_unpin = (u64)-1;
> -                       list_del_init(&caching_ctl->list);
> -                       btrfs_put_caching_control(caching_ctl);
Now the caching_ctl is not removed from fs_info->caching_block_groups,
and remains there until close_ctree(). So eventually it will be
removed, but for now just occupying memory. Is this intended?

> -               } else {
> -                       cache->last_byte_to_unpin = caching_ctl->progress;
> -               }
> -       }
> -       write_unlock(&fs_info->block_group_cache_lock);
>         up_write(&fs_info->commit_root_sem);
>  }
>
> diff --git a/fs/btrfs/zoned.c b/fs/btrfs/zoned.c
> index 61ae58c3a354..56a147a6e571 100644
> --- a/fs/btrfs/zoned.c
> +++ b/fs/btrfs/zoned.c
> @@ -1558,7 +1558,6 @@ void btrfs_calc_zone_unusable(struct btrfs_block_group *cache)
>         free = cache->zone_capacity - cache->alloc_offset;
>
>         /* We only need ->free_space in ALLOC_SEQ block groups */
> -       cache->last_byte_to_unpin = (u64)-1;
>         cache->cached = BTRFS_CACHE_FINISHED;
>         cache->free_space_ctl->free_space = free;
>         cache->zone_unusable = unusable;
> --
> 2.37.2
>

Thanks,
Alex.

Re: [PATCH 2/2] btrfs: get rid of block group caching progress logic

[Omar Sandoval]

On Thu, Feb 20, 2025 at 09:26:47PM +0200, Alex Lyakas wrote:
> Hi Omar,
> 
> On Wed, Aug 17, 2022 at 2:13 AM Omar Sandoval <osandov@osandov.com> wrote:
> >
> > From: Omar Sandoval <osandov@fb.com>
> >
> > struct btrfs_caching_ctl::progress and struct
> > btrfs_block_group::last_byte_to_unpin were previously needed to ensure
> > that unpin_extent_range() didn't return a range to the free space cache
> > before the caching thread had a chance to cache that range. However, the
> > previous commit made it so that we always synchronously cache the block
> > group at the time that we pin the extent, so this machinery is no longer
> > necessary.
> >
> > Signed-off-by: Omar Sandoval <osandov@fb.com>
> > ---
> >  fs/btrfs/block-group.c     | 13 ------------
> >  fs/btrfs/block-group.h     |  2 --
> >  fs/btrfs/extent-tree.c     |  9 ++-------
> >  fs/btrfs/free-space-tree.c |  8 --------
> >  fs/btrfs/transaction.c     | 41 --------------------------------------
> >  fs/btrfs/zoned.c           |  1 -
> >  6 files changed, 2 insertions(+), 72 deletions(-)
> >
> > diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
> > index 1af6fc395a52..68992ad9ff2a 100644
> > --- a/fs/btrfs/block-group.c
> > +++ b/fs/btrfs/block-group.c
> > @@ -593,8 +593,6 @@ static int load_extent_tree_free(struct btrfs_caching_control *caching_ctl)
> >
> >                         if (need_resched() ||
> >                             rwsem_is_contended(&fs_info->commit_root_sem)) {
> > -                               if (wakeup)
> > -                                       caching_ctl->progress = last;
> >                                 btrfs_release_path(path);
> >                                 up_read(&fs_info->commit_root_sem);
> >                                 mutex_unlock(&caching_ctl->mutex);
> > @@ -618,9 +616,6 @@ static int load_extent_tree_free(struct btrfs_caching_control *caching_ctl)
> >                         key.objectid = last;
> >                         key.offset = 0;
> >                         key.type = BTRFS_EXTENT_ITEM_KEY;
> > -
> > -                       if (wakeup)
> > -                               caching_ctl->progress = last;
> >                         btrfs_release_path(path);
> >                         goto next;
> >                 }
> > @@ -655,7 +650,6 @@ static int load_extent_tree_free(struct btrfs_caching_control *caching_ctl)
> >
> >         total_found += add_new_free_space(block_group, last,
> >                                 block_group->start + block_group->length);
> > -       caching_ctl->progress = (u64)-1;
> >
> >  out:
> >         btrfs_free_path(path);
> > @@ -725,8 +719,6 @@ static noinline void caching_thread(struct btrfs_work *work)
> >         }
> >  #endif
> >
> > -       caching_ctl->progress = (u64)-1;
> > -
> >         up_read(&fs_info->commit_root_sem);
> >         btrfs_free_excluded_extents(block_group);
> >         mutex_unlock(&caching_ctl->mutex);
> > @@ -755,7 +747,6 @@ int btrfs_cache_block_group(struct btrfs_block_group *cache, bool wait)
> >         mutex_init(&caching_ctl->mutex);
> >         init_waitqueue_head(&caching_ctl->wait);
> >         caching_ctl->block_group = cache;
> > -       caching_ctl->progress = cache->start;
> >         refcount_set(&caching_ctl->count, 2);
> >         btrfs_init_work(&caching_ctl->work, caching_thread, NULL, NULL);
> >
> > @@ -2076,11 +2067,9 @@ static int read_one_block_group(struct btrfs_fs_info *info,
> >                 /* Should not have any excluded extents. Just in case, though. */
> >                 btrfs_free_excluded_extents(cache);
> >         } else if (cache->length == cache->used) {
> > -               cache->last_byte_to_unpin = (u64)-1;
> >                 cache->cached = BTRFS_CACHE_FINISHED;
> >                 btrfs_free_excluded_extents(cache);
> >         } else if (cache->used == 0) {
> > -               cache->last_byte_to_unpin = (u64)-1;
> >                 cache->cached = BTRFS_CACHE_FINISHED;
> >                 add_new_free_space(cache, cache->start,
> >                                    cache->start + cache->length);
> > @@ -2136,7 +2125,6 @@ static int fill_dummy_bgs(struct btrfs_fs_info *fs_info)
> >                 /* Fill dummy cache as FULL */
> >                 bg->length = em->len;
> >                 bg->flags = map->type;
> > -               bg->last_byte_to_unpin = (u64)-1;
> >                 bg->cached = BTRFS_CACHE_FINISHED;
> >                 bg->used = em->len;
> >                 bg->flags = map->type;
> > @@ -2482,7 +2470,6 @@ struct btrfs_block_group *btrfs_make_block_group(struct btrfs_trans_handle *tran
> >         set_free_space_tree_thresholds(cache);
> >         cache->used = bytes_used;
> >         cache->flags = type;
> > -       cache->last_byte_to_unpin = (u64)-1;
> >         cache->cached = BTRFS_CACHE_FINISHED;
> >         cache->global_root_id = calculate_global_root_id(fs_info, cache->start);
> >
> > diff --git a/fs/btrfs/block-group.h b/fs/btrfs/block-group.h
> > index 9dba28bb1806..59a86e82a28e 100644
> > --- a/fs/btrfs/block-group.h
> > +++ b/fs/btrfs/block-group.h
> > @@ -63,7 +63,6 @@ struct btrfs_caching_control {
> >         wait_queue_head_t wait;
> >         struct btrfs_work work;
> >         struct btrfs_block_group *block_group;
> > -       u64 progress;
> >         refcount_t count;
> >  };
> >
> > @@ -115,7 +114,6 @@ struct btrfs_block_group {
> >         /* Cache tracking stuff */
> >         int cached;
> >         struct btrfs_caching_control *caching_ctl;
> > -       u64 last_byte_to_unpin;
> >
> >         struct btrfs_space_info *space_info;
> >
> > diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> > index 86ac953c69ac..bcd0e72cded3 100644
> > --- a/fs/btrfs/extent-tree.c
> > +++ b/fs/btrfs/extent-tree.c
> > @@ -2686,13 +2686,8 @@ static int unpin_extent_range(struct btrfs_fs_info *fs_info,
> >                 len = cache->start + cache->length - start;
> >                 len = min(len, end + 1 - start);
> >
> > -               down_read(&fs_info->commit_root_sem);
> > -               if (start < cache->last_byte_to_unpin && return_free_space) {
> > -                       u64 add_len = min(len, cache->last_byte_to_unpin - start);
> > -
> > -                       btrfs_add_free_space(cache, start, add_len);
> > -               }
> > -               up_read(&fs_info->commit_root_sem);
> > +               if (return_free_space)
> > +                       btrfs_add_free_space(cache, start, len);
> >
> >                 start += len;
> >                 total_unpinned += len;
> > diff --git a/fs/btrfs/free-space-tree.c b/fs/btrfs/free-space-tree.c
> > index 1bf89aa67216..367bcfcf68f5 100644
> > --- a/fs/btrfs/free-space-tree.c
> > +++ b/fs/btrfs/free-space-tree.c
> > @@ -1453,8 +1453,6 @@ static int load_free_space_bitmaps(struct btrfs_caching_control *caching_ctl,
> >                 ASSERT(key.type == BTRFS_FREE_SPACE_BITMAP_KEY);
> >                 ASSERT(key.objectid < end && key.objectid + key.offset <= end);
> >
> > -               caching_ctl->progress = key.objectid;
> > -
> >                 offset = key.objectid;
> >                 while (offset < key.objectid + key.offset) {
> >                         bit = free_space_test_bit(block_group, path, offset);
> > @@ -1490,8 +1488,6 @@ static int load_free_space_bitmaps(struct btrfs_caching_control *caching_ctl,
> >                 goto out;
> >         }
> >
> > -       caching_ctl->progress = (u64)-1;
> > -
> >         ret = 0;
> >  out:
> >         return ret;
> > @@ -1531,8 +1527,6 @@ static int load_free_space_extents(struct btrfs_caching_control *caching_ctl,
> >                 ASSERT(key.type == BTRFS_FREE_SPACE_EXTENT_KEY);
> >                 ASSERT(key.objectid < end && key.objectid + key.offset <= end);
> >
> > -               caching_ctl->progress = key.objectid;
> > -
> >                 total_found += add_new_free_space(block_group, key.objectid,
> >                                                   key.objectid + key.offset);
> >                 if (total_found > CACHING_CTL_WAKE_UP) {
> > @@ -1552,8 +1546,6 @@ static int load_free_space_extents(struct btrfs_caching_control *caching_ctl,
> >                 goto out;
> >         }
> >
> > -       caching_ctl->progress = (u64)-1;
> > -
> >         ret = 0;
> >  out:
> >         return ret;
> > diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
> > index 6e3b2cb6a04a..4c87bf2abc14 100644
> > --- a/fs/btrfs/transaction.c
> > +++ b/fs/btrfs/transaction.c
> > @@ -161,7 +161,6 @@ static noinline void switch_commit_roots(struct btrfs_trans_handle *trans)
> >         struct btrfs_transaction *cur_trans = trans->transaction;
> >         struct btrfs_fs_info *fs_info = trans->fs_info;
> >         struct btrfs_root *root, *tmp;
> > -       struct btrfs_caching_control *caching_ctl, *next;
> >
> >         /*
> >          * At this point no one can be using this transaction to modify any tree
> > @@ -196,46 +195,6 @@ static noinline void switch_commit_roots(struct btrfs_trans_handle *trans)
> >         }
> >         spin_unlock(&cur_trans->dropped_roots_lock);
> >
> > -       /*
> > -        * We have to update the last_byte_to_unpin under the commit_root_sem,
> > -        * at the same time we swap out the commit roots.
> > -        *
> > -        * This is because we must have a real view of the last spot the caching
> > -        * kthreads were while caching.  Consider the following views of the
> > -        * extent tree for a block group
> > -        *
> > -        * commit root
> > -        * +----+----+----+----+----+----+----+
> > -        * |\\\\|    |\\\\|\\\\|    |\\\\|\\\\|
> > -        * +----+----+----+----+----+----+----+
> > -        * 0    1    2    3    4    5    6    7
> > -        *
> > -        * new commit root
> > -        * +----+----+----+----+----+----+----+
> > -        * |    |    |    |\\\\|    |    |\\\\|
> > -        * +----+----+----+----+----+----+----+
> > -        * 0    1    2    3    4    5    6    7
> > -        *
> > -        * If the cache_ctl->progress was at 3, then we are only allowed to
> > -        * unpin [0,1) and [2,3], because the caching thread has already
> > -        * processed those extents.  We are not allowed to unpin [5,6), because
> > -        * the caching thread will re-start it's search from 3, and thus find
> > -        * the hole from [4,6) to add to the free space cache.
> > -        */
> > -       write_lock(&fs_info->block_group_cache_lock);
> > -       list_for_each_entry_safe(caching_ctl, next,
> > -                                &fs_info->caching_block_groups, list) {
> > -               struct btrfs_block_group *cache = caching_ctl->block_group;
> > -
> > -               if (btrfs_block_group_done(cache)) {
> > -                       cache->last_byte_to_unpin = (u64)-1;
> > -                       list_del_init(&caching_ctl->list);
> > -                       btrfs_put_caching_control(caching_ctl);
> Now the caching_ctl is not removed from fs_info->caching_block_groups,
> and remains there until close_ctree(). So eventually it will be
> removed, but for now just occupying memory. Is this intended?

No, I don't think this was intentional.

Thanks,
Omar

Re: [PATCH 2/2] btrfs: get rid of block group caching progress logic

[Alex Lyakas]

Hi Omar,

Thanks for your response. Does it make sense to remove the caching
control when the caching completes in caching_thread?

Thanks,
Alex.


On Thu, Feb 20, 2025 at 11:59 PM Omar Sandoval <osandov@osandov.com> wrote:
>
> On Thu, Feb 20, 2025 at 09:26:47PM +0200, Alex Lyakas wrote:
> > Hi Omar,
> >
> > On Wed, Aug 17, 2022 at 2:13 AM Omar Sandoval <osandov@osandov.com> wrote:
> > >
> > > From: Omar Sandoval <osandov@fb.com>
> > >
> > > struct btrfs_caching_ctl::progress and struct
> > > btrfs_block_group::last_byte_to_unpin were previously needed to ensure
> > > that unpin_extent_range() didn't return a range to the free space cache
> > > before the caching thread had a chance to cache that range. However, the
> > > previous commit made it so that we always synchronously cache the block
> > > group at the time that we pin the extent, so this machinery is no longer
> > > necessary.
> > >
> > > Signed-off-by: Omar Sandoval <osandov@fb.com>
> > > ---
> > >  fs/btrfs/block-group.c     | 13 ------------
> > >  fs/btrfs/block-group.h     |  2 --
> > >  fs/btrfs/extent-tree.c     |  9 ++-------
> > >  fs/btrfs/free-space-tree.c |  8 --------
> > >  fs/btrfs/transaction.c     | 41 --------------------------------------
> > >  fs/btrfs/zoned.c           |  1 -
> > >  6 files changed, 2 insertions(+), 72 deletions(-)
> > >
> > > diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
> > > index 1af6fc395a52..68992ad9ff2a 100644
> > > --- a/fs/btrfs/block-group.c
> > > +++ b/fs/btrfs/block-group.c
> > > @@ -593,8 +593,6 @@ static int load_extent_tree_free(struct btrfs_caching_control *caching_ctl)
> > >
> > >                         if (need_resched() ||
> > >                             rwsem_is_contended(&fs_info->commit_root_sem)) {
> > > -                               if (wakeup)
> > > -                                       caching_ctl->progress = last;
> > >                                 btrfs_release_path(path);
> > >                                 up_read(&fs_info->commit_root_sem);
> > >                                 mutex_unlock(&caching_ctl->mutex);
> > > @@ -618,9 +616,6 @@ static int load_extent_tree_free(struct btrfs_caching_control *caching_ctl)
> > >                         key.objectid = last;
> > >                         key.offset = 0;
> > >                         key.type = BTRFS_EXTENT_ITEM_KEY;
> > > -
> > > -                       if (wakeup)
> > > -                               caching_ctl->progress = last;
> > >                         btrfs_release_path(path);
> > >                         goto next;
> > >                 }
> > > @@ -655,7 +650,6 @@ static int load_extent_tree_free(struct btrfs_caching_control *caching_ctl)
> > >
> > >         total_found += add_new_free_space(block_group, last,
> > >                                 block_group->start + block_group->length);
> > > -       caching_ctl->progress = (u64)-1;
> > >
> > >  out:
> > >         btrfs_free_path(path);
> > > @@ -725,8 +719,6 @@ static noinline void caching_thread(struct btrfs_work *work)
> > >         }
> > >  #endif
> > >
> > > -       caching_ctl->progress = (u64)-1;
> > > -
> > >         up_read(&fs_info->commit_root_sem);
> > >         btrfs_free_excluded_extents(block_group);
> > >         mutex_unlock(&caching_ctl->mutex);
> > > @@ -755,7 +747,6 @@ int btrfs_cache_block_group(struct btrfs_block_group *cache, bool wait)
> > >         mutex_init(&caching_ctl->mutex);
> > >         init_waitqueue_head(&caching_ctl->wait);
> > >         caching_ctl->block_group = cache;
> > > -       caching_ctl->progress = cache->start;
> > >         refcount_set(&caching_ctl->count, 2);
> > >         btrfs_init_work(&caching_ctl->work, caching_thread, NULL, NULL);
> > >
> > > @@ -2076,11 +2067,9 @@ static int read_one_block_group(struct btrfs_fs_info *info,
> > >                 /* Should not have any excluded extents. Just in case, though. */
> > >                 btrfs_free_excluded_extents(cache);
> > >         } else if (cache->length == cache->used) {
> > > -               cache->last_byte_to_unpin = (u64)-1;
> > >                 cache->cached = BTRFS_CACHE_FINISHED;
> > >                 btrfs_free_excluded_extents(cache);
> > >         } else if (cache->used == 0) {
> > > -               cache->last_byte_to_unpin = (u64)-1;
> > >                 cache->cached = BTRFS_CACHE_FINISHED;
> > >                 add_new_free_space(cache, cache->start,
> > >                                    cache->start + cache->length);
> > > @@ -2136,7 +2125,6 @@ static int fill_dummy_bgs(struct btrfs_fs_info *fs_info)
> > >                 /* Fill dummy cache as FULL */
> > >                 bg->length = em->len;
> > >                 bg->flags = map->type;
> > > -               bg->last_byte_to_unpin = (u64)-1;
> > >                 bg->cached = BTRFS_CACHE_FINISHED;
> > >                 bg->used = em->len;
> > >                 bg->flags = map->type;
> > > @@ -2482,7 +2470,6 @@ struct btrfs_block_group *btrfs_make_block_group(struct btrfs_trans_handle *tran
> > >         set_free_space_tree_thresholds(cache);
> > >         cache->used = bytes_used;
> > >         cache->flags = type;
> > > -       cache->last_byte_to_unpin = (u64)-1;
> > >         cache->cached = BTRFS_CACHE_FINISHED;
> > >         cache->global_root_id = calculate_global_root_id(fs_info, cache->start);
> > >
> > > diff --git a/fs/btrfs/block-group.h b/fs/btrfs/block-group.h
> > > index 9dba28bb1806..59a86e82a28e 100644
> > > --- a/fs/btrfs/block-group.h
> > > +++ b/fs/btrfs/block-group.h
> > > @@ -63,7 +63,6 @@ struct btrfs_caching_control {
> > >         wait_queue_head_t wait;
> > >         struct btrfs_work work;
> > >         struct btrfs_block_group *block_group;
> > > -       u64 progress;
> > >         refcount_t count;
> > >  };
> > >
> > > @@ -115,7 +114,6 @@ struct btrfs_block_group {
> > >         /* Cache tracking stuff */
> > >         int cached;
> > >         struct btrfs_caching_control *caching_ctl;
> > > -       u64 last_byte_to_unpin;
> > >
> > >         struct btrfs_space_info *space_info;
> > >
> > > diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> > > index 86ac953c69ac..bcd0e72cded3 100644
> > > --- a/fs/btrfs/extent-tree.c
> > > +++ b/fs/btrfs/extent-tree.c
> > > @@ -2686,13 +2686,8 @@ static int unpin_extent_range(struct btrfs_fs_info *fs_info,
> > >                 len = cache->start + cache->length - start;
> > >                 len = min(len, end + 1 - start);
> > >
> > > -               down_read(&fs_info->commit_root_sem);
> > > -               if (start < cache->last_byte_to_unpin && return_free_space) {
> > > -                       u64 add_len = min(len, cache->last_byte_to_unpin - start);
> > > -
> > > -                       btrfs_add_free_space(cache, start, add_len);
> > > -               }
> > > -               up_read(&fs_info->commit_root_sem);
> > > +               if (return_free_space)
> > > +                       btrfs_add_free_space(cache, start, len);
> > >
> > >                 start += len;
> > >                 total_unpinned += len;
> > > diff --git a/fs/btrfs/free-space-tree.c b/fs/btrfs/free-space-tree.c
> > > index 1bf89aa67216..367bcfcf68f5 100644
> > > --- a/fs/btrfs/free-space-tree.c
> > > +++ b/fs/btrfs/free-space-tree.c
> > > @@ -1453,8 +1453,6 @@ static int load_free_space_bitmaps(struct btrfs_caching_control *caching_ctl,
> > >                 ASSERT(key.type == BTRFS_FREE_SPACE_BITMAP_KEY);
> > >                 ASSERT(key.objectid < end && key.objectid + key.offset <= end);
> > >
> > > -               caching_ctl->progress = key.objectid;
> > > -
> > >                 offset = key.objectid;
> > >                 while (offset < key.objectid + key.offset) {
> > >                         bit = free_space_test_bit(block_group, path, offset);
> > > @@ -1490,8 +1488,6 @@ static int load_free_space_bitmaps(struct btrfs_caching_control *caching_ctl,
> > >                 goto out;
> > >         }
> > >
> > > -       caching_ctl->progress = (u64)-1;
> > > -
> > >         ret = 0;
> > >  out:
> > >         return ret;
> > > @@ -1531,8 +1527,6 @@ static int load_free_space_extents(struct btrfs_caching_control *caching_ctl,
> > >                 ASSERT(key.type == BTRFS_FREE_SPACE_EXTENT_KEY);
> > >                 ASSERT(key.objectid < end && key.objectid + key.offset <= end);
> > >
> > > -               caching_ctl->progress = key.objectid;
> > > -
> > >                 total_found += add_new_free_space(block_group, key.objectid,
> > >                                                   key.objectid + key.offset);
> > >                 if (total_found > CACHING_CTL_WAKE_UP) {
> > > @@ -1552,8 +1546,6 @@ static int load_free_space_extents(struct btrfs_caching_control *caching_ctl,
> > >                 goto out;
> > >         }
> > >
> > > -       caching_ctl->progress = (u64)-1;
> > > -
> > >         ret = 0;
> > >  out:
> > >         return ret;
> > > diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
> > > index 6e3b2cb6a04a..4c87bf2abc14 100644
> > > --- a/fs/btrfs/transaction.c
> > > +++ b/fs/btrfs/transaction.c
> > > @@ -161,7 +161,6 @@ static noinline void switch_commit_roots(struct btrfs_trans_handle *trans)
> > >         struct btrfs_transaction *cur_trans = trans->transaction;
> > >         struct btrfs_fs_info *fs_info = trans->fs_info;
> > >         struct btrfs_root *root, *tmp;
> > > -       struct btrfs_caching_control *caching_ctl, *next;
> > >
> > >         /*
> > >          * At this point no one can be using this transaction to modify any tree
> > > @@ -196,46 +195,6 @@ static noinline void switch_commit_roots(struct btrfs_trans_handle *trans)
> > >         }
> > >         spin_unlock(&cur_trans->dropped_roots_lock);
> > >
> > > -       /*
> > > -        * We have to update the last_byte_to_unpin under the commit_root_sem,
> > > -        * at the same time we swap out the commit roots.
> > > -        *
> > > -        * This is because we must have a real view of the last spot the caching
> > > -        * kthreads were while caching.  Consider the following views of the
> > > -        * extent tree for a block group
> > > -        *
> > > -        * commit root
> > > -        * +----+----+----+----+----+----+----+
> > > -        * |\\\\|    |\\\\|\\\\|    |\\\\|\\\\|
> > > -        * +----+----+----+----+----+----+----+
> > > -        * 0    1    2    3    4    5    6    7
> > > -        *
> > > -        * new commit root
> > > -        * +----+----+----+----+----+----+----+
> > > -        * |    |    |    |\\\\|    |    |\\\\|
> > > -        * +----+----+----+----+----+----+----+
> > > -        * 0    1    2    3    4    5    6    7
> > > -        *
> > > -        * If the cache_ctl->progress was at 3, then we are only allowed to
> > > -        * unpin [0,1) and [2,3], because the caching thread has already
> > > -        * processed those extents.  We are not allowed to unpin [5,6), because
> > > -        * the caching thread will re-start it's search from 3, and thus find
> > > -        * the hole from [4,6) to add to the free space cache.
> > > -        */
> > > -       write_lock(&fs_info->block_group_cache_lock);
> > > -       list_for_each_entry_safe(caching_ctl, next,
> > > -                                &fs_info->caching_block_groups, list) {
> > > -               struct btrfs_block_group *cache = caching_ctl->block_group;
> > > -
> > > -               if (btrfs_block_group_done(cache)) {
> > > -                       cache->last_byte_to_unpin = (u64)-1;
> > > -                       list_del_init(&caching_ctl->list);
> > > -                       btrfs_put_caching_control(caching_ctl);
> > Now the caching_ctl is not removed from fs_info->caching_block_groups,
> > and remains there until close_ctree(). So eventually it will be
> > removed, but for now just occupying memory. Is this intended?
>
> No, I don't think this was intentional.
>
> Thanks,
> Omar

Re: [PATCH 0/2] btrfs: fix filesystem corruption caused by space cache race

[David Sterba]

On Tue, Aug 16, 2022 at 04:12:14PM -0700, Omar Sandoval wrote:
> From: Omar Sandoval <osandov@fb.com>
> 
> Hello,
> 
> We recently deployed space_cache v2 on a large set of machines to do
> some performance comparisons and found a nasty filesystem corruption bug
> that was introduced by the fsync transaction optimization in 5.12. It's
> much more likely to affect space_cache=v2 and nospace_cache, but since
> space_cache=v1 effectively falls back to nospace_cache if there is a
> free space inode generation mismatch, space_cache=v1 could also
> theoretically be affected. discard/discard=sync also makes the bug much
> easier to hit by making the race window larger.
> 
> Patch 1 is the fix itself with a lot more details. Patch 2 is a followup
> cleanup.
> 
> I'm still working on a reproducer, but I wanted to get this fix out
> ASAP.
> 
> Thanks!
> 
> Omar Sandoval (2):
>   btrfs: fix space cache corruption and potential double allocations
>   btrfs: get rid of block group caching progress logic

Added to misc-next, thanks. A backport for 5.15 would be needed, the
patch does not apply cleanly.

Re: [PATCH 0/2] btrfs: fix filesystem corruption caused by space cache race

[David Sterba]

On Tue, Aug 16, 2022 at 04:12:14PM -0700, Omar Sandoval wrote:
> From: Omar Sandoval <osandov@fb.com>
> 
> Hello,
> 
> We recently deployed space_cache v2 on a large set of machines to do
> some performance comparisons and found a nasty filesystem corruption bug
> that was introduced by the fsync transaction optimization in 5.12. It's
> much more likely to affect space_cache=v2 and nospace_cache, but since
> space_cache=v1 effectively falls back to nospace_cache if there is a
> free space inode generation mismatch, space_cache=v1 could also
> theoretically be affected. discard/discard=sync also makes the bug much
> easier to hit by making the race window larger.
> 
> Patch 1 is the fix itself with a lot more details. Patch 2 is a followup
> cleanup.
> 
> I'm still working on a reproducer, but I wanted to get this fix out
> ASAP.
> 
> Thanks!
> 
> Omar Sandoval (2):
>   btrfs: fix space cache corruption and potential double allocations
>   btrfs: get rid of block group caching progress logic

The patches apply cleanly on misc-next but if you want this fixed in 6.0
I'll need a backported version, and then misc-next will be rebased on
top of that.

Re: [PATCH 0/2] btrfs: fix filesystem corruption caused by space cache race

[Christoph Anton Mitterer]

Any chance to get this ASAP in the stable kernels?


Cheers,
Chris.

Re: [PATCH 0/2] btrfs: fix filesystem corruption caused by space cache race

[David Sterba]

On Tue, Aug 23, 2022 at 09:20:57PM +0200, Christoph Anton Mitterer wrote:
> Any chance to get this ASAP in the stable kernels?

Yes, it'll be in the -rc3 batch and then it gets picked to stable within
a week, depending on the stable release schedule.