From d21e8776f6578be155714ae95c7d6c1bb03e8e34 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Tue, 24 Nov 2015 14:46:44 +0100 Subject: [PATCH 1/4] iothread: include id in thread name This makes it easier to find the desired thread. Use "IO" plus the id; even with the 14 character limit on the thread name, enough of the id should be readable (e.g. "IO iothreadNNN" with three characters for the number). Signed-off-by: Paolo Bonzini Reviewed-by: Dr. David Alan Gilbert Message-id: 1448372804-5034-1-git-send-email-pbonzini@redhat.com Signed-off-by: Stefan Hajnoczi --- iothread.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/iothread.c b/iothread.c index da6ce7b308..1b8c2bbecb 100644 --- a/iothread.c +++ b/iothread.c @@ -72,6 +72,7 @@ static void iothread_complete(UserCreatable *obj, Error **errp) { Error *local_error = NULL; IOThread *iothread = IOTHREAD(obj); + char *name, *thread_name; iothread->stopping = false; iothread->thread_id = -1; @@ -87,8 +88,12 @@ static void iothread_complete(UserCreatable *obj, Error **errp) /* This assumes we are called from a thread with useful CPU affinity for us * to inherit. */ - qemu_thread_create(&iothread->thread, "iothread", iothread_run, + name = object_get_canonical_path_component(OBJECT(obj)); + thread_name = g_strdup_printf("IO %s", name); + qemu_thread_create(&iothread->thread, thread_name, iothread_run, iothread, QEMU_THREAD_JOINABLE); + g_free(thread_name); + g_free(name); /* Wait for initialization to complete */ qemu_mutex_lock(&iothread->init_done_lock); From 61408b250eaa6157e49fbdcfe21f2f53e50b9277 Mon Sep 17 00:00:00 2001 From: Fam Zheng Date: Tue, 1 Dec 2015 17:36:28 +0800 Subject: [PATCH 2/4] block: Don't wait serialising for non-COR read requests The assertion problem was noticed in 06c3916b35a, but it wasn't completely fixed, because even though the req is not marked as serialising, it still gets serialised by wait_serialising_requests against other serialising requests, which could lead to the same assertion failure. Fix it by even more explicitly skipping the serialising for this specific case. Signed-off-by: Fam Zheng Message-id: 1448962590-2842-2-git-send-email-famz@redhat.com Signed-off-by: Stefan Hajnoczi --- block/backup.c | 2 +- block/io.c | 12 +++++++----- include/block/block.h | 4 ++-- trace-events | 2 +- 4 files changed, 11 insertions(+), 9 deletions(-) diff --git a/block/backup.c b/block/backup.c index 3b39119256..705bb77661 100644 --- a/block/backup.c +++ b/block/backup.c @@ -132,7 +132,7 @@ static int coroutine_fn backup_do_cow(BlockDriverState *bs, qemu_iovec_init_external(&bounce_qiov, &iov, 1); if (is_write_notifier) { - ret = bdrv_co_no_copy_on_readv(bs, + ret = bdrv_co_readv_no_serialising(bs, start * BACKUP_SECTORS_PER_CLUSTER, n, &bounce_qiov); } else { diff --git a/block/io.c b/block/io.c index adc1eabef0..e00fb5d690 100644 --- a/block/io.c +++ b/block/io.c @@ -863,7 +863,9 @@ static int coroutine_fn bdrv_aligned_preadv(BlockDriverState *bs, mark_request_serialising(req, bdrv_get_cluster_size(bs)); } - wait_serialising_requests(req); + if (!(flags & BDRV_REQ_NO_SERIALISING)) { + wait_serialising_requests(req); + } if (flags & BDRV_REQ_COPY_ON_READ) { int pnum; @@ -952,7 +954,7 @@ static int coroutine_fn bdrv_co_do_preadv(BlockDriverState *bs, } /* Don't do copy-on-read if we read data before write operation */ - if (bs->copy_on_read && !(flags & BDRV_REQ_NO_COPY_ON_READ)) { + if (bs->copy_on_read && !(flags & BDRV_REQ_NO_SERIALISING)) { flags |= BDRV_REQ_COPY_ON_READ; } @@ -1021,13 +1023,13 @@ int coroutine_fn bdrv_co_readv(BlockDriverState *bs, int64_t sector_num, return bdrv_co_do_readv(bs, sector_num, nb_sectors, qiov, 0); } -int coroutine_fn bdrv_co_no_copy_on_readv(BlockDriverState *bs, +int coroutine_fn bdrv_co_readv_no_serialising(BlockDriverState *bs, int64_t sector_num, int nb_sectors, QEMUIOVector *qiov) { - trace_bdrv_co_no_copy_on_readv(bs, sector_num, nb_sectors); + trace_bdrv_co_readv_no_serialising(bs, sector_num, nb_sectors); return bdrv_co_do_readv(bs, sector_num, nb_sectors, qiov, - BDRV_REQ_NO_COPY_ON_READ); + BDRV_REQ_NO_SERIALISING); } int coroutine_fn bdrv_co_copy_on_readv(BlockDriverState *bs, diff --git a/include/block/block.h b/include/block/block.h index 73edb1a79c..3477328737 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -61,7 +61,7 @@ typedef enum { * opened with BDRV_O_UNMAP. */ BDRV_REQ_MAY_UNMAP = 0x4, - BDRV_REQ_NO_COPY_ON_READ = 0x8, + BDRV_REQ_NO_SERIALISING = 0x8, } BdrvRequestFlags; typedef struct BlockSizes { @@ -248,7 +248,7 @@ int coroutine_fn bdrv_co_readv(BlockDriverState *bs, int64_t sector_num, int nb_sectors, QEMUIOVector *qiov); int coroutine_fn bdrv_co_copy_on_readv(BlockDriverState *bs, int64_t sector_num, int nb_sectors, QEMUIOVector *qiov); -int coroutine_fn bdrv_co_no_copy_on_readv(BlockDriverState *bs, +int coroutine_fn bdrv_co_readv_no_serialising(BlockDriverState *bs, int64_t sector_num, int nb_sectors, QEMUIOVector *qiov); int coroutine_fn bdrv_co_writev(BlockDriverState *bs, int64_t sector_num, int nb_sectors, QEMUIOVector *qiov); diff --git a/trace-events b/trace-events index 0b0ff02442..2fce98e762 100644 --- a/trace-events +++ b/trace-events @@ -69,7 +69,7 @@ bdrv_aio_write_zeroes(void *bs, int64_t sector_num, int nb_sectors, int flags, v bdrv_lock_medium(void *bs, bool locked) "bs %p locked %d" bdrv_co_readv(void *bs, int64_t sector_num, int nb_sector) "bs %p sector_num %"PRId64" nb_sectors %d" bdrv_co_copy_on_readv(void *bs, int64_t sector_num, int nb_sector) "bs %p sector_num %"PRId64" nb_sectors %d" -bdrv_co_no_copy_on_readv(void *bs, int64_t sector_num, int nb_sector) "bs %p sector_num %"PRId64" nb_sectors %d" +bdrv_co_readv_no_serialising(void *bs, int64_t sector_num, int nb_sector) "bs %p sector_num %"PRId64" nb_sectors %d" bdrv_co_writev(void *bs, int64_t sector_num, int nb_sector) "bs %p sector_num %"PRId64" nb_sectors %d" bdrv_co_write_zeroes(void *bs, int64_t sector_num, int nb_sector, int flags) "bs %p sector_num %"PRId64" nb_sectors %d flags %#x" bdrv_co_io_em(void *bs, int64_t sector_num, int nb_sectors, int is_write, void *acb) "bs %p sector_num %"PRId64" nb_sectors %d is_write %d acb %p" From 78b666f46b160441516c194a14432611262429d3 Mon Sep 17 00:00:00 2001 From: Fam Zheng Date: Tue, 1 Dec 2015 17:36:29 +0800 Subject: [PATCH 3/4] iotests: Add "add_drive_raw" method This offers full manual control over the "-drive" options. Signed-off-by: Fam Zheng Message-id: 1448962590-2842-3-git-send-email-famz@redhat.com Signed-off-by: Stefan Hajnoczi --- tests/qemu-iotests/iotests.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py index ff5905f379..e02245ed07 100644 --- a/tests/qemu-iotests/iotests.py +++ b/tests/qemu-iotests/iotests.py @@ -140,6 +140,11 @@ class VM(object): self._args.append('-monitor') self._args.append(args) + def add_drive_raw(self, opts): + self._args.append('-drive') + self._args.append(opts) + return self + def add_drive(self, path, opts='', interface='virtio'): '''Add a virtio-blk drive to the VM''' options = ['if=%s' % interface, From 9cc0f19de213fcb7098f0ea8f42448728f2cfcde Mon Sep 17 00:00:00 2001 From: Fam Zheng Date: Tue, 1 Dec 2015 17:36:30 +0800 Subject: [PATCH 4/4] iotests: Add regresion test case for write notifier assertion failure The idea is to let the top level bs have a big request alignment with blkdebug, so that the aio_write request issued from monitor will be serialised. This tests that QEMU doesn't crash upon the read request from the backup job's write notifier, which is a very special case of "reentrant" request. Signed-off-by: Fam Zheng Message-id: 1448962590-2842-4-git-send-email-famz@redhat.com Signed-off-by: Stefan Hajnoczi --- tests/qemu-iotests/056 | 25 +++++++++++++++++++++++++ tests/qemu-iotests/056.out | 4 ++-- 2 files changed, 27 insertions(+), 2 deletions(-) diff --git a/tests/qemu-iotests/056 b/tests/qemu-iotests/056 index 54e4bd0692..04f2c3c841 100755 --- a/tests/qemu-iotests/056 +++ b/tests/qemu-iotests/056 @@ -82,6 +82,31 @@ class TestSyncModesNoneAndTop(iotests.QMPTestCase): time.sleep(1) self.assertEqual(-1, qemu_io('-c', 'read -P0x41 0 512', target_img).find("verification failed")) +class TestBeforeWriteNotifier(iotests.QMPTestCase): + def setUp(self): + self.vm = iotests.VM().add_drive_raw("file=blkdebug::null-co://,id=drive0,align=65536,driver=blkdebug") + self.vm.launch() + + def tearDown(self): + self.vm.shutdown() + os.remove(target_img) + + def test_before_write_notifier(self): + self.vm.pause_drive("drive0") + result = self.vm.qmp('drive-backup', device='drive0', + sync='full', target=target_img, + format="file", speed=1) + self.assert_qmp(result, 'return', {}) + result = self.vm.qmp('block-job-pause', device="drive0") + self.assert_qmp(result, 'return', {}) + # Speed is low enough that this must be an uncopied range, which will + # trigger the before write notifier + self.vm.hmp_qemu_io('drive0', 'aio_write -P 1 512512 512') + self.vm.resume_drive("drive0") + result = self.vm.qmp('block-job-resume', device="drive0") + self.assert_qmp(result, 'return', {}) + event = self.cancel_and_wait() + self.assert_qmp(event, 'data/type', 'backup') if __name__ == '__main__': iotests.main(supported_fmts=['qcow2', 'qed']) diff --git a/tests/qemu-iotests/056.out b/tests/qemu-iotests/056.out index fbc63e62f8..8d7e996700 100644 --- a/tests/qemu-iotests/056.out +++ b/tests/qemu-iotests/056.out @@ -1,5 +1,5 @@ -.. +... ---------------------------------------------------------------------- -Ran 2 tests +Ran 3 tests OK