From 07cb2b1d22be82abb87fd2a5f95ae86e760c87e6 Mon Sep 17 00:00:00 2001 From: Tom Clegg Date: Sat, 13 Jan 2018 15:09:40 -0500 Subject: [PATCH] 13111: Improve deadlock prevention. Prevent unsupported renames. Arvados-DCO-1.1-Signed-off-by: Tom Clegg --- sdk/go/arvados/fs_base.go | 51 +++++++++++++++++++++++++++++----- sdk/go/arvados/fs_site.go | 13 +++++---- sdk/go/arvados/fs_site_test.go | 21 ++++++++++++++ 3 files changed, 72 insertions(+), 13 deletions(-) diff --git a/sdk/go/arvados/fs_base.go b/sdk/go/arvados/fs_base.go index fd8c18b3de..6402a8ec3f 100644 --- 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 d3ca5102be..9f3dbce0d8 100644 --- 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 a8c369f45b..26a2212a07 100644 --- 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) } -- 2.30.2