You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@kudu.apache.org by to...@apache.org on 2016/03/01 23:01:23 UTC
[2/3] incubator-kudu git commit: diskrowset: Fix reentrant
acquisition of DiskRowSet component_lock
diskrowset: Fix reentrant acquisition of DiskRowSet component_lock
Our RW spinlocks are not safe to acquire re-entrantly, since they
avoid writer starvation. If a thread holds a read lock, then a writer
waits, and then the thread tries to acquire another read lock, it will
block to prevent starving the writer. Of course the writer cannot
proceed either, and we have a deadlock.
I found this due to TSAN flagging a lock order inversion on these
locks. The trace didn't quite make sense, but when I looked at this
code I spotted the reentrant read-lock acquisition. We'll see if it
silences the trace or if there's another issue lurking.
Change-Id: Iee583a1442ac270238112427e37f2ec6565dfd38
Reviewed-on: http://gerrit.cloudera.org:8080/2375
Tested-by: Kudu Jenkins
Reviewed-by: David Ribeiro Alves <da...@cloudera.com>
Project: http://git-wip-us.apache.org/repos/asf/incubator-kudu/repo
Commit: http://git-wip-us.apache.org/repos/asf/incubator-kudu/commit/0ffa754a
Tree: http://git-wip-us.apache.org/repos/asf/incubator-kudu/tree/0ffa754a
Diff: http://git-wip-us.apache.org/repos/asf/incubator-kudu/diff/0ffa754a
Branch: refs/heads/master
Commit: 0ffa754a741fad11061b3efa6eed27fa308d1191
Parents: a8c6de9
Author: Todd Lipcon <to...@apache.org>
Authored: Tue Mar 1 10:58:42 2016 -0800
Committer: Todd Lipcon <to...@apache.org>
Committed: Tue Mar 1 22:01:00 2016 +0000
----------------------------------------------------------------------
src/kudu/tablet/diskrowset.cc | 2 +-
src/kudu/util/rw_semaphore.h | 8 ++++++++
2 files changed, 9 insertions(+), 1 deletion(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/incubator-kudu/blob/0ffa754a/src/kudu/tablet/diskrowset.cc
----------------------------------------------------------------------
diff --git a/src/kudu/tablet/diskrowset.cc b/src/kudu/tablet/diskrowset.cc
index 55039e1..875f2a7 100644
--- a/src/kudu/tablet/diskrowset.cc
+++ b/src/kudu/tablet/diskrowset.cc
@@ -666,7 +666,7 @@ uint64_t DiskRowSet::EstimateDeltaDiskSize() const {
uint64_t DiskRowSet::EstimateOnDiskSize() const {
DCHECK(open_);
boost::shared_lock<rw_spinlock> lock(component_lock_.get_lock());
- return EstimateBaseDataDiskSize() + EstimateDeltaDiskSize();
+ return base_data_->EstimateOnDiskSize() + delta_tracker_->EstimateOnDiskSize();
}
size_t DiskRowSet::DeltaMemStoreSize() const {
http://git-wip-us.apache.org/repos/asf/incubator-kudu/blob/0ffa754a/src/kudu/util/rw_semaphore.h
----------------------------------------------------------------------
diff --git a/src/kudu/util/rw_semaphore.h b/src/kudu/util/rw_semaphore.h
index 6ce5462..de0bb59 100644
--- a/src/kudu/util/rw_semaphore.h
+++ b/src/kudu/util/rw_semaphore.h
@@ -36,6 +36,14 @@ namespace kudu {
// This rw-semaphore makes no attempt at fairness, though it does avoid write
// starvation (no new readers may obtain the lock if a write is waiting).
//
+// NOTE: this means that it is not safe to reentrantly acquire the read lock,
+// due to the following deadlock:
+// - T1: acquire read lock
+// - T2: wait for write lock
+// (blocks waiting for readers)
+// - T1: try to acquire read-lock reentrantly
+// (blocks to avoid starving writers)
+//
// Given that this is currently based only on spinning (and not futex),
// it should only be used in cases where the lock is held for very short
// time intervals.