rbd: take header_rwsem in rbd_dev_refresh() only when updating (6e37de4a) · Commits · e / devices / android_kernel_fairphone_FP5

commit 0b207d02bd9ab8dcc31b262ca9f60dbc1822500d upstream. rbd_dev_refresh() has been holding header_rwsem across header and parent info read-in unnecessarily for ages. With commit 870611e4877e ("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 # 0b035401c570: rbd: move rbd_dev_refresh() definition Cc: stable@vger.kernel.org # 510a7330c82a: rbd: decouple header read-in from updating rbd_dev->header Cc: stable@vger.kernel.org # c10311776f0a: rbd: decouple parent info read-in from updating rbd_dev Cc: stable@vger.kernel.org Fixes: 870611e4877e ("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> [idryomov@gmail.com: backport to 5.4: open-code rbd_is_snap(), preserve rbd_exists_validate() call] Signed-off-by:

Sasha Levin <sashal@kernel.org>

drivers/block/rbd.c

+16 −15

Original line number	Original line	Diff line number	Diff line
	@@ -7001,8 +7001,20 @@ static void rbd_dev_update_header(struct rbd_device *rbd_dev,
	rbd_assert(rbd_image_format_valid(rbd_dev->image_format));		rbd_assert(rbd_image_format_valid(rbd_dev->image_format));
	rbd_assert(rbd_dev->header.object_prefix); /* !first_time */		rbd_assert(rbd_dev->header.object_prefix); /* !first_time */

			if (rbd_dev->header.image_size != header->image_size) {
	rbd_dev->header.image_size = header->image_size;		rbd_dev->header.image_size = header->image_size;

			if (rbd_dev->spec->snap_id == CEPH_NOSNAP) {
			rbd_dev->mapping.size = header->image_size;
			rbd_dev_update_size(rbd_dev);
			}
			}

			if (rbd_dev->spec->snap_id != CEPH_NOSNAP) {
			/* validate mapped snapshot's EXISTS flag */
			rbd_exists_validate(rbd_dev);
			}

	ceph_put_snap_context(rbd_dev->header.snapc);		ceph_put_snap_context(rbd_dev->header.snapc);
	rbd_dev->header.snapc = header->snapc;		rbd_dev->header.snapc = header->snapc;
	header->snapc = NULL;		header->snapc = NULL;
	@@ -7059,11 +7071,9 @@ static int rbd_dev_refresh(struct rbd_device *rbd_dev)
	{		{
	struct rbd_image_header header = { 0 };		struct rbd_image_header header = { 0 };
	struct parent_image_info pii = { 0 };		struct parent_image_info pii = { 0 };
	u64 mapping_size;
	int ret;		int ret;

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

	ret = rbd_dev_header_info(rbd_dev, &header, false);		ret = rbd_dev_header_info(rbd_dev, &header, false);
	if (ret)		if (ret)
	@@ -7079,22 +7089,13 @@ static int rbd_dev_refresh(struct rbd_device *rbd_dev)
	goto out;		goto out;
	}		}

			down_write(&rbd_dev->header_rwsem);
	rbd_dev_update_header(rbd_dev, &header);		rbd_dev_update_header(rbd_dev, &header);
	if (rbd_dev->parent)		if (rbd_dev->parent)
	rbd_dev_update_parent(rbd_dev, &pii);		rbd_dev_update_parent(rbd_dev, &pii);

	if (rbd_dev->spec->snap_id == CEPH_NOSNAP) {
	rbd_dev->mapping.size = rbd_dev->header.image_size;
	} else {
	/* validate mapped snapshot's EXISTS flag */
	rbd_exists_validate(rbd_dev);
	}

	out:
	up_write(&rbd_dev->header_rwsem);		up_write(&rbd_dev->header_rwsem);
	if (!ret && mapping_size != rbd_dev->mapping.size)
	rbd_dev_update_size(rbd_dev);

			out:
	rbd_parent_info_cleanup(&pii);		rbd_parent_info_cleanup(&pii);
	rbd_image_header_cleanup(&header);		rbd_image_header_cleanup(&header);
	return ret;		return ret;