librbd: fix a deadlock on image_lock caused by Mirror::image_disable()

With Mirror::image_disable() taking image_lock for write and calling
list_children() under it, the following deadlock is possible:

1. Mirror::image_disable() takes image_lock for write and calls
   list_children()
2. AbstractWriteLog::periodic_stats() timer fires (it runs every
   5 seconds) and ImageCacheState::write_image_cache_state() is called
   under a global timer_lock
3. ImageCacheState::write_image_cache_state() successfully takes
   owner_lock and blocks attempting to take image_lock for read because
   it's already held for write by Mirror::image_disable()
4. list_children() blocks inside of a call to ImageState::close() on
   a descendant image
5. The descendant image close can't proceed because TokenBucketThrottle
   requires a global timer_lock to complete QosImageDispatch shutdown
6. safe_timer thread which is holding timer_lock can't proceed because
   ImageCacheState::write_image_cache_state() is effectively blocked on
   the descendant image close through Mirror::image_disable()

Until commit 281a64acf920 ("librbd: remove snapshot mirror image-meta
when disabling"), Mirror::image_disable() was taking image_lock only for
read meaning that this deadlock wasn't possible.  The only other change
that commit 281a64acf920 made to the code block protected by image_lock
was using child_mirror_image_internal for cls_client::mirror_image_get()
call on descendant images instead of mirror_image_internal to preserve
the value of mirror_image_internal for later.  Both are local variables
that have nothing to do with image_lock, so I'm going back and making
Mirror::image_disable() take image_lock only for read again.

Fixes: https://tracker.ceph.com/issues/70190
Signed-off-by: Ilya Dryomov <idryomov@gmail.com>

diff --git a/src/librbd/api/Mirror.cc b/src/librbd/api/Mirror.cc
index 910d3e4fba83fa8e1ca2a3552446d93d4ba325d1..2f8eef1ae9a6592a69515bba25d726f9a5d87a23 100644 (file)
--- a/src/librbd/api/Mirror.cc
+++ b/src/librbd/api/Mirror.cc
@@ -574,9 +574,8 @@ int Mirror<I>::image_disable(I *ictx, bool force) {
     }
   };
 
-  std::unique_lock image_locker{ictx->image_lock};
-  std::map<librados::snap_t, SnapInfo> snap_info = ictx->snap_info;
-  for (auto &info : snap_info) {
+  std::shared_lock image_locker{ictx->image_lock};
+  for (const auto& info : ictx->snap_info) {
     cls::rbd::ParentImageSpec parent_spec{ictx->md_ctx.get_id(),
                                           ictx->md_ctx.get_namespace(),
                                           ictx->id, info.first};