Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix(openfile): avoid returning stale slice caches #5192

Closed
wants to merge 1 commit into from

Conversation

polyrabbit
Copy link
Contributor

Some users cache opened files for a long time for performance reasons, which causes them read stale slices from openfiles cache - we only check expiration when opening a file, not reading slices.

One scenario is one client opens a file and reads occasionally - the read slices are cached forever. Another client compacts the file and old slices are deleted after trash-days. Now when the first client reads the file again, it gets stale slices. The penalty of false positive cache is one-second delay of a successful read. We could avoid this by applying expire to slices cache as well.

for err != nil && tried < 2 {
time.Sleep(time.Second * time.Duration(tried*tried))
if tried > 0 {
logger.Warnf("GET %s: %s; retrying", key, err)
store.objectReqErrors.Add(1)
start = time.Now()
}
in, err = store.storage.Get(key, 0, -1, object.WithRequestID(&reqID), object.WithStorageClass(&sc))
tried++
}

@SandyXSD SandyXSD self-assigned this Sep 26, 2024
@@ -222,6 +226,7 @@ func (o *openfiles) CacheChunk(ino Ino, indx uint32, cs []Slice) {
}
of.chunks[indx] = cs
}
of.lastCheck = time.Now().Unix()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The lastCheck is used to indicate the check time of attribute, so should not be updated here.

pkg/meta/openfile.go Show resolved Hide resolved
@polyrabbit
Copy link
Contributor Author

@SandyXSD Do you think chunk cache should also obey the open-cache expire flag? If not, I'm OK to close this PR and keep it privately.

@davies
Copy link
Contributor

davies commented Sep 26, 2024

The current behavior is 1 seconds delay in the case of stale slice (rarely), so it's better to remove the retry or retry without delay.

@polyrabbit
Copy link
Contributor Author

Agreed, I suppose it's better to remove the inner retry. The current read retry is twice as user expected.

@polyrabbit
Copy link
Contributor Author

Taken over by #5199 as discussed.

@polyrabbit polyrabbit closed this Sep 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants