nodefs: issue GetAttr on file if we have open files

Fix and test Nlink==0 test (issue #137)

Do not overwrite Attr.Ino, otherwise recycled inodes risk giving tools
like RSync the impression there are more hardlinks than there are.

nodefs/bridge.go

@@ -44,6 +44,7 @@ type rawBridge struct {
 }
 
 // newInode creates creates new inode pointing to node.
+// XXX - should store the Ino number we expose in GetAttr too ?
 func (b *rawBridge) newInode(node Node, mode uint32, id FileID, persistent bool) *Inode {
 	b.mu.Lock()
 	defer b.mu.Unlock()
@@ -213,10 +214,6 @@ func (b *rawBridge) addNewChild(parent *Inode, name string, child *Inode, file F
 	}
 
 	out.NodeId = child.nodeID
-	// NOSUBMIT - or should let FS expose Attr.Ino? This makes
-	// testing semantics hard though, because os.Lstat doesn't
-	// reflect the FUSE FS
-	out.Attr.Ino = child.nodeID
 	out.Generation = b.nodes[out.NodeId].generation
 	b.mu.Unlock()
 	unlockNodes(parent, child)
@@ -300,16 +297,8 @@ func (b *rawBridge) GetAttr(input *fuse.GetAttrIn, out *fuse.AttrOut) fuse.Statu
 		f = nil
 	}
 
-	code = n.node.GetAttr(context.TODO(), f, out)
-	if out.Nlink == 0 {
-		// With Nlink == 0, newer kernels will refuse link
-		// operations.
-		out.Nlink = 1
-	}
-
+	code := n.node.GetAttr(context.TODO(), f, out)
 	b.setAttrTimeout(out)
-
-	out.Ino = input.NodeId
 	return code
 }
 

nodefs/files.go

@@ -14,11 +14,11 @@ import (
 	"github.com/hanwen/go-fuse/fuse"
 )
 
