Skip to content

Commit 0b207d0

Browse files
committed
rbd: take header_rwsem in rbd_dev_refresh() only when updating
rbd_dev_refresh() has been holding header_rwsem across header and parent info read-in unnecessarily for ages. With commit 870611e ("rbd: get snapshot context after exclusive lock is ensured to be held"), the potential for deadlocks became much more real owning to a) header_rwsem now nesting inside lock_rwsem and b) rw_semaphores not allowing new readers after a writer is registered. For example, assuming that I/O request 1, I/O request 2 and header read-in request all target the same OSD: 1. I/O request 1 comes in and gets submitted 2. watch error occurs 3. rbd_watch_errcb() takes lock_rwsem for write, clears owner_cid and releases lock_rwsem 4. after reestablishing the watch, rbd_reregister_watch() calls rbd_dev_refresh() which takes header_rwsem for write and submits a header read-in request 5. I/O request 2 comes in: after taking lock_rwsem for read in __rbd_img_handle_request(), it blocks trying to take header_rwsem for read in rbd_img_object_requests() 6. another watch error occurs 7. rbd_watch_errcb() blocks trying to take lock_rwsem for write 8. I/O request 1 completion is received by the messenger but can't be processed because lock_rwsem won't be granted anymore 9. header read-in request completion can't be received, let alone processed, because the messenger is stranded Change rbd_dev_refresh() to take header_rwsem only for actually updating rbd_dev->header. Header and parent info read-in don't need any locking. Cc: stable@vger.kernel.org # 0b03540: rbd: move rbd_dev_refresh() definition Cc: stable@vger.kernel.org # 510a733: rbd: decouple header read-in from updating rbd_dev->header Cc: stable@vger.kernel.org # c103117: rbd: decouple parent info read-in from updating rbd_dev Cc: stable@vger.kernel.org Fixes: 870611e ("rbd: get snapshot context after exclusive lock is ensured to be held") Signed-off-by: Ilya Dryomov <idryomov@gmail.com> Reviewed-by: Dongsheng Yang <dongsheng.yang@easystack.cn>
1 parent c103117 commit 0b207d0

File tree

1 file changed

+11
-11
lines changed

1 file changed

+11
-11
lines changed

drivers/block/rbd.c

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -6986,7 +6986,14 @@ static void rbd_dev_update_header(struct rbd_device *rbd_dev,
69866986
rbd_assert(rbd_image_format_valid(rbd_dev->image_format));
69876987
rbd_assert(rbd_dev->header.object_prefix); /* !first_time */
69886988

6989-
rbd_dev->header.image_size = header->image_size;
6989+
if (rbd_dev->header.image_size != header->image_size) {
6990+
rbd_dev->header.image_size = header->image_size;
6991+
6992+
if (!rbd_is_snap(rbd_dev)) {
6993+
rbd_dev->mapping.size = header->image_size;
6994+
rbd_dev_update_size(rbd_dev);
6995+
}
6996+
}
69906997

69916998
ceph_put_snap_context(rbd_dev->header.snapc);
69926999
rbd_dev->header.snapc = header->snapc;
@@ -7044,11 +7051,9 @@ static int rbd_dev_refresh(struct rbd_device *rbd_dev)
70447051
{
70457052
struct rbd_image_header header = { 0 };
70467053
struct parent_image_info pii = { 0 };
7047-
u64 mapping_size;
70487054
int ret;
70497055

7050-
down_write(&rbd_dev->header_rwsem);
7051-
mapping_size = rbd_dev->mapping.size;
7056+
dout("%s rbd_dev %p\n", __func__, rbd_dev);
70527057

70537058
ret = rbd_dev_header_info(rbd_dev, &header, false);
70547059
if (ret)
@@ -7064,18 +7069,13 @@ static int rbd_dev_refresh(struct rbd_device *rbd_dev)
70647069
goto out;
70657070
}
70667071

7072+
down_write(&rbd_dev->header_rwsem);
70677073
rbd_dev_update_header(rbd_dev, &header);
70687074
if (rbd_dev->parent)
70697075
rbd_dev_update_parent(rbd_dev, &pii);
7070-
7071-
rbd_assert(!rbd_is_snap(rbd_dev));
7072-
rbd_dev->mapping.size = rbd_dev->header.image_size;
7073-
7074-
out:
70757076
up_write(&rbd_dev->header_rwsem);
7076-
if (!ret && mapping_size != rbd_dev->mapping.size)
7077-
rbd_dev_update_size(rbd_dev);
70787077

7078+
out:
70797079
rbd_parent_info_cleanup(&pii);
70807080
rbd_image_header_cleanup(&header);
70817081
return ret;

0 commit comments

Comments
 (0)