kvserver: Clean up empty range directories after snapshots

[itsbilal]

Previously, we were creating subdirectories for ranges and
range snapshots in the auxiliary directory every time we
accepted a snapshot, but only cleaning up the snapshot
subdirectories after a snapshot scratch space closed. This
left empty parent range directories around on the FS,
slowing down future calls to Pebble.Capacity() and indirectly
slowing down AddSSTable in the future.

This change adds code to clean up empty range directories
in the aux directory if they're not being used. Some coordination
and synchronization code had to be added to ensure we wouldn't
remove a directory that was just created by a concurrent snapshot.

Fixes #83137

Release note (bug fix, performance improvement): Addresses issue where
imports and rebalances were being slowed down due to the accumulation
of empty directories from range snapshot applications.

[cockroach-teamcity]

This change is 

[erikgrinaker]

LGTM, thanks!

[erikgrinaker]

nit: any reason we're not just doing this in NewSSTSnapshotStorage()?

[itsbilal]

Good question. The goal is to avoid a Mutex copy as much as possible. The syntax to do it in one line without copying the return value in NewSSTSnapshotStorage is somewhat ugly but not as bad as it was in one of my earlier iterations (which had more stuff in mu), so I just went ahead and made that change.

[erikgrinaker]

Thanks! Why are we worried about the mutex copy? We're only calling this once on server startup, and there is no concurrency until it's constructed, but maybe I'm missing something.

[itsbilal]

TFTR!

[nicktrav]

Thanks for cleaning this up!

Reviewed 1 of 4 files at r1, 3 of 3 files at r2, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @erikgrinaker and @itsbilal)

---

-- commits line 19 at r2:
Do we not consider this to be a bug fix too? i.e. fixing the "leak"? No strong feelings either way, just wanted to check.

---

pkg/kv/kvserver/replica_sst_snapshot_storage.go line 96 at r2 (raw file):

		// a performance issue when we later walk directories in pebble.Capacity()
		// but not a correctness issue.
		_ = s.engine.RemoveAll(filepath.Join(s.dir, strconv.Itoa(int(rangeID))))

Are there any invariants worth checking in this deletion codepath? i.e. are there circumstances in which we should not delete a directory (or its children) that we should be checking for before proceeding? Or is the reference count enough.

[itsbilal]

TFTRs!

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @erikgrinaker and @nicktrav)

---

-- commits line 19 at r2:

Previously, nicktrav (Nick Travers) wrote…

Do we not consider this to be a bug fix too? i.e. fixing the "leak"? No strong feelings either way, just wanted to check.

Good point, fixed.

---

pkg/kv/kvserver/replica_sst_snapshot_storage.go line 51 at r1 (raw file):

Previously, erikgrinaker (Erik Grinaker) wrote…

Thanks! Why are we worried about the mutex copy? We're only calling this once on server startup, and there is no concurrency until it's constructed, but maybe I'm missing something.

It just threw a warning in my IDE and I was guessing it'd throw a warning in the linter as well, but I never confirmed it. Either way, this syntax sidesteps that question.

---

pkg/kv/kvserver/replica_sst_snapshot_storage.go line 96 at r2 (raw file):

Previously, nicktrav (Nick Travers) wrote…

Are there any invariants worth checking in this deletion codepath? i.e. are there circumstances in which we should not delete a directory (or its children) that we should be checking for before proceeding? Or is the reference count enough.

Seeing as this scratch is the only thing that creates and uses that directory, and we delete the entire aux directory on node restart, it should be pretty safe.

[nicktrav]

Reviewed all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @erikgrinaker)

---

pkg/kv/kvserver/replica_sst_snapshot_storage.go line 96 at r2 (raw file):

Previously, itsbilal (Bilal Akhtar) wrote…

Seeing as this scratch is the only thing that creates and uses that directory, and we delete the entire aux directory on node restart, it should be pretty safe.

sgtm

[itsbilal]

bors r=nicktrav

[craig]

Build succeeded:

• GitHub CI (Cockroach)

[sumeerbhola]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale)

---

pkg/kv/kvserver/replica_sst_snapshot_storage.go line 62 at r3 (raw file):

	rangeStorage := s.mu.ranges[rangeID]
	if rangeStorage == 0 {
		s.mu.ranges[rangeID] = 1

The use of map[roachpb.RangeID]int suggests this is a refcount, but it doesn't seem to be. What is the semantics of this value? What is the case where this if-condition will not be true?
nit: if this is a refcount, I'd suggest renaming ranges to rangeRefCount.

---

pkg/kv/kvserver/replica_sst_snapshot_storage.go line 89 at r3 (raw file):

		panic("inconsistent scratch ref count")
	}
	val--

looks like a refcount now. But we didn't ++ it in NewScratchSpace. I am confused.

[itsbilal]

pkg/kv/kvserver/replica_sst_snapshot_storage.go line 62 at r3 (raw file):

Previously, sumeerbhola wrote…

The use of map[roachpb.RangeID]int suggests this is a refcount, but it doesn't seem to be. What is the semantics of this value? What is the case where this if-condition will not be true?
nit: if this is a refcount, I'd suggest renaming ranges to rangeRefCount.

Oops, yes it's a refcount and it's missing a ++ here in the non-empty case. Good catch. I'm pushing a PR for this.