chore(fs): only cache the cache for case FS, not the entire FS #9701
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Member
Author
|
Not sure if this is really |
calmh
reviewed
Sep 12, 2024
lib/fs/casefs.go
Outdated
| } | ||
|
|
||
| func newDefaultRealCaser(fs Filesystem) *defaultRealCaser { | ||
| type caseCache = *lru.TwoQueueCache[string, *caseNode] |
Member
There was a problem hiding this comment.
This confused me up until reading this line, because the map[fskey]caseCache looks like a map of something not-pointer-shaped, and yet here we are with a pointer. I think I'd prefer the alias to not include the pointer star, and have that be visible at the places it's used instead...
calmh
reviewed
Sep 12, 2024
Member
calmh
left a comment
There was a problem hiding this comment.
Reasonable, apart from clarity complaint
3c64a68 to
86ca6ea
Compare
This would have addressed a recent issue that arose when re-ordering our "filesystem layers". Specifically moving the caseFilesystem to the outermost layer. The previous cache included the filesystem, and as such all the layers below. This isn't desirable (to put it mildly), as you can create different variants of filesystems with different layers for the same path and options. Concretely this did happen with the mtime layer, which isn't always present. A test for the mtime related breakage was added in syncthing#9687, and I intend to redo the caseFilesystem reordering after this. Ref: syncthing#9677 Followup to: syncthing#9687
86ca6ea to
3297e86
Compare
calmh
approved these changes
Sep 12, 2024
leoon-ding
pushed a commit
to leoon-ding/syncthing
that referenced
this pull request
Mar 19, 2025
…hing#9701) This would have addressed a recent issue that arose when re-ordering our "filesystem layers". Specifically moving the caseFilesystem to the outermost layer. The previous cache included the filesystem, and as such all the layers below. This isn't desirable (to put it mildly), as you can create different variants of filesystems with different layers for the same path and options. Concretely this did happen with the mtime layer, which isn't always present. A test for the mtime related breakage was added in syncthing#9687, and I intend to redo the caseFilesystem reordering after this. Ref: syncthing#9677 Followup to: syncthing#9687
marbens-arch
added a commit
to marbens-arch/syncthing
that referenced
this pull request
Aug 27, 2025
syncthing#9701)" This reverts commit ac8b334.
marbens-arch
added a commit
to marbens-arch/syncthing
that referenced
this pull request
Oct 17, 2025
In syncthing#9701 there was a semi-unrelated change to bind the `getExpireAdd` mutex and `getExpireAdd` itself to `defaultRealCaser`, instead of `caseCache`. This is erroneous because multiple `defaultRealCaser`s can share the same `caseCache`. Signed-off-by: Marcus B Spencer <marcus@marcusspencer.us>
imsodin
pushed a commit
that referenced
this pull request
Oct 18, 2025
) In #9701 there was a change that put the mutex used for `getExpireAdd` directly in `defaultRealCaser`, which is erroneous because multiple filesystems can share the same `caseCache`. ### Purpose Fixes #9836 and [Slow sync sending files from Android](https://forum.syncthing.net/t/slow-sync-sending-files-from-android/24208?u=marbens). There may be other issues caused by `getExpireAdd` conflicting with itself, though. ### Testing Unit tests pass and the case cache and conflict detection _seem_ to behave correctly. Signed-off-by: Marcus B Spencer <marcus@marcusspencer.us>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This would have addressed a recent issue that arose when re-ordering our "filesystem layers". Specifically moving the caseFilesystem to the outermost layer. The previous cache included the filesystem, and as such all the layers below. This isn't desirable (to put it mildly), as you can create different variants of filesystems with different layers for the same path and options. Concretely this did happen with the mtime layer, which isn't always present. A test for the mtime related breakage was added in #9687, and I intend to redo the caseFilesystem reordering after this.
Ref: #9677
Followup to: #9687