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.

Sign up for GitHub

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

Jump to bottom

Fix SIGABRT caused by uninitialized mutex #296

Merged
merged 3 commits into from
Jul 7, 2022
Merged

Fix SIGABRT caused by uninitialized mutex #296

merged 3 commits into from
Jul 7, 2022

Conversation

ethercflow
Copy link
Member

Fixed bug where wrong use of uninitialized Mutex caused SIGABRT.

Signed-off-by: Wenbo Zhang [email protected]

@ethercflow ethercflow requested review from Connor1996 and tabokie July 5, 2022 12:28
@ethercflow
Copy link
Member Author

@Connor1996 @tabokie PTAL, thanks!

tabokie
tabokie reviewed Jul 6, 2022
View reviewed changes
db/write_thread.h Outdated
@@ -185,6 +185,7 @@ class WriteThread {
CommitRequest* request;
SequenceNumber sequence; // the sequence number to use for the first key
Status status;
std::mutex status_mu;
Copy link
Member

@tabokie tabokie Jul 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

You should figure out another solution. Maybe spinlock? Each write creates a writer, this puts too much load on futex infrastructure (last I checked each futex creation insert to a global kernel map).

@ethercflow ethercflow requested a review from tabokie July 6, 2022 08:11
tabokie
tabokie reviewed Jul 6, 2022
View reviewed changes
Copy link
Member

@tabokie tabokie left a comment

Choose a reason for hiding this comment

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

Needs to add a new method for reading status. There're a lot of places that read status using db_mutex.

db/write_thread.h Outdated Show resolved Hide resolved
db/write_thread.h Outdated Show resolved Hide resolved
@ethercflow
Copy link
Member Author

Needs to add a new method for reading status. There're a lot of places that read status using db_mutex.

I don't think it's needed because WriterThread returns status only after writer has finished committing (by which time helpers have also finished modifying the status for this writer)

@ethercflow ethercflow requested a review from tabokie July 6, 2022 09:05
tabokie
tabokie approved these changes Jul 6, 2022
View reviewed changes
db/write_thread.h Outdated Show resolved Hide resolved
Connor1996
Connor1996 reviewed Jul 6, 2022
View reviewed changes
Copy link
Member

@Connor1996 Connor1996 left a comment

Choose a reason for hiding this comment

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

why the mutex is uninitiliazed

@ethercflow
Copy link
Member Author

why the mutex is uninitiliazed

As this mutex is lazily created in BlockingAwaitState, only when the new writer enters JoinBatchGroup and not linked as the leader.

@ethercflow ethercflow requested a review from Connor1996 July 6, 2022 14:50
@Connor1996
Copy link
Member

why the mutex is uninitiliazed

As this mutex is lazily created in BlockingAwaitState, only when the new writer enters JoinBatchGroup and not linked as the leader.

Not deep dive into the code, can we call CreateMutex somewhere instead of introducing a new mutex

Connor1996
Connor1996 approved these changes Jul 7, 2022
View reviewed changes
Copy link
Member

@Connor1996 Connor1996 left a comment

Choose a reason for hiding this comment

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

LGTM

@tabokie tabokie merged commit 4037bda into tikv:6.4.tikv Jul 7, 2022
ethercflow added a commit to ethercflow/rocksdb that referenced this pull request Jul 7, 2022
@ethercflow @tabokie
Fix SIGABRT caused by uninitialized mutex (tikv#296)
fc5a903 
* Fix SIGABRT caused by uninitialized mutex

Signed-off-by: Wenbo Zhang <[email protected]>

* Use spinlock instead of mutex to reduce writer ctor cost

Signed-off-by: Wenbo Zhang <[email protected]>

* Update db/write_thread.h

Co-authored-by: Xinye Tao <[email protected]>
Signed-off-by: Wenbo Zhang <[email protected]>

Co-authored-by: Xinye Tao <[email protected]>
Signed-off-by: Wenbo Zhang <[email protected]>
tabokie added a commit that referenced this pull request Jul 7, 2022
@ethercflow @tabokie
Fix SIGABRT caused by uninitialized mutex (#296) (#298)
9bb7147 
* Fix SIGABRT caused by uninitialized mutex

Signed-off-by: Wenbo Zhang <[email protected]>

* Use spinlock instead of mutex to reduce writer ctor cost

Signed-off-by: Wenbo Zhang <[email protected]>

* Update db/write_thread.h

Co-authored-by: Xinye Tao <[email protected]>
Signed-off-by: Wenbo Zhang <[email protected]>

Co-authored-by: Xinye Tao <[email protected]>
Signed-off-by: Wenbo Zhang <[email protected]>

Co-authored-by: Xinye Tao <[email protected]>
ethercflow added a commit to ethercflow/rocksdb that referenced this pull request Aug 10, 2022
@ethercflow
Revert "Fix SIGABRT caused by uninitialized mutex (tikv#296)"
86a6874 
This reverts commit 4037bda.

Signed-off-by: Wenbo Zhang <[email protected]>
tabokie pushed a commit that referenced this pull request Aug 11, 2022
@ethercflow
Revert multi-batch write optimization (#313)
9ca5afb 
* Revert "Fix SIGABRT caused by uninitialized mutex (#296)"

This reverts commit 4037bda.

Signed-off-by: Wenbo Zhang <[email protected]>

* Revert "Optimize Commit pipeline performance (#286)"

This reverts commit e4bfc11.

Signed-off-by: Wenbo Zhang <[email protected]>

Signed-off-by: Wenbo Zhang <[email protected]>
v01dstar added a commit to v01dstar/rocksdb that referenced this pull request May 19, 2024
@v01dstar @tabokie
Implement multi batches write
05caaee 
Implement multi batches write

Signed-off-by: v01dstar <[email protected]>

Fix SIGABRT caused by uninitialized mutex (tikv#296) (tikv#298)

* Fix SIGABRT caused by uninitialized mutex

Signed-off-by: Wenbo Zhang <[email protected]>

* Use spinlock instead of mutex to reduce writer ctor cost

Signed-off-by: Wenbo Zhang <[email protected]>

* Update db/write_thread.h

Co-authored-by: Xinye Tao <[email protected]>
Signed-off-by: Wenbo Zhang <[email protected]>

Co-authored-by: Xinye Tao <[email protected]>
Signed-off-by: Wenbo Zhang <[email protected]>

Co-authored-by: Xinye Tao <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tabokie tabokie tabokie approved these changes

@Connor1996 Connor1996 Connor1996 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ethercflow @Connor1996 @tabokie