On Sun, Jul 7, 2019 at 11:32 PM kefu chai <tchaikov@gmail.com> wrote:

hi Patrick and list,

i am trying replace Mutex with ceph::mutex in Ceph. the goal is to deprecate and then remove Mutex and Cond from our project. as we have already ceph::mutex and ceph::condition_variable. it's confusing and, in the long run, i think, it will hurt us as a technical debt.

most of the refactory work is quite straightforward, and we can always replace Mutex::is_locked_by_me() with ceph_mutex_is_locked_by_me(). this macro will be expanded to `true` in release build. as we think it will be used only by `ceph_assert()` and `assert()`.

but i realized that we are also using Mutex::is_locked_by_me() in functional code which won't be optimised out. for instance, in MDSRank::MDSRank() we check "mds_lock.is_locked_by_me()", if it's true, we just `handle_write_error(r)` without acquiring the lock, otherwise, we will call this function within `mds_lock`. the same applies to `MDSDaemon::handle_conf_change()`. so i am wondering if it's okay to change `mds_lock` to a recursive lock, so we don't need to query this information from the mutex.

what do you think?

handle_conf_change should be easy enough to fix by using a finisher context like we do for the Monitor. handle_write_error() is trickier because we can't easily pass the locker down the stack since it's a callback from the osdc/Journaler. I think we can find a way to avoid this issue via some reworking; a recursive mutex is not really desirable to resolve this one edge-case. I'll create a tracker for this. -- Patrick Donnelly, Ph.D. He / Him / His Senior Software Engineer Red Hat Sunnyvale, CA GPG: 19F28A586F808C2402351B93C3301A3E258DD79D