[ARVADOS] updated: 2.1.0-933-g9f55b356f

[Git user]

Summary of changes:
 sdk/go/arvados/fs_base.go         | 19 +++++++++++++++++++
 sdk/go/arvados/fs_collection.go   | 11 +++++++++--
 sdk/go/arvados/fs_site.go         |  6 ++++++
 sdk/go/arvados/fs_site_test.go    |  9 +++++++++
 services/keep-web/handler_test.go |  4 ++++
 5 files changed, 47 insertions(+), 2 deletions(-)

       via  9f55b356fb37e06d2aef1c1c5acb8a9878dfe85b (commit)
      from  07b765184292a503111416ef0df168282350ce2e (commit)

Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.


commit 9f55b356fb37e06d2aef1c1c5acb8a9878dfe85b
Author: Tom Clegg <tom at curii.com>
Date:   Fri Jun 18 10:11:27 2021 -0400

    17464: Add DebugLocksPanicMode and fix some missing locks.
    
    refs #17464
    
    Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom at curii.com>

diff --git a/sdk/go/arvados/fs_base.go b/sdk/go/arvados/fs_base.go
index 2478641df..65c207162 100644
--- a/sdk/go/arvados/fs_base.go
+++ b/sdk/go/arvados/fs_base.go
@@ -29,12 +29,29 @@ var (
 	ErrIsDirectory       = errors.New("cannot rename file to overwrite existing directory")
 	ErrNotADirectory     = errors.New("not a directory")
 	ErrPermission        = os.ErrPermission
+	DebugLocksPanicMode  = false
 )
 
 type syncer interface {
 	Sync() error
 }
 
+func debugPanicIfNotLocked(l sync.Locker) {
+	if !DebugLocksPanicMode {
+		return
+	}
+	race := false
+	go func() {
+		l.Lock()
+		race = true
+		l.Unlock()
+	}()
+	time.Sleep(10)
+	if race {
+		panic("bug: caller-must-have-lock func called, but nobody has lock")
+	}
+}
+
 // A File is an *os.File-like interface for reading and writing files
 // in a FileSystem.
 type File interface {
@@ -271,6 +288,7 @@ func (n *treenode) IsDir() bool {
 }
 
 func (n *treenode) Child(name string, replace func(inode) (inode, error)) (child inode, err error) {
+	debugPanicIfNotLocked(n)
 	child = n.inodes[name]
 	if name == "" || name == "." || name == ".." {
 		err = ErrInvalidArgument
@@ -333,6 +351,7 @@ func (n *treenode) Sync() error {
 func (n *treenode) MemorySize() (size int64) {
 	n.RLock()
 	defer n.RUnlock()
+	debugPanicIfNotLocked(n)
 	for _, inode := range n.inodes {
 		size += inode.MemorySize()
 	}
diff --git a/sdk/go/arvados/fs_collection.go b/sdk/go/arvados/fs_collection.go
index 0233826a7..22e2b31d5 100644
--- a/sdk/go/arvados/fs_collection.go
+++ b/sdk/go/arvados/fs_collection.go
@@ -1167,9 +1167,12 @@ func (dn *dirnode) createFileAndParents(path string) (fn *filenode, err error) {
 			node = node.Parent()
 			continue
 		}
+		modtime := node.Parent().FileInfo().ModTime()
+		node.Lock()
+		locked := node
 		node, err = node.Child(name, func(child inode) (inode, error) {
 			if child == nil {
-				child, err := node.FS().newNode(name, 0755|os.ModeDir, node.Parent().FileInfo().ModTime())
+				child, err := node.FS().newNode(name, 0755|os.ModeDir, modtime)
 				if err != nil {
 					return nil, err
 				}
@@ -1181,6 +1184,7 @@ func (dn *dirnode) createFileAndParents(path string) (fn *filenode, err error) {
 				return child, nil
 			}
 		})
+		locked.Unlock()
 		if err != nil {
 			return
 		}
@@ -1191,10 +1195,13 @@ func (dn *dirnode) createFileAndParents(path string) (fn *filenode, err error) {
 		err = fmt.Errorf("invalid file part %q in path %q", basename, path)
 		return
 	}
+	modtime := node.FileInfo().ModTime()
+	node.Lock()
+	defer node.Unlock()
 	_, err = node.Child(basename, func(child inode) (inode, error) {
 		switch child := child.(type) {
 		case nil:
-			child, err = node.FS().newNode(basename, 0755, node.FileInfo().ModTime())
+			child, err = node.FS().newNode(basename, 0755, modtime)
 			if err != nil {
 				return nil, err
 			}
diff --git a/sdk/go/arvados/fs_site.go b/sdk/go/arvados/fs_site.go
index 900893aa3..5225df59e 100644
--- a/sdk/go/arvados/fs_site.go
+++ b/sdk/go/arvados/fs_site.go
@@ -54,6 +54,8 @@ func (c *Client) CustomFileSystem(kc keepClient) CustomFileSystem {
 }
 
 func (fs *customFileSystem) MountByID(mount string) {
+	fs.root.treenode.Lock()
+	defer fs.root.treenode.Unlock()
 	fs.root.treenode.Child(mount, func(inode) (inode, error) {
 		return &vdirnode{
 			treenode: treenode{
@@ -72,12 +74,16 @@ func (fs *customFileSystem) MountByID(mount string) {
 }
 
 func (fs *customFileSystem) MountProject(mount, uuid string) {
+	fs.root.treenode.Lock()
+	defer fs.root.treenode.Unlock()
 	fs.root.treenode.Child(mount, func(inode) (inode, error) {
 		return fs.newProjectNode(fs.root, mount, uuid), nil
 	})
 }
 
 func (fs *customFileSystem) MountUsers(mount string) {
+	fs.root.treenode.Lock()
+	defer fs.root.treenode.Unlock()
 	fs.root.treenode.Child(mount, func(inode) (inode, error) {
 		return &lookupnode{
 			stale:   fs.Stale,
diff --git a/sdk/go/arvados/fs_site_test.go b/sdk/go/arvados/fs_site_test.go
index b1c627f89..a41a9f4da 100644
--- a/sdk/go/arvados/fs_site_test.go
+++ b/sdk/go/arvados/fs_site_test.go
@@ -32,6 +32,15 @@ const (
 
 var _ = check.Suite(&SiteFSSuite{})
 
+func init() {
+	// Enable DebugLocksPanicMode sometimes. Don't enable it all
+	// the time, though -- it adds many calls to time.Sleep(),
+	// which could hide different bugs.
+	if time.Now().Seconds()&1 == 0 {
+		DebugLocksPanicMode = true
+	}
+}
+
 type SiteFSSuite struct {
 	client *Client
 	fs     CustomFileSystem
diff --git a/services/keep-web/handler_test.go b/services/keep-web/handler_test.go
index 446d591bf..8715ab24f 100644
--- a/services/keep-web/handler_test.go
+++ b/services/keep-web/handler_test.go
@@ -32,6 +32,10 @@ import (
 
 var _ = check.Suite(&UnitSuite{})
 
+func init() {
+	arvados.DebugLocksPanicMode = true
+}
+
 type UnitSuite struct {
 	Config *arvados.Config
 }

-----------------------------------------------------------------------


hooks/post-receive
--