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