Fix DeadLock in Kernel List Cache Feature (#2100)

* Able to reproduce the hang

* Adding composite test for deadlock case

* Fixing lint issue

* reverting minor line deletion

* go import

* comment changes

* Review comments

* Liting fix

* Added more concurrent test

internal/fs/fs.go

@@ -2138,7 +2138,6 @@ func (fs *fileSystem) OpenDir(
 	ctx context.Context,
 	op *fuseops.OpenDirOp) (err error) {
 	fs.mu.Lock()
-	defer fs.mu.Unlock()
 
 	// Make sure the inode still exists and is a directory. If not, something has
 	// screwed up because the VFS layer shouldn't have let us forget the inode
@@ -2152,16 +2151,13 @@ func (fs *fileSystem) OpenDir(
 	fs.handles[handleID] = handle.NewDirHandle(in, fs.implicitDirs)
 	op.Handle = handleID
 
+	fs.mu.Unlock()
+
 	// Enables kernel list-cache in case of non-zero kernelListCacheTTL.
 	if fs.kernelListCacheTTL > 0 {
-
-		// Taking RLock() since ShouldInvalidateKernelListCache only reads the DirInode
-		// properties, no modification.
-		in.RLock()
 		// Invalidates the kernel list-cache once the last cached response is out of
 		// kernelListCacheTTL.
 		op.KeepCache = !in.ShouldInvalidateKernelListCache(fs.kernelListCacheTTL)
-		in.RUnlock()
 
 		op.CacheDir = true
 	}

internal/fs/inode/dir.go

@@ -201,7 +201,7 @@ type dirInode struct {
 	// (via kernel) the directory listing from the filesystem.
 	// Specially used when kernelListCacheTTL > 0 that means kernel list-cache is
 	// enabled.
-	prevDirListingTimeStamp *time.Time
+	prevDirListingTimeStamp time.Time
 }
 
 var _ DirInode = &dirInode{}
@@ -695,8 +695,7 @@ func (d *dirInode) ReadEntries(
 		entries = append(entries, entry)
 	}
 
-	nowTime := d.cacheClock.Now()
-	d.prevDirListingTimeStamp = &nowTime
+	d.prevDirListingTimeStamp = d.cacheClock.Now()
 	return
 }
 
@@ -892,13 +891,15 @@ func (d *dirInode) LocalFileEntries(localFileInodes map[Name]Inode) (localEntrie
 	return
 }
 
+// ShouldInvalidateKernelListCache doesn't require any lock as d.prevDirListingTimeStamp
+// is concurrency safe, and we are okay with the in-consistent value.
 func (d *dirInode) ShouldInvalidateKernelListCache(ttl time.Duration) bool {
-	// prevDirListingTimeStamp = nil means listing has not happened yet, and we should
+	// prevDirListingTimeStamp.IsZero() true means listing has not happened yet, and we should
 	// invalidate for clean start.
-	if d.prevDirListingTimeStamp == nil {
+	if d.prevDirListingTimeStamp.IsZero() {
 		return true
 	}
 
-	cachedDuration := d.cacheClock.Now().Sub(*d.prevDirListingTimeStamp)
+	cachedDuration := d.cacheClock.Now().Sub(d.prevDirListingTimeStamp)
 	return cachedDuration >= ttl
 }

internal/fs/inode/dir_test.go

@@ -750,13 +750,13 @@ func (t *DirTest) ReadDescendants_NonEmpty() {
 func (t *DirTest) ReadEntries_Empty() {
 	d := t.in.(*dirInode)
 	AssertNe(nil, d)
-	AssertEq(nil, d.prevDirListingTimeStamp)
+	AssertTrue(d.prevDirListingTimeStamp.IsZero())
 	entries, err := t.readAllEntries()
 
 	AssertEq(nil, err)
 	ExpectThat(entries, ElementsAre())
 	// Make sure prevDirListingTimeStamp is initialized.
-	AssertNe(nil, d.prevDirListingTimeStamp)
+	AssertFalse(d.prevDirListingTimeStamp.IsZero())
 }
 
 func (t *DirTest) ReadEntries_NonEmpty_ImplicitDirsDisabled() {
@@ -783,7 +783,7 @@ func (t *DirTest) ReadEntries_NonEmpty_ImplicitDirsDisabled() {
 	// Nil prevDirListingTimeStamp
 	d := t.in.(*dirInode)
 	AssertNe(nil, d)
-	AssertEq(nil, d.prevDirListingTimeStamp)
+	AssertTrue(d.prevDirListingTimeStamp.IsZero())
 
 	// Read entries.
 	entries, err := t.readAllEntries()
@@ -811,8 +811,8 @@ func (t *DirTest) ReadEntries_NonEmpty_ImplicitDirsDisabled() {
 	ExpectEq(fuseutil.DT_Link, entry.Type)
 	ExpectEq(metadata.SymlinkType, t.getTypeFromCache("symlink"))
 
-	// Make sure prevDirListingTimeStamp is not nil.
-	AssertNe(nil, d.prevDirListingTimeStamp)
+	// Make sure prevDirListingTimeStamp is initialized.
+	AssertFalse(d.prevDirListingTimeStamp.IsZero())
 }
 
 func (t *DirTest) ReadEntries_NonEmpty_ImplicitDirsEnabled() {
@@ -842,7 +842,7 @@ func (t *DirTest) ReadEntries_NonEmpty_ImplicitDirsEnabled() {
 	// Nil prevDirListingTimeStamp
 	d := t.in.(*dirInode)
 	AssertNe(nil, d)
-	AssertEq(nil, d.prevDirListingTimeStamp)
+	AssertTrue(d.prevDirListingTimeStamp.IsZero())
 
 	// Read entries.
 	entries, err := t.readAllEntries()
@@ -875,8 +875,8 @@ func (t *DirTest) ReadEntries_NonEmpty_ImplicitDirsEnabled() {
 	ExpectEq(fuseutil.DT_Link, entry.Type)
 	ExpectEq(metadata.SymlinkType, t.getTypeFromCache("symlink"))
 
-	// Make sure prevDirListingTimeStamp is not nil.
-	AssertNe(nil, d.prevDirListingTimeStamp)
+	// Make sure prevDirListingTimeStamp is initialized.
+	AssertFalse(d.prevDirListingTimeStamp.IsZero())
 }
 
 func (t *DirTest) ReadEntries_TypeCaching() {
@@ -893,7 +893,7 @@ func (t *DirTest) ReadEntries_TypeCaching() {
 	// Nil prevDirListingTimeStamp
 	d := t.in.(*dirInode)
 	AssertNe(nil, d)
-	AssertEq(nil, d.prevDirListingTimeStamp)
+	AssertTrue(d.prevDirListingTimeStamp.IsZero())
 
 	// Read the directory, priming the type cache.
 	_, err = t.readAllEntries()
@@ -925,8 +925,8 @@ func (t *DirTest) ReadEntries_TypeCaching() {
 
 	ExpectEq(dirObjName, result.MinObject.Name)
 
-	// Make sure prevDirListingTimeStamp is not nil.
-	AssertNe(nil, d.prevDirListingTimeStamp)
+	// Make sure prevDirListingTimeStamp is initialized.
+	AssertFalse(d.prevDirListingTimeStamp.IsZero())
 }
 
 func (t *DirTest) CreateChildFile_DoesntExist() {
@@ -1484,7 +1484,7 @@ func (t *DirTest) LocalFileEntriesWithUnlinkedLocalChildFiles() {
 
 func (t *DirTest) Test_ShouldInvalidateKernelListCache_ListingNotHappenedYet() {
 	d := t.in.(*dirInode)
-	d.prevDirListingTimeStamp = nil
+	d.prevDirListingTimeStamp = time.Time{}
 
 	// Irrespective of the ttl value, this should always return true.
 	shouldInvalidate := t.in.ShouldInvalidateKernelListCache(util.MaxTimeDuration)
@@ -1494,8 +1494,7 @@ func (t *DirTest) Test_ShouldInvalidateKernelListCache_ListingNotHappenedYet() {
 
 func (t *DirTest) Test_ShouldInvalidateKernelListCache_WithinTtl() {
 	d := t.in.(*dirInode)
-	currentTime := d.cacheClock.Now()
-	d.prevDirListingTimeStamp = &currentTime
+	d.prevDirListingTimeStamp = d.cacheClock.Now()
 	ttl := time.Second * 10
 	t.clock.AdvanceTime(ttl / 2)
 
@@ -1506,8 +1505,7 @@ func (t *DirTest) Test_ShouldInvalidateKernelListCache_WithinTtl() {
 
 func (t *DirTest) Test_ShouldInvalidateKernelListCache_ExpiredTtl() {
 	d := t.in.(*dirInode)
-	currentTime := d.cacheClock.Now()
-	d.prevDirListingTimeStamp = &currentTime
+	d.prevDirListingTimeStamp = d.cacheClock.Now()
 	ttl := 10 * time.Second
 	t.clock.AdvanceTime(ttl + time.Second)
 
@@ -1518,8 +1516,7 @@ func (t *DirTest) Test_ShouldInvalidateKernelListCache_ExpiredTtl() {
 
 func (t *DirTest) Test_ShouldInvalidateKernelListCache_ZeroTtl() {
 	d := t.in.(*dirInode)
-	currentTime := d.cacheClock.Now()
-	d.prevDirListingTimeStamp = &currentTime
+	d.prevDirListingTimeStamp = d.cacheClock.Now()
 	ttl := time.Duration(0)
 
 	shouldInvalidate := t.in.ShouldInvalidateKernelListCache(ttl)