Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12075 )
Change subject: KUDU-2636: LBM supports deleting dead and full containers ...................................................................... Patch Set 5: (1 comment) http://gerrit.cloudera.org:8080/#/c/12075/5/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: http://gerrit.cloudera.org:8080/#/c/12075/5/src/kudu/fs/log_block_manager.cc@1632 PS5, Line 1632: log_block_.reset(); : if (container_->metrics()) { : container_->metrics()->generic_metrics.blocks_open_reading->Decrement(); : } I think this may be the source of the TSAN data race. Given the ordering of these operations, log_block_.reset() may cause the LogBlock() to lose its last reference and be destroyed. This may cause a registered LogBlockDeletionTransaction to lose its last reference and be destroyed, which will trigger the asynchronous code that will eventually destroy the container. Thus, this access to container_ is unsafe. It should be fixable by just swapping the order of the two operations here. Please also leave a comment with this example that explains how, once log_block_.reset() is called, it's no longer safe to access container_. In fact we may want to null out container_ to further emphasize this (if someone makes this mistake in the future, better to get a SIGSEGV on a 0x0 memory access than a data race). -- To view, visit http://gerrit.cloudera.org:8080/12075 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7d3672ae3c3dd9acd489120d653c44a641537f10 Gerrit-Change-Number: 12075 Gerrit-PatchSet: 5 Gerrit-Owner: helifu <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: helifu <[email protected]> Gerrit-Comment-Date: Tue, 18 Dec 2018 22:13:49 +0000 Gerrit-HasComments: Yes
