fs/accounting: fix deadlock on GetBytes

A deadlock could occur since we have now put a mutex on GetBytes from
StatsInfo.String (s.mu) - progress (acc.statmu) and read (acc.statmu)
- GetBytes (s.mu).

Fix this by giving stringSet its own locking and excluding the call
which caused the deadlock from the mutex in StatsInfo.String.

fs/accounting/stats.go

@@ -22,14 +22,14 @@ func init() {
 
 // StatsInfo accounts all transfers
 type StatsInfo struct {
-	lock         sync.RWMutex
+	mu           sync.RWMutex
 	bytes        int64
 	errors       int64
 	lastError    error
 	checks       int64
-	checking     stringSet
+	checking     *stringSet
 	transfers    int64
-	transferring stringSet
+	transferring *stringSet
 	deletes      int64
 	start        time.Time
 	inProgress   *inProgress
@@ -38,8 +38,8 @@ type StatsInfo struct {
 // NewStats cretates an initialised StatsInfo
 func NewStats() *StatsInfo {
 	return &StatsInfo{
-		checking:     make(stringSet, fs.Config.Checkers),
+		checking:     newStringSet(fs.Config.Checkers),
-		transferring: make(stringSet, fs.Config.Transfers),
+		transferring: newStringSet(fs.Config.Transfers),
 		start:        time.Now(),
 		inProgress:   newInProgress(),
 	}
@@ -47,8 +47,8 @@ func NewStats() *StatsInfo {
 
 // String convert the StatsInfo to a string for printing
 func (s *StatsInfo) String() string {
-	s.lock.RLock()
+	s.mu.RLock()
-	defer s.lock.RUnlock()
+
 	dt := time.Now().Sub(s.start)
 	dtSeconds := dt.Seconds()
 	speed := 0.0
@@ -74,10 +74,15 @@ Elapsed time:  %10v
 		s.checks,
 		s.transfers,
 		dtRounded)
-	if len(s.checking) > 0 {
+
+	// checking and transferring have their own locking so unlock
+	// here to prevent deadlock on GetBytes
+	s.mu.RUnlock()
+
+	if !s.checking.empty() {
 		fmt.Fprintf(buf, "Checking:\n%s\n", s.checking)
 	}
-	if len(s.transferring) > 0 {
+	if !s.transferring.empty() {
 		fmt.Fprintf(buf, "Transferring:\n%s\n", s.transferring)
 	}
 	return buf.String()
@@ -90,51 +95,51 @@ func (s *StatsInfo) Log() {
 
 // Bytes updates the stats for bytes bytes
 func (s *StatsInfo) Bytes(bytes int64) {
-	s.lock.Lock()
+	s.mu.Lock()
-	defer s.lock.Unlock()
+	defer s.mu.Unlock()
 	s.bytes += bytes
 }
 
 // GetBytes returns the number of bytes transferred so far
 func (s *StatsInfo) GetBytes() int64 {
-	s.lock.Lock()
+	s.mu.Lock()
-	defer s.lock.Unlock()
+	defer s.mu.Unlock()
 	return s.bytes
 }
 
 // Errors updates the stats for errors
 func (s *StatsInfo) Errors(errors int64) {
-	s.lock.Lock()
+	s.mu.Lock()
-	defer s.lock.Unlock()
+	defer s.mu.Unlock()
 	s.errors += errors
 }
 
 // GetErrors reads the number of errors
 func (s *StatsInfo) GetErrors() int64 {
-	s.lock.RLock()
+	s.mu.RLock()
-	defer s.lock.RUnlock()
+	defer s.mu.RUnlock()
 	return s.errors
 }
 
 // GetLastError returns the lastError
 func (s *StatsInfo) GetLastError() error {
-	s.lock.RLock()
+	s.mu.RLock()
-	defer s.lock.RUnlock()
+	defer s.mu.RUnlock()
 	return s.lastError
 }
 
 // Deletes updates the stats for deletes
 func (s *StatsInfo) Deletes(deletes int64) int64 {
-	s.lock.Lock()
+	s.mu.Lock()
-	defer s.lock.Unlock()
+	defer s.mu.Unlock()
 	s.deletes += deletes
 	return s.deletes
 }
 
 // ResetCounters sets the counters (bytes, checks, errors, transfers) to 0
 func (s *StatsInfo) ResetCounters() {
-	s.lock.RLock()
+	s.mu.RLock()
-	defer s.lock.RUnlock()
+	defer s.mu.RUnlock()
 	s.bytes = 0
 	s.errors = 0
 	s.checks = 0
@@ -144,63 +149,59 @@ func (s *StatsInfo) ResetCounters() {
 
 // ResetErrors sets the errors count to 0
 func (s *StatsInfo) ResetErrors() {
-	s.lock.RLock()
+	s.mu.RLock()
-	defer s.lock.RUnlock()
+	defer s.mu.RUnlock()
 	s.errors = 0
 }
 
 // Errored returns whether there have been any errors
 func (s *StatsInfo) Errored() bool {
-	s.lock.RLock()
+	s.mu.RLock()
-	defer s.lock.RUnlock()
+	defer s.mu.RUnlock()
 	return s.errors != 0
 }
 
 // Error adds a single error into the stats and assigns lastError
 func (s *StatsInfo) Error(err error) {
-	s.lock.Lock()
+	s.mu.Lock()
-	defer s.lock.Unlock()
+	defer s.mu.Unlock()
 	s.errors++
 	s.lastError = err
 }
 
 // Checking adds a check into the stats
 func (s *StatsInfo) Checking(remote string) {
-	s.lock.Lock()
+	s.checking.add(remote)
-	defer s.lock.Unlock()
-	s.checking[remote] = struct{}{}
 }
 
 // DoneChecking removes a check from the stats
 func (s *StatsInfo) DoneChecking(remote string) {
-	s.lock.Lock()
+	s.checking.del(remote)
-	defer s.lock.Unlock()
+	s.mu.Lock()
-	delete(s.checking, remote)
 	s.checks++
+	s.mu.Unlock()
 }
 
 // GetTransfers reads the number of transfers
 func (s *StatsInfo) GetTransfers() int64 {
-	s.lock.RLock()
+	s.mu.RLock()
-	defer s.lock.RUnlock()
+	defer s.mu.RUnlock()
 	return s.transfers
 }
 
 // Transferring adds a transfer into the stats
 func (s *StatsInfo) Transferring(remote string) {
-	s.lock.Lock()
+	s.transferring.add(remote)
-	defer s.lock.Unlock()
-	s.transferring[remote] = struct{}{}
 }
 
 // DoneTransferring removes a transfer from the stats
 //
 // if ok is true then it increments the transfers count
 func (s *StatsInfo) DoneTransferring(remote string, ok bool) {
-	s.lock.Lock()
+	s.transferring.del(remote)
-	defer s.lock.Unlock()
-	delete(s.transferring, remote)
 	if ok {
+		s.mu.Lock()
 		s.transfers++
+		s.mu.Unlock()
 	}
 }

fs/accounting/stringset.go

@@ -3,15 +3,49 @@ package accounting
 import (
 	"sort"
 	"strings"
+	"sync"
 )
 
 // stringSet holds a set of strings
-type stringSet map[string]struct{}
+type stringSet struct {
+	mu    sync.RWMutex
+	items map[string]struct{}
+}
+
+// newStringSet creates a new empty string set of capacity size
+func newStringSet(size int) *stringSet {
+	return &stringSet{
+		items: make(map[string]struct{}, size),
+	}
+}
+
+// add adds remote to the set
+func (ss *stringSet) add(remote string) {
+	ss.mu.Lock()
+	ss.items[remote] = struct{}{}
+	ss.mu.Unlock()
+}
+
+// del removes remote from the set
+func (ss *stringSet) del(remote string) {
+	ss.mu.Lock()
+	delete(ss.items, remote)
+	ss.mu.Unlock()
+}
+
+// empty returns whether the set has any items
+func (ss *stringSet) empty() bool {
+	ss.mu.RLock()
+	defer ss.mu.RUnlock()
+	return len(ss.items) == 0
+}
 
 // Strings returns all the strings in the stringSet
-func (ss stringSet) Strings() []string {
+func (ss *stringSet) Strings() []string {
-	strings := make([]string, 0, len(ss))
+	ss.mu.RLock()
-	for name := range ss {
+	defer ss.mu.RUnlock()
+	strings := make([]string, 0, len(ss.items))
+	for name := range ss.items {
 		var out string
 		if acc := Stats.inProgress.get(name); acc != nil {
 			out = acc.String()
@@ -26,6 +60,6 @@ func (ss stringSet) Strings() []string {
 }
 
 // String returns all the file names in the stringSet joined by newline
-func (ss stringSet) String() string {
+func (ss *stringSet) String() string {
 	return strings.Join(ss.Strings(), "\n")
 }