You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@kudu.apache.org by ad...@apache.org on 2017/09/08 22:54:51 UTC
[1/2] kudu git commit: catalog_manager: fix unprotected data access
in TableInfo::AddRemoveTablets
Repository: kudu
Updated Branches:
refs/heads/master 7e391c473 -> 9f0649448
catalog_manager: fix unprotected data access in TableInfo::AddRemoveTablets
RWCLock::HasWriteLock never worked properly because last_writer_tid_ wasn't
reset when the lock was released. Well, it worked properly (though it's far
less useful) in non-DEBUG builds, but then the various HasWriteLock DCHECKs
are compiled out. Who knew?
When fixed, TableInfo::AddRemoveTablets broke; we had been reading some
tablet metadata without first acquiring a lock! This was frustrasting to
address because it meant retreading over the most error-prone aspect of the
catalog manager: for operations that require several writes in order to
"publish" their results, in what order should those writes occur? I tried my
best to get this right, but who knows how I did...
Change-Id: Ifcc5d3a9b985210f1fdd6f0495326fa3eb707841
Reviewed-on: http://gerrit.cloudera.org:8080/7995
Tested-by: Kudu Jenkins
Reviewed-by: Dan Burkert <da...@apache.org>
Reviewed-by: Alexey Serbin <as...@cloudera.com>
Project: http://git-wip-us.apache.org/repos/asf/kudu/repo
Commit: http://git-wip-us.apache.org/repos/asf/kudu/commit/c0e18fd7
Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/c0e18fd7
Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/c0e18fd7
Branch: refs/heads/master
Commit: c0e18fd743e36b20ef3ab364c33ff0954fb4898e
Parents: 7e391c4
Author: Adar Dembo <ad...@cloudera.com>
Authored: Thu Sep 7 03:55:59 2017 -0700
Committer: Adar Dembo <ad...@cloudera.com>
Committed: Fri Sep 8 22:54:03 2017 +0000
----------------------------------------------------------------------
src/kudu/master/catalog_manager-test.cc | 19 +++++-----
src/kudu/master/catalog_manager.cc | 55 ++++++++++++++++++++++++++--
src/kudu/master/catalog_manager.h | 3 ++
src/kudu/util/cow_object.h | 3 ++
src/kudu/util/rwc_lock.cc | 32 +++++++++++-----
src/kudu/util/rwc_lock.h | 6 ++-
6 files changed, 96 insertions(+), 22 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/kudu/blob/c0e18fd7/src/kudu/master/catalog_manager-test.cc
----------------------------------------------------------------------
diff --git a/src/kudu/master/catalog_manager-test.cc b/src/kudu/master/catalog_manager-test.cc
index 8a5ce2e..c553d01 100644
--- a/src/kudu/master/catalog_manager-test.cc
+++ b/src/kudu/master/catalog_manager-test.cc
@@ -57,13 +57,16 @@ TEST(TableInfoTest, TestAssignmentRanges) {
string tablet_id = Substitute("tablet-$0-$1", start_key, end_key);
scoped_refptr<TabletInfo> tablet = new TabletInfo(table, tablet_id);
- TabletMetadataLock meta_lock(tablet.get(), TabletMetadataLock::WRITE);
-
- PartitionPB* partition = meta_lock.mutable_data()->pb.mutable_partition();
- partition->set_partition_key_start(start_key);
- partition->set_partition_key_end(end_key);
- meta_lock.mutable_data()->pb.set_state(SysTabletsEntryPB::RUNNING);
- meta_lock.Commit();
+ {
+ TabletMetadataLock meta_lock(tablet.get(), TabletMetadataLock::WRITE);
+ PartitionPB* partition = meta_lock.mutable_data()->pb.mutable_partition();
+ partition->set_partition_key_start(start_key);
+ partition->set_partition_key_end(end_key);
+ meta_lock.mutable_data()->pb.set_state(SysTabletsEntryPB::RUNNING);
+ meta_lock.Commit();
+ }
+
+ TabletMetadataLock meta_lock(tablet.get(), TabletMetadataLock::READ);
table->AddRemoveTablets({ tablet }, {});
tablets.push_back(tablet);
}
@@ -89,8 +92,6 @@ TEST(TableInfoTest, TestAssignmentRanges) {
ASSERT_EQ(tablet_id, (*tablets_in_range.begin())->tablet_id());
LOG(INFO) << "Key " << start_key << " found in tablet " << tablet_id;
}
-
- table->AddRemoveTablets({}, tablets);
}
TEST(TestTSDescriptor, TestReplicaCreationsDecay) {
http://git-wip-us.apache.org/repos/asf/kudu/blob/c0e18fd7/src/kudu/master/catalog_manager.cc
----------------------------------------------------------------------
diff --git a/src/kudu/master/catalog_manager.cc b/src/kudu/master/catalog_manager.cc
index ef23b9c..509f3c5 100644
--- a/src/kudu/master/catalog_manager.cc
+++ b/src/kudu/master/catalog_manager.cc
@@ -347,10 +347,13 @@ class TabletLoader : public TabletVisitor {
// Add the tablet to the tablet manager.
catalog_manager_->tablet_map_[tablet->tablet_id()] = tablet;
- // Add the tablet to the Tablet.
+ // Add the tablet to the table.
bool is_deleted = l.mutable_data()->is_deleted();
l.Commit();
if (!is_deleted) {
+ // Need to use a new tablet lock here because AddRemoveTablets() reads
+ // from clean state, which is uninitialized for these brand new tablets.
+ TabletMetadataLock l(tablet.get(), TabletMetadataLock::READ);
table->AddRemoveTablets({ tablet }, {});
LOG(INFO) << Substitute("Loaded metadata for tablet $0 (table $1)",
tablet_id, table->ToString());
@@ -1475,6 +1478,12 @@ Status CatalogManager::CreateTable(const CreateTableRequestPB* orig_req,
// It's not yet in any global maps; that will happen in step g below.
table = CreateTableInfo(req, schema, partition_schema);
vector<scoped_refptr<TabletInfo>> tablets;
+ auto abort_mutations = MakeScopedCleanup([&table, &tablets]() {
+ table->mutable_metadata()->AbortMutation();
+ for (const auto& e : tablets) {
+ e->mutable_metadata()->AbortMutation();
+ }
+ });
for (const Partition& partition : partitions) {
PartitionPB partition_pb;
partition.ToPB(&partition_pb);
@@ -1512,9 +1521,22 @@ Status CatalogManager::CreateTable(const CreateTableRequestPB* orig_req,
for (const auto& tablet : tablets) {
tablet->mutable_metadata()->CommitMutation();
}
+ abort_mutations.cancel();
+
+ // g. Add the tablets to the table.
+ //
+ // We can't reuse the above WRITE tablet locks for this because
+ // AddRemoveTablets() will read from the clean state, which is empty for
+ // these brand new tablets.
+ for (const auto& tablet : tablets) {
+ tablet->metadata().ReadLock();
+ }
table->AddRemoveTablets(tablets, {});
+ for (const auto& tablet : tablets) {
+ tablet->metadata().ReadUnlock();
+ }
- // g. Make the new table and tablets visible in the catalog.
+ // h. Make the new table and tablets visible in the catalog.
{
std::lock_guard<LockType> l(lock_);
@@ -1789,6 +1811,11 @@ Status CatalogManager::ApplyAlterPartitioningSteps(
TableInfo::TabletInfoMap existing_tablets = table->tablet_map();
TableInfo::TabletInfoMap new_tablets;
+ auto abort_mutations = MakeScopedCleanup([&new_tablets]() {
+ for (const auto& e : new_tablets) {
+ e.second->mutable_metadata()->AbortMutation();
+ }
+ });
for (const auto& step : steps) {
vector<DecodedRowOperation> ops;
@@ -1917,6 +1944,7 @@ Status CatalogManager::ApplyAlterPartitioningSteps(
tablets_to_drop->emplace_back(existing_iter->second);
existing_tablets.erase(existing_iter);
} else if (found_new) {
+ new_iter->second->mutable_metadata()->AbortMutation();
new_tablets.erase(new_iter);
} else {
return Status::InvalidArgument(
@@ -1936,6 +1964,7 @@ Status CatalogManager::ApplyAlterPartitioningSteps(
for (auto& tablet : new_tablets) {
tablets_to_add->emplace_back(std::move(tablet.second));
}
+ abort_mutations.cancel();
return Status::OK();
}
@@ -2185,7 +2214,18 @@ Status CatalogManager::AlterTable(const AlterTableRequestPB* req,
// are impossible.
// * the new tablets can not heartbeat yet, since they don't get created
// until further down.
+ //
+ // We acquire new READ locks for tablets_to_add because we've already
+ // committed our WRITE locks above, and reordering the operations such that
+ // the WRITE locks could be reused would open a short window wherein
+ // uninitialized tablet state is published to the world.
+ for (const auto& tablet : tablets_to_add) {
+ tablet->metadata().ReadLock();
+ }
table->AddRemoveTablets(tablets_to_add, tablets_to_drop);
+ for (const auto& tablet : tablets_to_add) {
+ tablet->metadata().ReadUnlock();
+ }
// Commit state change for dropped tablets. This comes after removing the
// tablets from their associated tables so that if a GetTableLocations or
@@ -3828,10 +3868,19 @@ Status CatalogManager::ProcessPendingAssignments(
// Expose tablet metadata changes before the new tablets themselves.
committer_out.Commit();
committer_in.Commit();
+
+ for (const auto& t : deferred.tablets_to_add) {
+ // We can't reuse the WRITE tablet locks from committer_out for this
+ // because AddRemoveTablets() will read from the clean state, which is
+ // empty for these brand new tablets.
+ TabletMetadataLock l(t.get(), TabletMetadataLock::READ);
+ t->table()->AddRemoveTablets({ t }, {});
+ }
+
+ // Acquire the global lock to publish the new tablets.
{
std::lock_guard<LockType> l(lock_);
for (const auto& t : deferred.tablets_to_add) {
- t->table()->AddRemoveTablets({ t }, {});
tablet_map_[t->tablet_id()] = t;
}
}
http://git-wip-us.apache.org/repos/asf/kudu/blob/c0e18fd7/src/kudu/master/catalog_manager.h
----------------------------------------------------------------------
diff --git a/src/kudu/master/catalog_manager.h b/src/kudu/master/catalog_manager.h
index 47f8ba3..1f7dd91 100644
--- a/src/kudu/master/catalog_manager.h
+++ b/src/kudu/master/catalog_manager.h
@@ -240,6 +240,9 @@ class TableInfo : public RefCountedThreadSafe<TableInfo> {
const std::string& id() const { return table_id_; }
// Atomically add and remove multiple tablets from this table.
+ //
+ // Tablet locks in READ mode or greater must be held for all tablets to be
+ // added or dropped.
void AddRemoveTablets(const std::vector<scoped_refptr<TabletInfo>>& tablets_to_add,
const std::vector<scoped_refptr<TabletInfo>>& tablets_to_drop);
http://git-wip-us.apache.org/repos/asf/kudu/blob/c0e18fd7/src/kudu/util/cow_object.h
----------------------------------------------------------------------
diff --git a/src/kudu/util/cow_object.h b/src/kudu/util/cow_object.h
index ff83b5c..ca4877c 100644
--- a/src/kudu/util/cow_object.h
+++ b/src/kudu/util/cow_object.h
@@ -57,6 +57,7 @@ class CowObject {
// Abort the current mutation. This drops the write lock without applying any
// changes made to the mutable copy.
void AbortMutation() {
+ DCHECK(lock_.HasWriteLock());
dirty_state_.reset();
lock_.WriteUnlock();
}
@@ -65,6 +66,7 @@ class CowObject {
// blocks any concurrent readers or writers, swaps in the new version of the
// State, and then drops the commit lock.
void CommitMutation() {
+ DCHECK(lock_.HasWriteLock());
if (!dirty_state_) {
AbortMutation();
return;
@@ -97,6 +99,7 @@ class CowObject {
}
const State& dirty() const {
+ DCHECK(lock_.HasWriteLock());
if (!dirty_state_) {
return state_;
}
http://git-wip-us.apache.org/repos/asf/kudu/blob/c0e18fd7/src/kudu/util/rwc_lock.cc
----------------------------------------------------------------------
diff --git a/src/kudu/util/rwc_lock.cc b/src/kudu/util/rwc_lock.cc
index 0749822..ffe4cbb 100644
--- a/src/kudu/util/rwc_lock.cc
+++ b/src/kudu/util/rwc_lock.cc
@@ -35,14 +35,15 @@ RWCLock::RWCLock()
write_locked_(false) {
#else
write_locked_(false),
- last_writer_tid_(0),
+ writer_tid_(0),
last_writelock_acquire_time_(0) {
last_writer_backtrace_[0] = '\0';
#endif // NDEBUG
}
RWCLock::~RWCLock() {
- CHECK_EQ(reader_count_, 0);
+ DCHECK(!HasReaders());
+ DCHECK(!HasWriteLock());
}
void RWCLock::ReadLock() {
@@ -52,7 +53,7 @@ void RWCLock::ReadLock() {
void RWCLock::ReadUnlock() {
MutexLock l(lock_);
- DCHECK_GT(reader_count_, 0);
+ DCHECK(HasReadersUnlocked());
reader_count_--;
if (reader_count_ == 0) {
no_readers_.Signal();
@@ -61,13 +62,23 @@ void RWCLock::ReadUnlock() {
bool RWCLock::HasReaders() const {
MutexLock l(lock_);
+ return HasReadersUnlocked();
+}
+
+bool RWCLock::HasReadersUnlocked() const {
+ lock_.AssertAcquired();
return reader_count_ > 0;
}
bool RWCLock::HasWriteLock() const {
MutexLock l(lock_);
+ return HasWriteLockUnlocked();
+}
+
+bool RWCLock::HasWriteLockUnlocked() const {
+ lock_.AssertAcquired();
#ifndef NDEBUG
- return last_writer_tid_ == Thread::CurrentThreadId();
+ return writer_tid_ == Thread::CurrentThreadId();
#else
return write_locked_;
#endif
@@ -81,7 +92,7 @@ void RWCLock::WriteLock() {
}
#ifndef NDEBUG
last_writelock_acquire_time_ = GetCurrentTimeMicros();
- last_writer_tid_ = Thread::CurrentThreadId();
+ writer_tid_ = Thread::CurrentThreadId();
HexStackTraceToString(last_writer_backtrace_, kBacktraceBufSize);
#endif // NDEBUG
write_locked_ = true;
@@ -89,9 +100,10 @@ void RWCLock::WriteLock() {
void RWCLock::WriteUnlock() {
MutexLock l(lock_);
- DCHECK(write_locked_);
+ DCHECK(HasWriteLockUnlocked());
write_locked_ = false;
#ifndef NDEBUG
+ writer_tid_ = 0;
last_writer_backtrace_[0] = '\0';
#endif // NDEBUG
no_mutators_.Signal();
@@ -99,20 +111,22 @@ void RWCLock::WriteUnlock() {
void RWCLock::UpgradeToCommitLock() {
lock_.lock();
- DCHECK(write_locked_);
+ DCHECK(HasWriteLockUnlocked());
while (reader_count_ > 0) {
no_readers_.Wait();
}
- DCHECK(write_locked_);
+ DCHECK(HasWriteLockUnlocked());
// Leaves the lock held, which prevents any new readers
// or writers.
}
void RWCLock::CommitUnlock() {
- DCHECK_EQ(0, reader_count_);
+ DCHECK(!HasReadersUnlocked());
+ DCHECK(HasWriteLockUnlocked());
write_locked_ = false;
#ifndef NDEBUG
+ writer_tid_ = 0;
last_writer_backtrace_[0] = '\0';
#endif // NDEBUG
no_mutators_.Broadcast();
http://git-wip-us.apache.org/repos/asf/kudu/blob/c0e18fd7/src/kudu/util/rwc_lock.h
----------------------------------------------------------------------
diff --git a/src/kudu/util/rwc_lock.h b/src/kudu/util/rwc_lock.h
index 98c7965..7b78e35 100644
--- a/src/kudu/util/rwc_lock.h
+++ b/src/kudu/util/rwc_lock.h
@@ -115,6 +115,10 @@ class RWCLock {
void CommitUnlock();
private:
+ // Variants of the functions above that must be called with lock_ held.
+ bool HasReadersUnlocked() const;
+ bool HasWriteLockUnlocked() const;
+
// Lock which protects reader_count_ and write_locked_.
// Additionally, while the commit lock is held, the
// locking thread holds this mutex, which prevents any new
@@ -126,7 +130,7 @@ class RWCLock {
#ifndef NDEBUG
static const int kBacktraceBufSize = 1024;
- int64_t last_writer_tid_;
+ int64_t writer_tid_;
int64_t last_writelock_acquire_time_;
char last_writer_backtrace_[kBacktraceBufSize];
#endif // NDEBUG
[2/2] kudu git commit: KUDU-2137: protect against concurrent schema
version change and tablet drop
Posted by ad...@apache.org.
KUDU-2137: protect against concurrent schema version change and tablet drop
Try as I might, I could not reproduce the failure in the bug report. I
looped the failed test several thousand times. I also looped the entire
alter_table-test suite a thousand times. Finally, I wrote a unit test that
hammers one table with concurrent add column, add partition, and drop
partition operations. Nothing worked.
So, here's my best guess at what's going on: if a tablet is dropped
while the master is processing its report, it's conceivable that we could
wind up in TabletInfo::set_reported_schema_version() with the table spinlock
held just after TableInfo::AddRemoveTablets() dropped the tablet. This would
cause us to decrement the tablet's "old" schema version from the table's
count map twice: once when dropping the tablet and a second time in
set_reported_schema_version().
The fix is straight-forward: after acquiring both spinlocks, double check
that the tablet is still a member of the table. We need to take the tablet
metadata lock to do this, but the saving grace is that tablet partition keys
are immutable, so this lock acquisition needn't overlap with the spinlocks.
Change-Id: I371fc310a97ae94ec2ebf04405db99c5f2937e1a
Reviewed-on: http://gerrit.cloudera.org:8080/7996
Tested-by: Kudu Jenkins
Reviewed-by: Dan Burkert <da...@apache.org>
Reviewed-by: Alexey Serbin <as...@cloudera.com>
Project: http://git-wip-us.apache.org/repos/asf/kudu/repo
Commit: http://git-wip-us.apache.org/repos/asf/kudu/commit/9f064944
Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/9f064944
Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/9f064944
Branch: refs/heads/master
Commit: 9f064944816f154af079713adc79a0dff8270742
Parents: c0e18fd
Author: Adar Dembo <ad...@cloudera.com>
Authored: Thu Sep 7 02:06:23 2017 -0700
Committer: Adar Dembo <ad...@cloudera.com>
Committed: Fri Sep 8 22:54:06 2017 +0000
----------------------------------------------------------------------
src/kudu/master/catalog_manager.cc | 23 +++++++++++++++++++++--
1 file changed, 21 insertions(+), 2 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/kudu/blob/9f064944/src/kudu/master/catalog_manager.cc
----------------------------------------------------------------------
diff --git a/src/kudu/master/catalog_manager.cc b/src/kudu/master/catalog_manager.cc
index 509f3c5..4619129 100644
--- a/src/kudu/master/catalog_manager.cc
+++ b/src/kudu/master/catalog_manager.cc
@@ -4326,8 +4326,17 @@ void TabletInfo::set_reported_schema_version(int64_t version) {
// Slow path: we have a schema version change.
//
- // We need to hold both the table and tablet locks to make the change. By
- // convention, the table lock is always acquired first.
+ // We need to hold both the table and tablet spinlocks to make the change. By
+ // convention, table locks are always acquired first.
+ //
+ // We also need to hold the tablet metadata lock in order to read the partition
+ // key, but it's OK to make a local copy of it (and release the lock) because
+ // the key is immutable.
+ string key_start;
+ {
+ TabletMetadataLock l(this, TabletMetadataLock::READ);
+ key_start = l.data().pb.partition().partition_key_start();
+ }
std::lock_guard<rw_spinlock> table_l(table_->lock_);
std::lock_guard<simple_spinlock> tablet_l(lock_);
@@ -4337,6 +4346,16 @@ void TabletInfo::set_reported_schema_version(int64_t version) {
return;
}
+ // Check that we weren't dropped from the table before acquiring the table lock.
+ //
+ // We also have to compare the returned object to 'this' in case our entry in
+ // the map was replaced with a new tablet (i.e. DROP RANGE PARTITION followed
+ // by ADD RANGE PARTITION).
+ auto* t = FindPtrOrNull(table_->tablet_map_, key_start);
+ if (!t || t != this) {
+ return;
+ }
+
// Perform the changes.
VLOG(3) << Substitute("$0: schema version changed from $1 to $2",
ToString(), old_version, version);