79 lines
3.3 KiB
Diff
79 lines
3.3 KiB
Diff
|
From 6348063b91b2370cc27153fd58fd11a6681631f6 Mon Sep 17 00:00:00 2001
|
|||
|
|
From: Hanna Reitz <hreitz@redhat.com>
|
|||
|
|
Date: Wed, 16 Feb 2022 11:53:53 +0100
|
|||
|
|
Subject: [PATCH 22/24] block: Make bdrv_refresh_limits() non-recursive
|
|||
|
|
MIME-Version: 1.0
|
|||
|
|
Content-Type: text/plain; charset=UTF-8
|
|||
|
|
Content-Transfer-Encoding: 8bit
|
|||
|
|
|
|||
|
|
RH-Author: Hanna Reitz <hreitz@redhat.com>
|
|||
|
|
RH-MergeRequest: 189: block: Make bdrv_refresh_limits() non-recursive
|
|||
|
|
RH-Commit: [1/3] 1a1fe37f8d8f0344dd8639d6cc9d884d1aff9096
|
|||
|
|
RH-Bugzilla: 2072932
|
|||
|
|
RH-Acked-by: Eric Blake <eblake@redhat.com>
|
|||
|
|
RH-Acked-by: Stefan Hajnoczi <stefanha@redhat.com>
|
|||
|
|
RH-Acked-by: Kevin Wolf <kwolf@redhat.com>
|
|||
|
|
|
|||
|
|
bdrv_refresh_limits() recurses down to the node's children. That does
|
|||
|
|
not seem necessary: When we refresh limits on some node, and then
|
|||
|
|
recurse down and were to change one of its children's BlockLimits, then
|
|||
|
|
that would mean we noticed the changed limits by pure chance. The fact
|
|||
|
|
that we refresh the parent's limits has nothing to do with it, so the
|
|||
|
|
reason for the change probably happened before this point in time, and
|
|||
|
|
we should have refreshed the limits then.
|
|||
|
|
|
|||
|
|
Consequently, we should actually propagate block limits changes upwards,
|
|||
|
|
not downwards. That is a separate and pre-existing issue, though, and
|
|||
|
|
so will not be addressed in this patch.
|
|||
|
|
|
|||
|
|
The problem with recursing is that bdrv_refresh_limits() is not atomic.
|
|||
|
|
It begins with zeroing BDS.bl, and only then sets proper, valid limits.
|
|||
|
|
If we do not drain all nodes whose limits are refreshed, then concurrent
|
|||
|
|
I/O requests can encounter invalid request_alignment values and crash
|
|||
|
|
qemu. Therefore, a recursing bdrv_refresh_limits() requires the whole
|
|||
|
|
subtree to be drained, which is currently not ensured by most callers.
|
|||
|
|
|
|||
|
|
A non-recursive bdrv_refresh_limits() only requires the node in question
|
|||
|
|
to not receive I/O requests, and this is done by most callers in some
|
|||
|
|
way or another:
|
|||
|
|
- bdrv_open_driver() deals with a new node with no parents yet
|
|||
|
|
- bdrv_set_file_or_backing_noperm() acts on a drained node
|
|||
|
|
- bdrv_reopen_commit() acts only on drained nodes
|
|||
|
|
- bdrv_append() should in theory require the node to be drained; in
|
|||
|
|
practice most callers just lock the AioContext, which should at least
|
|||
|
|
be enough to prevent concurrent I/O requests from accessing invalid
|
|||
|
|
limits
|
|||
|
|
|
|||
|
|
So we can resolve the bug by making bdrv_refresh_limits() non-recursive.
|
|||
|
|
|
|||
|
|
Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1879437
|
|||
|
|
Signed-off-by: Hanna Reitz <hreitz@redhat.com>
|
|||
|
|
Reviewed-by: Eric Blake <eblake@redhat.com>
|
|||
|
|
Message-Id: <20220216105355.30729-2-hreitz@redhat.com>
|
|||
|
|
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
|
|||
|
|
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
|
|||
|
|
(cherry picked from commit 4d378bbd831bdd2f6e6adcd4ea5b77b6effaa627)
|
|||
|
|
Signed-off-by: Hanna Reitz <hreitz@redhat.com>
|
|||
|
|
---
|
|||
|
|
block/io.c | 4 ----
|
|||
|
|
1 file changed, 4 deletions(-)
|
|||
|
|
|
|||
|
|
diff --git a/block/io.c b/block/io.c
|
|||
|
|
index 4e4cb556c5..c3e7301613 100644
|
|||
|
|
--- a/block/io.c
|
|||
|
|
+++ b/block/io.c
|
|||
|
|
@@ -189,10 +189,6 @@ void bdrv_refresh_limits(BlockDriverState *bs, Transaction *tran, Error **errp)
|
|||
|
|
QLIST_FOREACH(c, &bs->children, next) {
|
|||
|
|
if (c->role & (BDRV_CHILD_DATA | BDRV_CHILD_FILTERED | BDRV_CHILD_COW))
|
|||
|
|
{
|
|||
|
|
- bdrv_refresh_limits(c->bs, tran, errp);
|
|||
|
|
- if (*errp) {
|
|||
|
|
- return;
|
|||
|
|
- }
|
|||
|
|
bdrv_merge_limits(&bs->bl, &c->bs->bl);
|
|||
|
|
have_limits = true;
|
|||
|
|
}
|
|||
|
|
--
|
|||
|
|
2.35.3
|
|||
|
|
|