From 909358d0a442c830d2c1fa32dac36fef2bc25736 Mon Sep 17 00:00:00 2001 From: Prince Kumar Date: Tue, 9 Jul 2024 20:35:12 +0530 Subject: [PATCH] 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 | 8 +- internal/fs/inode/dir.go | 13 +- internal/fs/inode/dir_test.go | 33 ++-- internal/fs/kernel_list_cache_test.go | 215 ++++++++++++++++++++++++++ 4 files changed, 239 insertions(+), 30 deletions(-) diff --git a/internal/fs/fs.go b/internal/fs/fs.go index fce7469413..65e79289bd 100644 --- a/internal/fs/fs.go +++ b/internal/fs/fs.go @@ -2141,7 +2141,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 @@ -2155,16 +2154,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 } diff --git a/internal/fs/inode/dir.go b/internal/fs/inode/dir.go index 3e3fcf6edb..87908b7874 100644 --- a/internal/fs/inode/dir.go +++ b/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 } @@ -890,13 +889,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 } diff --git a/internal/fs/inode/dir_test.go b/internal/fs/inode/dir_test.go index d67de340ab..ece1a50798 100644 --- a/internal/fs/inode/dir_test.go +++ b/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 = ¤tTime + 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 = ¤tTime + 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 = ¤tTime + d.prevDirListingTimeStamp = d.cacheClock.Now() ttl := time.Duration(0) shouldInvalidate := t.in.ShouldInvalidateKernelListCache(ttl) diff --git a/internal/fs/kernel_list_cache_test.go b/internal/fs/kernel_list_cache_test.go index 34be04cd57..38dec4d66a 100644 --- a/internal/fs/kernel_list_cache_test.go +++ b/internal/fs/kernel_list_cache_test.go @@ -26,6 +26,8 @@ package fs_test import ( "os" "path" + "strings" + "sync" "testing" "time" @@ -120,6 +122,219 @@ func TestKernelListCacheTestWithPositiveTtlSuite(t *testing.T) { suite.Run(t, new(KernelListCacheTestWithPositiveTtl)) } +// Test_Parallel_OpenDirAndLookUpInode helps in detecting the deadlock when +// OpenDir() and LookUpInode() request for same directory comes in parallel. +func (t *KernelListCacheTestWithPositiveTtl) Test_Parallel_OpenDirAndLookUpInode() { + var wg sync.WaitGroup + wg.Add(2) + // Fail if the operation takes more than timeout. + timeout := 5 * time.Second + iterationsPerGoroutine := 100 + + go func() { + defer wg.Done() + for i := 0; i < iterationsPerGoroutine; i++ { + f, err := os.Open(path.Join(mntDir, "explicitDir")) + assert.Nil(t.T(), err) + + err = f.Close() + assert.Nil(t.T(), err) + } + }() + go func() { + defer wg.Done() + for i := 0; i < iterationsPerGoroutine; i++ { + _, err := os.Stat(path.Join(mntDir, "explicitDir")) + assert.Nil(t.T(), err) + } + }() + + // Wait for goroutines or timeout. + done := make(chan bool, 1) + go func() { + wg.Wait() + done <- true + }() + select { + case <-done: + // Operation completed successfully before timeout. + case <-time.After(timeout): + assert.FailNow(t.T(), "Possible deadlock") + } +} + +// Test_Concurrent_ReadDir tests for potential deadlocks or race conditions +// when multiple goroutines call Readdir() concurrently on the same directory. +func (t *KernelListCacheTestWithPositiveTtl) Test_Concurrent_ReadDir() { + var wg sync.WaitGroup + goroutineCount := 10 // Number of concurrent goroutines + iterationsPerGoroutine := 10 // Number of iterations per goroutine + + wg.Add(goroutineCount) + timeout := 5 * time.Second + + dirPath := path.Join(mntDir, "explicitDir") + + for i := 0; i < goroutineCount; i++ { + go func() { + defer wg.Done() + + for j := 0; j < iterationsPerGoroutine; j++ { + f, err := os.Open(dirPath) + assert.Nil(t.T(), err) + + _, err = f.Readdirnames(-1) // Read all directory entries + assert.Nil(t.T(), err) + + err = f.Close() + assert.Nil(t.T(), err) + } + }() + } + + // Wait for goroutines or timeout + done := make(chan bool, 1) + go func() { + wg.Wait() + done <- true + }() + + select { + case <-done: + // Success: All Readdir operations finished before timeout + case <-time.After(timeout): + assert.FailNow(t.T(), "Possible deadlock or race condition detected during concurrent Readdir calls") + } +} + +// Test_Parallel_ReadDirAndFileOperations detects race conditions and deadlocks when one goroutine +// performs Readdir() while another concurrently creates and deletes files in the same directory. +func (t *KernelListCacheTestWithPositiveTtl) Test_Parallel_ReadDirAndFileOperations() { + var wg sync.WaitGroup + wg.Add(2) + timeout := 5 * time.Second // Adjust timeout as needed + iterationsPerGoroutine := 100 + + dirPath := path.Join(mntDir, "explicitDir") + + // Goroutine 1: Repeatedly calls Readdir + go func() { + defer wg.Done() + for i := 0; i < iterationsPerGoroutine; i++ { // Adjust iteration count if needed + f, err := os.Open(dirPath) + assert.Nil(t.T(), err) + + _, err = f.Readdirnames(-1) + if err != nil { + // This is expected, see the documentation for fixConflictingNames() call in dir_handle.go. + assert.True(t.T(), strings.Contains(err.Error(), "input/output error")) + } + + err = f.Close() + assert.Nil(t.T(), err) + } + }() + + // Goroutine 2: Creates and deletes files + go func() { + defer wg.Done() + for i := 0; i < iterationsPerGoroutine; i++ { // Adjust iteration count if needed + filePath := path.Join(dirPath, "tmp_file.txt") + renamedFilePath := path.Join(dirPath, "renamed_tmp_file.txt") + + // Create + f, err := os.Create(filePath) + assert.Nil(t.T(), err) + + err = f.Close() + assert.Nil(t.T(), err) + + // Rename + err = os.Rename(filePath, renamedFilePath) + assert.Nil(t.T(), err) + + // Delete + err = os.Remove(renamedFilePath) + assert.Nil(t.T(), err) + } + }() + + // Wait for goroutines or timeout + done := make(chan bool, 1) + go func() { + wg.Wait() + done <- true + }() + + select { + case <-done: + // Success: Both operations finished before timeout + case <-time.After(timeout): + assert.FailNow(t.T(), "Possible deadlock or race condition detected") + } +} + +// Test_Parallel_ReadDirAndDirOperations tests for potential deadlocks or race conditions when +// ReadDir() is called concurrently with directory creation and deletion operations. +func (t *KernelListCacheTestWithPositiveTtl) Test_Parallel_ReadDirAndDirOperations() { + var wg sync.WaitGroup + wg.Add(2) + timeout := 5 * time.Second + iterationsPerGoroutine := 100 + + parentDir := path.Join(mntDir, "explicitDir") + + // Goroutine 1: Repeatedly calls Readdir + go func() { + defer wg.Done() + for i := 0; i < iterationsPerGoroutine; i++ { + f, err := os.Open(parentDir) + assert.Nil(t.T(), err) + + _, err = f.Readdirnames(0) + assert.Nil(t.T(), err) + + err = f.Close() + assert.Nil(t.T(), err) + } + }() + + // Goroutine 2: Creates and deletes directories + go func() { + defer wg.Done() + for i := 0; i < iterationsPerGoroutine; i++ { + dirPath := path.Join(parentDir, "test_dir") + renamedDirPath := path.Join(parentDir, "renamed_test_dir") + + // Create + err := os.Mkdir(dirPath, 0755) + assert.Nil(t.T(), err) + + // Rename + err = os.Rename(dirPath, renamedDirPath) + assert.Nil(t.T(), err) + + // Delete + err = os.Remove(renamedDirPath) + assert.Nil(t.T(), err) + } + }() + + // Wait for goroutines or timeout + done := make(chan bool, 1) + go func() { + wg.Wait() + done <- true + }() + + select { + case <-done: + // Success: Both operations finished before timeout + case <-time.After(timeout): + assert.FailNow(t.T(), "Possible deadlock or race condition detected during Readdir and directory operations") + } +} + // (a) First ReadDir() will be served from GCSFuse filesystem. // (b) Second ReadDir() within ttl will be served from kernel page cache. func (t *KernelListCacheTestWithPositiveTtl) TestKernelListCache_CacheHit() {