The following changes since commit 508ba0f7e2092d3ca56e3f75e894d52d8b94818e:

Merge remote-tracking branch 'remotes/cohuck/tags/s390x-20171109' into staging (2017-11-13 11:41:47 +0000)

for you to fetch changes up to 0761562687e0d8135310a94b1d3e08376387c027:

qemu-iotests: Test I/O limits with removable media (2017-11-13 15:46:26 +0000)

----------------------------------------------------------------

Pull request

The following disk I/O throttling fixes solve recent bugs.

Alberto Garcia (3):

block: Check for inserted BlockDriverState in blk_io_limits_disable()

block: Leave valid throttle timers when removing a BDS from a backend

qemu-iotests: Test I/O limits with removable media

Stefan Hajnoczi (1):

throttle-groups: drain before detaching ThrottleState

Zhengui (1):

block: all I/O should be completed before removing throttle timers.

block/block-backend.c | 36 ++++++++++++++++++---------

block/throttle-groups.c | 6 +++++

tests/qemu-iotests/093 | 62 ++++++++++++++++++++++++++++++++++++++++++++++

tests/qemu-iotests/093.out | 4 +--

4 files changed, 94 insertions(+), 14 deletions(-)

2.13.6

From: Zhengui <lizhengui@huawei.com>

In blk_remove_bs, all I/O should be completed before removing throttle

timers. If there has inflight I/O, removing throttle timers here will

cause the inflight I/O never return.

This patch add bdrv_drained_begin before throttle_timers_detach_aio_context

to let all I/O completed before removing throttle timers.

[Moved declaration of bs as suggested by Alberto Garcia

<berto@igalia.com>.

--Stefan]

Signed-off-by: Zhengui <lizhengui@huawei.com>

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

Reviewed-by: Alberto Garcia <berto@igalia.com>

Message-id: 1508564040-120700-1-git-send-email-lizhengui@huawei.com

block/block-backend.c | 4 ++++

1 file changed, 4 insertions(+)

diff --git a/block/block-backend.c b/block/block-backend.c

--- a/block/block-backend.c

+++ b/block/block-backend.c

@@ -XXX,XX +XXX,XX @@ BlockBackend *blk_by_public(BlockBackendPublic *public)

*/

void blk_remove_bs(BlockBackend *blk)

