From 27d98c4c36f840686d5b7414500c7b006e80c114 Mon Sep 17 00:00:00 2001
From: Tom Clegg <tclegg@veritasgenetics.com>
Date: Sat, 22 Sep 2018 02:07:22 -0400
Subject: [PATCH] 10181: Fix race in MarshalManifest.

Ensure child nodes stay locked between sync and marshal. Otherwise
concurrent file writes can add new memSegments that can't be
marshaled.

Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tclegg@veritasgenetics.com>
---
 sdk/go/arvados/fs_collection.go      | 30 +++++++++++-----------------
 sdk/go/arvados/fs_collection_test.go | 14 ++++++++-----
 2 files changed, 21 insertions(+), 23 deletions(-)

diff --git a/sdk/go/arvados/fs_collection.go b/sdk/go/arvados/fs_collection.go
index bcf2f48104..1fe4d8f566 100644
--- a/sdk/go/arvados/fs_collection.go
+++ b/sdk/go/arvados/fs_collection.go
@@ -548,9 +548,10 @@ func (dn *dirnode) Child(name string, replace func(inode) (inode, error)) (inode
 	return dn.treenode.Child(name, replace)
 }
 
-// sync flushes in-memory data (for all files in the tree rooted at
-// dn) to persistent storage. Caller must hold dn.Lock().
-func (dn *dirnode) sync() error {
+// sync flushes in-memory data (for the children with the given names,
+// which must be children of dn) to persistent storage. Caller must
+// have write lock on dn and the named children.
+func (dn *dirnode) sync(names []string) error {
 	type shortBlock struct {
 		fn  *filenode
 		idx int
@@ -586,19 +587,11 @@ func (dn *dirnode) sync() error {
 		return nil
 	}
 
-	names := make([]string, 0, len(dn.inodes))
-	for name := range dn.inodes {
-		names = append(names, name)
-	}
-	sort.Strings(names)
-
 	for _, name := range names {
 		fn, ok := dn.inodes[name].(*filenode)
 		if !ok {
 			continue
 		}
-		fn.Lock()
-		defer fn.Unlock()
 		for idx, seg := range fn.segments {
 			seg, ok := seg.(*memSegment)
 			if !ok {
@@ -636,18 +629,19 @@ func (dn *dirnode) marshalManifest(prefix string) (string, error) {
 	var subdirs string
 	var blocks []string
 
-	if err := dn.sync(); err != nil {
-		return "", err
-	}
-
 	names := make([]string, 0, len(dn.inodes))
-	for name, node := range dn.inodes {
+	for name := range dn.inodes {
 		names = append(names, name)
+	}
+	sort.Strings(names)
+	for _, name := range names {
+		node := dn.inodes[name]
 		node.Lock()
 		defer node.Unlock()
 	}
-	sort.Strings(names)
-
+	if err := dn.sync(names); err != nil {
+		return "", err
+	}
 	for _, name := range names {
 		switch node := dn.inodes[name].(type) {
 		case *dirnode:
diff --git a/sdk/go/arvados/fs_collection_test.go b/sdk/go/arvados/fs_collection_test.go
index ac3bbbbada..96347737f8 100644
--- a/sdk/go/arvados/fs_collection_test.go
+++ b/sdk/go/arvados/fs_collection_test.go
@@ -491,15 +491,19 @@ func (s *CollectionFSSuite) TestConcurrentWriters(c *check.C) {
 			c.Assert(err, check.IsNil)
 			defer f.Close()
 			for i := 0; i < 6502; i++ {
-				switch rand.Int() & 3 {
-				case 0:
+				r := rand.Uint32()
+				switch {
+				case r%11 == 0:
+					_, err := s.fs.MarshalManifest(".")
+					c.Check(err, check.IsNil)
+				case r&3 == 0:
 					f.Truncate(int64(rand.Intn(64)))
-				case 1:
+				case r&3 == 1:
 					f.Seek(int64(rand.Intn(64)), io.SeekStart)
-				case 2:
+				case r&3 == 2:
 					_, err := f.Write([]byte("beep boop"))
 					c.Check(err, check.IsNil)
-				case 3:
+				case r&3 == 3:
 					_, err := ioutil.ReadAll(f)
 					c.Check(err, check.IsNil)
 				}
-- 
2.30.2