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>

diff --git a/sdk/go/arvados/fs_collection.go b/sdk/go/arvados/fs_collection.go
index bcf2f4810432039dc436ca03f1a8a9200f1c0b0a..1fe4d8f56680a6b51b0e82e973ab7fc2570986f4 100644 (file)
--- 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 ac3bbbbada39dc2a68727aee6a1ab27ae45b35e9..96347737f8f24dce8c56518cf28a106069ef4a28 100644 (file)
--- 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)
                                }