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

Review and improve locks in Storage #187

Closed
ikopylov opened this issue Aug 31, 2022 · 3 comments · Fixed by #198
Closed

Review and improve locks in Storage #187

ikopylov opened this issue Aug 31, 2022 · 3 comments · Fixed by #198
Labels
enhancement New feature or request performance

Comments

@ikopylov
Copy link
Member

ikopylov commented Aug 31, 2022

RWLock on safe does protect active_blob + blobs updates inside, so the write lock should be acquired only when these 2 fields updating. But if we look at write_with_optional_meta function, it is incorrectly acquire write lock (https://github.com/qoollo/pearl/blob/master/src/storage/core.rs#L313). It should acquire read lock, because it does not update neither active_blob nor blobs fields.
Same thing here: https://github.com/qoollo/pearl/blob/master/src/storage/core.rs#L890 and here: https://github.com/qoollo/pearl/blob/master/src/storage/core.rs#L912. Probably, there are more places where write lock taken incorrectly.

Also, it is strange that blobs protected with additional RWLock. In every places this lock acquired right after acquire of RWLock on safe, thus can be removed.

This incorrect locking mechanism results in significant performance degradation

@idruzhitskiy
Copy link
Contributor

RwLock on blobs can be removed. But lock can't be made read in write_with_optional_meta, since blob.write(...) requires mutable reference (due to index modification). But mutex on current_offset can be removed instead.

@idruzhitskiy
Copy link
Contributor

Also any redundant write acquisition in most cases should be reported by compiler as redundant mut.

@ikopylov
Copy link
Member Author

ikopylov commented Sep 4, 2022

Exclusive lock should be as close to actual write to file as possible, so it is better to keep lock on current_offset and somehow avoid write lock in write_with_optional_meta

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request performance
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants