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
|
||
|