vfs: fix open files disappearing from directory listings

In this commit

aaadb48d48 vfs: keep virtual directory status accurate and reduce deadlock potential

We reworked the virtual directory detection to use an atomic bool so
that we could run part of the cache forgetting only with a read lock.

Unfortunately this had a bug which meant that directories with virtual
items could be forgotten.

This commit changes the boolean into a count of virtual entries which
should be more accurate.

Fixes #8082

vfs/dir.go

@@ -40,7 +40,7 @@ type Dir struct {
 	modTimeMu sync.Mutex // protects the following
 	modTime   time.Time
 
-	_hasVirtual atomic.Bool // shows if the directory has virtual entries
+	_virtuals atomic.Int32 // number of virtual directory entries in this directory and children
 }
 
 //go:generate stringer -type=vState
@@ -67,7 +67,6 @@ func newDir(vfs *VFS, f fs.Fs, parent *Dir, fsDir fs.Directory) *Dir {
 		items:   make(map[string]Node),
 	}
 	d.cleanupTimer = time.AfterFunc(time.Duration(vfs.Opt.DirCacheTime*2), d.cacheCleanup)
-	d.setHasVirtual(false)
 	return d
 }
 
@@ -198,58 +197,60 @@ func (d *Dir) Node() Node {
 	return d
 }
 
-// hasVirtual returns whether the directory has virtual entries
+// hasVirtual returns whether the directory or children has virtual entries
 func (d *Dir) hasVirtual() bool {
-	return d._hasVirtual.Load()
+	return d._virtuals.Load() != 0
 }
 
-// setHasVirtual sets the hasVirtual flag for the directory
+// addVirtual adds n virtual items to this directory and all of its parents
-func (d *Dir) setHasVirtual(hasVirtual bool) {
+func (d *Dir) addVirtual(n int32) {
-	d._hasVirtual.Store(hasVirtual)
+	for d != nil {
+		d._virtuals.Add(n)
+		d = d.parent
+	}
 }
 
 // ForgetAll forgets directory entries for this directory and any children.
 //
 // It does not invalidate or clear the cache of the parent directory.
 //
-// It returns true if the directory or any of its children had virtual entries
+// It returns true if the directory or any of its children had virtual
-// so could not be forgotten. Children which didn't have virtual entries and
+// entries so could not be forgotten. Children which didn't have
-// children with virtual entries will be forgotten even if true is returned.
+// virtual entries will be forgotten even if true is returned.
 func (d *Dir) ForgetAll() (hasVirtual bool) {
+	// We run this part with RLock only to avoid deadlocks in the recursion
+
 	d.mu.RLock()
 
 	fs.Debugf(d.path, "forgetting directory cache")
 	for _, node := range d.items {
 		if dir, ok := node.(*Dir); ok {
-			if dir.ForgetAll() {
+			dir.ForgetAll()
-				d.setHasVirtual(true)
-			}
 		}
 	}
 
 	d.mu.RUnlock()
 
+	// We run this part with Lock so we can modify the Dir
+
 	d.mu.Lock()
 	defer d.mu.Unlock()
 
 	// Purge any unnecessary virtual entries
 	d._purgeVirtual()
 
-	d.read = time.Time{}
-
-	// Check if this dir has virtual entries
-	if len(d.virtual) != 0 {
-		d.setHasVirtual(true)
-	}
-
 	// Don't clear directory entries if there are virtual entries in this
 	// directory or any children
-	if !d.hasVirtual() {
+	hasVirtual = d.hasVirtual()
+	if !hasVirtual {
+		d.read = time.Time{}
 		d.items = make(map[string]Node)
 		d.cleanupTimer.Stop()
+	} else {
+		d.cleanupTimer.Reset(time.Duration(d.vfs.Opt.DirCacheTime * 2))
 	}
 
-	return d.hasVirtual()
+	return hasVirtual
 }
 
 // forgetDirPath clears the cache for itself and all subdirectories if
@@ -448,8 +449,10 @@ func (d *Dir) addObject(node Node) {
 	if node.IsDir() {
 		vAdd = vAddDir
 	}
+	if _, found := d.virtual[leaf]; !found {
+		d.addVirtual(1)
+	}
 	d.virtual[leaf] = vAdd
-	d.setHasVirtual(true)
 	fs.Debugf(d.path, "Added virtual directory entry %v: %q", vAdd, leaf)
 	d.mu.Unlock()
 }
@@ -492,8 +495,10 @@ func (d *Dir) delObject(leaf string) {
 	if d.virtual == nil {
 		d.virtual = make(map[string]vState)
 	}
+	if _, found := d.virtual[leaf]; !found {
+		d.addVirtual(1)
+	}
 	d.virtual[leaf] = vDel
-	d.setHasVirtual(true)
 	fs.Debugf(d.path, "Added virtual directory entry %v: %q", vDel, leaf)
 	d.mu.Unlock()
 }
@@ -580,9 +585,9 @@ func (d *Dir) _deleteVirtual(name string) {
 		return
 	}
 	delete(d.virtual, name)
+	d.addVirtual(-1)
 	if len(d.virtual) == 0 {
 		d.virtual = nil
-		d.setHasVirtual(false)
 	}
 	fs.Debugf(d.path, "Removed virtual directory entry %v: %q", virtualState, name)
 }

vfs/dir_test.go

@@ -607,3 +607,51 @@ func TestDirRename(t *testing.T) {
 func TestDirStructSize(t *testing.T) {
 	t.Logf("Dir struct has size %d bytes", unsafe.Sizeof(Dir{}))
 }
+
+// Check that open files appear in the directory listing properly after a forget
+func TestDirFileOpen(t *testing.T) {
+	_, vfs, dir, _ := dirCreate(t)
+
+	assert.False(t, dir.hasVirtual())
+	assert.False(t, dir.parent.hasVirtual())
+
+	_, err := dir.Mkdir("sub")
+	require.NoError(t, err)
+
+	assert.True(t, dir.hasVirtual())
+	assert.True(t, dir.parent.hasVirtual())
+
+	fd0, err := vfs.Create("dir/sub/file0")
+	require.NoError(t, err)
+	_, err = fd0.Write([]byte("hello"))
+	require.NoError(t, err)
+	defer func() {
+		require.NoError(t, fd0.Close())
+	}()
+
+	fd2, err := vfs.Create("dir/sub/file2")
+	require.NoError(t, err)
+	_, err = fd2.Write([]byte("hello world!"))
+	require.NoError(t, err)
+	require.NoError(t, fd2.Close())
+	assert.True(t, dir.hasVirtual())
+
+	assert.True(t, dir.hasVirtual())
+	assert.True(t, dir.parent.hasVirtual())
+
+	// Now forget the directory
+	hasVirtual := dir.parent.ForgetAll()
+	assert.True(t, hasVirtual)
+
+	assert.True(t, dir.hasVirtual())
+	assert.True(t, dir.parent.hasVirtual())
+
+	// Check the files can still be found
+	fi, err := vfs.Stat("dir/sub/file0")
+	require.NoError(t, err)
+	assert.Equal(t, int64(5), fi.Size())
+
+	fi, err = vfs.Stat("dir/sub/file2")
+	require.NoError(t, err)
+	assert.Equal(t, int64(12), fi.Size())
+}