Skip to content

Commit a02cc13

Browse files
committed
librbd/migration/QCOWFormat: don't complete read_clusters() inline
When the cluster needs to be read, the completion is posted to ASIO. However, in the two special cases (cluster DNE and zero cluster), the completion is completed inline at the moment. This violates invariants and can eventually lead to a lockup. For example, in a scenario of a read from a clone image whose parent is under migration: io::ObjectReadRequest::read_parent() io::util::read_parent() < image_lock is taken for read > io::ImageDispatchSpec::send() migration::ImageDispatch::read() migration::QCOWFormat::ReadRequest::send() ... migration::QCOWFormat::ReadRequest::read_clusters() < cluster DNE > migration::QCOWFormat::ReadRequest::handle_read_clusters() io::AioCompletion::complete() io::ObjectReadRequest::copyup() is_copy_on_read() < image_lock is taken for read > copyup() expects to be called with no locks held, but going through QCOWFormat in the "cluster DNE" case essentially maintains image_lock taken in read_parent() and then it's taken again by the same thread in is_copy_on_read(). Under pthreads, it's not a problem: A thread may hold multiple concurrent read locks on rwlock (that is, successfully call the pthread_rwlock_rdlock() function n times). If so, the thread must perform matching unlocks (that is, it must call the pthread_rwlock_unlock() function n times). But according to C++ standard it's undefined behavior: If lock_shared is called by a thread that already owns the mutex in any mode (exclusive or shared), the behavior is undefined. Other, longer and more elaborate, call chains are possible too and there it may end up being a write lock, a tripped assertion, etc. To avoid this, make the special cases in read_clusters() behave the same as the main path. Fixes: https://tracker.ceph.com/issues/71838 Signed-off-by: Ilya Dryomov <[email protected]>
1 parent 9d9c734 commit a02cc13

File tree

1 file changed

+4
-2
lines changed

1 file changed

+4
-2
lines changed

src/librbd/migration/QCOWFormat.cc

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -641,11 +641,13 @@ class QCOWFormat<I>::ReadRequest {
641641

642642
if (cluster_extent.cluster_offset == 0) {
643643
// QCOW header is at offset 0, implies cluster DNE
644-
log_ctx->complete(-ENOENT);
644+
boost::asio::post(*qcow_format->m_image_ctx->asio_engine,
645+
[log_ctx] { log_ctx->complete(-ENOENT); });
645646
} else if (cluster_extent.cluster_offset == QCOW_OFLAG_ZERO) {
646647
// explicitly zeroed section
647648
read_ctx->bl.append_zero(cluster_extent.cluster_length);
648-
log_ctx->complete(0);
649+
boost::asio::post(*qcow_format->m_image_ctx->asio_engine,
650+
[log_ctx] { log_ctx->complete(0); });
649651
} else {
650652
// request the (sub)cluster from the cluster cache
651653
qcow_format->m_cluster_cache->get_cluster(

0 commit comments

Comments
 (0)