13111: Improve deadlock prevention. Prevent unsupported renames.

Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tclegg@veritasgenetics.com>

diff --git a/sdk/go/arvados/fs_base.go b/sdk/go/arvados/fs_base.go
index fd8c18b3def852cb48f11c25439e1fa1f3a5c75f..6402a8ec3fa49c189636d6198d4fa1eb2f5e4ed8 100644 (file)
--- a/sdk/go/arvados/fs_base.go
+++ b/sdk/go/arvados/fs_base.go
@@ -53,6 +53,10 @@ type FileSystem interface {
 
        rootnode() inode
 
+       // filesystem-wide lock: used by Rename() to prevent deadlock
+       // while locking multiple inodes.
+       locker() sync.Locker
+
        // create a new node with nil parent.
        newNode(name string, perm os.FileMode, modTime time.Time) (node inode, err error)
 
@@ -272,12 +276,17 @@ func (n *treenode) Readdir() (fi []os.FileInfo) {
 type fileSystem struct {
        root inode
        fsBackend
+       mutex sync.Mutex
 }
 
 func (fs *fileSystem) rootnode() inode {
        return fs.root
 }
 
+func (fs *fileSystem) locker() sync.Locker {
+       return &fs.mutex
+}
+
 // OpenFile is analogous to os.OpenFile().
 func (fs *fileSystem) OpenFile(name string, flag int, perm os.FileMode) (File, error) {
        return fs.openFile(name, flag, perm)
@@ -424,12 +433,31 @@ func (fs *fileSystem) Rename(oldname, newname string) error {
        }
        defer newdirf.Close()
 
-       // When acquiring locks on multiple nodes, all common
-       // ancestors must be locked first in order to avoid
-       // deadlock. This is assured by locking the path from
-       // filesystem root to newdir, then locking the path from
-       // filesystem root to olddir, skipping any already-locked
-       // nodes.
+       // TODO: If the nearest common ancestor ("nca") of olddirf and
+       // newdirf is on a different filesystem than fs, we should
+       // call nca.FS().Rename() instead of proceeding. Until then
+       // it's awkward for filesystems to implement their own Rename
+       // methods effectively: the only one that runs is the one on
+       // the root filesystem exposed to the caller (webdav, fuse,
+       // etc).
+
+       // When acquiring locks on multiple inodes, avoid deadlock by
+       // locking the entire containing filesystem first.
+       cfs := olddirf.inode.FS()
+       cfs.locker().Lock()
+       defer cfs.locker().Unlock()
+
+       if cfs != newdirf.inode.FS() {
+               // Moving inodes across filesystems is not (yet)
+               // supported. Locking inodes from different
+               // filesystems could deadlock, so we must error out
+               // now.
+               return ErrInvalidArgument
+       }
+
+       // To ensure we can test reliably whether we're about to move
+       // a directory into itself, lock all potential common
+       // ancestors of olddir and newdir.
        needLock := []sync.Locker{}
        for _, node := range []inode{olddirf.inode, newdirf.inode} {
                needLock = append(needLock, node)
@@ -447,8 +475,11 @@ func (fs *fileSystem) Rename(oldname, newname string) error {
                }
        }
 
-       err = nil
+       // Return ErrInvalidOperation if olddirf.inode doesn't even
+       // bother calling our "remove oldname entry" replacer func.
+       err = ErrInvalidArgument
        olddirf.inode.Child(oldname, func(oldinode inode) inode {
+               err = nil
                if oldinode == nil {
                        err = os.ErrNotExist
                        return nil
@@ -458,6 +489,12 @@ func (fs *fileSystem) Rename(oldname, newname string) error {
                        err = ErrInvalidArgument
                        return oldinode
                }
+               if oldinode.FS() != cfs && newdirf.inode != olddirf.inode {
+                       // moving a mount point to a different parent
+                       // is not (yet) supported.
+                       err = ErrInvalidArgument
+                       return oldinode
+               }
                accepted := newdirf.inode.Child(newname, func(existing inode) inode {
                        if existing != nil && existing.IsDir() {
                                err = ErrIsDirectory

diff --git a/sdk/go/arvados/fs_site.go b/sdk/go/arvados/fs_site.go
index d3ca5102bebee4ee2865b018e0c8d94abfc8add5..9f3dbce0d89147c569ac81489a5adcfcbdfc05ea 100644 (file)
--- a/sdk/go/arvados/fs_site.go
+++ b/sdk/go/arvados/fs_site.go
@@ -20,13 +20,16 @@ type siteFileSystem struct {
 // there are significant known bugs and shortcomings. For example,
 // writes are not persisted until Sync() is called.
 func (c *Client) SiteFileSystem(kc keepClient) FileSystem {
+       root := &vdirnode{}
        fs := &siteFileSystem{
                fileSystem: fileSystem{
                        fsBackend: keepBackend{apiClient: c, keepClient: kc},
+                       root:      root,
                },
        }
-       root := &treenode{
-               fs: fs,
+       root.inode = &treenode{
+               fs:     fs,
+               parent: root,
                fileinfo: fileinfo{
                        name:    "/",
                        mode:    os.ModeDir | 0755,
@@ -34,13 +37,12 @@ func (c *Client) SiteFileSystem(kc keepClient) FileSystem {
                },
                inodes: make(map[string]inode),
        }
-       root.parent = root
-       root.Child("by_id", func(inode) inode {
+       root.inode.Child("by_id", func(inode) inode {
                var vn inode
                vn = &vdirnode{
                        inode: &treenode{
                                fs:     fs,
-                               parent: root,
+                               parent: fs.root,
                                inodes: make(map[string]inode),
                                fileinfo: fileinfo{
                                        name:    "by_id",
@@ -52,7 +54,6 @@ func (c *Client) SiteFileSystem(kc keepClient) FileSystem {
                }
                return vn
        })
-       fs.root = root
        return fs
 }
 

diff --git a/sdk/go/arvados/fs_site_test.go b/sdk/go/arvados/fs_site_test.go
index a8c369f45b7b4c99fcc4216f991f721e16f21ba2..26a2212a0771b1c12e5cb7c2825e59b48dd6cfcd 100644 (file)
--- a/sdk/go/arvados/fs_site_test.go
+++ b/sdk/go/arvados/fs_site_test.go
@@ -6,6 +6,7 @@ package arvados
 
 import (
        "net/http"
+       "os"
 
        "git.curoverse.com/arvados.git/sdk/go/arvadostest"
        check "gopkg.in/check.v1"
@@ -47,6 +48,12 @@ func (s *SiteFSSuite) TestByUUID(c *check.C) {
        c.Check(err, check.IsNil)
        c.Check(len(fis), check.Equals, 0)
 
+       err = s.fs.Mkdir("/by_id/"+arvadostest.FooCollection, 0755)
+       c.Check(err, check.Equals, os.ErrExist)
+
+       f, err = s.fs.Open("/by_id/" + arvadostest.NonexistentCollection)
+       c.Assert(err, check.Equals, os.ErrNotExist)
+
        f, err = s.fs.Open("/by_id/" + arvadostest.FooCollection)
        c.Assert(err, check.IsNil)
        fis, err = f.Readdir(-1)
@@ -55,4 +62,18 @@ func (s *SiteFSSuite) TestByUUID(c *check.C) {
                names = append(names, fi.Name())
        }
        c.Check(names, check.DeepEquals, []string{"foo"})
+
+       _, err = s.fs.OpenFile("/by_id/"+arvadostest.NonexistentCollection, os.O_RDWR|os.O_CREATE, 0755)
+       c.Check(err, check.Equals, ErrInvalidOperation)
+       err = s.fs.Rename("/by_id/"+arvadostest.FooCollection, "/by_id/beep")
+       c.Check(err, check.Equals, ErrInvalidArgument)
+       err = s.fs.Rename("/by_id/"+arvadostest.FooCollection+"/foo", "/by_id/beep")
+       c.Check(err, check.Equals, ErrInvalidArgument)
+       _, err = s.fs.Stat("/by_id/beep")
+       c.Check(err, check.Equals, os.ErrNotExist)
+       err = s.fs.Rename("/by_id/"+arvadostest.FooCollection+"/foo", "/by_id/"+arvadostest.FooCollection+"/bar")
+       c.Check(err, check.IsNil)
+
+       err = s.fs.Rename("/by_id", "/beep")
+       c.Check(err, check.Equals, ErrInvalidArgument)
 }