Merge remote-tracking branch 'origin/main' into fix-race-conditions

component/file_cache/file_cache.go

@@ -119,6 +119,7 @@ const (
 	defaultMaxThreshold     = 80
 	defaultMinThreshold     = 60
 	defaultFileCacheTimeout = 120
+	minimumFileCacheTimeout = 1
 	defaultCacheUpdateCount = 100
 	MB                      = 1024 * 1024
 )
@@ -220,9 +221,9 @@ func (c *FileCache) Configure(_ bool) error {
 
 	c.createEmptyFile = conf.CreateEmptyFile
 	if config.IsSet(compName + ".file-cache-timeout-in-seconds") {
-		c.cacheTimeout = float64(conf.V1Timeout)
+		c.cacheTimeout = max(float64(conf.V1Timeout), minimumFileCacheTimeout)
 	} else if config.IsSet(compName + ".timeout-sec") {
-		c.cacheTimeout = float64(conf.Timeout)
+		c.cacheTimeout = max(float64(conf.Timeout), minimumFileCacheTimeout)
 	} else {
 		c.cacheTimeout = float64(defaultFileCacheTimeout)
 	}
@@ -341,7 +342,7 @@ func (c *FileCache) OnConfigChange() {
 	}
 
 	c.createEmptyFile = conf.CreateEmptyFile
-	c.cacheTimeout = float64(conf.Timeout)
+	c.cacheTimeout = max(float64(conf.Timeout), minimumFileCacheTimeout)
 	c.policyTrace = conf.EnablePolicyTrace
 	c.offloadIO = conf.OffloadIO
 	c.maxCacheSize = conf.MaxSizeMB

component/file_cache/file_cache_test.go

@@ -333,7 +333,7 @@ func (suite *fileCacheTestSuite) TestConfigZero() {
 
 	suite.assert.Equal(suite.fileCache.createEmptyFile, createEmptyFile)
 	suite.assert.Equal(suite.fileCache.allowNonEmpty, allowNonEmptyTemp)
-	suite.assert.EqualValues(suite.fileCache.cacheTimeout, cacheTimeout)
+	suite.assert.EqualValues(suite.fileCache.cacheTimeout, minimumFileCacheTimeout)
 	suite.assert.Equal(suite.fileCache.cleanupOnStart, cleanupOnStart)
 }
 
@@ -936,9 +936,8 @@ func (suite *fileCacheTestSuite) TestCloseFile() {
 func (suite *fileCacheTestSuite) TestCloseFileTimeout() {
 	defer suite.cleanupTest()
 	suite.cleanupTest() // teardown the default file cache generated
-	cacheTimeout := 1
 	config := fmt.Sprintf("file_cache:\n  path: %s\n  offload-io: true\n  timeout-sec: %d\n\nloopbackfs:\n  path: %s",
-		suite.cache_path, cacheTimeout, suite.fake_storage_path)
+		suite.cache_path, minimumFileCacheTimeout, suite.fake_storage_path)
 	suite.setupTestHelper(config) // setup a new file cache with a custom config (teardown will occur after the test as usual)
 
 	path := "file10"
@@ -957,7 +956,7 @@ func (suite *fileCacheTestSuite) TestCloseFileTimeout() {
 
 	// loop until file does not exist - done due to async nature of eviction
 	_, err = os.Stat(filepath.Join(suite.cache_path, path))
-	for i := 0; i < (cacheTimeout*300) && !os.IsNotExist(err); i++ {
+	for i := 0; i < (minimumFileCacheTimeout*300) && !os.IsNotExist(err); i++ {
 		time.Sleep(10 * time.Millisecond)
 		_, err = os.Stat(filepath.Join(suite.cache_path, path))
 	}
@@ -993,9 +992,8 @@ func (suite *fileCacheTestSuite) TestOpenPreventsEviction() {
 	defer suite.cleanupTest()
 	// Setup
 	suite.cleanupTest() // teardown the default file cache generated
-	cacheTimeout := 1
 	config := fmt.Sprintf("file_cache:\n  path: %s\n  offload-io: true\n  timeout-sec: %d\n\nloopbackfs:\n  path: %s",
-		suite.cache_path, cacheTimeout, suite.fake_storage_path)
+		suite.cache_path, minimumFileCacheTimeout, suite.fake_storage_path)
 	suite.setupTestHelper(config) // setup a new file cache with a custom config (teardown will occur after the test as usual)
 
 	path := "file12"
@@ -1013,7 +1011,7 @@ func (suite *fileCacheTestSuite) TestOpenPreventsEviction() {
 	suite.assert.NoError(err)
 
 	// wait until file would be evicted (if not for being opened)
-	time.Sleep(time.Second * time.Duration(cacheTimeout*3))
+	time.Sleep(time.Second * time.Duration(minimumFileCacheTimeout*3))
 
 	// File should still be in cache
 	suite.assert.FileExists(filepath.Join(suite.cache_path, path))
@@ -1377,34 +1375,6 @@ func (suite *fileCacheTestSuite) TestRenameFileAndCacheCleanup() {
 	suite.assert.FileExists(suite.cache_path + "/" + dst) // Dst shall not exists in cache
 }
 
-func (suite *fileCacheTestSuite) TestRenameFileAndCacheCleanupWithNoTimeout() {
-	defer suite.cleanupTest()
-
-	src := "source5"
-	dst := "destination5"
-	handle, _ := suite.fileCache.CreateFile(internal.CreateFileOptions{Name: src, Mode: 0666})
-	suite.fileCache.CloseFile(internal.CloseFileOptions{Handle: handle})
-
-	// Path _might_ be in the file cache
-	// Path _should_ be in fake storage
-	suite.assert.FileExists(suite.fake_storage_path + "/" + src)
-
-	// RenameFile
-	err := suite.fileCache.RenameFile(internal.RenameFileOptions{Src: src, Dst: dst})
-	suite.assert.NoError(err)
-
-	// Dst _might_ exist in cache, and the rename should be complete in fake storage
-	suite.assert.NoFileExists(suite.fake_storage_path + "/" + src) // Src does not exist
-	suite.assert.FileExists(suite.fake_storage_path + "/" + dst)   // Dst does exist
-
-	time.Sleep(200 * time.Millisecond) // Wait for the cache cleanup to occur
-	// cache should be completely clean
-	suite.assert.False(suite.fileCache.policy.IsCached(filepath.Join(suite.cache_path, src)))
-	suite.assert.False(suite.fileCache.policy.IsCached(filepath.Join(suite.cache_path, dst)))
-	suite.assert.NoFileExists(suite.cache_path + "/" + src)
-	suite.assert.NoFileExists(suite.cache_path + "/" + dst)
-}
-
 func (suite *fileCacheTestSuite) TestTruncateFileNotInCache() {
 	defer suite.cleanupTest()
 	// Setup
@@ -1480,9 +1450,8 @@ func (suite *fileCacheTestSuite) TestTruncateFileCase2() {
 
 func (suite *fileCacheTestSuite) TestZZMountPathConflict() {
 	defer suite.cleanupTest()
-	cacheTimeout := 1
 	configuration := fmt.Sprintf("file_cache:\n  path: %s\n  offload-io: true\n  timeout-sec: %d\n\nloopbackfs:\n  path: %s",
-		suite.cache_path, cacheTimeout, suite.fake_storage_path)
+		suite.cache_path, minimumFileCacheTimeout, suite.fake_storage_path)
 
 	fileCache := NewFileCacheComponent()
 	config.ReadConfigFromReader(strings.NewReader(configuration))

component/file_cache/lru_policy.go

@@ -76,8 +76,7 @@ type lruPolicy struct {
 
 const (
 	// Check for disk usage in below number of minutes
-	DiskUsageCheckInterval  = 1
-	minimumEvictionInterval = 100 * time.Millisecond
+	DiskUsageCheckInterval = 1
 )
 
 var _ cachePolicy = &lruPolicy{}
@@ -127,9 +126,8 @@ func (p *lruPolicy) StartPolicy() error {
 
 	log.Info("lruPolicy::StartPolicy : Policy set with %v timeout", p.cacheTimeout)
 
-	// run the timeout monitor even with timeout set to zero
-	timeoutInterval := time.Duration(p.cacheTimeout) * time.Second
-	p.cacheTimeoutMonitor = time.Tick(max(timeoutInterval, minimumEvictionInterval))
+	// start the timeout monitor
+	p.cacheTimeoutMonitor = time.Tick(time.Duration(p.cacheTimeout) * time.Second)
 
 	go p.clearCache()
 	go p.asyncCacheValid()
@@ -238,7 +236,8 @@ func (p *lruPolicy) cacheValidate(name string) {
 	if node == p.head {
 		return
 	}
-	p.moveToHead(node)
+	p.extractNode(node)
+	p.setHead(node)
 }
 
 // For all other timer based activities we check the stuff here
@@ -305,51 +304,44 @@ func (p *lruPolicy) removeNode(name string) {
 	node.deleted = true
 	node.Unlock()
 
-	if node == p.head {
-		p.head = node.next
-		p.head.prev = nil
-		node.next = nil
-		return
-	}
-
-	if node.next != nil {
-		node.next.prev = node.prev
-	}
-
-	if node.prev != nil {
-		node.prev.next = node.next
-	}
-	node.prev = nil
-	node.next = nil
+	p.extractNode(node)
 }
 
 func (p *lruPolicy) updateMarker() {
 	log.Trace("lruPolicy::updateMarker")
 
 	p.Lock()
-	p.moveToHead(p.lastMarker)
-	// evict everything when timeout is zero
-	if p.cacheTimeout == 0 {
-		p.moveToHead(p.currMarker)
-	} else {
-		// swap lastMarker with currMarker
-		swap := p.lastMarker
-		p.lastMarker = p.currMarker
-		p.currMarker = swap
-	}
+	p.extractNode(p.lastMarker)
+	p.setHead(p.lastMarker)
+	// swap lastMarker with currMarker
+	swap := p.lastMarker
+	p.lastMarker = p.currMarker
+	p.currMarker = swap
 
 	p.Unlock()
 }
 
-func (p *lruPolicy) moveToHead(node *lruNode) {
-	// remove the node from its position
+func (p *lruPolicy) extractNode(node *lruNode) {
+	// remove the node from its position in the list
+
+	// head case
+	if node == p.head {
+		p.head = node.next
+	}
+
 	if node.next != nil {
 		node.next.prev = node.prev
 	}
 	if node.prev != nil {
 		node.prev.next = node.next
 	}
-	// and insert it at the head
+
+	node.prev = nil
+	node.next = nil
+}
+
+func (p *lruPolicy) setHead(node *lruNode) {
+	// insert node at the head
 	node.prev = nil
 	node.next = p.head
 	p.head.prev = node

setup/baseConfig.yaml

@@ -114,7 +114,7 @@ file_cache:
 
 # Attribute cache related configuration
 attr_cache:
-  timeout-sec: <time attributes and directory contents can be cached (in sec). Default - 120 sec>
+  timeout-sec: <time attributes and directory contents can be cached (in sec). Minimum is 1. Default - 120 sec>
   no-cache-on-list: true|false <do not cache attributes or directory contents during listing. Enabling may cause performance problems.>
   enable-symlinks: true|false <enable symlink support. When false, symlinks will be treated like regular files. Enabling may cause performance problems.>
   max-files: <maximum number of files in the attribute cache at a time. Default - 5000000>