kvserver: move destroyStatus to shMu

This is to emphasize that both raftMu and mu need to be locked to set
the destroy status; and that either of the mutexes can be held to read
it.

Technically, we must also hold readOnlyCmdMu to set this field, so it
fits neither of Replica.{mu,shMu} section. But shMu is strictly closer
by semantics.

Epic: none
Release note: none

Author: pav-kv

pkg/kv/kvserver/replica.go

@@ -571,6 +571,14 @@ type Replica struct {
 	//
 	// TODO(pav-kv): audit all other fields and include here.
 	shMu struct {
+		// The destroyed status of a replica indicating if it's alive, corrupt,
+		// scheduled for destruction or has been GCed. destroyStatus should only be
+		// set while also holding the raftMu and readOnlyCmdMu.
+		//
+		// When this replica is being removed, the destroyStatus is updated and
+		// RangeTombstone is written in the same raftMu critical section.
+		destroyStatus
+
 		// The state of the Raft state machine.
 		// Invariant: state.TruncatedState == nil. The field is being phased out in
 		// favour of the one contained in logStorage.
@@ -587,14 +595,6 @@ type Replica struct {
 	mu struct {
 		// Protects all fields in the mu struct.
 		ReplicaMutex
-		// The destroyed status of a replica indicating if it's alive, corrupt,
-		// scheduled for destruction or has been GCed.
-		// destroyStatus should only be set while also holding the raftMu and
-		// readOnlyCmdMu.
-		//
-		// When this replica is being removed, the destroyStatus is updated and
-		// RangeTombstone is written in the same raftMu critical section.
-		destroyStatus
 		// Is the range quiescent? Quiescent ranges are not Tick()'d and unquiesce
 		// whenever a Raft operation is performed.
 		//
@@ -1250,7 +1250,7 @@ func (r *Replica) IsDestroyed() (DestroyReason, error) {
 }
 
 func (r *Replica) isDestroyedRLocked() (DestroyReason, error) {
-	return r.mu.destroyStatus.reason, r.mu.destroyStatus.err
+	return r.shMu.destroyStatus.reason, r.shMu.destroyStatus.err
 }
 
 // IsQuiescent returns whether the replica is quiescent or not.
@@ -2672,11 +2672,11 @@ func (r *Replica) maybeWatchForMergeLocked(ctx context.Context) (bool, error) {
 		r.raftMu.Lock()
 		r.readOnlyCmdMu.Lock()
 		r.mu.Lock()
-		if mergeCommitted && r.mu.destroyStatus.IsAlive() {
+		if mergeCommitted && r.shMu.destroyStatus.IsAlive() {
 			// The merge committed but the left-hand replica on this store hasn't
 			// subsumed this replica yet. Mark this replica as destroyed so it
 			// doesn't serve requests when we close the mergeCompleteCh below.
-			r.mu.destroyStatus.Set(kvpb.NewRangeNotFoundError(r.RangeID, r.store.StoreID()), destroyReasonMergePending)
+			r.shMu.destroyStatus.Set(kvpb.NewRangeNotFoundError(r.RangeID, r.store.StoreID()), destroyReasonMergePending)
 		}
 		// Unblock pending requests. If the merge committed, the requests will
 		// notice that the replica has been destroyed and return an appropriate

pkg/kv/kvserver/replica_app_batch.go

@@ -353,7 +353,7 @@ func (b *replicaAppBatch) runPostAddTriggersReplicaOnly(
 		// commits by handleMergeResult() to finish the removal.
 		rhsRepl.readOnlyCmdMu.Lock()
 		rhsRepl.mu.Lock()
-		rhsRepl.mu.destroyStatus.Set(
+		rhsRepl.shMu.destroyStatus.Set(
 			kvpb.NewRangeNotFoundError(rhsRepl.RangeID, rhsRepl.store.StoreID()),
 			destroyReasonRemoved)
 		rhsRepl.mu.Unlock()
@@ -443,7 +443,7 @@ func (b *replicaAppBatch) runPostAddTriggersReplicaOnly(
 		// application.
 		b.r.readOnlyCmdMu.Lock()
 		b.r.mu.Lock()
-		b.r.mu.destroyStatus.Set(
+		b.r.shMu.destroyStatus.Set(
 			kvpb.NewRangeNotFoundError(b.r.RangeID, b.r.store.StoreID()),
 			destroyReasonRemoved)
 		span := b.r.descRLocked().RSpan()

pkg/kv/kvserver/replica_application_state_machine_test.go

@@ -153,7 +153,7 @@ func TestReplicaStateMachineChangeReplicas(t *testing.T) {
 		defer r.readOnlyCmdMu.Unlock()
 		r.mu.Lock()
 		defer r.mu.Unlock()
-		r.mu.destroyStatus.Set(errors.New("test done"), destroyReasonRemoved)
+		r.shMu.destroyStatus.Set(errors.New("test done"), destroyReasonRemoved)
 	})
 }
 
@@ -241,7 +241,7 @@ func TestReplicaStateMachineRaftLogTruncationStronglyCoupled(t *testing.T) {
 			r.readOnlyCmdMu.Lock()
 			r.mu.Lock()
 			defer r.mu.Unlock()
-			r.mu.destroyStatus.Set(errors.New("test done"), destroyReasonRemoved)
+			r.shMu.destroyStatus.Set(errors.New("test done"), destroyReasonRemoved)
 			r.readOnlyCmdMu.Unlock()
 
 			require.Equal(t, raftAppliedIndex+1, r.shMu.state.RaftAppliedIndex)
@@ -436,7 +436,7 @@ func TestReplicaStateMachineEphemeralAppBatchRejection(t *testing.T) {
 		defer r.readOnlyCmdMu.Unlock()
 		r.mu.Lock()
 		defer r.mu.Unlock()
-		r.mu.destroyStatus.Set(errors.New("boom"), destroyReasonRemoved)
+		r.shMu.destroyStatus.Set(errors.New("boom"), destroyReasonRemoved)
 	}()
 
 	sm := r.getStateMachine()

pkg/kv/kvserver/replica_command_test.go

@@ -682,7 +682,7 @@ func TestWaitForLeaseAppliedIndex(t *testing.T) {
 		defer tc.repl.readOnlyCmdMu.Unlock()
 		tc.repl.mu.Lock()
 		defer tc.repl.mu.Unlock()
-		tc.repl.mu.destroyStatus.Set(destroyErr, destroyReasonRemoved)
+		tc.repl.shMu.destroyStatus.Set(destroyErr, destroyReasonRemoved)
 	}()
 
 	_, err = tc.repl.WaitForLeaseAppliedIndex(ctx, maxLAI)

pkg/kv/kvserver/replica_corruption.go

@@ -43,7 +43,7 @@ func (r *Replica) setCorruptRaftMuLocked(
 
 	log.Dev.ErrorfDepth(ctx, 1, "stalling replica due to: %s", cErr.ErrorMsg)
 	cErr.Processed = true
-	r.mu.destroyStatus.Set(cErr, destroyReasonRemoved)
+	r.shMu.destroyStatus.Set(cErr, destroyReasonRemoved)
 
 	auxDir := r.store.TODOEngine().GetAuxiliaryDir()
 	_ = r.store.TODOEngine().Env().MkdirAll(auxDir, os.ModePerm)

pkg/kv/kvserver/replica_destroy.go

@@ -167,7 +167,7 @@ func (r *Replica) disconnectReplicationRaftMuLocked(ctx context.Context) {
 		p.finishApplication(ctx, makeProposalResultErr(kvpb.NewAmbiguousResultError(apply.ErrRemoved)))
 	}
 
-	if !r.mu.destroyStatus.Removed() {
+	if !r.shMu.destroyStatus.Removed() {
 		log.Dev.Fatalf(ctx, "removing raft group before destroying replica %s", r)
 	}
 	r.mu.internalRaftGroup = nil

pkg/kv/kvserver/replica_proposal_buf.go

@@ -1116,7 +1116,7 @@ func (rp *replicaProposer) getReplicaID() roachpb.ReplicaID {
 }
 
 func (rp *replicaProposer) destroyed() destroyStatus {
-	return rp.mu.destroyStatus
+	return rp.shMu.destroyStatus
 }
 
 func (rp *replicaProposer) leaseAppliedIndex() kvpb.LeaseAppliedIndex {

pkg/kv/kvserver/replica_raft.go

@@ -1648,7 +1648,7 @@ func (r *Replica) refreshProposalsLocked(
 	}
 
 	r.mu.slowProposalCount = slowProposalCount
-	destroyed := r.mu.destroyStatus.Removed()
+	destroyed := r.shMu.destroyStatus.Removed()
 
 	// If the breaker isn't tripped yet but we've detected commands that have
 	// taken too long to replicate, and the replica is not destroyed, trip the
@@ -2222,7 +2222,7 @@ func (s pendingCmdSlice) Less(i, j int) bool {
 func (r *Replica) withRaftGroupLocked(
 	f func(r *raft.RawNode) (unquiesceAndWakeLeader bool, _ error),
 ) error {
-	if r.mu.destroyStatus.Removed() {
+	if r.shMu.destroyStatus.Removed() {
 		// Callers know to detect errRemoved as non-fatal.
 		return errRemoved
 	}

pkg/kv/kvserver/replica_raftstorage.go

@@ -577,7 +577,7 @@ func (r *Replica) applySnapshotRaftMuLocked(
 		// erroneously return empty data.
 		sr.readOnlyCmdMu.Lock()
 		sr.mu.Lock()
-		sr.mu.destroyStatus.Set(
+		sr.shMu.destroyStatus.Set(
 			kvpb.NewRangeNotFoundError(sr.RangeID, sr.store.StoreID()),
 			destroyReasonRemoved)
 		sr.mu.Unlock()

pkg/kv/kvserver/replica_test.go

@@ -13786,7 +13786,7 @@ func TestAdminScatterDestroyedReplica(t *testing.T) {
 		defer tc.repl.readOnlyCmdMu.Unlock()
 		tc.repl.mu.Lock()
 		defer tc.repl.mu.Unlock()
-		tc.repl.mu.destroyStatus.Set(errBoom, destroyReasonMergePending)
+		tc.repl.shMu.destroyStatus.Set(errBoom, destroyReasonMergePending)
 	}()
 
 	desc := tc.repl.Desc()