From 0cdc93fbdcf68a31e6aada38b0cb9d66efdc512d Mon Sep 17 00:00:00 2001 From: Jason Dillaman Date: Mon, 16 Mar 2015 11:04:22 -0400 Subject: [PATCH] librbd: acquire cache_lock before refreshing parent cache_lock needs to be acquired before snap_lock to avoid the potential for deadlock. Fixes: #5488 Signed-off-by: Jason Dillaman (cherry picked from commit 703ba377e3de4007920f2ed7d8a0780f68676fe2) Conflicts: src/librbd/internal.cc resolved by moving int r; in the scope of the block --- src/librbd/ImageCtx.cc | 3 +-- src/librbd/internal.cc | 30 ++++++++++++++++++++++-------- 2 files changed, 23 insertions(+), 10 deletions(-) diff --git a/src/librbd/ImageCtx.cc b/src/librbd/ImageCtx.cc index 564f3437f0bd9..3325d73e0ee5b 100644 --- a/src/librbd/ImageCtx.cc +++ b/src/librbd/ImageCtx.cc @@ -710,11 +710,10 @@ namespace librbd { } void ImageCtx::clear_nonexistence_cache() { + assert(cache_lock.is_locked()); if (!object_cacher) return; - cache_lock.Lock(); object_cacher->clear_nonexistence(object_set); - cache_lock.Unlock(); } int ImageCtx::register_watch() { diff --git a/src/librbd/internal.cc b/src/librbd/internal.cc index c356c79fe3b5e..75d28a45990fa 100644 --- a/src/librbd/internal.cc +++ b/src/librbd/internal.cc @@ -1349,6 +1349,10 @@ reprotect_and_return_err: int open_parent(ImageCtx *ictx) { + assert(ictx->cache_lock.is_locked()); + assert(ictx->snap_lock.is_wlocked()); + assert(ictx->parent_lock.is_wlocked()); + string pool_name; Rados rados(ictx->md_ctx); @@ -1392,11 +1396,13 @@ reprotect_and_return_err: return r; } + ictx->parent->cache_lock.Lock(); ictx->parent->snap_lock.get_write(); r = ictx->parent->get_snap_name(parent_snap_id, &ictx->parent->snap_name); if (r < 0) { lderr(ictx->cct) << "parent snapshot does not exist" << dendl; ictx->parent->snap_lock.put_write(); + ictx->parent->cache_lock.Unlock(); close_image(ictx->parent); ictx->parent = NULL; return r; @@ -1410,12 +1416,14 @@ reprotect_and_return_err: << ictx->parent->snap_name << dendl; ictx->parent->parent_lock.put_write(); ictx->parent->snap_lock.put_write(); + ictx->parent->cache_lock.Unlock(); close_image(ictx->parent); ictx->parent = NULL; return r; } ictx->parent->parent_lock.put_write(); ictx->parent->snap_lock.put_write(); + ictx->parent->cache_lock.Unlock(); return 0; } @@ -1853,6 +1861,10 @@ reprotect_and_return_err: } int refresh_parent(ImageCtx *ictx) { + assert(ictx->cache_lock.is_locked()); + assert(ictx->snap_lock.is_wlocked()); + assert(ictx->parent_lock.is_wlocked()); + // close the parent if it changed or this image no longer needs // to read from it int r; @@ -1904,10 +1916,11 @@ reprotect_and_return_err: vector snap_protection; vector snap_flags; { - int r; - RWLock::WLocker l(ictx->snap_lock); + Mutex::Locker cache_locker(ictx->cache_lock); + RWLock::WLocker snap_locker(ictx->snap_lock); { - RWLock::WLocker l2(ictx->parent_lock); + int r; + RWLock::WLocker parent_locker(ictx->parent_lock); ictx->lockers.clear(); if (ictx->old_format) { r = read_header(ictx->md_ctx, ictx->header_oid, &ictx->header, NULL); @@ -2058,7 +2071,7 @@ reprotect_and_return_err: ictx->object_map.refresh(ictx->snap_id); ictx->data_ctx.selfmanaged_snap_set_write_ctx(ictx->snapc.seq, ictx->snaps); - } // release snap_lock + } // release snap_lock and cache_lock if (new_snap) { _flush(ictx); @@ -2308,10 +2321,11 @@ reprotect_and_return_err: int _snap_set(ImageCtx *ictx, const char *snap_name) { - RWLock::WLocker l(ictx->owner_lock); - RWLock::RLocker l1(ictx->md_lock); - RWLock::WLocker l2(ictx->snap_lock); - RWLock::WLocker l3(ictx->parent_lock); + RWLock::WLocker owner_locker(ictx->owner_lock); + RWLock::RLocker md_locker(ictx->md_lock); + Mutex::Locker cache_locker(ictx->cache_lock); + RWLock::WLocker snap_locker(ictx->snap_lock); + RWLock::WLocker parent_locker(ictx->parent_lock); int r; if ((snap_name != NULL) && (strlen(snap_name) != 0)) { r = ictx->snap_set(snap_name); -- 2.39.5