nodefs: move the parent tracking from pathfs into nodefs

The reason we have to do parent tracking is combination of (a) the Node
structure has a map to its children (b) FORGET messages don't enumerate
parents of a node, just the node to be forgotten.
Hence to keep the map in sync with the kernel's FS tree, we have to update
the parents of a node on FORGET too.

This passes 90+ iterations of fsstress-loopback.bash .
( https://github.com/rfjakob/gocryptfs/blob/master/stress_tests/fsstress-loopback.bash )

Change-Id: Idea478d25703a59d17086db59c110ac55281176a

fuse/nodefs/api.go

@@ -37,14 +37,20 @@ type Node interface {
 	// for directory Nodes.
 	Lookup(out *fuse.Attr, name string, context *fuse.Context) (*Inode, fuse.Status)
 
-	// Deletable() should return true if this inode may be
-	// discarded from the children list. This will be called from
-	// within the treeLock critical section, so you cannot look at
-	// other inodes.
+	// Deletable() should return true if this node may be discarded once
+	// the kernel forgets its reference.
+	// If it returns false, OnForget will never get called for this node. This
+	// is appropriate if the filesystem has no persistent backing store
+	// (in-memory filesystems) where discarding the node loses the stored data.
+	// Deletable will be called from within the treeLock critical section, so you
+	// cannot look at other nodes.
 	Deletable() bool
 
-	// OnForget is called when the reference to this inode is
-	// dropped from the tree.
+	// OnForget is called when the kernel forgets its reference to this node and
+	// sends a FORGET request. It should perform cleanup and free memory as
+	// appropriate for the filesystem.
+	// OnForget is not called if the node is a directory and has children.
+	// This is called from within a treeLock critical section.
 	OnForget()
 
 	// Misc.

fuse/nodefs/fsconnector.go

@@ -128,14 +128,33 @@ func (c *FileSystemConnector) forgetUpdate(nodeID uint64, forgetCount int) {
 	if forgotten, handled := c.inodeMap.Forget(nodeID, forgetCount); forgotten {
 		node := (*Inode)(unsafe.Pointer(handled))
 
+		// Prevent concurrent modification of the tree while we are processing
+		// the FORGET
+		//
+		// TODO actually the lock should be taken BEFORE running inodeMap.Forget().
+		// However, treeLock is per-submount, and we don't know which one to lock
+		// in advance. Still passes fsstress testing just fine.
 		node.mount.treeLock.Lock()
+		defer node.mount.treeLock.Unlock()
+
 		if len(node.children) > 0 || !node.Node().Deletable() ||
 			node == c.rootNode || node.mountPoint != nil {
-
-			node.mount.treeLock.Unlock()
+			// We cannot forget a directory that still has children as these
+			// would become unreachable.
 			return
 		}
-		node.mount.treeLock.Unlock()
+		// We have to remove ourself from all parents.
+		// Create a copy of node.parents so we can safely iterate over it
+		// while modifying the original.
+		parents := make(map[parentData]struct{}, len(node.parents))
+		for k, v := range node.parents {
+			parents[k] = v
+		}
+
+		for p := range parents {
+			// This also modifies node.parents
+			p.parent.rmChild(p.name)
+		}
 
 		node.fsInode.OnForget()
 	}

fuse/nodefs/fsmount.go

@@ -27,8 +27,9 @@ type fileSystemMount struct {
 	// Options for the mount.
 	options *Options
 
-	// Protects Children hashmaps within the mount.  treeLock
-	// should be acquired before openFilesLock.
+	// Protects the "children" and "parents" hashmaps of the inodes
+	// within the mount.
+	// treeLock should be acquired before openFilesLock.
 	//
 	// If multiple treeLocks must be acquired, the treeLocks
 	// closer to the root must be acquired first.

fuse/nodefs/inode.go

@@ -7,6 +7,11 @@ import (
 	"github.com/hanwen/go-fuse/fuse"
 )
 
+type parentData struct {
+	parent *Inode
+	name   string
+}
+
 // An Inode reflects the kernel's idea of the inode.  Inodes have IDs
 // that are communicated to the kernel, and they have a tree
 // structure: a directory Inode may contain named children.  Each
@@ -32,6 +37,8 @@ type Inode struct {
 
 	// All data below is protected by treeLock.
 	children map[string]*Inode
+	// Due to hard links, an Inode can have many parents.
+	parents map[parentData]struct{}
 
 	// Non-nil if this inode is a mountpoint, ie. the Root of a
 	// NodeFileSystem.
@@ -40,6 +47,7 @@ type Inode struct {
 
 func newInode(isDir bool, fsNode Node) *Inode {
 	me := new(Inode)
+	me.parents = map[parentData]struct{}{}
 	if isDir {
 		me.children = make(map[string]*Inode, initDirSize)
 	}
@@ -75,6 +83,19 @@ func (n *Inode) Children() (out map[string]*Inode) {
 	return out
 }
 
+// Parent returns a random parent and the name this inode has under this parent.
+// This function can be used to walk up the directory tree. It will not cross
+// sub-mounts.
+func (n *Inode) Parent() (parent *Inode, name string) {
+	if n.mountPoint != nil {
+		return nil, ""
+	}
+	for k := range n.parents {
+		return k.parent, k.name
+	}
+	return nil, ""
+}
+
 // FsChildren returns all the children from the same filesystem.  It
 // will skip mountpoints.
 func (n *Inode) FsChildren() (out map[string]*Inode) {
@@ -144,7 +165,7 @@ func (n *Inode) AddChild(name string, child *Inode) {
 
 // TreeWatcher is an additional interface that Nodes can implement.
 // If they do, the OnAdd and OnRemove are called for operations on the
-// file system tree. The functions run under a lock, so they should
+// file system tree. These functions run under a lock, so they should
 // not do blocking operations.
 type TreeWatcher interface {
 	OnAdd(parent *Inode, name string)
@@ -173,17 +194,20 @@ func (n *Inode) addChild(name string, child *Inode) {
 		}
 	}
 	n.children[name] = child
+	child.parents[parentData{n, name}] = struct{}{}
 	if w, ok := child.Node().(TreeWatcher); ok && child.mountPoint == nil {
 		w.OnAdd(n, name)
 	}
 }
 
-// rmChild drops "name" from our children.
+// rmChild throws out child "name". This means (1) deleting "name" from our
+// "children" map and (2) deleting ourself from the child's "parents" map.
 // Must be called with treeLock for the mount held.
 func (n *Inode) rmChild(name string) *Inode {
 	ch := n.children[name]
 	if ch != nil {
 		delete(n.children, name)
+		delete(ch.parents, parentData{n, name})
 		if w, ok := ch.Node().(TreeWatcher); ok && ch.mountPoint == nil {
 			w.OnRemove(n, name)
 		}

fuse/pathfs/pathfs.go

@@ -12,11 +12,11 @@ import (
 	"github.com/hanwen/go-fuse/fuse/nodefs"
 )
 
-// A parent pointer: node should be reachable as parent.children[name]
-type clientInodePath struct {
-	parent *pathInode
-	name   string
-	node   *pathInode
+// refCountedInode is used in clientInodeMap. The reference count is used to decide
+// if the entry in clientInodeMap can be dropped.
+type refCountedInode struct {
+	node     *pathInode
+	refCount int
 }
 
 // PathNodeFs is the file system that can translate an inode back to a
@@ -32,12 +32,11 @@ type PathNodeFs struct {
 	root      *pathInode
 	connector *nodefs.FileSystemConnector
 
-	// protects pathInode.parents
+	// protects clientInodeMap
 	pathLock sync.RWMutex
 
-	// This map lists all the parent links known for a given
-	// nodeId.
-	clientInodeMap map[uint64][]*clientInodePath
+	// This map lists all the parent links known for a given inode number.
+	clientInodeMap map[uint64]*refCountedInode
 
 	options *PathNodeFsOptions
 }
@@ -68,7 +67,7 @@ func (fs *PathNodeFs) ForgetClientInodes() {
 		return
 	}
 	fs.pathLock.Lock()
-	fs.clientInodeMap = map[uint64][]*clientInodePath{}
+	fs.clientInodeMap = map[uint64]*refCountedInode{}
 	fs.root.forgetClientInodes()
 	fs.pathLock.Unlock()
 }
@@ -188,9 +187,7 @@ func (fs *PathNodeFs) AllFiles(name string, mask uint32) []nodefs.WithFlags {
 // NewPathNodeFs returns a file system that translates from inodes to
 // path names.
 func NewPathNodeFs(fs FileSystem, opts *PathNodeFsOptions) *PathNodeFs {
-	root := &pathInode{
-		parents: map[parentData]struct{}{},
-	}
+	root := &pathInode{}
 	root.fs = fs
 
 	if opts == nil {
@@ -200,7 +197,7 @@ func NewPathNodeFs(fs FileSystem, opts *PathNodeFsOptions) *PathNodeFs {
 	pfs := &PathNodeFs{
 		fs:             fs,
 		root:           root,
-		clientInodeMap: map[uint64][]*clientInodePath{},
+		clientInodeMap: map[uint64]*refCountedInode{},
 		options:        opts,
 	}
 	root.pathFs = pfs
@@ -212,11 +209,6 @@ func (fs *PathNodeFs) Root() nodefs.Node {
 	return fs.root
 }
 
-type parentData struct {
-	parent *pathInode
-	name   string
-}
-
 // This is a combination of dentry (entry in the file/directory and
 // the inode). This structure is used to implement glue for FSes where
 // there is a one-to-one mapping of paths and inodes.
@@ -224,8 +216,6 @@ type pathInode struct {
 	pathFs *PathNodeFs
 	fs     FileSystem
 
-	parents map[parentData]struct{}
-
 	// This is to correctly resolve hardlinks of the underlying
 	// real filesystem.
 	clientInode uint64
@@ -256,14 +246,6 @@ func (n *pathInode) Inode() *nodefs.Inode {
 	return n.inode
 }
 
-// TODO - return *parentData?
-func (n *pathInode) parent() parentData {
-	for k := range n.parents {
-		return k
-	}
-	return parentData{}
-}
-
 func (n *pathInode) SetInode(node *nodefs.Inode) {
 	n.inode = node
 }
@@ -291,24 +273,24 @@ func (n *pathInode) GetPath() string {
 	// effort to avoid allocations.
 
 	n.pathFs.pathLock.RLock()
-	walkUp := n
+	walkUp := n.Inode()
 
-	// TODO - guess depth? use *parentData?
-	parents := make([]parentData, 0, 10)
+	// TODO - guess depth?
+	segments := make([]string, 0, 10)
 	for {
-		p := walkUp.parent()
-		if p.parent == nil {
+		parent, name := walkUp.Parent()
+		if parent == nil {
 			break
 		}
-		parents = append(parents, p)
-		pathLen += len(p.name) + 1
-		walkUp = p.parent
+		segments = append(segments, name)
+		pathLen += len(name) + 1
+		walkUp = parent
 	}
 	pathLen--
 
 	pathBytes := make([]byte, 0, pathLen)
-	for i := len(parents) - 1; i >= 0; i-- {
-		pathBytes = append(pathBytes, parents[i].name...)
+	for i := len(segments) - 1; i >= 0; i-- {
+		pathBytes = append(pathBytes, segments[i]...)
 		if i > 0 {
 			pathBytes = append(pathBytes, '/')
 		}
@@ -320,7 +302,7 @@ func (n *pathInode) GetPath() string {
 		log.Printf("Inode = %q (%s)", path, n.fs.String())
 	}
 
-	if walkUp != n.pathFs.root {
+	if walkUp != n.pathFs.root.Inode() {
 		// This might happen if the node has been removed from
 		// the tree using unlink, but we are forced to run
 		// some file system operation, because the file is
@@ -334,20 +316,9 @@ func (n *pathInode) GetPath() string {
 }
 
 func (n *pathInode) OnAdd(parent *nodefs.Inode, name string) {
-	n.pathFs.pathLock.Lock()
-	defer n.pathFs.pathLock.Unlock()
-
-	pathParent := parent.Node().(*pathInode)
-	n.parents[parentData{pathParent, name}] = struct{}{}
-
-	if n.clientInode > 0 && n.pathFs.options.ClientInodes {
-		m := n.pathFs.clientInodeMap[n.clientInode]
-		e := &clientInodePath{
-			pathParent, name, n,
-		}
-		m = append(m, e)
-		n.pathFs.clientInodeMap[n.clientInode] = m
-	}
+	// TODO it would be logical to increment the clientInodeMap reference count
+	// here. However, as the inode number is loaded lazily, we cannot do it
+	// yet.
 }
 
 func (n *pathInode) rmChild(name string) *pathInode {
@@ -359,79 +330,40 @@ func (n *pathInode) rmChild(name string) *pathInode {
 }
 
 func (n *pathInode) OnRemove(parent *nodefs.Inode, name string) {
-	n.pathFs.pathLock.Lock()
-	defer n.pathFs.pathLock.Unlock()
-
-	// TODO - paranoia: what if the cast fails? Can this happen?
-	parentPI := parent.Node().(*pathInode)
-
-	delete(n.parents, parentData{parentPI, name})
-
-	if n.clientInode > 0 && n.pathFs.options.ClientInodes {
-		m := n.pathFs.clientInodeMap[n.clientInode]
+	if n.clientInode == 0 || !n.pathFs.options.ClientInodes || n.Inode().IsDir() {
+		return
+	}
 
-		idx := -1
-		// Find the right entry: both "parent" and "name" must match
-		for i, v := range m {
-			if v.parent == parentPI && v.name == name {
-				idx = i
-				break
-			}
-		}
-		if idx >= 0 {
-			// Delete the "idx" entry from the middle of the slice by moving the
-			// last element over it and truncating the slice
-			m[idx] = m[len(m)-1]
-			m = m[:len(m)-1]
-			n.pathFs.clientInodeMap[n.clientInode] = m
-		}
-		if len(m) == 0 {
+	n.pathFs.pathLock.Lock()
+	r := n.pathFs.clientInodeMap[n.clientInode]
+	if r != nil {
+		r.refCount--
+		if r.refCount == 0 {
 			delete(n.pathFs.clientInodeMap, n.clientInode)
 		}
 	}
+	n.pathFs.pathLock.Unlock()
 }
 
-// Handle a change in clientInode number for an other wise unchanged
-// pathInode.
+// setClientInode sets the inode number if has not been set yet.
+// This function exists to allow lazy-loading of the inode number.
 func (n *pathInode) setClientInode(ino uint64) {
-	if ino == n.clientInode || !n.pathFs.options.ClientInodes {
+	if ino == 0 || n.clientInode != 0 || !n.pathFs.options.ClientInodes || n.Inode().IsDir() {
 		return
 	}
 	n.pathFs.pathLock.Lock()
 	defer n.pathFs.pathLock.Unlock()
-	if n.clientInode != 0 {
-		delete(n.pathFs.clientInodeMap, n.clientInode)
-	}
-
 	n.clientInode = ino
-	if p := n.parent(); p.parent != nil {
-		e := &clientInodePath{
-			p.parent, p.name, n,
-		}
-		n.pathFs.clientInodeMap[ino] = append(n.pathFs.clientInodeMap[ino], e)
-	}
+	n.pathFs.clientInodeMap[ino] = &refCountedInode{node: n, refCount: 1}
 }
 
 func (n *pathInode) OnForget() {
-	if n.clientInode == 0 || !n.pathFs.options.ClientInodes {
+	if n.clientInode == 0 || !n.pathFs.options.ClientInodes || n.Inode().IsDir() {
 		return
 	}
 	n.pathFs.pathLock.Lock()
-	parents := make(map[parentData]struct{}, len(n.parents))
-	for k, v := range n.parents {
-		parents[k] = v
-	}
+	delete(n.pathFs.clientInodeMap, n.clientInode)
 	n.pathFs.pathLock.Unlock()
-
-	// Remove ourself from all parents. This will also remove the entry from
-	// clientInodeMap when the last parent is removed.
-	for p := range parents {
-		if ch := p.parent.Inode().GetChild(p.name); ch != nil && ch.Node() != n {
-			// Another node has taken our place - leave it alone.
-			continue
-		}
-		p.parent.Inode().RmChild(p.name)
-	}
 }
 
 ////////////////////////////////////////////////////////////////
@@ -533,6 +465,8 @@ func (n *pathInode) Rename(oldName string, newParent nodefs.Node, newName string
 	newPath := filepath.Join(p.GetPath(), newName)
 	code = n.fs.Rename(oldPath, newPath, context)
 	if code.Ok() {
+		// The rename may have overwritten another file, remove it from the tree
+		p.Inode().RmChild(newName)
 		ch := n.Inode().RmChild(oldName)
 		if ch != nil {
 			// oldName may have been forgotten in the meantime.
@@ -584,13 +518,12 @@ func (n *pathInode) Create(name string, flags uint32, mode uint32, context *fuse
 
 func (n *pathInode) createChild(name string, isDir bool) *pathInode {
 	i := &pathInode{
-		parents: map[parentData]struct{}{},
+		fs:     n.fs,
+		pathFs: n.pathFs,
 	}
 
-	i.fs = n.fs
-	i.pathFs = n.pathFs
-
 	n.Inode().NewChild(name, isDir, i)
+
 	return i
 }
 
@@ -620,12 +553,12 @@ func (n *pathInode) Lookup(out *fuse.Attr, name string, context *fuse.Context) (
 func (n *pathInode) findChild(fi *fuse.Attr, name string, fullPath string) (out *pathInode) {
 	if fi.Ino > 0 {
 		n.pathFs.pathLock.RLock()
-		v := n.pathFs.clientInodeMap[fi.Ino]
-		if len(v) > 0 {
-			out = v[0].node
-
+		r := n.pathFs.clientInodeMap[fi.Ino]
+		if r != nil {
+			out = r.node
+			r.refCount++
 			if fi.Nlink == 1 {
-				log.Println("Found linked inode, but Nlink == 1, ino=%d", fullPath, fi.Ino)
+				log.Printf("Found linked inode, but Nlink == 1, ino=%d, fullPath=%q", fi.Ino, fullPath)
 			}
 		}
 		n.pathFs.pathLock.RUnlock()
@@ -633,7 +566,7 @@ func (n *pathInode) findChild(fi *fuse.Attr, name string, fullPath string) (out
 
 	if out == nil {
 		out = n.createChild(name, fi.IsDir())
-		out.clientInode = fi.Ino
+		out.setClientInode(fi.Ino)
 	} else {
 		n.Inode().AddChild(name, out.Inode())
 	}