Copilot commented on code in PR #66019:
URL: https://github.com/apache/doris/pull/66019#discussion_r3645686948
##########
be/src/cloud/cloud_meta_mgr.cpp:
##########
@@ -514,7 +514,7 @@ Status CloudMetaMgr::_log_mow_delete_bitmap(CloudTablet*
tablet, GetRowsetRespon
std::vector<RowsetSharedPtr> old_rowsets;
RowsetIdUnorderedSet old_rowset_ids;
{
- std::lock_guard<std::shared_mutex>
rlock(tablet->get_header_lock());
+ std::lock_guard rlock(tablet->get_header_lock());
Review Comment:
This acquires the header lock exclusively (`lock_guard` calls `lock()`), but
the block appears to be read-only (`get_all_rs_id_unlocked` +
`get_rowset_by_ids`). With the new writer-preferring RW lock, using an
exclusive lock here can unnecessarily block other readers and increase
contention. Prefer `std::shared_lock` on `tablet->get_header_lock()` when only
shared access is needed.
##########
be/src/util/bthread_shared_mutex.h:
##########
@@ -0,0 +1,125 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#pragma once
+
+#include <bthread/condition_variable.h>
+#include <bthread/mutex.h>
+
+#include <climits>
+#include <mutex>
+
+namespace doris {
+
+// A reader-writer lock for bthread contexts. It is a port of libc++'s
+// std::shared_mutex (the two-gate condition-variable algorithm) onto
+// bthread::Mutex/bthread::ConditionVariable. Unlike std::shared_mutex
+// (pthread_rwlock_t), ownership carries no OS-thread identity, so it is safe
to
+// lock on one bthread worker and unlock on another after a bthread migrates.
+// Satisfies the C++ SharedMutex requirements (usable with std::unique_lock /
+// std::shared_lock). Writer-preferring.
+class BthreadSharedMutex {
+public:
+ BthreadSharedMutex() = default;
+ ~BthreadSharedMutex() = default;
+
+ BthreadSharedMutex(const BthreadSharedMutex&) = delete;
+ BthreadSharedMutex& operator=(const BthreadSharedMutex&) = delete;
+
+ void lock() {
+ std::unique_lock<bthread::Mutex> lk(_mutex);
+ while (_state & _write_entered) {
+ _gate1.wait(lk);
+ }
+ _state |= _write_entered;
+ while (_state & _n_readers) {
+ _gate2.wait(lk);
+ }
+ }
+
+ bool try_lock() {
+ std::unique_lock<bthread::Mutex> lk(_mutex);
+ if (_state == 0) {
+ _state = _write_entered;
+ return true;
+ }
+ return false;
+ }
+
+ void unlock() {
+ std::lock_guard<bthread::Mutex> lk(_mutex);
+ _state = 0;
+ // Notify while still holding `_mutex`. Releasing the mutex before
+ // notifying can lose a wakeup with bthread::ConditionVariable when the
+ // waiter is a pthread and the notifier is a bthread (or vice versa).
+ _gate1.notify_all();
+ }
+
+ void lock_shared() {
+ std::unique_lock<bthread::Mutex> lk(_mutex);
+ while ((_state & _write_entered) || (_state & _n_readers) ==
_n_readers) {
+ _gate1.wait(lk);
+ }
+ unsigned num_readers = (_state & _n_readers) + 1;
+ _state &= ~_n_readers;
+ _state |= num_readers;
+ }
+
+ bool try_lock_shared() {
+ std::unique_lock<bthread::Mutex> lk(_mutex);
+ unsigned num_readers = _state & _n_readers;
+ if (!(_state & _write_entered) && num_readers != _n_readers) {
+ ++num_readers;
+ _state &= ~_n_readers;
+ _state |= num_readers;
+ return true;
+ }
+ return false;
+ }
+
+ void unlock_shared() {
+ std::unique_lock<bthread::Mutex> lk(_mutex);
+ unsigned num_readers = (_state & _n_readers) - 1;
+ _state &= ~_n_readers;
+ _state |= num_readers;
+ // Notify while still holding `_mutex` (do not unlock first): with
+ // bthread::ConditionVariable a notify issued after releasing the mutex
+ // can be lost across pthread/bthread contexts, leaving a writer parked
+ // forever on a reader count that is already zero.
+ if (_state & _write_entered) {
+ if (num_readers == 0) {
+ _gate2.notify_one();
+ }
+ } else {
+ if (num_readers == _n_readers - 1) {
+ _gate1.notify_one();
+ }
+ }
+ }
Review Comment:
`unlock_shared()` will underflow if called when the reader count is 0
(`(_state & _n_readers) - 1` wraps to a large value). Even if callers are
expected to be correct, this can turn a misuse into a permanent wedge or very
hard-to-debug corruption. Add a defensive invariant check (e.g., DCHECK that
the reader count is > 0, and optionally that `_write_entered` isn’t the only
bit set in a way that violates expectations) before decrementing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
