From 582d4210eb2f2ab5baac328fe4b479cd86da1647 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Tue, 9 Feb 2021 09:27:58 -0600 Subject: [PATCH 01/14] qemu-nbd: Use SOMAXCONN for socket listen() backlog MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Our default of a backlog of 1 connection is rather puny; it gets in the way when we are explicitly allowing multiple clients (such as qemu-nbd -e N [--shared], or nbd-server-start with its default "max-connections":0 for unlimited), but is even a problem when we stick to qemu-nbd's default of only 1 active client but use -t [--persistent] where a second client can start using the server once the first finishes. While the effects are less noticeable on TCP sockets (since the client can poll() to learn when the server is ready again), it is definitely observable on Unix sockets, where on Linux, a client will fail with EAGAIN and no recourse but to sleep an arbitrary amount of time before retrying if the server backlog is already full. Since QMP nbd-server-start is always persistent, it now always requests a backlog of SOMAXCONN; meanwhile, qemu-nbd will request SOMAXCONN if persistent, otherwise its backlog should be based on the expected number of clients. See https://bugzilla.redhat.com/1925045 for a demonstration of where our low backlog prevents libnbd from connecting as many parallel clients as it wants. Reported-by: Richard W.M. Jones Signed-off-by: Eric Blake CC: qemu-stable@nongnu.org Message-Id: <20210209152759.209074-2-eblake@redhat.com> Tested-by: Richard W.M. Jones Reviewed-by: Daniel P. Berrangé Signed-off-by: Eric Blake --- blockdev-nbd.c | 7 ++++++- qemu-nbd.c | 10 +++++++++- 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/blockdev-nbd.c b/blockdev-nbd.c index d8443d235b..b264620b98 100644 --- a/blockdev-nbd.c +++ b/blockdev-nbd.c @@ -134,7 +134,12 @@ void nbd_server_start(SocketAddress *addr, const char *tls_creds, qio_net_listener_set_name(nbd_server->listener, "nbd-listener"); - if (qio_net_listener_open_sync(nbd_server->listener, addr, 1, errp) < 0) { + /* + * Because this server is persistent, a backlog of SOMAXCONN is + * better than trying to size it to max_connections. + */ + if (qio_net_listener_open_sync(nbd_server->listener, addr, SOMAXCONN, + errp) < 0) { goto error; } diff --git a/qemu-nbd.c b/qemu-nbd.c index 608c63e82a..1a340ea485 100644 --- a/qemu-nbd.c +++ b/qemu-nbd.c @@ -964,8 +964,16 @@ int main(int argc, char **argv) server = qio_net_listener_new(); if (socket_activation == 0) { + int backlog; + + if (persistent) { + backlog = SOMAXCONN; + } else { + backlog = MIN(shared, SOMAXCONN); + } saddr = nbd_build_socket_address(sockpath, bindto, port); - if (qio_net_listener_open_sync(server, saddr, 1, &local_err) < 0) { + if (qio_net_listener_open_sync(server, saddr, backlog, + &local_err) < 0) { object_unref(OBJECT(server)); error_report_err(local_err); exit(EXIT_FAILURE); From 3dcf56e625c684178c0062d845f9fe05ecce346f Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Tue, 9 Feb 2021 09:27:59 -0600 Subject: [PATCH 02/14] qemu-nbd: Permit --shared=0 for unlimited clients MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This gives us better feature parity with QMP nbd-server-start, where max-connections defaults to 0 for unlimited. Signed-off-by: Eric Blake Message-Id: <20210209152759.209074-3-eblake@redhat.com> Reviewed-by: Daniel P. Berrangé --- docs/tools/qemu-nbd.rst | 4 ++-- qemu-nbd.c | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/docs/tools/qemu-nbd.rst b/docs/tools/qemu-nbd.rst index fe41336dc5..ee862fa0bc 100644 --- a/docs/tools/qemu-nbd.rst +++ b/docs/tools/qemu-nbd.rst @@ -136,8 +136,8 @@ driver options if ``--image-opts`` is specified. .. option:: -e, --shared=NUM Allow up to *NUM* clients to share the device (default - ``1``). Safe for readers, but for now, consistency is not - guaranteed between multiple writers. + ``1``), 0 for unlimited. Safe for readers, but for now, + consistency is not guaranteed between multiple writers. .. option:: -t, --persistent diff --git a/qemu-nbd.c b/qemu-nbd.c index 1a340ea485..b1b9430a8f 100644 --- a/qemu-nbd.c +++ b/qemu-nbd.c @@ -328,7 +328,7 @@ static void *nbd_client_thread(void *arg) static int nbd_can_accept(void) { - return state == RUNNING && nb_fds < shared; + return state == RUNNING && (shared == 0 || nb_fds < shared); } static void nbd_update_server_watch(void); @@ -707,7 +707,7 @@ int main(int argc, char **argv) break; case 'e': if (qemu_strtoi(optarg, NULL, 0, &shared) < 0 || - shared < 1) { + shared < 0) { error_report("Invalid shared device number '%s'", optarg); exit(EXIT_FAILURE); } @@ -966,7 +966,7 @@ int main(int argc, char **argv) if (socket_activation == 0) { int backlog; - if (persistent) { + if (persistent || shared == 0) { backlog = SOMAXCONN; } else { backlog = MIN(shared, SOMAXCONN); From e055a5c8dc53212ede81f2dd828c688ee4f7c00b Mon Sep 17 00:00:00 2001 From: Max Reitz Date: Tue, 9 Feb 2021 19:19:23 +0100 Subject: [PATCH 03/14] iotests/210: Fix reference output Commit 69b55e03f has changed an error message, adjust the reference output to account for it. Fixes: 69b55e03f7e65a36eb954d0b7d4698b258df2708 ("block: refactor bdrv_check_request: add errp") Signed-off-by: Max Reitz Message-Id: <20210209181923.497688-1-mreitz@redhat.com> Reviewed-by: Eric Blake Reviewed-by: Vladimir Sementsov-Ogievskiy Signed-off-by: Eric Blake --- tests/qemu-iotests/210.out | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/qemu-iotests/210.out b/tests/qemu-iotests/210.out index dc1a3c9786..2e9fc596eb 100644 --- a/tests/qemu-iotests/210.out +++ b/tests/qemu-iotests/210.out @@ -182,7 +182,7 @@ Job failed: The requested file size is too large === Resize image with invalid sizes === {"execute": "block_resize", "arguments": {"node-name": "node1", "size": 9223372036854775296}} -{"error": {"class": "GenericError", "desc": "Required too big image size, it must be not greater than 9223372035781033984"}} +{"error": {"class": "GenericError", "desc": "offset(9223372036854775296) exceeds maximum(9223372035781033984)"}} {"execute": "block_resize", "arguments": {"node-name": "node1", "size": 9223372036854775808}} {"error": {"class": "GenericError", "desc": "Invalid parameter type for 'size', expected: integer"}} {"execute": "block_resize", "arguments": {"node-name": "node1", "size": 18446744073709551104}} From c90e3512a4683345a8e7074961d8275ceaec578d Mon Sep 17 00:00:00 2001 From: Jagannathan Raman Date: Fri, 12 Feb 2021 06:16:07 -0500 Subject: [PATCH 04/14] io: error_prepend() in qio_channel_readv_full_all() causes segfault MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Using error_prepend() in qio_channel_readv_full_all() causes a segfault as errp is not set when ret is 0. This results in the failure of iotest 83. Replacing with error_setg() fixes the problem. Additionally, removes a full stop at the end of error message Reported-by: Max Reitz Signed-off-by: Jagannathan Raman Fixes: bebab91ebdfc591f8793a9a17370df1bfbe8b2ca (io: add qio_channel_readv_full_all_eof & qio_channel_readv_full_all helpers) Message-Id: Reviewed-by: Daniel P. Berrangé Signed-off-by: Eric Blake --- io/channel.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/io/channel.c b/io/channel.c index 4555021b62..e8b019dc36 100644 --- a/io/channel.c +++ b/io/channel.c @@ -202,8 +202,7 @@ int qio_channel_readv_full_all(QIOChannel *ioc, int ret = qio_channel_readv_full_all_eof(ioc, iov, niov, fds, nfds, errp); if (ret == 0) { - error_prepend(errp, - "Unexpected end-of-file before all data were read."); + error_setg(errp, "Unexpected end-of-file before all data were read"); return -1; } if (ret == 1) { From bd54669a4adf0931be3b0574d07ce4809bf67807 Mon Sep 17 00:00:00 2001 From: Vladimir Sementsov-Ogievskiy Date: Fri, 5 Feb 2021 19:37:11 +0300 Subject: [PATCH 05/14] block: add new BlockDriver handler: bdrv_cancel_in_flight It will be used to stop retrying NBD requests on mirror cancel. Signed-off-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Eric Blake Message-Id: <20210205163720.887197-2-vsementsov@virtuozzo.com> Signed-off-by: Eric Blake --- block/io.c | 11 +++++++++++ include/block/block.h | 3 +++ include/block/block_int.h | 9 +++++++++ 3 files changed, 23 insertions(+) diff --git a/block/io.c b/block/io.c index b0435ed670..ca2dca3007 100644 --- a/block/io.c +++ b/block/io.c @@ -3460,3 +3460,14 @@ out: return ret; } + +void bdrv_cancel_in_flight(BlockDriverState *bs) +{ + if (!bs || !bs->drv) { + return; + } + + if (bs->drv->bdrv_cancel_in_flight) { + bs->drv->bdrv_cancel_in_flight(bs); + } +} diff --git a/include/block/block.h b/include/block/block.h index 0a9f2c187c..2f2698074e 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -849,4 +849,7 @@ int coroutine_fn bdrv_co_copy_range(BdrvChild *src, int64_t src_offset, BdrvChild *dst, int64_t dst_offset, int64_t bytes, BdrvRequestFlags read_flags, BdrvRequestFlags write_flags); + +void bdrv_cancel_in_flight(BlockDriverState *bs); + #endif diff --git a/include/block/block_int.h b/include/block/block_int.h index 22a2789d35..88e4111939 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -352,6 +352,15 @@ struct BlockDriver { bool want_zero, int64_t offset, int64_t bytes, int64_t *pnum, int64_t *map, BlockDriverState **file); + /* + * This informs the driver that we are no longer interested in the result + * of in-flight requests, so don't waste the time if possible. + * + * One example usage is to avoid waiting for an nbd target node reconnect + * timeout during job-cancel. + */ + void (*bdrv_cancel_in_flight)(BlockDriverState *bs); + /* * Invalidate any cached meta-data. */ From c4f7f24e1f6e81804a7f15356614d9249280ab02 Mon Sep 17 00:00:00 2001 From: Vladimir Sementsov-Ogievskiy Date: Fri, 5 Feb 2021 19:37:12 +0300 Subject: [PATCH 06/14] block/nbd: implement .bdrv_cancel_in_flight Just stop waiting for connection in existing requests. Signed-off-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Eric Blake Message-Id: <20210205163720.887197-3-vsementsov@virtuozzo.com> Signed-off-by: Eric Blake --- block/nbd.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/block/nbd.c b/block/nbd.c index b3cbbeb4b0..c26dc5a54f 100644 --- a/block/nbd.c +++ b/block/nbd.c @@ -2458,6 +2458,18 @@ static const char *const nbd_strong_runtime_opts[] = { NULL }; +static void nbd_cancel_in_flight(BlockDriverState *bs) +{ + BDRVNBDState *s = (BDRVNBDState *)bs->opaque; + + reconnect_delay_timer_del(s); + + if (s->state == NBD_CLIENT_CONNECTING_WAIT) { + s->state = NBD_CLIENT_CONNECTING_NOWAIT; + qemu_co_queue_restart_all(&s->free_sema); + } +} + static BlockDriver bdrv_nbd = { .format_name = "nbd", .protocol_name = "nbd", @@ -2484,6 +2496,7 @@ static BlockDriver bdrv_nbd = { .bdrv_co_block_status = nbd_client_co_block_status, .bdrv_dirname = nbd_dirname, .strong_runtime_opts = nbd_strong_runtime_opts, + .bdrv_cancel_in_flight = nbd_cancel_in_flight, }; static BlockDriver bdrv_nbd_tcp = { @@ -2512,6 +2525,7 @@ static BlockDriver bdrv_nbd_tcp = { .bdrv_co_block_status = nbd_client_co_block_status, .bdrv_dirname = nbd_dirname, .strong_runtime_opts = nbd_strong_runtime_opts, + .bdrv_cancel_in_flight = nbd_cancel_in_flight, }; static BlockDriver bdrv_nbd_unix = { @@ -2540,6 +2554,7 @@ static BlockDriver bdrv_nbd_unix = { .bdrv_co_block_status = nbd_client_co_block_status, .bdrv_dirname = nbd_dirname, .strong_runtime_opts = nbd_strong_runtime_opts, + .bdrv_cancel_in_flight = nbd_cancel_in_flight, }; static void bdrv_nbd_init(void) From 3fc1ec3725a92268cb896e7fd82b4b4b4718203b Mon Sep 17 00:00:00 2001 From: Vladimir Sementsov-Ogievskiy Date: Fri, 5 Feb 2021 19:37:13 +0300 Subject: [PATCH 07/14] block/raw-format: implement .bdrv_cancel_in_flight handler We are going to cancel in-flight requests on mirror nbd target on job cancel. Still nbd is often used not directly but as raw-format child. So, add pass-through handler here. Signed-off-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Eric Blake Message-Id: <20210205163720.887197-4-vsementsov@virtuozzo.com> Signed-off-by: Eric Blake --- block/raw-format.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/block/raw-format.c b/block/raw-format.c index 42ec50802b..7717578ed6 100644 --- a/block/raw-format.c +++ b/block/raw-format.c @@ -575,6 +575,11 @@ static const char *const raw_strong_runtime_opts[] = { NULL }; +static void raw_cancel_in_flight(BlockDriverState *bs) +{ + bdrv_cancel_in_flight(bs->file->bs); +} + BlockDriver bdrv_raw = { .format_name = "raw", .instance_size = sizeof(BDRVRawState), @@ -608,6 +613,7 @@ BlockDriver bdrv_raw = { .bdrv_has_zero_init = &raw_has_zero_init, .strong_runtime_opts = raw_strong_runtime_opts, .mutable_opts = mutable_opts, + .bdrv_cancel_in_flight = raw_cancel_in_flight, }; static void bdrv_raw_init(void) From 9820933b57b24c21a509680650f669123651b60d Mon Sep 17 00:00:00 2001 From: Vladimir Sementsov-Ogievskiy Date: Fri, 5 Feb 2021 19:37:14 +0300 Subject: [PATCH 08/14] job: add .cancel handler for the driver To be used in mirror in the following commit to cancel in-flight io on target to not waste the time. Signed-off-by: Vladimir Sementsov-Ogievskiy Message-Id: <20210205163720.887197-5-vsementsov@virtuozzo.com> Reviewed-by: Eric Blake Signed-off-by: Eric Blake --- include/qemu/job.h | 5 +++++ job.c | 3 +++ 2 files changed, 8 insertions(+) diff --git a/include/qemu/job.h b/include/qemu/job.h index 32aabb1c60..efc6fa7544 100644 --- a/include/qemu/job.h +++ b/include/qemu/job.h @@ -251,6 +251,11 @@ struct JobDriver { */ void (*clean)(Job *job); + /** + * If the callback is not NULL, it will be invoked in job_cancel_async + */ + void (*cancel)(Job *job); + /** Called when the job is freed */ void (*free)(Job *job); diff --git a/job.c b/job.c index 3aaaebafe2..289edee143 100644 --- a/job.c +++ b/job.c @@ -715,6 +715,9 @@ static int job_finalize_single(Job *job) static void job_cancel_async(Job *job, bool force) { + if (job->driver->cancel) { + job->driver->cancel(job); + } if (job->user_paused) { /* Do not call job_enter here, the caller will handle it. */ if (job->driver->user_resume) { From 521ff8b779b11c394dbdc43f02e158dd99df308a Mon Sep 17 00:00:00 2001 From: Vladimir Sementsov-Ogievskiy Date: Fri, 5 Feb 2021 19:37:15 +0300 Subject: [PATCH 09/14] block/mirror: implement .cancel job handler Cancel in-flight io on target to not waste the time. Signed-off-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Eric Blake Message-Id: <20210205163720.887197-6-vsementsov@virtuozzo.com> Signed-off-by: Eric Blake --- block/mirror.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/block/mirror.c b/block/mirror.c index 8e1ad6eceb..9faffe4707 100644 --- a/block/mirror.c +++ b/block/mirror.c @@ -1179,6 +1179,14 @@ static bool mirror_drained_poll(BlockJob *job) return !!s->in_flight; } +static void mirror_cancel(Job *job) +{ + MirrorBlockJob *s = container_of(job, MirrorBlockJob, common.job); + BlockDriverState *target = blk_bs(s->target); + + bdrv_cancel_in_flight(target); +} + static const BlockJobDriver mirror_job_driver = { .job_driver = { .instance_size = sizeof(MirrorBlockJob), @@ -1190,6 +1198,7 @@ static const BlockJobDriver mirror_job_driver = { .abort = mirror_abort, .pause = mirror_pause, .complete = mirror_complete, + .cancel = mirror_cancel, }, .drained_poll = mirror_drained_poll, }; From 46bd6f8c364170d58f9b17a42fa9289872509f6b Mon Sep 17 00:00:00 2001 From: Vladimir Sementsov-Ogievskiy Date: Fri, 5 Feb 2021 19:37:16 +0300 Subject: [PATCH 10/14] iotests/264: move to python unittest We are going to add more test cases, so use the library supporting test cases. Signed-off-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Eric Blake Message-Id: <20210205163720.887197-7-vsementsov@virtuozzo.com> Signed-off-by: Eric Blake --- tests/qemu-iotests/264 | 93 ++++++++++++++++++++++---------------- tests/qemu-iotests/264.out | 20 ++------ 2 files changed, 58 insertions(+), 55 deletions(-) diff --git a/tests/qemu-iotests/264 b/tests/qemu-iotests/264 index e725cefd47..6feeaa4056 100755 --- a/tests/qemu-iotests/264 +++ b/tests/qemu-iotests/264 @@ -20,13 +20,10 @@ # import time +import os import iotests -from iotests import qemu_img_create, file_path, qemu_nbd_popen, log - -iotests.script_initialize( - supported_fmts=['qcow2'], -) +from iotests import qemu_img_create, file_path, qemu_nbd_popen disk_a, disk_b, nbd_sock = file_path('disk_a', 'disk_b', 'nbd-sock') nbd_uri = 'nbd+unix:///?socket=' + nbd_sock @@ -34,46 +31,62 @@ size = 5 * 1024 * 1024 wait_limit = 3.0 wait_step = 0.2 -qemu_img_create('-f', iotests.imgfmt, disk_a, str(size)) -qemu_img_create('-f', iotests.imgfmt, disk_b, str(size)) -with qemu_nbd_popen('-k', nbd_sock, '-f', iotests.imgfmt, disk_b): - vm = iotests.VM().add_drive(disk_a) - vm.launch() - vm.hmp_qemu_io('drive0', 'write 0 {}'.format(size)) +class TestNbdReconnect(iotests.QMPTestCase): + def setUp(self): + qemu_img_create('-f', iotests.imgfmt, disk_a, str(size)) + qemu_img_create('-f', iotests.imgfmt, disk_b, str(size)) + self.vm = iotests.VM().add_drive(disk_a) + self.vm.launch() + self.vm.hmp_qemu_io('drive0', 'write 0 {}'.format(size)) - vm.qmp_log('blockdev-add', filters=[iotests.filter_qmp_testfiles], - **{'node_name': 'backup0', - 'driver': 'raw', - 'file': {'driver': 'nbd', - 'server': {'type': 'unix', 'path': nbd_sock}, - 'reconnect-delay': 10}}) - vm.qmp_log('blockdev-backup', device='drive0', sync='full', - target='backup0', speed=(1 * 1024 * 1024)) + def tearDown(self): + self.vm.shutdown() + os.remove(disk_a) + os.remove(disk_b) - # Wait for some progress - t = 0.0 - while t < wait_limit: - jobs = vm.qmp('query-block-jobs')['return'] - if jobs and jobs[0]['offset'] > 0: - break - time.sleep(wait_step) - t += wait_step + def test(self): + with qemu_nbd_popen('-k', nbd_sock, '-f', iotests.imgfmt, disk_b): + result = self.vm.qmp('blockdev-add', + **{'node_name': 'backup0', + 'driver': 'raw', + 'file': {'driver': 'nbd', + 'server': {'type': 'unix', + 'path': nbd_sock}, + 'reconnect-delay': 10}}) + self.assert_qmp(result, 'return', {}) + result = self.vm.qmp('blockdev-backup', device='drive0', + sync='full', target='backup0', + speed=(1 * 1024 * 1024)) + self.assert_qmp(result, 'return', {}) - if jobs and jobs[0]['offset'] > 0: - log('Backup job is started') + # Wait for some progress + t = 0.0 + while t < wait_limit: + jobs = self.vm.qmp('query-block-jobs')['return'] + if jobs and jobs[0]['offset'] > 0: + break + time.sleep(wait_step) + t += wait_step -jobs = vm.qmp('query-block-jobs')['return'] -if jobs and jobs[0]['offset'] < jobs[0]['len']: - log('Backup job is still in progress') + self.assertTrue(jobs and jobs[0]['offset'] > 0) # job started -vm.qmp_log('block-job-set-speed', device='drive0', speed=0) + jobs = self.vm.qmp('query-block-jobs')['return'] + # Check that job is still in progress + self.assertTrue(jobs and jobs[0]['offset'] < jobs[0]['len']) -# Emulate server down time for 1 second -time.sleep(1) + result = self.vm.qmp('block-job-set-speed', device='drive0', speed=0) + self.assert_qmp(result, 'return', {}) -with qemu_nbd_popen('-k', nbd_sock, '-f', iotests.imgfmt, disk_b): - e = vm.event_wait('BLOCK_JOB_COMPLETED') - log('Backup completed: {}'.format(e['data']['offset'])) - vm.qmp_log('blockdev-del', node_name='backup0') - vm.shutdown() + # Emulate server down time for 1 second + time.sleep(1) + + with qemu_nbd_popen('-k', nbd_sock, '-f', iotests.imgfmt, disk_b): + e = self.vm.event_wait('BLOCK_JOB_COMPLETED') + self.assertEqual(e['data']['offset'], size) + result = self.vm.qmp('blockdev-del', node_name='backup0') + self.assert_qmp(result, 'return', {}) + + +if __name__ == '__main__': + iotests.main(supported_fmts=['qcow2']) diff --git a/tests/qemu-iotests/264.out b/tests/qemu-iotests/264.out index c45b1e81ef..ae1213e6f8 100644 --- a/tests/qemu-iotests/264.out +++ b/tests/qemu-iotests/264.out @@ -1,15 +1,5 @@ -Start NBD server -{"execute": "blockdev-add", "arguments": {"driver": "raw", "file": {"driver": "nbd", "reconnect-delay": 10, "server": {"path": "TEST_DIR/PID-nbd-sock", "type": "unix"}}, "node-name": "backup0"}} -{"return": {}} -{"execute": "blockdev-backup", "arguments": {"device": "drive0", "speed": 1048576, "sync": "full", "target": "backup0"}} -{"return": {}} -Backup job is started -Kill NBD server -Backup job is still in progress -{"execute": "block-job-set-speed", "arguments": {"device": "drive0", "speed": 0}} -{"return": {}} -Start NBD server -Backup completed: 5242880 -{"execute": "blockdev-del", "arguments": {"node-name": "backup0"}} -{"return": {}} -Kill NBD server +. +---------------------------------------------------------------------- +Ran 1 tests + +OK From 3f7db418d10ee7b48c2ef718f6b66bc3e28282a6 Mon Sep 17 00:00:00 2001 From: Vladimir Sementsov-Ogievskiy Date: Fri, 5 Feb 2021 19:37:17 +0300 Subject: [PATCH 11/14] iotests.py: qemu_nbd_popen: remove pid file after use To not interfere with other qemu_nbd_popen() calls in same test. Signed-off-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Eric Blake Message-Id: <20210205163720.887197-8-vsementsov@virtuozzo.com> Signed-off-by: Eric Blake --- tests/qemu-iotests/iotests.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py index 00be68eca3..4e758308f2 100644 --- a/tests/qemu-iotests/iotests.py +++ b/tests/qemu-iotests/iotests.py @@ -296,7 +296,9 @@ def qemu_nbd_list_log(*args: str) -> str: @contextmanager def qemu_nbd_popen(*args): '''Context manager running qemu-nbd within the context''' - pid_file = file_path("pid") + pid_file = file_path("qemu_nbd_popen-nbd-pid-file") + + assert not os.path.exists(pid_file) cmd = list(qemu_nbd_args) cmd.extend(('--persistent', '--pid-file', pid_file)) @@ -314,6 +316,8 @@ def qemu_nbd_popen(*args): time.sleep(0.01) yield finally: + if os.path.exists(pid_file): + os.remove(pid_file) log('Kill NBD server') p.kill() p.wait() From d00dd63135f7e18ddca2642d1933da1507f3f1cd Mon Sep 17 00:00:00 2001 From: Vladimir Sementsov-Ogievskiy Date: Fri, 5 Feb 2021 19:37:18 +0300 Subject: [PATCH 12/14] iotests/264: add mirror-cancel test-case Check that cancel doesn't wait for 10s of nbd reconnect timeout. Signed-off-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Eric Blake Message-Id: <20210205163720.887197-9-vsementsov@virtuozzo.com> Signed-off-by: Eric Blake --- tests/qemu-iotests/264 | 38 ++++++++++++++++++++++++++++++-------- tests/qemu-iotests/264.out | 4 ++-- 2 files changed, 32 insertions(+), 10 deletions(-) diff --git a/tests/qemu-iotests/264 b/tests/qemu-iotests/264 index 6feeaa4056..347e53add5 100755 --- a/tests/qemu-iotests/264 +++ b/tests/qemu-iotests/264 @@ -27,25 +27,26 @@ from iotests import qemu_img_create, file_path, qemu_nbd_popen disk_a, disk_b, nbd_sock = file_path('disk_a', 'disk_b', 'nbd-sock') nbd_uri = 'nbd+unix:///?socket=' + nbd_sock -size = 5 * 1024 * 1024 wait_limit = 3.0 wait_step = 0.2 class TestNbdReconnect(iotests.QMPTestCase): - def setUp(self): - qemu_img_create('-f', iotests.imgfmt, disk_a, str(size)) - qemu_img_create('-f', iotests.imgfmt, disk_b, str(size)) + def init_vm(self, disk_size): + qemu_img_create('-f', iotests.imgfmt, disk_a, str(disk_size)) + qemu_img_create('-f', iotests.imgfmt, disk_b, str(disk_size)) self.vm = iotests.VM().add_drive(disk_a) self.vm.launch() - self.vm.hmp_qemu_io('drive0', 'write 0 {}'.format(size)) + self.vm.hmp_qemu_io('drive0', 'write 0 {}'.format(disk_size)) def tearDown(self): self.vm.shutdown() os.remove(disk_a) os.remove(disk_b) - def test(self): + def start_job(self, job): + """Stat job with nbd target and kill the server""" + assert job in ('blockdev-backup', 'blockdev-mirror') with qemu_nbd_popen('-k', nbd_sock, '-f', iotests.imgfmt, disk_b): result = self.vm.qmp('blockdev-add', **{'node_name': 'backup0', @@ -55,7 +56,7 @@ class TestNbdReconnect(iotests.QMPTestCase): 'path': nbd_sock}, 'reconnect-delay': 10}}) self.assert_qmp(result, 'return', {}) - result = self.vm.qmp('blockdev-backup', device='drive0', + result = self.vm.qmp(job, device='drive0', sync='full', target='backup0', speed=(1 * 1024 * 1024)) self.assert_qmp(result, 'return', {}) @@ -73,7 +74,8 @@ class TestNbdReconnect(iotests.QMPTestCase): jobs = self.vm.qmp('query-block-jobs')['return'] # Check that job is still in progress - self.assertTrue(jobs and jobs[0]['offset'] < jobs[0]['len']) + self.assertTrue(jobs) + self.assertTrue(jobs[0]['offset'] < jobs[0]['len']) result = self.vm.qmp('block-job-set-speed', device='drive0', speed=0) self.assert_qmp(result, 'return', {}) @@ -81,12 +83,32 @@ class TestNbdReconnect(iotests.QMPTestCase): # Emulate server down time for 1 second time.sleep(1) + def test_backup(self): + size = 5 * 1024 * 1024 + self.init_vm(size) + self.start_job('blockdev-backup') + with qemu_nbd_popen('-k', nbd_sock, '-f', iotests.imgfmt, disk_b): e = self.vm.event_wait('BLOCK_JOB_COMPLETED') self.assertEqual(e['data']['offset'], size) result = self.vm.qmp('blockdev-del', node_name='backup0') self.assert_qmp(result, 'return', {}) + def test_mirror_cancel(self): + # Mirror speed limit doesn't work well enough, it seems that mirror + # will run many parallel requests anyway. MAX_IN_FLIGHT is 16 and + # MAX_IO_BYTES is 1M in mirror.c, so let's use 20M disk. + self.init_vm(20 * 1024 * 1024) + self.start_job('blockdev-mirror') + + result = self.vm.qmp('block-job-cancel', device='drive0') + self.assert_qmp(result, 'return', {}) + + start_t = time.time() + self.vm.event_wait('BLOCK_JOB_CANCELLED') + delta_t = time.time() - start_t + self.assertTrue(delta_t < 2.0) + if __name__ == '__main__': iotests.main(supported_fmts=['qcow2']) diff --git a/tests/qemu-iotests/264.out b/tests/qemu-iotests/264.out index ae1213e6f8..fbc63e62f8 100644 --- a/tests/qemu-iotests/264.out +++ b/tests/qemu-iotests/264.out @@ -1,5 +1,5 @@ -. +.. ---------------------------------------------------------------------- -Ran 1 tests +Ran 2 tests OK From ff789bf5a93cede32a01e648dc010032791c84e1 Mon Sep 17 00:00:00 2001 From: Vladimir Sementsov-Ogievskiy Date: Fri, 5 Feb 2021 19:37:19 +0300 Subject: [PATCH 13/14] block/backup: implement .cancel job handler Cancel in-flight io on target to not waste the time. Signed-off-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Eric Blake Message-Id: <20210205163720.887197-10-vsementsov@virtuozzo.com> Signed-off-by: Eric Blake --- block/backup.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/block/backup.c b/block/backup.c index cc525d5544..94e6dcd72e 100644 --- a/block/backup.c +++ b/block/backup.c @@ -35,6 +35,7 @@ typedef struct BackupBlockJob { BlockJob common; BlockDriverState *backup_top; BlockDriverState *source_bs; + BlockDriverState *target_bs; BdrvDirtyBitmap *sync_bitmap; @@ -329,6 +330,13 @@ static void coroutine_fn backup_set_speed(BlockJob *job, int64_t speed) } } +static void backup_cancel(Job *job) +{ + BackupBlockJob *s = container_of(job, BackupBlockJob, common.job); + + bdrv_cancel_in_flight(s->target_bs); +} + static const BlockJobDriver backup_job_driver = { .job_driver = { .instance_size = sizeof(BackupBlockJob), @@ -340,6 +348,7 @@ static const BlockJobDriver backup_job_driver = { .abort = backup_abort, .clean = backup_clean, .pause = backup_pause, + .cancel = backup_cancel, }, .set_speed = backup_set_speed, }; @@ -528,6 +537,7 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs, job->backup_top = backup_top; job->source_bs = bs; + job->target_bs = target; job->on_source_error = on_source_error; job->on_target_error = on_target_error; job->sync_mode = sync_mode; From 594427fc56758cb944a85914eefe722cc2c667b8 Mon Sep 17 00:00:00 2001 From: Vladimir Sementsov-Ogievskiy Date: Fri, 5 Feb 2021 19:37:20 +0300 Subject: [PATCH 14/14] iotests/264: add backup-cancel test-case Check that cancel doesn't wait for 10s of nbd reconnect timeout. Signed-off-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Eric Blake Message-Id: <20210205163720.887197-11-vsementsov@virtuozzo.com> Signed-off-by: Eric Blake --- tests/qemu-iotests/264 | 21 ++++++++++++++------- tests/qemu-iotests/264.out | 4 ++-- 2 files changed, 16 insertions(+), 9 deletions(-) diff --git a/tests/qemu-iotests/264 b/tests/qemu-iotests/264 index 347e53add5..4f96825a22 100755 --- a/tests/qemu-iotests/264 +++ b/tests/qemu-iotests/264 @@ -94,13 +94,7 @@ class TestNbdReconnect(iotests.QMPTestCase): result = self.vm.qmp('blockdev-del', node_name='backup0') self.assert_qmp(result, 'return', {}) - def test_mirror_cancel(self): - # Mirror speed limit doesn't work well enough, it seems that mirror - # will run many parallel requests anyway. MAX_IN_FLIGHT is 16 and - # MAX_IO_BYTES is 1M in mirror.c, so let's use 20M disk. - self.init_vm(20 * 1024 * 1024) - self.start_job('blockdev-mirror') - + def cancel_job(self): result = self.vm.qmp('block-job-cancel', device='drive0') self.assert_qmp(result, 'return', {}) @@ -109,6 +103,19 @@ class TestNbdReconnect(iotests.QMPTestCase): delta_t = time.time() - start_t self.assertTrue(delta_t < 2.0) + def test_mirror_cancel(self): + # Mirror speed limit doesn't work well enough, it seems that mirror + # will run many parallel requests anyway. MAX_IN_FLIGHT is 16 and + # MAX_IO_BYTES is 1M in mirror.c, so let's use 20M disk. + self.init_vm(20 * 1024 * 1024) + self.start_job('blockdev-mirror') + self.cancel_job() + + def test_backup_cancel(self): + self.init_vm(5 * 1024 * 1024) + self.start_job('blockdev-backup') + self.cancel_job() + if __name__ == '__main__': iotests.main(supported_fmts=['qcow2']) diff --git a/tests/qemu-iotests/264.out b/tests/qemu-iotests/264.out index fbc63e62f8..8d7e996700 100644 --- a/tests/qemu-iotests/264.out +++ b/tests/qemu-iotests/264.out @@ -1,5 +1,5 @@ -.. +... ---------------------------------------------------------------------- -Ran 2 tests +Ran 3 tests OK