Don't lock if we're only reading cache metadata #20053
Merged
Conversation
This file contains 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
ghost
added
the
@icewind1991 can you elaborate on this ? Are we using transactions that make that snapshot possible ? |
|
Yes, when scanning we use transactions to keep things consistent |
|
Is this guaranteed to work for all databases like for example SQlite ? |
|
The behavior in relation to |
icewind1991
force-pushed
the
getfileinfo-locking
branch
from
da27ddf
to
ba05e36
Compare
|
I don't see a transaction start at the beginning of I'm still worried that this risks bringing back the worst of the race conditions (#13391). It could happen that what's on the cache already doesn't match any more what's on disk. A concurrent process might have updated the cache + disk already, but what we observe in the new locked portion in this PR is that the disk contents doesn't match the old cached How did you test this concurrently ? |
In that case it tries to update the cache, for which it does try to get a lock While doing I noticed one place in the cache code where we can have an inconsistent state outside a transaction ( |
|
|
What about the case where a folder rename is pending and all the DB entries for the children are still being renamed ? Say we have half of the entries renamed. At this point we don't want a "cache->get()" to be able to read either the old parent entry nor the new parent entry nor the children. |
|
We rename the childs in a folder in a transaction |
|
Right, but the renaming on disk is done before that transaction. Also I see here https://github.com/owncloud/core/pull/20053/files?w=1#diff-d9747e68e80ce288dcea911cb8607546R1207 needsUpdate is also outside the lock, but is accessing the disk for |
Which doesn't matter, we dont care about any on-disk state |
|
👍 |
|
@karlitschek so you too think there is no big risk with this PR ? |
|
@PVince81 This is risky but I think we should do this because of the big performance problems. What do yuo think? |
|
Ok then, let's go ahead 👍 I'll assume that @icewind1991 carefully tested the concurrency cases in a way that proves his statements. |
|
@icewind1991 can you rebase to unstuck CI ? |
icewind1991
force-pushed
the
getfileinfo-locking
branch
from
ba05e36
to
87cb77a
Compare
|
Looks like there's trouble again in CI-land ? |
icewind1991
force-pushed
the
getfileinfo-locking
branch
from
87cb77a
to
fc7f7e5
Compare
|
@icewind1991 please fix the tests: |
|
All fixed |
|
Farntastic! Let's move forward with this 👍 |
DeepDiver1975
added a commit
that referenced
this pull request
Nov 4, 2015
Don't lock if we're only reading cache metadata
|
From my understanding this perf improvement was intended for #19888 for 8.2.1. @karlitschek we should backport this ? |
|
8.2: #20326 |
|
yes. please backport 👍 |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Since the db ensures we get a proper metadata snapshot we don't need to lock if we're only reading from the database.
Made possible by splitting
checkUpdateso we can still lock correctly if we need to re-scanMakes PROPFIND a non-locking operation (comparison)
cc @DeepDiver1975 @PVince81 @LukasReschke