nodefs: Allow for several Lookup requests to be served simultaneously

Commit d0fca860 introduced big lookup lock with the idea to "make sure
we don't return forgotten nodes to the kernel". However this way it also
started to prevent several Lookup handlers to be running simultaneously,
which can deadlock if e.g. Lookup handler somehow synchronizes with
other thread which caused the lookup:

https://github.com/hanwen/go-fuse/commit/d0fca860#commitcomment-37772099

On the surface the fix is easy: if we need to prevent lookups running in
parallel to forget, we can turn lookupLock into shared one wrt Lookup,
but exclusive wrt Forget.

However a more correct fix would be to review nodefs locking completely:
there is already per-filesystem treeLock RWMutex, which is _already_
used to synchronize for forgets and lookups. From this point of view
lookupLock is unneeded and the correct fix that d0fca860 should have been
doing is to correct the scope of treeLock to be used more precisely.

However that would be a more intrusive change, and given that nodefs is
considered deprecated today, imho it is better to proceed with making
lookupLock a shared one.

The test that is added deadlocks without provided fix.

I suggest not to reject this patch based on rationale that "nodefs is
deprecated" as there still are real filesystems that use nodefs.

I had not reviewed v2 go-fuse/fs locking yet.

Change-Id: I18e01457f474dea31dc17186dfe6db582c2e6337

fuse/nodefs/fsconnector.go

@@ -42,7 +42,10 @@ type FileSystemConnector struct {
 	// forgotten nodes to the kernel. Problems solved by this lock:
 	// https://github.com/hanwen/go-fuse/issues/168
 	// https://github.com/rfjakob/gocryptfs/issues/322
-	lookupLock sync.Mutex
+	//
+	// The lock is shared: several concurrent Lookups are allowed to be
+	// run simultaneously, while Forget is exclusive.
+	lookupLock sync.RWMutex
 }
 
 // NewOptions generates FUSE options that correspond to libfuse's

fuse/nodefs/fsops.go

@@ -95,8 +95,9 @@ func (c *FileSystemConnector) internalLookup(cancel <-chan struct{}, out *fuse.A
 
 func (c *rawBridge) Lookup(cancel <-chan struct{}, header *fuse.InHeader, name string, out *fuse.EntryOut) (code fuse.Status) {
 	// Prevent Lookup() and Forget() from running concurrently.
-	c.lookupLock.Lock()
-	defer c.lookupLock.Unlock()
+	// Allow several Lookups to be run simultaneously.
+	c.lookupLock.RLock()
+	defer c.lookupLock.RUnlock()
 
 	parent := c.toInode(header.NodeId)
 	if !parent.IsDir() {

fuse/test/node_parallel_lookup_test.go

+// Copyright 2019 the Go-FUSE Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package test
+
+// verify that several lookup requests can be served in parallel without deadlock.
+
+import (
+	"context"
+	"fmt"
+	"io/ioutil"
+	"log"
+	"os"
+	"testing"
+
+	"golang.org/x/sync/errgroup"
+
+	"github.com/hanwen/go-fuse/v2/fuse"
+	"github.com/hanwen/go-fuse/v2/fuse/nodefs"
+	"github.com/hanwen/go-fuse/v2/internal/testutil"
+)
+
+// tRoot implements simple root node which Lookups children in predefined .nodes.
+// The Lookup is synchronized with main test driver on .lookupq and .lookupGo.
+type tRoot struct {
+	nodefs.Node
+	nodes map[string]nodefs.Node // name -> Node
+
+	lookupq  chan string   // main <- fssrv: lookup(name) request received
+	lookupGo chan struct{} // main -> fssrv: ok to further process lookup requests
+}
+
+func (r *tRoot) Lookup(out *fuse.Attr, name string, fctx *fuse.Context) (*nodefs.Inode, fuse.Status) {
+	node, ok := r.nodes[name]
+	if !ok {
+		// e.g. it can be lookup for .Trash automatically issued by volume manager
+		return nil, fuse.ENOENT
+	}
+
+	r.lookupq <- name // tell main driver that we received lookup(name)
+	<-r.lookupGo      // wait for main to allow us to continue
+
+	st := node.GetAttr(out, nil, fctx)
+	return node.Inode(), st
+}
+
+
+// verifyFileRead verifies that file @path has content == dataOK.
+func verifyFileRead(path string, dataOK string) error {
+	v, err := ioutil.ReadFile(path)
+	if err != nil {
+		return err
+	}
+	if string(v) != dataOK {
+		return fmt.Errorf("%s: file read: got %q  ; want %q", path, v, dataOK)
+	}
+	return nil
+}
+
+func TestNodeParallelLookup(t *testing.T) {
+	dir := testutil.TempDir()
+	defer func() {
+		err := os.Remove(dir)
+		if err != nil {
+			t.Fatal(err)
+		}
+	}()
+
+	root := &tRoot{
+		Node:     nodefs.NewDefaultNode(),
+		nodes:    make(map[string]nodefs.Node),
+		lookupq:  make(chan string),
+		lookupGo: make(chan struct{}),
+	}
+
+	opts := nodefs.NewOptions()
+	opts.LookupKnownChildren = true
+	opts.Debug = testutil.VerboseTest()
+	srv, _, err := nodefs.MountRoot(dir, root, opts)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	root.nodes["hello"] = NewDataNode([]byte("abc"))
+	root.nodes["world"] = NewDataNode([]byte("def"))
+	root.Inode().NewChild("hello", false, root.nodes["hello"])
+	root.Inode().NewChild("world", false, root.nodes["world"])
+
+	go srv.Serve()
+	if err := srv.WaitMount(); err != nil {
+		t.Fatal("WaitMount", err)
+	}
+	defer func() {
+		err := srv.Unmount()
+		if err != nil {
+			t.Fatal(err)
+		}
+	}()
+
+	// spawn 2 threads to access the files in parallel
+	// this will deadlock if nodefs does not allow simultaneous Lookups to be handled.
+	// see https://github.com/hanwen/go-fuse/commit/d0fca860 for context.
+	ctx0, cancel := context.WithCancel(context.Background())
+	defer cancel()
+	wg, ctx := errgroup.WithContext(ctx0)
+	wg.Go(func() error {
+		return verifyFileRead(dir + "/hello", "abc")
+	})
+	wg.Go(func() error {
+		return verifyFileRead(dir + "/world", "def")
+	})
+
+	// wait till both threads queue into Lookup
+	expect := map[string]struct{}{ // set of expected lookups
+		"hello": struct{}{},
+		"world": struct{}{},
+	}
+loop:
+	for len(expect) > 0 {
+		var lookup string
+		select {
+		case <-ctx.Done():
+			break loop // wg.Wait will return the error
+		case lookup = <-root.lookupq:
+			// ok
+		}
+
+		if testutil.VerboseTest() {
+			log.Printf("I: <- lookup %q", lookup)
+		}
+		_, ok := expect[lookup]
+		if !ok {
+			t.Fatalf("unexpected lookup: %q  ; expect: %q", lookup, expect)
+		}
+		delete(expect, lookup)
+	}
+
+	// let both lookups continue
+	close(root.lookupGo)
+
+	err = wg.Wait()
+	if err != nil {
+		t.Fatal(err)
+	}
+}