-// LoopbackFile delegates all operations back to an underlying os.File.
-func NewLoopbackFile(f *os.File) File {
+func newLoopbackFile(f *os.File) *loopbackFile {
 	return &loopbackFile{File: f}
 }
 
+// loopbackFile delegates all operations back to an underlying os.File.
 type loopbackFile struct {
 	File *os.File
 

nodefs/inode.go

@@ -21,14 +21,18 @@ type parentData struct {
 }
 
 // FileID provides a identifier for file objects defined by FUSE
-// filesystems.
+// filesystems. The identifier is divided into a (Device, Inode) pair,
+// so files in underlying file systems can easily be represented, but
+// FUSE filesystems are free to use any other information as 128-bit
+// key.
 //
 // XXX name: PersistentID ? NodeID ?
 type FileID struct {
-	Dev uint64 // XXX Rdev?
+	Dev uint64
 	Ino uint64
 }
 
+// Zero returns if the FileID is zeroed out
 func (i *FileID) Zero() bool {
 	return i.Dev == 0 && i.Ino == 0
 }
@@ -39,10 +43,12 @@ func (i *FileID) Zero() bool {
 // "persistent" Inodes.
 type Inode struct {
 	// The filetype bits from the mode.
-	mode     uint32
+	mode uint32
+
 	opaqueID FileID
-	node     Node
-	bridge   *rawBridge
+
+	node   Node
+	bridge *rawBridge
 
 	// Following data is mutable.
 
@@ -51,10 +57,8 @@ type Inode struct {
 	// lockNodes/unlockNodes
 	mu sync.Mutex
 
-	// ID of the inode; 0 if inode was forgotten.  Forgotten
-	// inodes could be persistent, not yet are unlinked from
-	// parent and children, but could be still not yet removed
-	// from bridge.nodes .
+	// ID of the inode for talking to the kernel, 0 if the kernel
+	// does not know this inode.
 	nodeID uint64
 
 	// persistent indicates that this node should not be removed
@@ -127,7 +131,9 @@ func lockNodes(ns ...*Inode) {
 
 // lockNode2 locks a and b in order consistent with lockNodes.
 func lockNode2(a, b *Inode) {
-	if nodeLess(a, b) {
+	if a == b {
+		a.mu.Lock()
+	} else if nodeLess(a, b) {
 		a.mu.Lock()
 		b.mu.Lock()
 	} else {
@@ -138,8 +144,12 @@ func lockNode2(a, b *Inode) {
 
 // unlockNode2 unlocks a and b
 func unlockNode2(a, b *Inode) {
-	a.mu.Unlock()
-	b.mu.Unlock()
+	if a == b {
+		a.mu.Unlock()
+	} else {
+		a.mu.Unlock()
+		b.mu.Unlock()
+	}
 }
 
 // unlockNodes releases locks taken by lockNodes.

nodefs/loopback.go

@@ -8,6 +8,7 @@ import (
 	"context"
 	"os"
 	"path/filepath"
+	"sync"
 	"syscall"
 
 	"github.com/hanwen/go-fuse/fuse"
@@ -19,6 +20,13 @@ type loopbackRoot struct {
 	root string
 }
 
+func (n *loopbackRoot) newLoopbackNode() *loopbackNode {
+	return &loopbackNode{
+		rootNode:  n,
+		openFiles: map[*loopbackFile]struct{}{},
+	}
+}
+
 func (n *loopbackRoot) GetAttr(ctx context.Context, f File, out *fuse.AttrOut) fuse.Status {
 	var err error = nil
 	st := syscall.Stat_t{}
@@ -34,6 +42,19 @@ type loopbackNode struct {
 	DefaultNode
 
 	rootNode *loopbackRoot
+
+	mu        sync.Mutex
+	openFiles map[*loopbackFile]struct{}
+}
+
+func (n *loopbackNode) Release(ctx context.Context, f File) {
+	if f != nil {
+		n.mu.Lock()
+		defer n.mu.Unlock()
+		lf := f.(*loopbackFile)
+		delete(n.openFiles, lf)
+		f.Release(ctx)
+	}
 }
 
 func (n *loopbackNode) path() string {
@@ -57,7 +78,7 @@ func (n *loopbackNode) Lookup(ctx context.Context, name string, out *fuse.EntryO
 		Ino: out.Attr.Ino,
 	}
 
-	node := &loopbackNode{rootNode: n.rootNode}
+	node := n.rootNode.newLoopbackNode()
 	ch := n.inode().NewInode(node, out.Attr.Mode, opaque)
 	return ch, fuse.OK
 }
@@ -76,7 +97,7 @@ func (n *loopbackNode) Mknod(ctx context.Context, name string, mode, rdev uint32
 
 	out.Attr.FromStat(&st)
 
-	node := &loopbackNode{rootNode: n.rootNode}
+	node := n.rootNode.newLoopbackNode()
 	opaque := FileID{
 		Dev: uint64(out.Attr.Rdev),
 		Ino: out.Attr.Ino,
@@ -101,7 +122,7 @@ func (n *loopbackNode) Mkdir(ctx context.Context, name string, mode uint32, out
 
 	out.Attr.FromStat(&st)
 
-	node := &loopbackNode{rootNode: n.rootNode}
+	node := n.rootNode.newLoopbackNode()
 	opaque := FileID{
 		Dev: uint64(out.Attr.Rdev),
 		Ino: out.Attr.Ino,
@@ -156,14 +177,18 @@ func (n *loopbackNode) Create(ctx context.Context, name string, flags uint32, mo
 		return nil, nil, 0, fuse.ToStatus(err)
 	}
 
-	node := &loopbackNode{rootNode: n.rootNode}
+	node := n.rootNode.newLoopbackNode()
 	opaque := FileID{
 		Dev: st.Rdev,
 		Ino: st.Ino,
 	}
 
 	ch := n.inode().NewInode(node, st.Mode, opaque)
-	return ch, NewLoopbackFile(f), 0, fuse.OK
+	lf := newLoopbackFile(f)
+	n.mu.Lock()
+	defer n.mu.Unlock()
+	n.openFiles[lf] = struct{}{}
+	return ch, lf, 0, fuse.OK
 }
 
 func (n *loopbackNode) Open(ctx context.Context, flags uint32) (fh File, fuseFlags uint32, code fuse.Status) {
@@ -172,12 +197,32 @@ func (n *loopbackNode) Open(ctx context.Context, flags uint32) (fh File, fuseFla
 	if err != nil {
 		return nil, 0, fuse.ToStatus(err)
 	}
-	return NewLoopbackFile(f), 0, fuse.OK
+	n.mu.Lock()
+	defer n.mu.Unlock()
+	lf := newLoopbackFile(f)
+	n.openFiles[lf] = struct{}{}
+	return lf, 0, fuse.OK
+}
+
+func (n *loopbackNode) fGetAttr(ctx context.Context, out *fuse.AttrOut) (fuse.Status, bool) {
+	n.mu.Lock()
+	defer n.mu.Unlock()
+	for f := range n.openFiles {
+		if f != nil {
+			return f.GetAttr(ctx, out), true
+		}
+	}
+	return fuse.EBADF, false
 }
 
 func (n *loopbackNode) GetAttr(ctx context.Context, f File, out *fuse.AttrOut) fuse.Status {
 	if f != nil {
+		// this never happens because the kernel never sends FH on getattr.
 		return f.GetAttr(ctx, out)
+
+	}
+	if code, ok := n.fGetAttr(ctx, out); ok {
+		return code
 	}
 
 	p := n.path()
@@ -197,6 +242,6 @@ func NewLoopback(root string) Node {
 		root: root,
 	}
 	n.rootNode = n
-
+	n.openFiles = map[*loopbackFile]struct{}{}
 	return n
 }

nodefs/simple_test.go

@@ -7,9 +7,11 @@ package nodefs
 import (
 	"bytes"
 	"io/ioutil"
+	"log"
 	"os"
 	"path/filepath"
 	"runtime"
+	"sync"
 	"syscall"
 	"testing"
 	"time"
@@ -20,6 +22,8 @@ import (
 	"github.com/hanwen/go-fuse/internal/testutil"
 )
 
+var _ = log.Println
+
 type testCase struct {
 	*testing.T
 
@@ -335,3 +339,71 @@ func TestRenameNoOverwrite(t *testing.T) {
 		t.Errorf("got %v (%T) want %v (%T)", err, err, syscall.EINVAL, syscall.EINVAL)
 	}
 }
+
+func TestNlinkZero(t *testing.T) {
+	// xfstest generic/035.
+	tc := newTestCase(t)
+	defer tc.Clean()
+
+	src := tc.mntDir + "/src"
+	dst := tc.mntDir + "/dst"
+	if err := ioutil.WriteFile(src, []byte("source"), 0644); err != nil {
+		t.Fatalf("WriteFile: %v", err)
+	}
+
+	if err := ioutil.WriteFile(dst, []byte("dst"), 0644); err != nil {
+		t.Fatalf("WriteFile: %v", err)
+	}
+
+	f, err := os.Open(dst)
+	if err != nil {
+		t.Fatalf("Open: %v", err)
+	}
+	defer f.Close()
+
+	var st syscall.Stat_t
+	if err := syscall.Fstat(int(f.Fd()), &st); err != nil {
+		t.Errorf("Fstat before: %v", err)
+	} else if st.Nlink != 1 {
+		t.Errorf("Nlink of file: got %d, want 1", st.Nlink)
+	}
+
+	if err := os.Rename(src, dst); err != nil {
+		t.Fatalf("Rename: %v", err)
+	}
+
+	if err := syscall.Fstat(int(f.Fd()), &st); err != nil {
+		t.Errorf("Fstat after: %v", err)
+	} else if st.Nlink != 0 {
+		t.Errorf("Nlink of overwritten file: got %d, want 0", st.Nlink)
+	}
+}
+
+func TestParallelFileOpen(t *testing.T) {
+	tc := newTestCase(t)
+	defer tc.Clean()
+
+	fn := tc.mntDir + "/file"
+	if err := ioutil.WriteFile(fn, []byte("content"), 0644); err != nil {
+		t.Fatalf("WriteFile: %v", err)
+	}
+
+	var wg sync.WaitGroup
+	one := func(b byte) {
+		f, err := os.OpenFile(fn, os.O_RDWR, 0644)
+		if err != nil {
+			t.Fatalf("OpenFile: %v", err)
+		}
+		var buf [10]byte
+		f.Read(buf[:])
+		buf[0] = b
+		f.WriteAt(buf[0:1], 2)
+		f.Close()
+		wg.Done()
+	}
+	for i := 0; i < 10; i++ {
+		wg.Add(1)
+		go one(byte(i))
+	}
+	wg.Wait()
+}