Skip to content

Commit

Permalink
Fix DeadLock in Kernel List Cache Feature (#2100)
Browse files Browse the repository at this point in the history
* 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
  • Loading branch information
raj-prince authored and gargnitingoogle committed Jul 11, 2024
1 parent 9072a39 commit 909358d
Show file tree
Hide file tree
Showing 4 changed files with 239 additions and 30 deletions.
8 changes: 2 additions & 6 deletions internal/fs/fs.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
}
Expand Down
13 changes: 7 additions & 6 deletions internal/fs/inode/dir.go
Original file line number Diff line number Diff line change
Expand Up @@ -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{}
Expand Down Expand Up @@ -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
}

Expand Down Expand Up @@ -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
}
33 changes: 15 additions & 18 deletions internal/fs/inode/dir_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand All @@ -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()
Expand Down Expand Up @@ -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() {
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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() {
Expand All @@ -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()
Expand Down Expand Up @@ -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() {
Expand Down Expand Up @@ -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)
Expand All @@ -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)

Expand All @@ -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)

Expand All @@ -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)
Expand Down
Loading

0 comments on commit 909358d

Please sign in to comment.