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