{

+ BlockDriverState *bs;

ThrottleTimers *tt;

notifier_list_notify(&blk->remove_bs_notifiers, blk);

if (blk->public.throttle_group_member.throttle_state) {

tt = &blk->public.throttle_group_member.throttle_timers;

+ bs = blk_bs(blk);

+ bdrv_drained_begin(bs);

throttle_timers_detach_aio_context(tt);

+ bdrv_drained_end(bs);

blk_update_root_state(blk);

2.13.6

I/O requests hang after stop/cont commands at least since QEMU 2.10.0

with -drive iops=100:

(guest)$ dd if=/dev/zero of=/dev/vdb oflag=direct count=1000

(qemu) stop

(qemu) cont

...I/O is stuck...

This happens because blk_set_aio_context() detaches the ThrottleState

while requests may still be in flight:

if (tgm->throttle_state) {

throttle_group_detach_aio_context(tgm);

throttle_group_attach_aio_context(tgm, new_context);

}

This patch encloses the detach/attach calls in a drained region so no

I/O request is left hanging. Also add assertions so we don't make the

same mistake again in the future.

Reported-by: Yongxue Hong <yhong@redhat.com>

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

Reviewed-by: Alberto Garcia <berto@igalia.com>

Message-id: 20171110151934.16883-1-stefanha@redhat.com

block/block-backend.c | 2 ++

block/throttle-groups.c | 6 ++++++

2 files changed, 8 insertions(+)

diff --git a/block/block-backend.c b/block/block-backend.c

--- a/block/block-backend.c

+++ b/block/block-backend.c

@@ -XXX,XX +XXX,XX @@ void blk_set_aio_context(BlockBackend *blk, AioContext *new_context)

if (bs) {

if (tgm->throttle_state) {

+ bdrv_drained_begin(bs);

throttle_group_detach_aio_context(tgm);

throttle_group_attach_aio_context(tgm, new_context);

+ bdrv_drained_end(bs);

}

bdrv_set_aio_context(bs, new_context);

}

diff --git a/block/throttle-groups.c b/block/throttle-groups.c

index XXXXXXX..XXXXXXX 100644

--- a/block/throttle-groups.c

+++ b/block/throttle-groups.c

@@ -XXX,XX +XXX,XX @@ void throttle_group_attach_aio_context(ThrottleGroupMember *tgm,

void throttle_group_detach_aio_context(ThrottleGroupMember *tgm)

{

ThrottleTimers *tt = &tgm->throttle_timers;

+

+ /* Requests must have been drained */

+ assert(tgm->pending_reqs[0] == 0 && tgm->pending_reqs[1] == 0);

+ assert(qemu_co_queue_empty(&tgm->throttled_reqs[0]));

+ assert(qemu_co_queue_empty(&tgm->throttled_reqs[1]));

+

throttle_timers_detach_aio_context(tt);

tgm->aio_context = NULL;

}

2.13.6

When you set I/O limits using block_set_io_throttle or the command

line throttling.* options they are kept in the BlockBackend regardless

of whether a BlockDriverState is attached to the backend or not.

Therefore when removing the limits using blk_io_limits_disable() we

need to check if there's a BDS before attempting to drain it, else it

will crash QEMU. This can be reproduced very easily using HMP:

(qemu) drive_add 0 if=none,throttling.iops-total=5000

(qemu) drive_del none0

Reported-by: sochin jiang <sochin.jiang@huawei.com>

Reviewed-by: Max Reitz <mreitz@redhat.com>

Message-id: 0d3a67ce8d948bb33e08672564714dcfb76a3d8c.1510339534.git.berto@igalia.com

block/block-backend.c | 14 ++++++++++----

1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/block/block-backend.c b/block/block-backend.c

--- a/block/block-backend.c

+++ b/block/block-backend.c

@@ -XXX,XX +XXX,XX @@ void blk_set_io_limits(BlockBackend *blk, ThrottleConfig *cfg)

void blk_io_limits_disable(BlockBackend *blk)

{

- assert(blk->public.throttle_group_member.throttle_state);

- bdrv_drained_begin(blk_bs(blk));

- throttle_group_unregister_tgm(&blk->public.throttle_group_member);

- bdrv_drained_end(blk_bs(blk));

+ BlockDriverState *bs = blk_bs(blk);

+ ThrottleGroupMember *tgm = &blk->public.throttle_group_member;

+ assert(tgm->throttle_state);

+ if (bs) {

+ bdrv_drained_begin(bs);

+ }

+ throttle_group_unregister_tgm(tgm);

+ if (bs) {

+ bdrv_drained_end(bs);

+ }

/* should be called before blk_set_io_limits if a limit is set */

2.13.6

If a BlockBackend has I/O limits set then its ThrottleGroupMember

structure uses the AioContext from its attached BlockDriverState.

Those two contexts must be kept in sync manually. This is not

ideal and will be fixed in the future by removing the throttling

configuration from the BlockBackend and storing it in an implicit

filter node instead, but for now we have to live with this.

Reviewed-by: Max Reitz <mreitz@redhat.com>

Message-id: e089c66e7c20289b046d782cea4373b765c5bc1d.1510339534.git.berto@igalia.com

block/block-backend.c | 16 ++++++++--------

1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/block/block-backend.c b/block/block-backend.c

--- a/block/block-backend.c

+++ b/block/block-backend.c

@@ -XXX,XX +XXX,XX @@ BlockBackend *blk_by_public(BlockBackendPublic *public)

*/

void blk_remove_bs(BlockBackend *blk)

+ ThrottleGroupMember *tgm = &blk->public.throttle_group_member;

BlockDriverState *bs;

- ThrottleTimers *tt;

notifier_list_notify(&blk->remove_bs_notifiers, blk);

- if (blk->public.throttle_group_member.throttle_state) {

- tt = &blk->public.throttle_group_member.throttle_timers;

+ if (tgm->throttle_state) {

bs = blk_bs(blk);

bdrv_drained_begin(bs);

- throttle_timers_detach_aio_context(tt);

+ throttle_group_detach_aio_context(tgm);

+ throttle_group_attach_aio_context(tgm, qemu_get_aio_context());

bdrv_drained_end(bs);

@@ -XXX,XX +XXX,XX @@ void blk_remove_bs(BlockBackend *blk)

*/

int blk_insert_bs(BlockBackend *blk, BlockDriverState *bs, Error **errp)

{

+ ThrottleGroupMember *tgm = &blk->public.throttle_group_member;

blk->root = bdrv_root_attach_child(bs, "root", &child_root,

blk->perm, blk->shared_perm, blk, errp);

if (blk->root == NULL) {

@@ -XXX,XX +XXX,XX @@ int blk_insert_bs(BlockBackend *blk, BlockDriverState *bs, Error **errp)

bdrv_ref(bs);

notifier_list_notify(&blk->insert_bs_notifiers, blk);

- if (blk->public.throttle_group_member.throttle_state) {

- throttle_timers_attach_aio_context(

- &blk->public.throttle_group_member.throttle_timers,

- bdrv_get_aio_context(bs));

+ if (tgm->throttle_state) {

+ throttle_group_detach_aio_context(tgm);

+ throttle_group_attach_aio_context(tgm, bdrv_get_aio_context(bs));

return 0;

2.13.6

This test hotplugs a CD drive to a VM and checks that I/O limits can

be set only when the drive has media inserted and that they are kept

when the media is replaced.

This also tests the removal of a device with valid I/O limits set but

no media inserted. This involves deleting and disabling the limits

of a BlockBackend without BlockDriverState, a scenario that has been

crashing until the fixes from the last couple of patches.

[Python PEP8 fixup: "Don't use spaces are the = sign when used to

indicate a keyword argument or a default parameter value"

--Stefan]

Reviewed-by: Max Reitz <mreitz@redhat.com>

Message-id: 071eb397118ed207c5a7f01d58766e415ee18d6a.1510339534.git.berto@igalia.com

tests/qemu-iotests/093 | 62 ++++++++++++++++++++++++++++++++++++++++++++++

tests/qemu-iotests/093.out | 4 +--

2 files changed, 64 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/093 b/tests/qemu-iotests/093

index XXXXXXX..XXXXXXX 100755

--- a/tests/qemu-iotests/093

+++ b/tests/qemu-iotests/093

@@ -XXX,XX +XXX,XX @@ class ThrottleTestGroupNames(iotests.QMPTestCase):

groupname = "group%d" % i

self.verify_name(devname, groupname)

+class ThrottleTestRemovableMedia(iotests.QMPTestCase):

+ def setUp(self):

+ self.vm = iotests.VM()

+ if iotests.qemu_default_machine == 's390-ccw-virtio':

+ self.vm.add_device("virtio-scsi-ccw,id=virtio-scsi")

+ else:

+ self.vm.add_device("virtio-scsi-pci,id=virtio-scsi")

+ self.vm.launch()

+

+ def tearDown(self):

+ self.vm.shutdown()

+

+ def test_removable_media(self):

+ # Add a couple of dummy nodes named cd0 and cd1

+ result = self.vm.qmp("blockdev-add", driver="null-aio",

+ node_name="cd0")

+ self.assert_qmp(result, 'return', {})

+ result = self.vm.qmp("blockdev-add", driver="null-aio",

+ node_name="cd1")

+ self.assert_qmp(result, 'return', {})

+

+ # Attach a CD drive with cd0 inserted

+ result = self.vm.qmp("device_add", driver="scsi-cd",

+ id="dev0", drive="cd0")

+ self.assert_qmp(result, 'return', {})

+

+ # Set I/O limits

+ args = { "id": "dev0", "iops": 100, "iops_rd": 0, "iops_wr": 0,

+ "bps": 50, "bps_rd": 0, "bps_wr": 0 }

+ result = self.vm.qmp("block_set_io_throttle", conv_keys=False, **args)

+ self.assert_qmp(result, 'return', {})

+

+ # Check that the I/O limits have been set

+ result = self.vm.qmp("query-block")

+ self.assert_qmp(result, 'return[0]/inserted/iops', 100)

+ self.assert_qmp(result, 'return[0]/inserted/bps', 50)

+

+ # Now eject cd0 and insert cd1

+ result = self.vm.qmp("blockdev-open-tray", id='dev0')

+ self.assert_qmp(result, 'return', {})

+ result = self.vm.qmp("x-blockdev-remove-medium", id='dev0')

+ self.assert_qmp(result, 'return', {})

+ result = self.vm.qmp("x-blockdev-insert-medium", id='dev0', node_name='cd1')

+ self.assert_qmp(result, 'return', {})

+

+ # Check that the I/O limits are still the same

+ result = self.vm.qmp("query-block")

+ self.assert_qmp(result, 'return[0]/inserted/iops', 100)

+ self.assert_qmp(result, 'return[0]/inserted/bps', 50)

+

+ # Eject cd1

+ result = self.vm.qmp("x-blockdev-remove-medium", id='dev0')

+ self.assert_qmp(result, 'return', {})

+

+ # Check that we can't set limits if the device has no medium

+ result = self.vm.qmp("block_set_io_throttle", conv_keys=False, **args)

+ self.assert_qmp(result, 'error/class', 'GenericError')

+

+ # Remove the CD drive

+ result = self.vm.qmp("device_del", id='dev0')

+ self.assert_qmp(result, 'return', {})

+

if __name__ == '__main__':

iotests.main(supported_fmts=["raw"])

diff --git a/tests/qemu-iotests/093.out b/tests/qemu-iotests/093.out

--- a/tests/qemu-iotests/093.out

+++ b/tests/qemu-iotests/093.out

@@ -XXX,XX +XXX,XX @@

-.......

+........

----------------------------------------------------------------------

-Ran 7 tests

+Ran 8 tests

OK

2.13.6