From 8f3a73bc57ea83e5b3930d14fc596ea51859987a Mon Sep 17 00:00:00 2001 From: Max Reitz Date: Fri, 29 Jan 2016 20:49:10 +0100 Subject: [PATCH 01/50] block: Add blk_dev_has_tray() Pull out the check whether a block device has a tray from blk_dev_is_tray_open() into its own function so both attributes (whether there is a tray vs. whether that tray is open) can be queried independently. Cc: qemu-stable Signed-off-by: Max Reitz Reviewed-by: Eric Blake Reviewed-by: Alberto Garcia Message-id: 1454096953-31773-2-git-send-email-mreitz@redhat.com --- block/block-backend.c | 10 +++++++++- include/block/block_int.h | 1 + 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/block/block-backend.c b/block/block-backend.c index efd61464da..a4208f18c2 100644 --- a/block/block-backend.c +++ b/block/block-backend.c @@ -458,6 +458,14 @@ bool blk_dev_has_removable_media(BlockBackend *blk) return !blk->dev || (blk->dev_ops && blk->dev_ops->change_media_cb); } +/* + * Does @blk's attached device model have a tray? + */ +bool blk_dev_has_tray(BlockBackend *blk) +{ + return blk->dev_ops && blk->dev_ops->is_tray_open; +} + /* * Notify @blk's attached device model of a media eject request. * If @force is true, the medium is about to be yanked out forcefully. @@ -474,7 +482,7 @@ void blk_dev_eject_request(BlockBackend *blk, bool force) */ bool blk_dev_is_tray_open(BlockBackend *blk) { - if (blk->dev_ops && blk->dev_ops->is_tray_open) { + if (blk_dev_has_tray(blk)) { return blk->dev_ops->is_tray_open(blk->dev_opaque); } return false; diff --git a/include/block/block_int.h b/include/block/block_int.h index 428fa3397e..ec31df1106 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -695,6 +695,7 @@ void blk_set_bs(BlockBackend *blk, BlockDriverState *bs); void blk_dev_change_media_cb(BlockBackend *blk, bool load); bool blk_dev_has_removable_media(BlockBackend *blk); +bool blk_dev_has_tray(BlockBackend *blk); void blk_dev_eject_request(BlockBackend *blk, bool force); bool blk_dev_is_tray_open(BlockBackend *blk); bool blk_dev_is_medium_locked(BlockBackend *blk); From 12c7ec87a7d88919b23736176eba3118d1521372 Mon Sep 17 00:00:00 2001 From: Max Reitz Date: Fri, 29 Jan 2016 20:49:11 +0100 Subject: [PATCH 02/50] blockdev: Fix 'change' for slot devices 'change' and related operations did not work when used on guest devices featuring removable media but no actual tray, because blk_dev_is_tray_open() always returned false for them and the blockdev-{insert,remove}-medium commands required it to return true. Fix this by making blockdev-{insert,remove}-medium work on tray-less devices. Also, blockdev-{open,close}-tray are now explicitly no-ops when invoked on such devices, and blk_dev_change_media_cb() is instead called by blockdev-{insert,remove}-medium (for tray-less devices only). Reported-by: Peter Maydell Cc: qemu-stable Signed-off-by: Max Reitz Reviewed-by: Alberto Garcia Message-id: 1454096953-31773-3-git-send-email-mreitz@redhat.com Reviewed-by: Eric Blake --- blockdev.c | 31 +++++++++++++++++++++++++++++-- qapi/block-core.json | 3 +-- 2 files changed, 30 insertions(+), 4 deletions(-) diff --git a/blockdev.c b/blockdev.c index 07cfe25e1e..1044a6acad 100644 --- a/blockdev.c +++ b/blockdev.c @@ -2304,6 +2304,11 @@ void qmp_blockdev_open_tray(const char *device, bool has_force, bool force, return; } + if (!blk_dev_has_tray(blk)) { + /* Ignore this command on tray-less devices */ + return; + } + if (blk_dev_is_tray_open(blk)) { return; } @@ -2334,6 +2339,11 @@ void qmp_blockdev_close_tray(const char *device, Error **errp) return; } + if (!blk_dev_has_tray(blk)) { + /* Ignore this command on tray-less devices */ + return; + } + if (!blk_dev_is_tray_open(blk)) { return; } @@ -2363,7 +2373,7 @@ void qmp_x_blockdev_remove_medium(const char *device, Error **errp) return; } - if (has_device && !blk_dev_is_tray_open(blk)) { + if (has_device && blk_dev_has_tray(blk) && !blk_dev_is_tray_open(blk)) { error_setg(errp, "Tray of device '%s' is not open", device); return; } @@ -2388,6 +2398,14 @@ void qmp_x_blockdev_remove_medium(const char *device, Error **errp) blk_remove_bs(blk); + if (!blk_dev_has_tray(blk)) { + /* For tray-less devices, blockdev-open-tray is a no-op (or may not be + * called at all); therefore, the medium needs to be ejected here. + * Do it after blk_remove_bs() so blk_is_inserted(blk) returns the @load + * value passed here (i.e. false). */ + blk_dev_change_media_cb(blk, false); + } + out: aio_context_release(aio_context); } @@ -2413,7 +2431,7 @@ static void qmp_blockdev_insert_anon_medium(const char *device, return; } - if (has_device && !blk_dev_is_tray_open(blk)) { + if (has_device && blk_dev_has_tray(blk) && !blk_dev_is_tray_open(blk)) { error_setg(errp, "Tray of device '%s' is not open", device); return; } @@ -2426,6 +2444,15 @@ static void qmp_blockdev_insert_anon_medium(const char *device, blk_insert_bs(blk, bs); QTAILQ_INSERT_TAIL(&bdrv_states, bs, device_list); + + if (!blk_dev_has_tray(blk)) { + /* For tray-less devices, blockdev-close-tray is a no-op (or may not be + * called at all); therefore, the medium needs to be pushed into the + * slot here. + * Do it after blk_insert_bs() so blk_is_inserted(blk) returns the @load + * value passed here (i.e. true). */ + blk_dev_change_media_cb(blk, true); + } } void qmp_x_blockdev_insert_medium(const char *device, const char *node_name, diff --git a/qapi/block-core.json b/qapi/block-core.json index 0a915eda59..40239bfcf9 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -2098,8 +2098,7 @@ # respond to the eject request # - if the BlockBackend denoted by @device does not have a guest device attached # to it -# - if the guest device does not have an actual tray and is empty, for instance -# for floppy disk drives +# - if the guest device does not have an actual tray # # @device: block device name # From abb3e55b5b718d6392441f56ba0729a62105ac56 Mon Sep 17 00:00:00 2001 From: Max Reitz Date: Fri, 29 Jan 2016 20:49:12 +0100 Subject: [PATCH 03/50] Revert "hw/block/fdc: Implement tray status" This reverts the changes that commit 2e1280e8ff95b3145bc6262accc9d447718e5318 applied to hw/block/fdc.c; also, an additional case of drv->media_inserted use has crept in since, which is replaced by a call to blk_is_inserted(). That commit changed tests/fdc-test.c, too, because after it, one less TRAY_MOVED event would be emitted when executing 'change' on an empty drive. However, now, no TRAY_MOVED events will be emitted at all, and the tray_open status returned by query-block will always be false, necessitating (different) changes to tests/fdc-test.c and iotest 118, which is why this patch is not a pure revert of said commit. Signed-off-by: Max Reitz Message-id: 1454096953-31773-4-git-send-email-mreitz@redhat.com Reviewed-by: Eric Blake --- hw/block/fdc.c | 23 +++----- tests/fdc-test.c | 2 - tests/qemu-iotests/118 | 117 +++++++++++++---------------------------- 3 files changed, 43 insertions(+), 99 deletions(-) diff --git a/hw/block/fdc.c b/hw/block/fdc.c index e3b0e1e60c..818e8a4072 100644 --- a/hw/block/fdc.c +++ b/hw/block/fdc.c @@ -173,7 +173,6 @@ typedef struct FDrive { uint8_t media_changed; /* Is media changed */ uint8_t media_rate; /* Data rate of medium */ - bool media_inserted; /* Is there a medium in the tray */ bool media_validated; /* Have we validated the media? */ } FDrive; @@ -249,7 +248,7 @@ static int fd_seek(FDrive *drv, uint8_t head, uint8_t track, uint8_t sect, #endif drv->head = head; if (drv->track != track) { - if (drv->media_inserted) { + if (drv->blk != NULL && blk_is_inserted(drv->blk)) { drv->media_changed = 0; } ret = 1; @@ -258,7 +257,7 @@ static int fd_seek(FDrive *drv, uint8_t head, uint8_t track, uint8_t sect, drv->sect = sect; } - if (!drv->media_inserted) { + if (drv->blk == NULL || !blk_is_inserted(drv->blk)) { ret = 2; } @@ -288,7 +287,9 @@ static int pick_geometry(FDrive *drv) bool magic = drv->drive == FLOPPY_DRIVE_TYPE_AUTO; /* We can only pick a geometry if we have a diskette. */ - if (!drv->media_inserted || drv->drive == FLOPPY_DRIVE_TYPE_NONE) { + if (!drv->blk || !blk_is_inserted(drv->blk) || + drv->drive == FLOPPY_DRIVE_TYPE_NONE) + { return -1; } @@ -390,7 +391,7 @@ static void fd_revalidate(FDrive *drv) FLOPPY_DPRINTF("revalidate\n"); if (drv->blk != NULL) { drv->ro = blk_is_read_only(drv->blk); - if (!drv->media_inserted) { + if (!blk_is_inserted(drv->blk)) { FLOPPY_DPRINTF("No disk in drive\n"); drv->disk = FLOPPY_DRIVE_TYPE_NONE; } else if (!drv->media_validated) { @@ -793,7 +794,7 @@ static bool fdrive_media_changed_needed(void *opaque) { FDrive *drive = opaque; - return (drive->media_inserted && drive->media_changed != 1); + return (drive->blk != NULL && drive->media_changed != 1); } static const VMStateDescription vmstate_fdrive_media_changed = { @@ -2285,22 +2286,13 @@ static void fdctrl_change_cb(void *opaque, bool load) { FDrive *drive = opaque; - drive->media_inserted = load && drive->blk && blk_is_inserted(drive->blk); - drive->media_changed = 1; drive->media_validated = false; fd_revalidate(drive); } -static bool fdctrl_is_tray_open(void *opaque) -{ - FDrive *drive = opaque; - return !drive->media_inserted; -} - static const BlockDevOps fdctrl_block_ops = { .change_media_cb = fdctrl_change_cb, - .is_tray_open = fdctrl_is_tray_open, }; /* Init functions */ @@ -2327,7 +2319,6 @@ static void fdctrl_connect_drives(FDCtrl *fdctrl, Error **errp) fd_init(drive); if (drive->blk) { blk_set_dev_ops(drive->blk, &fdctrl_block_ops, drive); - drive->media_inserted = blk_is_inserted(drive->blk); pick_drive_type(drive); } fd_revalidate(drive); diff --git a/tests/fdc-test.c b/tests/fdc-test.c index 526d4595c3..dbabf50a9a 100644 --- a/tests/fdc-test.c +++ b/tests/fdc-test.c @@ -304,7 +304,6 @@ static void test_media_insert(void) qmp_discard_response("{'execute':'change', 'arguments':{" " 'device':'floppy0', 'target': %s, 'arg': 'raw' }}", test_image); - qmp_discard_response(""); /* ignore event (open -> close) */ dir = inb(FLOPPY_BASE + reg_dir); assert_bit_set(dir, DSKCHG); @@ -335,7 +334,6 @@ static void test_media_change(void) * reset the bit. */ qmp_discard_response("{'execute':'eject', 'arguments':{" " 'device':'floppy0' }}"); - qmp_discard_response(""); /* ignore event */ dir = inb(FLOPPY_BASE + reg_dir); assert_bit_set(dir, DSKCHG); diff --git a/tests/qemu-iotests/118 b/tests/qemu-iotests/118 index 114d0e2dea..7caa38cbe9 100755 --- a/tests/qemu-iotests/118 +++ b/tests/qemu-iotests/118 @@ -42,6 +42,9 @@ class ChangeBaseClass(iotests.QMPTestCase): self.has_opened = True def wait_for_open(self): + if not self.has_real_tray: + return + timeout = time.clock() + 3 while not self.has_opened and time.clock() < timeout: self.process_events() @@ -49,6 +52,9 @@ class ChangeBaseClass(iotests.QMPTestCase): self.fail('Timeout while waiting for the tray to open') def wait_for_close(self): + if not self.has_real_tray: + return + timeout = time.clock() + 3 while not self.has_closed and time.clock() < timeout: self.process_events() @@ -65,7 +71,8 @@ class GeneralChangeTestsBaseClass(ChangeBaseClass): self.wait_for_close() result = self.vm.qmp('query-block') - self.assert_qmp(result, 'return[0]/tray_open', False) + if self.has_real_tray: + self.assert_qmp(result, 'return[0]/tray_open', False) self.assert_qmp(result, 'return[0]/inserted/image/filename', new_img) def test_blockdev_change_medium(self): @@ -78,7 +85,8 @@ class GeneralChangeTestsBaseClass(ChangeBaseClass): self.wait_for_close() result = self.vm.qmp('query-block') - self.assert_qmp(result, 'return[0]/tray_open', False) + if self.has_real_tray: + self.assert_qmp(result, 'return[0]/tray_open', False) self.assert_qmp(result, 'return[0]/inserted/image/filename', new_img) def test_eject(self): @@ -88,7 +96,8 @@ class GeneralChangeTestsBaseClass(ChangeBaseClass): self.wait_for_open() result = self.vm.qmp('query-block') - self.assert_qmp(result, 'return[0]/tray_open', True) + if self.has_real_tray: + self.assert_qmp(result, 'return[0]/tray_open', True) self.assert_qmp_absent(result, 'return[0]/inserted') def test_tray_eject_change(self): @@ -98,7 +107,8 @@ class GeneralChangeTestsBaseClass(ChangeBaseClass): self.wait_for_open() result = self.vm.qmp('query-block') - self.assert_qmp(result, 'return[0]/tray_open', True) + if self.has_real_tray: + self.assert_qmp(result, 'return[0]/tray_open', True) self.assert_qmp_absent(result, 'return[0]/inserted') result = self.vm.qmp('blockdev-change-medium', device='drive0', @@ -109,7 +119,8 @@ class GeneralChangeTestsBaseClass(ChangeBaseClass): self.wait_for_close() result = self.vm.qmp('query-block') - self.assert_qmp(result, 'return[0]/tray_open', False) + if self.has_real_tray: + self.assert_qmp(result, 'return[0]/tray_open', False) self.assert_qmp(result, 'return[0]/inserted/image/filename', new_img) def test_tray_open_close(self): @@ -119,7 +130,8 @@ class GeneralChangeTestsBaseClass(ChangeBaseClass): self.wait_for_open() result = self.vm.qmp('query-block') - self.assert_qmp(result, 'return[0]/tray_open', True) + if self.has_real_tray: + self.assert_qmp(result, 'return[0]/tray_open', True) if self.was_empty == True: self.assert_qmp_absent(result, 'return[0]/inserted') else: @@ -132,10 +144,8 @@ class GeneralChangeTestsBaseClass(ChangeBaseClass): self.wait_for_close() result = self.vm.qmp('query-block') - if self.has_real_tray or not self.was_empty: + if self.has_real_tray: self.assert_qmp(result, 'return[0]/tray_open', False) - else: - self.assert_qmp(result, 'return[0]/tray_open', True) if self.was_empty == True: self.assert_qmp_absent(result, 'return[0]/inserted') else: @@ -148,20 +158,18 @@ class GeneralChangeTestsBaseClass(ChangeBaseClass): self.wait_for_open() result = self.vm.qmp('query-block') - self.assert_qmp(result, 'return[0]/tray_open', True) + if self.has_real_tray: + self.assert_qmp(result, 'return[0]/tray_open', True) self.assert_qmp_absent(result, 'return[0]/inserted') result = self.vm.qmp('blockdev-close-tray', device='drive0') self.assert_qmp(result, 'return', {}) - if self.has_real_tray: - self.wait_for_close() + self.wait_for_close() result = self.vm.qmp('query-block') if self.has_real_tray: self.assert_qmp(result, 'return[0]/tray_open', False) - else: - self.assert_qmp(result, 'return[0]/tray_open', True) self.assert_qmp_absent(result, 'return[0]/inserted') def test_tray_open_change(self): @@ -171,7 +179,8 @@ class GeneralChangeTestsBaseClass(ChangeBaseClass): self.wait_for_open() result = self.vm.qmp('query-block') - self.assert_qmp(result, 'return[0]/tray_open', True) + if self.has_real_tray: + self.assert_qmp(result, 'return[0]/tray_open', True) if self.was_empty == True: self.assert_qmp_absent(result, 'return[0]/inserted') else: @@ -185,7 +194,8 @@ class GeneralChangeTestsBaseClass(ChangeBaseClass): self.wait_for_close() result = self.vm.qmp('query-block') - self.assert_qmp(result, 'return[0]/tray_open', False) + if self.has_real_tray: + self.assert_qmp(result, 'return[0]/tray_open', False) self.assert_qmp(result, 'return[0]/inserted/image/filename', new_img) def test_cycle(self): @@ -202,7 +212,8 @@ class GeneralChangeTestsBaseClass(ChangeBaseClass): self.wait_for_open() result = self.vm.qmp('query-block') - self.assert_qmp(result, 'return[0]/tray_open', True) + if self.has_real_tray: + self.assert_qmp(result, 'return[0]/tray_open', True) if self.was_empty == True: self.assert_qmp_absent(result, 'return[0]/inserted') else: @@ -212,7 +223,8 @@ class GeneralChangeTestsBaseClass(ChangeBaseClass): self.assert_qmp(result, 'return', {}) result = self.vm.qmp('query-block') - self.assert_qmp(result, 'return[0]/tray_open', True) + if self.has_real_tray: + self.assert_qmp(result, 'return[0]/tray_open', True) self.assert_qmp_absent(result, 'return[0]/inserted') result = self.vm.qmp('x-blockdev-insert-medium', device='drive0', @@ -220,7 +232,8 @@ class GeneralChangeTestsBaseClass(ChangeBaseClass): self.assert_qmp(result, 'return', {}) result = self.vm.qmp('query-block') - self.assert_qmp(result, 'return[0]/tray_open', True) + if self.has_real_tray: + self.assert_qmp(result, 'return[0]/tray_open', True) self.assert_qmp(result, 'return[0]/inserted/image/filename', new_img) result = self.vm.qmp('blockdev-close-tray', device='drive0') @@ -229,7 +242,8 @@ class GeneralChangeTestsBaseClass(ChangeBaseClass): self.wait_for_close() result = self.vm.qmp('query-block') - self.assert_qmp(result, 'return[0]/tray_open', False) + if self.has_real_tray: + self.assert_qmp(result, 'return[0]/tray_open', False) self.assert_qmp(result, 'return[0]/inserted/image/filename', new_img) def test_close_on_closed(self): @@ -239,16 +253,14 @@ class GeneralChangeTestsBaseClass(ChangeBaseClass): self.assertEquals(self.vm.get_qmp_events(wait=False), []) def test_remove_on_closed(self): - if self.has_opened: - # Empty floppy drive + if not self.has_real_tray: return result = self.vm.qmp('x-blockdev-remove-medium', device='drive0') self.assert_qmp(result, 'error/class', 'GenericError') def test_insert_on_closed(self): - if self.has_opened: - # Empty floppy drive + if not self.has_real_tray: return result = self.vm.qmp('blockdev-add', @@ -366,7 +378,6 @@ class TestChangeReadOnly(ChangeBaseClass): self.vm.launch() result = self.vm.qmp('query-block') - self.assert_qmp(result, 'return[0]/tray_open', False) self.assert_qmp(result, 'return[0]/inserted/ro', True) self.assert_qmp(result, 'return[0]/inserted/image/filename', old_img) @@ -376,11 +387,7 @@ class TestChangeReadOnly(ChangeBaseClass): read_only_mode='retain') self.assert_qmp(result, 'return', {}) - self.wait_for_open() - self.wait_for_close() - result = self.vm.qmp('query-block') - self.assert_qmp(result, 'return[0]/tray_open', False) self.assert_qmp(result, 'return[0]/inserted/ro', True) self.assert_qmp(result, 'return[0]/inserted/image/filename', new_img) @@ -390,7 +397,6 @@ class TestChangeReadOnly(ChangeBaseClass): self.vm.launch() result = self.vm.qmp('query-block') - self.assert_qmp(result, 'return[0]/tray_open', False) self.assert_qmp(result, 'return[0]/inserted/ro', True) self.assert_qmp(result, 'return[0]/inserted/image/filename', old_img) @@ -400,11 +406,7 @@ class TestChangeReadOnly(ChangeBaseClass): read_only_mode='retain') self.assert_qmp(result, 'return', {}) - self.wait_for_open() - self.wait_for_close() - result = self.vm.qmp('query-block') - self.assert_qmp(result, 'return[0]/tray_open', False) self.assert_qmp(result, 'return[0]/inserted/ro', True) self.assert_qmp(result, 'return[0]/inserted/image/filename', new_img) @@ -414,7 +416,6 @@ class TestChangeReadOnly(ChangeBaseClass): self.vm.launch() result = self.vm.qmp('query-block') - self.assert_qmp(result, 'return[0]/tray_open', False) self.assert_qmp(result, 'return[0]/inserted/ro', False) self.assert_qmp(result, 'return[0]/inserted/image/filename', old_img) @@ -427,7 +428,6 @@ class TestChangeReadOnly(ChangeBaseClass): self.assertEquals(self.vm.get_qmp_events(wait=False), []) result = self.vm.qmp('query-block') - self.assert_qmp(result, 'return[0]/tray_open', False) self.assert_qmp(result, 'return[0]/inserted/ro', False) self.assert_qmp(result, 'return[0]/inserted/image/filename', old_img) @@ -437,7 +437,6 @@ class TestChangeReadOnly(ChangeBaseClass): self.vm.launch() result = self.vm.qmp('query-block') - self.assert_qmp(result, 'return[0]/tray_open', False) self.assert_qmp(result, 'return[0]/inserted/ro', True) self.assert_qmp(result, 'return[0]/inserted/image/filename', old_img) @@ -448,11 +447,7 @@ class TestChangeReadOnly(ChangeBaseClass): read_only_mode='read-write') self.assert_qmp(result, 'return', {}) - self.wait_for_open() - self.wait_for_close() - result = self.vm.qmp('query-block') - self.assert_qmp(result, 'return[0]/tray_open', False) self.assert_qmp(result, 'return[0]/inserted/ro', False) self.assert_qmp(result, 'return[0]/inserted/image/filename', new_img) @@ -462,7 +457,6 @@ class TestChangeReadOnly(ChangeBaseClass): self.vm.launch() result = self.vm.qmp('query-block') - self.assert_qmp(result, 'return[0]/tray_open', False) self.assert_qmp(result, 'return[0]/inserted/ro', False) self.assert_qmp(result, 'return[0]/inserted/image/filename', old_img) @@ -473,11 +467,7 @@ class TestChangeReadOnly(ChangeBaseClass): read_only_mode='read-only') self.assert_qmp(result, 'return', {}) - self.wait_for_open() - self.wait_for_close() - result = self.vm.qmp('query-block') - self.assert_qmp(result, 'return[0]/tray_open', False) self.assert_qmp(result, 'return[0]/inserted/ro', True) self.assert_qmp(result, 'return[0]/inserted/image/filename', new_img) @@ -486,7 +476,6 @@ class TestChangeReadOnly(ChangeBaseClass): self.vm.launch() result = self.vm.qmp('query-block') - self.assert_qmp(result, 'return[0]/tray_open', False) self.assert_qmp(result, 'return[0]/inserted/ro', False) self.assert_qmp(result, 'return[0]/inserted/image/filename', old_img) @@ -497,11 +486,7 @@ class TestChangeReadOnly(ChangeBaseClass): read_only_mode='read-only') self.assert_qmp(result, 'return', {}) - self.wait_for_open() - self.wait_for_close() - result = self.vm.qmp('query-block') - self.assert_qmp(result, 'return[0]/tray_open', False) self.assert_qmp(result, 'return[0]/inserted/ro', True) self.assert_qmp(result, 'return[0]/inserted/image/filename', new_img) @@ -511,7 +496,6 @@ class TestChangeReadOnly(ChangeBaseClass): self.vm.launch() result = self.vm.qmp('query-block') - self.assert_qmp(result, 'return[0]/tray_open', False) self.assert_qmp(result, 'return[0]/inserted/ro', False) self.assert_qmp(result, 'return[0]/inserted/image/filename', old_img) @@ -522,10 +506,7 @@ class TestChangeReadOnly(ChangeBaseClass): read_only_mode='read-write') self.assert_qmp(result, 'error/class', 'GenericError') - self.assertEquals(self.vm.get_qmp_events(wait=False), []) - result = self.vm.qmp('query-block') - self.assert_qmp(result, 'return[0]/tray_open', False) self.assert_qmp(result, 'return[0]/inserted/ro', False) self.assert_qmp(result, 'return[0]/inserted/image/filename', old_img) @@ -535,7 +516,6 @@ class TestChangeReadOnly(ChangeBaseClass): self.vm.launch() result = self.vm.qmp('query-block') - self.assert_qmp(result, 'return[0]/tray_open', False) self.assert_qmp(result, 'return[0]/inserted/ro', True) self.assert_qmp(result, 'return[0]/inserted/image/filename', old_img) @@ -545,11 +525,7 @@ class TestChangeReadOnly(ChangeBaseClass): read_only_mode='retain') self.assert_qmp(result, 'return', {}) - self.wait_for_open() - self.wait_for_close() - result = self.vm.qmp('query-block') - self.assert_qmp(result, 'return[0]/tray_open', False) self.assert_qmp(result, 'return[0]/inserted/ro', True) self.assert_qmp(result, 'return[0]/inserted/image/filename', new_img) @@ -559,7 +535,6 @@ class TestChangeReadOnly(ChangeBaseClass): self.vm.launch() result = self.vm.qmp('query-block') - self.assert_qmp(result, 'return[0]/tray_open', False) self.assert_qmp(result, 'return[0]/inserted/ro', False) self.assert_qmp(result, 'return[0]/inserted/image/filename', old_img) @@ -569,10 +544,7 @@ class TestChangeReadOnly(ChangeBaseClass): read_only_mode='retain') self.assert_qmp(result, 'error/class', 'GenericError') - self.assertEquals(self.vm.get_qmp_events(wait=False), []) - result = self.vm.qmp('query-block') - self.assert_qmp(result, 'return[0]/tray_open', False) self.assert_qmp(result, 'return[0]/inserted/ro', False) self.assert_qmp(result, 'return[0]/inserted/image/filename', old_img) @@ -582,7 +554,6 @@ class TestChangeReadOnly(ChangeBaseClass): self.vm.launch() result = self.vm.qmp('query-block') - self.assert_qmp(result, 'return[0]/tray_open', False) self.assert_qmp(result, 'return[0]/inserted/ro', False) self.assert_qmp(result, 'return[0]/inserted/image/filename', old_img) @@ -594,13 +565,7 @@ class TestChangeReadOnly(ChangeBaseClass): 'driver': 'file'}}) self.assert_qmp(result, 'return', {}) - result = self.vm.qmp('blockdev-open-tray', device='drive0', force=True) - self.assert_qmp(result, 'return', {}) - - self.wait_for_open() - result = self.vm.qmp('query-block') - self.assert_qmp(result, 'return[0]/tray_open', True) self.assert_qmp(result, 'return[0]/inserted/ro', False) self.assert_qmp(result, 'return[0]/inserted/image/filename', old_img) @@ -608,7 +573,6 @@ class TestChangeReadOnly(ChangeBaseClass): self.assert_qmp(result, 'return', {}) result = self.vm.qmp('query-block') - self.assert_qmp(result, 'return[0]/tray_open', True) self.assert_qmp_absent(result, 'return[0]/inserted') result = self.vm.qmp('x-blockdev-insert-medium', device='drive0', @@ -616,17 +580,10 @@ class TestChangeReadOnly(ChangeBaseClass): self.assert_qmp(result, 'return', {}) result = self.vm.qmp('query-block') - self.assert_qmp(result, 'return[0]/tray_open', True) self.assert_qmp(result, 'return[0]/inserted/ro', True) self.assert_qmp(result, 'return[0]/inserted/image/filename', new_img) - result = self.vm.qmp('blockdev-close-tray', device='drive0') - self.assert_qmp(result, 'return', {}) - - self.wait_for_close() - result = self.vm.qmp('query-block') - self.assert_qmp(result, 'return[0]/tray_open', False) self.assert_qmp(result, 'return[0]/inserted/ro', True) self.assert_qmp(result, 'return[0]/inserted/image/filename', new_img) @@ -648,7 +605,6 @@ class TestBlockJobsAfterCycle(ChangeBaseClass): self.assert_qmp(result, 'return', {}) result = self.vm.qmp('query-block') - self.assert_qmp(result, 'return[0]/tray_open', False) self.assert_qmp(result, 'return[0]/inserted/image/format', 'null-co') # For device-less BBs, calling blockdev-open-tray or blockdev-close-tray @@ -671,7 +627,6 @@ class TestBlockJobsAfterCycle(ChangeBaseClass): self.assert_qmp(result, 'return', {}) result = self.vm.qmp('query-block') - self.assert_qmp(result, 'return[0]/tray_open', False) self.assert_qmp(result, 'return[0]/inserted/image/filename', old_img) def tearDown(self): From 327032ce74d0d9fbd4e18528d0f124b68bc56cea Mon Sep 17 00:00:00 2001 From: Max Reitz Date: Fri, 29 Jan 2016 20:49:13 +0100 Subject: [PATCH 04/50] block/qapi: Emit tray_open only if there is a tray Signed-off-by: Max Reitz Reviewed-by: Eric Blake Reviewed-by: Alberto Garcia Message-id: 1454096953-31773-5-git-send-email-mreitz@redhat.com --- block/qapi.c | 2 +- qapi/block-core.json | 4 ++-- tests/qemu-iotests/067.out | 4 ---- 3 files changed, 3 insertions(+), 7 deletions(-) diff --git a/block/qapi.c b/block/qapi.c index a49c118ba0..bbe0c9dddb 100644 --- a/block/qapi.c +++ b/block/qapi.c @@ -300,7 +300,7 @@ static void bdrv_query_info(BlockBackend *blk, BlockInfo **p_info, info->locked = blk_dev_is_medium_locked(blk); info->removable = blk_dev_has_removable_media(blk); - if (blk_dev_has_removable_media(blk)) { + if (blk_dev_has_tray(blk)) { info->has_tray_open = true; info->tray_open = blk_dev_is_tray_open(blk); } diff --git a/qapi/block-core.json b/qapi/block-core.json index 40239bfcf9..628a41dfca 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -382,8 +382,8 @@ # @locked: True if the guest has locked this device from having its media # removed # -# @tray_open: #optional True if the device has a tray and it is open -# (only present if removable is true) +# @tray_open: #optional True if the device's tray is open +# (only present if it has a tray) # # @dirty-bitmaps: #optional dirty bitmaps information (only present if the # driver has one or more dirty bitmaps) (Since 2.0) diff --git a/tests/qemu-iotests/067.out b/tests/qemu-iotests/067.out index 27ad56fe2f..ae3fccb15f 100644 --- a/tests/qemu-iotests/067.out +++ b/tests/qemu-iotests/067.out @@ -169,7 +169,6 @@ Testing: -drive file=TEST_DIR/t.qcow2,format=qcow2,if=none,id=disk "file": "TEST_DIR/t.qcow2", "encryption_key_missing": false }, - "tray_open": false, "type": "unknown" } ] @@ -289,7 +288,6 @@ Testing: "file": "TEST_DIR/t.qcow2", "encryption_key_missing": false }, - "tray_open": false, "type": "unknown" } ] @@ -410,7 +408,6 @@ Testing: "file": "TEST_DIR/t.qcow2", "encryption_key_missing": false }, - "tray_open": false, "type": "unknown" } ] @@ -501,7 +498,6 @@ Testing: "file": "TEST_DIR/t.qcow2", "encryption_key_missing": false }, - "tray_open": false, "type": "unknown" } ] From 3db1d98a20262228373bb973ca62b1ab64b29af4 Mon Sep 17 00:00:00 2001 From: Fam Zheng Date: Mon, 25 Jan 2016 10:26:23 +0800 Subject: [PATCH 05/50] vmdk: Fix converting to streamOptimized Commit d62d9dc4b8 lifted streamOptimized images's version to 3, but we now refuse to open version 3 images read-write. We need to make streamOptimized an exception to allow converting to it. This fixes the accidentally broken iotests case 059 for the same reason. Signed-off-by: Fam Zheng Signed-off-by: Kevin Wolf Signed-off-by: Max Reitz --- block/vmdk.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/block/vmdk.c b/block/vmdk.c index 698679d12c..4a5850bcf0 100644 --- a/block/vmdk.c +++ b/block/vmdk.c @@ -571,6 +571,7 @@ static int vmdk_open_vmdk4(BlockDriverState *bs, VmdkExtent *extent; BDRVVmdkState *s = bs->opaque; int64_t l1_backup_offset = 0; + bool compressed; ret = bdrv_pread(file->bs, sizeof(magic), &header, sizeof(header)); if (ret < 0) { @@ -645,6 +646,8 @@ static int vmdk_open_vmdk4(BlockDriverState *bs, header = footer.header; } + compressed = + le16_to_cpu(header.compressAlgorithm) == VMDK4_COMPRESSION_DEFLATE; if (le32_to_cpu(header.version) > 3) { char buf[64]; snprintf(buf, sizeof(buf), "VMDK version %" PRId32, @@ -652,7 +655,8 @@ static int vmdk_open_vmdk4(BlockDriverState *bs, error_setg(errp, QERR_UNKNOWN_BLOCK_FORMAT_FEATURE, bdrv_get_device_or_node_name(bs), "vmdk", buf); return -ENOTSUP; - } else if (le32_to_cpu(header.version) == 3 && (flags & BDRV_O_RDWR)) { + } else if (le32_to_cpu(header.version) == 3 && (flags & BDRV_O_RDWR) && + !compressed) { /* VMware KB 2064959 explains that version 3 added support for * persistent changed block tracking (CBT), and backup software can * read it as version=1 if it doesn't care about the changed area From cc8c46b7c5fffd6b6ca099799be0af224c589603 Mon Sep 17 00:00:00 2001 From: Max Reitz Date: Mon, 25 Jan 2016 15:42:56 +0100 Subject: [PATCH 06/50] iotests: Limit supported formats for 118 Image formats used in test 118 need to support image creation. Reported-by: Markus Armbruster Signed-off-by: Max Reitz Reviewed-by: Markus Armbruster Signed-off-by: Kevin Wolf --- tests/qemu-iotests/118 | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/qemu-iotests/118 b/tests/qemu-iotests/118 index 7caa38cbe9..9e5951f645 100755 --- a/tests/qemu-iotests/118 +++ b/tests/qemu-iotests/118 @@ -672,4 +672,6 @@ if __name__ == '__main__': # We need floppy and IDE CD-ROM iotests.notrun('not suitable for this machine type: %s' % iotests.qemu_default_machine) - iotests.main() + # Need to support image creation + iotests.main(supported_fmts=['vpc', 'parallels', 'qcow', 'vdi', 'qcow2', + 'vmdk', 'raw', 'vhdx', 'qed']) From d3780c2dce2c452759ee9d94f9d824cf14cc3ab8 Mon Sep 17 00:00:00 2001 From: Max Reitz Date: Mon, 25 Jan 2016 19:41:08 +0100 Subject: [PATCH 07/50] nbd: client_close on error in nbd_co_client_start Use client_close() if an error in nbd_co_client_start() occurs instead of manually inlining parts of it. This fixes an assertion error on the server side if nbd_negotiate() fails. Signed-off-by: Max Reitz Acked-by: Paolo Bonzini Reviewed-by: Eric Blake Signed-off-by: Kevin Wolf --- nbd/server.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/nbd/server.c b/nbd/server.c index 256feafcec..7fa7f53768 100644 --- a/nbd/server.c +++ b/nbd/server.c @@ -1082,8 +1082,7 @@ static coroutine_fn void nbd_co_client_start(void *opaque) nbd_export_get(exp); } if (nbd_negotiate(data)) { - shutdown(client->sock, 2); - client->close(client); + client_close(client); goto out; } qemu_co_mutex_init(&client->send_lock); From 05d0fce49722dd18803a70f00a2ecb1ae92d98d3 Mon Sep 17 00:00:00 2001 From: Max Reitz Date: Mon, 25 Jan 2016 19:41:09 +0100 Subject: [PATCH 08/50] iotests: Rename filter_nbd to _filter_nbd in 083 In the patch after the next, this function is moved to common.filter. Therefore, its name should be preceded by an underscore to signify its global availability. To keep the code motion patch clean, we cannot rename it in the same patch, so we need to choose some order of renaming vs. motion. It is better to keep a supposedly global function used by only a single test in that test than to keep a supposedly local function in a common* file and use it from a test, so we should rename the function before moving it. Signed-off-by: Max Reitz Reviewed-by: John Snow Reviewed-by: Fam Zheng Reviewed-by: Kevin Wolf Signed-off-by: Kevin Wolf --- tests/qemu-iotests/083 | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/qemu-iotests/083 b/tests/qemu-iotests/083 index 566da99323..13495bc137 100755 --- a/tests/qemu-iotests/083 +++ b/tests/qemu-iotests/083 @@ -49,7 +49,7 @@ wait_for_tcp_port() { done } -filter_nbd() { +_filter_nbd() { # nbd.c error messages contain function names and line numbers that are prone # to change. Message ordering depends on timing between send and receive # callbacks sometimes, making them unreliable. @@ -84,7 +84,7 @@ EOF $PYTHON nbd-fault-injector.py $extra_args "127.0.0.1:$port" "$TEST_DIR/nbd-fault-injector.conf" 2>&1 >/dev/null & wait_for_tcp_port "127\\.0\\.0\\.1:$port" - $QEMU_IO -c "read 0 512" "$nbd_url" 2>&1 | _filter_qemu_io | filter_nbd + $QEMU_IO -c "read 0 512" "$nbd_url" 2>&1 | _filter_qemu_io | _filter_nbd echo } From d1f9cd70843f9d66948da46744b656babcc964f8 Mon Sep 17 00:00:00 2001 From: Max Reitz Date: Mon, 25 Jan 2016 19:41:10 +0100 Subject: [PATCH 09/50] iotests: Change coding style of _filter_nbd in 083 In order to be able to move _filter_nbd to common.filter in the next patch, its coding style needs to be adapted to that of common.filter. That means, we have to convert tabs to four spaces, adjust the alignment of the last line (done with spaces already, assuming one tab equals eight spaces), fix the line length of the comment, and add a line break before the opening brace. Signed-off-by: Max Reitz Reviewed-by: Eric Blake Signed-off-by: Kevin Wolf --- tests/qemu-iotests/083 | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/tests/qemu-iotests/083 b/tests/qemu-iotests/083 index 13495bc137..36e6de8168 100755 --- a/tests/qemu-iotests/083 +++ b/tests/qemu-iotests/083 @@ -49,15 +49,16 @@ wait_for_tcp_port() { done } -_filter_nbd() { - # nbd.c error messages contain function names and line numbers that are prone - # to change. Message ordering depends on timing between send and receive - # callbacks sometimes, making them unreliable. - # - # Filter out the TCP port number since this changes between runs. - sed -e 's#^.*nbd/.*\.c:.*##g' \ - -e 's#nbd:127\.0\.0\.1:[^:]*:#nbd:127\.0\.0\.1:PORT:#g' \ - -e 's#\(exportname=foo\|PORT\): Failed to .*$#\1#' +_filter_nbd() +{ + # nbd.c error messages contain function names and line numbers that are + # prone to change. Message ordering depends on timing between send and + # receive callbacks sometimes, making them unreliable. + # + # Filter out the TCP port number since this changes between runs. + sed -e 's#^.*nbd/.*\.c:.*##g' \ + -e 's#nbd:127\.0\.0\.1:[^:]*:#nbd:127\.0\.0\.1:PORT:#g' \ + -e 's#\(exportname=foo\|PORT\): Failed to .*$#\1#' } check_disconnect() { From 60d446881deefc280e60c195009f7f07ac50eeac Mon Sep 17 00:00:00 2001 From: Max Reitz Date: Mon, 25 Jan 2016 19:41:11 +0100 Subject: [PATCH 10/50] iotests: Move _filter_nbd into common.filter _filter_nbd can be useful for other NBD tests, too, therefore it should reside in common.filter. Signed-off-by: Max Reitz Reviewed-by: Eric Blake Signed-off-by: Kevin Wolf --- tests/qemu-iotests/083 | 12 ------------ tests/qemu-iotests/common.filter | 12 ++++++++++++ 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/tests/qemu-iotests/083 b/tests/qemu-iotests/083 index 36e6de8168..aa99278fd8 100755 --- a/tests/qemu-iotests/083 +++ b/tests/qemu-iotests/083 @@ -49,18 +49,6 @@ wait_for_tcp_port() { done } -_filter_nbd() -{ - # nbd.c error messages contain function names and line numbers that are - # prone to change. Message ordering depends on timing between send and - # receive callbacks sometimes, making them unreliable. - # - # Filter out the TCP port number since this changes between runs. - sed -e 's#^.*nbd/.*\.c:.*##g' \ - -e 's#nbd:127\.0\.0\.1:[^:]*:#nbd:127\.0\.0\.1:PORT:#g' \ - -e 's#\(exportname=foo\|PORT\): Failed to .*$#\1#' -} - check_disconnect() { event=$1 when=$2 diff --git a/tests/qemu-iotests/common.filter b/tests/qemu-iotests/common.filter index cfdb6338aa..33ed1e42ab 100644 --- a/tests/qemu-iotests/common.filter +++ b/tests/qemu-iotests/common.filter @@ -230,5 +230,17 @@ _filter_qemu_img_map() -e 's/Mapped to *//' | _filter_testdir | _filter_imgfmt } +_filter_nbd() +{ + # nbd.c error messages contain function names and line numbers that are + # prone to change. Message ordering depends on timing between send and + # receive callbacks sometimes, making them unreliable. + # + # Filter out the TCP port number since this changes between runs. + sed -e 's#^.*nbd/.*\.c:.*##g' \ + -e 's#nbd:127\.0\.0\.1:[^:]*:#nbd:127\.0\.0\.1:PORT:#g' \ + -e 's#\(exportname=foo\|PORT\): Failed to .*$#\1#' +} + # make sure this script returns success true From dd170c06777d11ecee2d4f4977398068b9b38f03 Mon Sep 17 00:00:00 2001 From: Max Reitz Date: Mon, 25 Jan 2016 19:41:12 +0100 Subject: [PATCH 11/50] iotests: Make _filter_nbd drop log lines The NBD log lines ("/your/source/dir/nbd/xyz.c:function():line: error") should not be converted to empty lines but removed altogether. Signed-off-by: Max Reitz Reviewed-by: Eric Blake Signed-off-by: Kevin Wolf --- tests/qemu-iotests/083.out | 10 ---------- tests/qemu-iotests/common.filter | 2 +- 2 files changed, 1 insertion(+), 11 deletions(-) diff --git a/tests/qemu-iotests/083.out b/tests/qemu-iotests/083.out index 78cc49ad91..ef3d1e32a5 100644 --- a/tests/qemu-iotests/083.out +++ b/tests/qemu-iotests/083.out @@ -51,7 +51,6 @@ no file open, try 'help open' === Check disconnect after neg2 === - read failed: Input/output error === Check disconnect 8 neg2 === @@ -66,42 +65,34 @@ no file open, try 'help open' === Check disconnect before request === - read failed: Input/output error === Check disconnect after request === - read failed: Input/output error === Check disconnect before reply === - read failed: Input/output error === Check disconnect after reply === - read failed: Input/output error === Check disconnect 4 reply === - read failed: Input/output error === Check disconnect 8 reply === - read failed: Input/output error === Check disconnect before data === - read failed: Input/output error === Check disconnect after data === - read 512/512 bytes at offset 0 512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) @@ -132,7 +123,6 @@ no file open, try 'help open' === Check disconnect after neg-classic === - read failed: Input/output error *** done diff --git a/tests/qemu-iotests/common.filter b/tests/qemu-iotests/common.filter index 33ed1e42ab..e1bfdb767c 100644 --- a/tests/qemu-iotests/common.filter +++ b/tests/qemu-iotests/common.filter @@ -237,7 +237,7 @@ _filter_nbd() # receive callbacks sometimes, making them unreliable. # # Filter out the TCP port number since this changes between runs. - sed -e 's#^.*nbd/.*\.c:.*##g' \ + sed -e '/nbd\/.*\.c:/d' \ -e 's#nbd:127\.0\.0\.1:[^:]*:#nbd:127\.0\.0\.1:PORT:#g' \ -e 's#\(exportname=foo\|PORT\): Failed to .*$#\1#' } From 4a940d14b3000bee4826f6938e6e0dd67393cf35 Mon Sep 17 00:00:00 2001 From: Max Reitz Date: Mon, 25 Jan 2016 19:41:13 +0100 Subject: [PATCH 12/50] iotests: Make _filter_nbd support more URL types This function should support URLs of the "nbd://" format (without swallowing the export name), and for "nbd:///" URLs it should replace "?socket=$TEST_DIR" by "?socket=TEST_DIR" because putting the Unix socket files into the test directory makes sense. Signed-off-by: Max Reitz Reviewed-by: John Snow Reviewed-by: Fam Zheng Reviewed-by: Kevin Wolf Signed-off-by: Kevin Wolf --- tests/qemu-iotests/common.filter | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/qemu-iotests/common.filter b/tests/qemu-iotests/common.filter index e1bfdb767c..84b7434bcf 100644 --- a/tests/qemu-iotests/common.filter +++ b/tests/qemu-iotests/common.filter @@ -238,7 +238,8 @@ _filter_nbd() # # Filter out the TCP port number since this changes between runs. sed -e '/nbd\/.*\.c:/d' \ - -e 's#nbd:127\.0\.0\.1:[^:]*:#nbd:127\.0\.0\.1:PORT:#g' \ + -e 's#nbd:\(//\)\?127\.0\.0\.1:[0-9]*#nbd:\1127.0.0.1:PORT#g' \ + -e "s#?socket=$TEST_DIR#?socket=TEST_DIR#g" \ -e 's#\(exportname=foo\|PORT\): Failed to .*$#\1#' } From 15cfba693b8a828315618b8df41acfb79017acf3 Mon Sep 17 00:00:00 2001 From: Max Reitz Date: Mon, 25 Jan 2016 19:41:14 +0100 Subject: [PATCH 13/50] iotests: Make redirecting qemu's stderr optional Redirecting qemu's stderr to stdout makes working with the stderr output difficult due to the other file descriptor magic performed in _launch_qemu ("ambiguous redirect"). Add an option which specifies whether stderr should be redirected to stdout or not (allowing for other modes to be added in the future). Signed-off-by: Max Reitz Reviewed-by: Kevin Wolf Reviewed-by: John Snow Reviewed-by: Fam Zheng Signed-off-by: Kevin Wolf --- tests/qemu-iotests/common.qemu | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/tests/qemu-iotests/common.qemu b/tests/qemu-iotests/common.qemu index 8bf3969418..2548a8700b 100644 --- a/tests/qemu-iotests/common.qemu +++ b/tests/qemu-iotests/common.qemu @@ -129,6 +129,8 @@ function _send_qemu_cmd() # $qemu_comm_method: set this variable to 'monitor' (case insensitive) # to use the QEMU HMP monitor for communication. # Otherwise, the default of QMP is used. +# $keep_stderr: Set this variable to 'y' to keep QEMU's stderr output on stderr. +# If this variable is empty, stderr will be redirected to stdout. # Returns: # $QEMU_HANDLE: set to a handle value to communicate with this QEMU instance. # @@ -151,11 +153,20 @@ function _launch_qemu() mkfifo "${fifo_out}" mkfifo "${fifo_in}" - QEMU_NEED_PID='y'\ - ${QEMU} -nographic -serial none ${comm} -machine accel=qtest "${@}" \ + if [ -z "$keep_stderr" ]; then + QEMU_NEED_PID='y'\ + ${QEMU} -nographic -serial none ${comm} -machine accel=qtest "${@}" \ >"${fifo_out}" \ 2>&1 \ <"${fifo_in}" & + elif [ "$keep_stderr" = "y" ]; then + QEMU_NEED_PID='y'\ + ${QEMU} -nographic -serial none ${comm} -machine accel=qtest "${@}" \ + >"${fifo_out}" \ + <"${fifo_in}" & + else + exit 1 + fi if [[ "${BASH_VERSINFO[0]}" -ge "5" || ("${BASH_VERSINFO[0]}" -ge "4" && "${BASH_VERSINFO[1]}" -ge "1") ]] From 34250395fe8f31efee5a736ced9c3b8e33e0dfa3 Mon Sep 17 00:00:00 2001 From: Max Reitz Date: Mon, 25 Jan 2016 19:41:15 +0100 Subject: [PATCH 14/50] iotests: Add test for a nonexistent NBD export Trying to connect to a nonexistent NBD export should not crash the server. Signed-off-by: Max Reitz Reviewed-by: Eric Blake Signed-off-by: Kevin Wolf --- tests/qemu-iotests/143 | 73 ++++++++++++++++++++++++++++++++++++++ tests/qemu-iotests/143.out | 7 ++++ tests/qemu-iotests/group | 1 + 3 files changed, 81 insertions(+) create mode 100755 tests/qemu-iotests/143 create mode 100644 tests/qemu-iotests/143.out diff --git a/tests/qemu-iotests/143 b/tests/qemu-iotests/143 new file mode 100755 index 0000000000..6207368f04 --- /dev/null +++ b/tests/qemu-iotests/143 @@ -0,0 +1,73 @@ +#!/bin/bash +# +# Test case for connecting to a non-existing NBD export name +# +# Copyright (C) 2016 Red Hat, Inc. +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 2 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see . +# + +# creator +owner=mreitz@redhat.com + +seq="$(basename $0)" +echo "QA output created by $seq" + +here="$PWD" +tmp=/tmp/$$ +status=1 # failure is the default! + +_cleanup() +{ + rm -f "$TEST_DIR/nbd" +} +trap "_cleanup; exit \$status" 0 1 2 3 15 + +# get standard environment, filters and checks +. ./common.rc +. ./common.filter +. ./common.qemu + +_supported_fmt generic +_supported_proto generic +_supported_os Linux + +keep_stderr=y \ +_launch_qemu 2> >(_filter_nbd) + +_send_qemu_cmd $QEMU_HANDLE \ + "{ 'execute': 'qmp_capabilities' }" \ + 'return' + +_send_qemu_cmd $QEMU_HANDLE \ + "{ 'execute': 'nbd-server-start', + 'arguments': { 'addr': { 'type': 'unix', + 'data': { 'path': '$TEST_DIR/nbd' }}}}" \ + 'return' + +# This should just result in a client error, not in the server crashing +$QEMU_IO_PROG -f raw -c quit \ + "nbd+unix:///no_such_export?socket=$TEST_DIR/nbd" 2>&1 \ + | _filter_qemu_io | _filter_nbd + +_send_qemu_cmd $QEMU_HANDLE \ + "{ 'execute': 'quit' }" \ + 'return' + +wait=1 _cleanup_qemu + +# success, all done +echo '*** done' +rm -f $seq.full +status=0 diff --git a/tests/qemu-iotests/143.out b/tests/qemu-iotests/143.out new file mode 100644 index 0000000000..dad20240a4 --- /dev/null +++ b/tests/qemu-iotests/143.out @@ -0,0 +1,7 @@ +QA output created by 143 +{"return": {}} +{"return": {}} +can't open device nbd+unix:///no_such_export?socket=TEST_DIR/nbd: Failed to read export length +{"return": {}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN"} +*** done diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group index d6e9219e56..ac6a959258 100644 --- a/tests/qemu-iotests/group +++ b/tests/qemu-iotests/group @@ -142,3 +142,4 @@ 138 rw auto quick 139 rw auto quick 142 auto +143 auto quick From e43f7f6f46fdf518de1bea6646455d91495a58c3 Mon Sep 17 00:00:00 2001 From: Fam Zheng Date: Thu, 28 Jan 2016 11:57:13 +0800 Subject: [PATCH 15/50] block: Remove unused struct definition BlockFinishData Unused since 94db6d2d3. Signed-off-by: Fam Zheng Reviewed-by: Stefan Hajnoczi Signed-off-by: Kevin Wolf Signed-off-by: Max Reitz --- blockjob.c | 8 -------- 1 file changed, 8 deletions(-) diff --git a/blockjob.c b/blockjob.c index 80adb9d52a..a69214254c 100644 --- a/blockjob.c +++ b/blockjob.c @@ -278,14 +278,6 @@ void block_job_iostatus_reset(BlockJob *job) } } -struct BlockFinishData { - BlockJob *job; - BlockCompletionFunc *cb; - void *opaque; - bool cancelled; - int ret; -}; - static int block_job_finish_sync(BlockJob *job, void (*finish)(BlockJob *, Error **errp), Error **errp) From c5acdc9ab4e6aa9b05e6242114479333b15d496b Mon Sep 17 00:00:00 2001 From: Max Reitz Date: Fri, 29 Jan 2016 16:36:01 +0100 Subject: [PATCH 16/50] block: Release named dirty bitmaps in bdrv_close() bdrv_delete() is not very happy about deleting BlockDriverStates with dirty bitmaps still attached to them. In the past, we got around that very easily by relying on bdrv_close_all() bypassing bdrv_delete(), and bdrv_close() simply ignoring that condition. We should fix that by releasing all named dirty bitmaps in bdrv_close() (there should not be any unnamed bitmaps left) and moving the assertion from bdrv_delete() there. Signed-off-by: Max Reitz Signed-off-by: Kevin Wolf --- block.c | 39 +++++++++++++++++++++++++++++++-------- 1 file changed, 31 insertions(+), 8 deletions(-) diff --git a/block.c b/block.c index 5709d3ddb4..41ab00efea 100644 --- a/block.c +++ b/block.c @@ -88,6 +88,8 @@ static int bdrv_open_inherit(BlockDriverState **pbs, const char *filename, const BdrvChildRole *child_role, Error **errp); static void bdrv_dirty_bitmap_truncate(BlockDriverState *bs); +static void bdrv_release_named_dirty_bitmaps(BlockDriverState *bs); + /* If non-zero, use only whitelisted block drivers */ static int use_bdrv_whitelist; @@ -2157,6 +2159,9 @@ void bdrv_close(BlockDriverState *bs) notifier_list_notify(&bs->close_notifiers, bs); + bdrv_release_named_dirty_bitmaps(bs); + assert(QLIST_EMPTY(&bs->dirty_bitmaps)); + if (bs->blk) { blk_dev_change_media_cb(bs->blk, false); } @@ -2366,7 +2371,6 @@ static void bdrv_delete(BlockDriverState *bs) assert(!bs->job); assert(bdrv_op_blocker_is_empty(bs)); assert(!bs->refcnt); - assert(QLIST_EMPTY(&bs->dirty_bitmaps)); bdrv_close(bs); @@ -3582,21 +3586,40 @@ static void bdrv_dirty_bitmap_truncate(BlockDriverState *bs) } } -void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap) +static void bdrv_do_release_matching_dirty_bitmap(BlockDriverState *bs, + BdrvDirtyBitmap *bitmap, + bool only_named) { BdrvDirtyBitmap *bm, *next; QLIST_FOREACH_SAFE(bm, &bs->dirty_bitmaps, list, next) { - if (bm == bitmap) { + if ((!bitmap || bm == bitmap) && (!only_named || bm->name)) { assert(!bdrv_dirty_bitmap_frozen(bm)); - QLIST_REMOVE(bitmap, list); - hbitmap_free(bitmap->bitmap); - g_free(bitmap->name); - g_free(bitmap); - return; + QLIST_REMOVE(bm, list); + hbitmap_free(bm->bitmap); + g_free(bm->name); + g_free(bm); + + if (bitmap) { + return; + } } } } +void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap) +{ + bdrv_do_release_matching_dirty_bitmap(bs, bitmap, false); +} + +/** + * Release all named dirty bitmaps attached to a BDS (for use in bdrv_close()). + * There must not be any frozen bitmaps attached. + */ +static void bdrv_release_named_dirty_bitmaps(BlockDriverState *bs) +{ + bdrv_do_release_matching_dirty_bitmap(bs, NULL, true); +} + void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap) { assert(!bdrv_dirty_bitmap_frozen(bitmap)); From 16dee4183acb3755b8d2e76e6466a6fec5f1350e Mon Sep 17 00:00:00 2001 From: Max Reitz Date: Fri, 29 Jan 2016 16:36:02 +0100 Subject: [PATCH 17/50] iotests: Add test for eject under NBD server This patch adds a test for ejecting the BlockBackend an NBD server is connected to (the NBD server is supposed to stop). Signed-off-by: Max Reitz Reviewed-by: Fam Zheng Reviewed-by: Kevin Wolf Signed-off-by: Kevin Wolf --- tests/qemu-iotests/140 | 92 ++++++++++++++++++++++++++++++++++++++ tests/qemu-iotests/140.out | 16 +++++++ tests/qemu-iotests/group | 1 + 3 files changed, 109 insertions(+) create mode 100755 tests/qemu-iotests/140 create mode 100644 tests/qemu-iotests/140.out diff --git a/tests/qemu-iotests/140 b/tests/qemu-iotests/140 new file mode 100755 index 0000000000..f78c3175a3 --- /dev/null +++ b/tests/qemu-iotests/140 @@ -0,0 +1,92 @@ +#!/bin/bash +# +# Test case for ejecting a BB with an NBD server attached to it +# +# Copyright (C) 2016 Red Hat, Inc. +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 2 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see . +# + +# creator +owner=mreitz@redhat.com + +seq="$(basename $0)" +echo "QA output created by $seq" + +here="$PWD" +tmp=/tmp/$$ +status=1 # failure is the default! + +_cleanup() +{ + _cleanup_test_img + rm -f "$TEST_DIR/nbd" +} +trap "_cleanup; exit \$status" 0 1 2 3 15 + +# get standard environment, filters and checks +. ./common.rc +. ./common.filter +. ./common.qemu + +_supported_fmt generic +_supported_proto file +_supported_os Linux + +_make_test_img 64k + +$QEMU_IO -c 'write -P 42 0 64k' "$TEST_IMG" | _filter_qemu_io + +keep_stderr=y \ +_launch_qemu -drive if=ide,media=cdrom,id=drv,file="$TEST_IMG",format=$IMGFMT \ + 2> >(_filter_nbd) + +_send_qemu_cmd $QEMU_HANDLE \ + "{ 'execute': 'qmp_capabilities' }" \ + 'return' + +_send_qemu_cmd $QEMU_HANDLE \ + "{ 'execute': 'nbd-server-start', + 'arguments': { 'addr': { 'type': 'unix', + 'data': { 'path': '$TEST_DIR/nbd' }}}}" \ + 'return' + +_send_qemu_cmd $QEMU_HANDLE \ + "{ 'execute': 'nbd-server-add', + 'arguments': { 'device': 'drv' }}" \ + 'return' + +$QEMU_IO_PROG -f raw -c 'read -P 42 0 64k' \ + "nbd+unix:///drv?socket=$TEST_DIR/nbd" 2>&1 \ + | _filter_qemu_io | _filter_nbd + +_send_qemu_cmd $QEMU_HANDLE \ + "{ 'execute': 'eject', + 'arguments': { 'device': 'drv' }}" \ + 'return' + +$QEMU_IO_PROG -f raw -c close \ + "nbd+unix:///drv?socket=$TEST_DIR/nbd" 2>&1 \ + | _filter_qemu_io | _filter_nbd + +_send_qemu_cmd $QEMU_HANDLE \ + "{ 'execute': 'quit' }" \ + 'return' + +wait=1 _cleanup_qemu + +# success, all done +echo '*** done' +rm -f $seq.full +status=0 diff --git a/tests/qemu-iotests/140.out b/tests/qemu-iotests/140.out new file mode 100644 index 0000000000..fdedeb3973 --- /dev/null +++ b/tests/qemu-iotests/140.out @@ -0,0 +1,16 @@ +QA output created by 140 +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=65536 +wrote 65536/65536 bytes at offset 0 +64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +{"return": {}} +{"return": {}} +{"return": {}} +read 65536/65536 bytes at offset 0 +64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "DEVICE_TRAY_MOVED", "data": {"device": "drv", "tray-open": true}} +{"return": {}} +can't open device nbd+unix:///drv?socket=TEST_DIR/nbd: Failed to read export length +no file open, try 'help open' +{"return": {}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN"} +*** done diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group index ac6a959258..ff1ff0d06b 100644 --- a/tests/qemu-iotests/group +++ b/tests/qemu-iotests/group @@ -141,5 +141,6 @@ 137 rw auto 138 rw auto quick 139 rw auto quick +140 rw auto quick 142 auto 143 auto quick From 3301f6c6e9b52e370b07b3a08fd11735d0f0f292 Mon Sep 17 00:00:00 2001 From: Max Reitz Date: Fri, 29 Jan 2016 16:36:03 +0100 Subject: [PATCH 18/50] block: Add BB-BDS remove/insert notifiers bdrv_close() no longer signifies ejection of a medium, this is now done by removing the BDS from the BB. Therefore, we want to have a notifier for that in the BB instead of a close notifier in the BDS. The former is added now, the latter is removed later. Symmetrically, another notifier list is added that is invoked whenever a BDS is inserted. We will need that for virtio-blk and virtio-scsi, which can then remove their op blockers on BDS ejection and set them up on insertion. Signed-off-by: Max Reitz Reviewed-by: Kevin Wolf Reviewed-by: Fam Zheng Signed-off-by: Kevin Wolf --- block/block-backend.c | 20 ++++++++++++++++++++ include/sysemu/block-backend.h | 2 ++ 2 files changed, 22 insertions(+) diff --git a/block/block-backend.c b/block/block-backend.c index a4208f18c2..1872191478 100644 --- a/block/block-backend.c +++ b/block/block-backend.c @@ -49,6 +49,8 @@ struct BlockBackend { BlockdevOnError on_read_error, on_write_error; bool iostatus_enabled; BlockDeviceIoStatus iostatus; + + NotifierList remove_bs_notifiers, insert_bs_notifiers; }; typedef struct BlockBackendAIOCB { @@ -99,6 +101,8 @@ BlockBackend *blk_new(const char *name, Error **errp) blk = g_new0(BlockBackend, 1); blk->name = g_strdup(name); blk->refcnt = 1; + notifier_list_init(&blk->remove_bs_notifiers); + notifier_list_init(&blk->insert_bs_notifiers); QTAILQ_INSERT_TAIL(&blk_backends, blk, link); return blk; } @@ -167,6 +171,8 @@ static void blk_delete(BlockBackend *blk) bdrv_unref(blk->bs); blk->bs = NULL; } + assert(QLIST_EMPTY(&blk->remove_bs_notifiers.notifiers)); + assert(QLIST_EMPTY(&blk->insert_bs_notifiers.notifiers)); if (blk->root_state.throttle_state) { g_free(blk->root_state.throttle_group); throttle_group_unref(blk->root_state.throttle_state); @@ -345,6 +351,8 @@ void blk_hide_on_behalf_of_hmp_drive_del(BlockBackend *blk) */ void blk_remove_bs(BlockBackend *blk) { + notifier_list_notify(&blk->remove_bs_notifiers, blk); + blk_update_root_state(blk); blk->bs->blk = NULL; @@ -361,6 +369,8 @@ void blk_insert_bs(BlockBackend *blk, BlockDriverState *bs) bdrv_ref(bs); blk->bs = bs; bs->blk = blk; + + notifier_list_notify(&blk->insert_bs_notifiers, blk); } /* @@ -1126,6 +1136,16 @@ void blk_remove_aio_context_notifier(BlockBackend *blk, } } +void blk_add_remove_bs_notifier(BlockBackend *blk, Notifier *notify) +{ + notifier_list_add(&blk->remove_bs_notifiers, notify); +} + +void blk_add_insert_bs_notifier(BlockBackend *blk, Notifier *notify) +{ + notifier_list_add(&blk->insert_bs_notifiers, notify); +} + void blk_add_close_notifier(BlockBackend *blk, Notifier *notify) { if (blk->bs) { diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h index 1568554e3e..e12be6759a 100644 --- a/include/sysemu/block-backend.h +++ b/include/sysemu/block-backend.h @@ -164,6 +164,8 @@ void blk_remove_aio_context_notifier(BlockBackend *blk, void *), void (*detach_aio_context)(void *), void *opaque); +void blk_add_remove_bs_notifier(BlockBackend *blk, Notifier *notify); +void blk_add_insert_bs_notifier(BlockBackend *blk, Notifier *notify); void blk_add_close_notifier(BlockBackend *blk, Notifier *notify); void blk_io_plug(BlockBackend *blk); void blk_io_unplug(BlockBackend *blk); From 1b1e0659a4c8888fba559e8d41051339e0a3cd8a Mon Sep 17 00:00:00 2001 From: Max Reitz Date: Fri, 29 Jan 2016 16:36:04 +0100 Subject: [PATCH 19/50] virtio-blk: Functions for op blocker management Put the code for setting up and removing op blockers into an own function, respectively. Then, we can invoke those functions whenever a BDS is removed from an virtio-blk BB or inserted into it. Signed-off-by: Max Reitz Reviewed-by: Fam Zheng Reviewed-by: Kevin Wolf Signed-off-by: Kevin Wolf --- hw/block/dataplane/virtio-blk.c | 77 +++++++++++++++++++++++++-------- 1 file changed, 59 insertions(+), 18 deletions(-) diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c index bc34046fb5..ee0c4d4070 100644 --- a/hw/block/dataplane/virtio-blk.c +++ b/hw/block/dataplane/virtio-blk.c @@ -40,6 +40,8 @@ struct VirtIOBlockDataPlane { EventNotifier *guest_notifier; /* irq */ QEMUBH *bh; /* bh for guest notification */ + Notifier insert_notifier, remove_notifier; + /* Note that these EventNotifiers are assigned by value. This is * fine as long as you do not call event_notifier_cleanup on them * (because you don't own the file descriptor or handle; you just @@ -137,6 +139,54 @@ static void handle_notify(EventNotifier *e) blk_io_unplug(s->conf->conf.blk); } +static void data_plane_set_up_op_blockers(VirtIOBlockDataPlane *s) +{ + assert(!s->blocker); + error_setg(&s->blocker, "block device is in use by data plane"); + blk_op_block_all(s->conf->conf.blk, s->blocker); + blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_RESIZE, s->blocker); + blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_DRIVE_DEL, s->blocker); + blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_BACKUP_SOURCE, s->blocker); + blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_CHANGE, s->blocker); + blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_COMMIT_SOURCE, s->blocker); + blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_COMMIT_TARGET, s->blocker); + blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_EJECT, s->blocker); + blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_EXTERNAL_SNAPSHOT, + s->blocker); + blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_INTERNAL_SNAPSHOT, + s->blocker); + blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_INTERNAL_SNAPSHOT_DELETE, + s->blocker); + blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_MIRROR_SOURCE, s->blocker); + blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_STREAM, s->blocker); + blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_REPLACE, s->blocker); +} + +static void data_plane_remove_op_blockers(VirtIOBlockDataPlane *s) +{ + if (s->blocker) { + blk_op_unblock_all(s->conf->conf.blk, s->blocker); + error_free(s->blocker); + s->blocker = NULL; + } +} + +static void data_plane_blk_insert_notifier(Notifier *n, void *data) +{ + VirtIOBlockDataPlane *s = container_of(n, VirtIOBlockDataPlane, + insert_notifier); + assert(s->conf->conf.blk == data); + data_plane_set_up_op_blockers(s); +} + +static void data_plane_blk_remove_notifier(Notifier *n, void *data) +{ + VirtIOBlockDataPlane *s = container_of(n, VirtIOBlockDataPlane, + remove_notifier); + assert(s->conf->conf.blk == data); + data_plane_remove_op_blockers(s); +} + /* Context: QEMU global mutex held */ void virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *conf, VirtIOBlockDataPlane **dataplane, @@ -179,22 +229,12 @@ void virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *conf, s->ctx = iothread_get_aio_context(s->iothread); s->bh = aio_bh_new(s->ctx, notify_guest_bh, s); - error_setg(&s->blocker, "block device is in use by data plane"); - blk_op_block_all(conf->conf.blk, s->blocker); - blk_op_unblock(conf->conf.blk, BLOCK_OP_TYPE_RESIZE, s->blocker); - blk_op_unblock(conf->conf.blk, BLOCK_OP_TYPE_DRIVE_DEL, s->blocker); - blk_op_unblock(conf->conf.blk, BLOCK_OP_TYPE_BACKUP_SOURCE, s->blocker); - blk_op_unblock(conf->conf.blk, BLOCK_OP_TYPE_CHANGE, s->blocker); - blk_op_unblock(conf->conf.blk, BLOCK_OP_TYPE_COMMIT_SOURCE, s->blocker); - blk_op_unblock(conf->conf.blk, BLOCK_OP_TYPE_COMMIT_TARGET, s->blocker); - blk_op_unblock(conf->conf.blk, BLOCK_OP_TYPE_EJECT, s->blocker); - blk_op_unblock(conf->conf.blk, BLOCK_OP_TYPE_EXTERNAL_SNAPSHOT, s->blocker); - blk_op_unblock(conf->conf.blk, BLOCK_OP_TYPE_INTERNAL_SNAPSHOT, s->blocker); - blk_op_unblock(conf->conf.blk, BLOCK_OP_TYPE_INTERNAL_SNAPSHOT_DELETE, - s->blocker); - blk_op_unblock(conf->conf.blk, BLOCK_OP_TYPE_MIRROR_SOURCE, s->blocker); - blk_op_unblock(conf->conf.blk, BLOCK_OP_TYPE_STREAM, s->blocker); - blk_op_unblock(conf->conf.blk, BLOCK_OP_TYPE_REPLACE, s->blocker); + s->insert_notifier.notify = data_plane_blk_insert_notifier; + s->remove_notifier.notify = data_plane_blk_remove_notifier; + blk_add_insert_bs_notifier(conf->conf.blk, &s->insert_notifier); + blk_add_remove_bs_notifier(conf->conf.blk, &s->remove_notifier); + + data_plane_set_up_op_blockers(s); *dataplane = s; } @@ -207,8 +247,9 @@ void virtio_blk_data_plane_destroy(VirtIOBlockDataPlane *s) } virtio_blk_data_plane_stop(s); - blk_op_unblock_all(s->conf->conf.blk, s->blocker); - error_free(s->blocker); + data_plane_remove_op_blockers(s); + notifier_remove(&s->insert_notifier); + notifier_remove(&s->remove_notifier); qemu_bh_delete(s->bh); object_unref(OBJECT(s->iothread)); g_free(s); From 5b9e0e46935538bf2b24fcb70b65477f758413fd Mon Sep 17 00:00:00 2001 From: Max Reitz Date: Fri, 29 Jan 2016 16:36:05 +0100 Subject: [PATCH 20/50] virtio-scsi: Catch BDS-BB removal/insertion Make use of the BDS-BB removal and insertion notifiers to remove or set up, respectively, virtio-scsi's op blockers. Signed-off-by: Max Reitz Signed-off-by: Kevin Wolf --- hw/scsi/virtio-scsi.c | 55 +++++++++++++++++++++++++++++++++ include/hw/virtio/virtio-scsi.h | 10 ++++++ 2 files changed, 65 insertions(+) diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c index 5f23ab2f60..1500c42728 100644 --- a/hw/scsi/virtio-scsi.c +++ b/hw/scsi/virtio-scsi.c @@ -758,6 +758,22 @@ static void virtio_scsi_change(SCSIBus *bus, SCSIDevice *dev, SCSISense sense) } } +static void virtio_scsi_blk_insert_notifier(Notifier *n, void *data) +{ + VirtIOSCSIBlkChangeNotifier *cn = DO_UPCAST(VirtIOSCSIBlkChangeNotifier, + n, n); + assert(cn->sd->conf.blk == data); + blk_op_block_all(cn->sd->conf.blk, cn->s->blocker); +} + +static void virtio_scsi_blk_remove_notifier(Notifier *n, void *data) +{ + VirtIOSCSIBlkChangeNotifier *cn = DO_UPCAST(VirtIOSCSIBlkChangeNotifier, + n, n); + assert(cn->sd->conf.blk == data); + blk_op_unblock_all(cn->sd->conf.blk, cn->s->blocker); +} + static void virtio_scsi_hotplug(HotplugHandler *hotplug_dev, DeviceState *dev, Error **errp) { @@ -766,6 +782,8 @@ static void virtio_scsi_hotplug(HotplugHandler *hotplug_dev, DeviceState *dev, SCSIDevice *sd = SCSI_DEVICE(dev); if (s->ctx && !s->dataplane_disabled) { + VirtIOSCSIBlkChangeNotifier *insert_notifier, *remove_notifier; + if (blk_op_is_blocked(sd->conf.blk, BLOCK_OP_TYPE_DATAPLANE, errp)) { return; } @@ -773,6 +791,20 @@ static void virtio_scsi_hotplug(HotplugHandler *hotplug_dev, DeviceState *dev, aio_context_acquire(s->ctx); blk_set_aio_context(sd->conf.blk, s->ctx); aio_context_release(s->ctx); + + insert_notifier = g_new0(VirtIOSCSIBlkChangeNotifier, 1); + insert_notifier->n.notify = virtio_scsi_blk_insert_notifier; + insert_notifier->s = s; + insert_notifier->sd = sd; + blk_add_insert_bs_notifier(sd->conf.blk, &insert_notifier->n); + QTAILQ_INSERT_TAIL(&s->insert_notifiers, insert_notifier, next); + + remove_notifier = g_new0(VirtIOSCSIBlkChangeNotifier, 1); + remove_notifier->n.notify = virtio_scsi_blk_remove_notifier; + remove_notifier->s = s; + remove_notifier->sd = sd; + blk_add_remove_bs_notifier(sd->conf.blk, &remove_notifier->n); + QTAILQ_INSERT_TAIL(&s->remove_notifiers, remove_notifier, next); } if (virtio_vdev_has_feature(vdev, VIRTIO_SCSI_F_HOTPLUG)) { @@ -788,6 +820,7 @@ static void virtio_scsi_hotunplug(HotplugHandler *hotplug_dev, DeviceState *dev, VirtIODevice *vdev = VIRTIO_DEVICE(hotplug_dev); VirtIOSCSI *s = VIRTIO_SCSI(vdev); SCSIDevice *sd = SCSI_DEVICE(dev); + VirtIOSCSIBlkChangeNotifier *insert_notifier, *remove_notifier; if (virtio_vdev_has_feature(vdev, VIRTIO_SCSI_F_HOTPLUG)) { virtio_scsi_push_event(s, sd, @@ -798,6 +831,25 @@ static void virtio_scsi_hotunplug(HotplugHandler *hotplug_dev, DeviceState *dev, if (s->ctx) { blk_op_unblock_all(sd->conf.blk, s->blocker); } + + QTAILQ_FOREACH(insert_notifier, &s->insert_notifiers, next) { + if (insert_notifier->sd == sd) { + notifier_remove(&insert_notifier->n); + QTAILQ_REMOVE(&s->insert_notifiers, insert_notifier, next); + g_free(insert_notifier); + break; + } + } + + QTAILQ_FOREACH(remove_notifier, &s->remove_notifiers, next) { + if (remove_notifier->sd == sd) { + notifier_remove(&remove_notifier->n); + QTAILQ_REMOVE(&s->remove_notifiers, remove_notifier, next); + g_free(remove_notifier); + break; + } + } + qdev_simple_device_unplug_cb(hotplug_dev, dev, errp); } @@ -912,6 +964,9 @@ static void virtio_scsi_device_realize(DeviceState *dev, Error **errp) add_migration_state_change_notifier(&s->migration_state_notifier); error_setg(&s->blocker, "block device is in use by data plane"); + + QTAILQ_INIT(&s->insert_notifiers); + QTAILQ_INIT(&s->remove_notifiers); } static void virtio_scsi_instance_init(Object *obj) diff --git a/include/hw/virtio/virtio-scsi.h b/include/hw/virtio/virtio-scsi.h index 088fe9f4b9..0394eb23de 100644 --- a/include/hw/virtio/virtio-scsi.h +++ b/include/hw/virtio/virtio-scsi.h @@ -76,6 +76,13 @@ typedef struct VirtIOSCSICommon { VirtQueue **cmd_vqs; } VirtIOSCSICommon; +typedef struct VirtIOSCSIBlkChangeNotifier { + Notifier n; + struct VirtIOSCSI *s; + SCSIDevice *sd; + QTAILQ_ENTRY(VirtIOSCSIBlkChangeNotifier) next; +} VirtIOSCSIBlkChangeNotifier; + typedef struct VirtIOSCSI { VirtIOSCSICommon parent_obj; @@ -86,6 +93,9 @@ typedef struct VirtIOSCSI { /* Fields for dataplane below */ AioContext *ctx; /* one iothread per virtio-scsi-pci for now */ + QTAILQ_HEAD(, VirtIOSCSIBlkChangeNotifier) insert_notifiers; + QTAILQ_HEAD(, VirtIOSCSIBlkChangeNotifier) remove_notifiers; + /* Vring is used instead of vq in dataplane code, because of the underlying * memory layer thread safety */ VirtIOSCSIVring *ctrl_vring; From 741cc431337891e0c1d69fcba6574aa8df3e4822 Mon Sep 17 00:00:00 2001 From: Max Reitz Date: Fri, 29 Jan 2016 16:36:06 +0100 Subject: [PATCH 21/50] nbd: Switch from close to eject notifier The NBD code uses the BDS close notifier to determine when a medium is ejected. However, now it should use the BB's BDS removal notifier for that instead of the BDS's close notifier. Signed-off-by: Max Reitz Reviewed-by: Fam Zheng Reviewed-by: Kevin Wolf Signed-off-by: Kevin Wolf --- blockdev-nbd.c | 40 +++++----------------------------------- nbd/server.c | 13 +++++++++++++ 2 files changed, 18 insertions(+), 35 deletions(-) diff --git a/blockdev-nbd.c b/blockdev-nbd.c index 4a758ac314..9d6a21c33d 100644 --- a/blockdev-nbd.c +++ b/blockdev-nbd.c @@ -45,37 +45,11 @@ void qmp_nbd_server_start(SocketAddress *addr, Error **errp) } } -/* - * Hook into the BlockBackend notifiers to close the export when the - * backend is closed. - */ -typedef struct NBDCloseNotifier { - Notifier n; - NBDExport *exp; - QTAILQ_ENTRY(NBDCloseNotifier) next; -} NBDCloseNotifier; - -static QTAILQ_HEAD(, NBDCloseNotifier) close_notifiers = - QTAILQ_HEAD_INITIALIZER(close_notifiers); - -static void nbd_close_notifier(Notifier *n, void *data) -{ - NBDCloseNotifier *cn = DO_UPCAST(NBDCloseNotifier, n, n); - - notifier_remove(&cn->n); - QTAILQ_REMOVE(&close_notifiers, cn, next); - - nbd_export_close(cn->exp); - nbd_export_put(cn->exp); - g_free(cn); -} - void qmp_nbd_server_add(const char *device, bool has_writable, bool writable, Error **errp) { BlockBackend *blk; NBDExport *exp; - NBDCloseNotifier *n; if (server_fd == -1) { error_setg(errp, "NBD server not running"); @@ -113,19 +87,15 @@ void qmp_nbd_server_add(const char *device, bool has_writable, bool writable, nbd_export_set_name(exp, device); - n = g_new0(NBDCloseNotifier, 1); - n->n.notify = nbd_close_notifier; - n->exp = exp; - blk_add_close_notifier(blk, &n->n); - QTAILQ_INSERT_TAIL(&close_notifiers, n, next); + /* The list of named exports has a strong reference to this export now and + * our only way of accessing it is through nbd_export_find(), so we can drop + * the strong reference that is @exp. */ + nbd_export_put(exp); } void qmp_nbd_server_stop(Error **errp) { - while (!QTAILQ_EMPTY(&close_notifiers)) { - NBDCloseNotifier *cn = QTAILQ_FIRST(&close_notifiers); - nbd_close_notifier(&cn->n, nbd_export_get_blockdev(cn->exp)); - } + nbd_export_close_all(); if (server_fd != -1) { qemu_set_fd_handler(server_fd, NULL, NULL, NULL); diff --git a/nbd/server.c b/nbd/server.c index 7fa7f53768..1ec79cf411 100644 --- a/nbd/server.c +++ b/nbd/server.c @@ -64,6 +64,8 @@ struct NBDExport { QTAILQ_ENTRY(NBDExport) next; AioContext *ctx; + + Notifier eject_notifier; }; static QTAILQ_HEAD(, NBDExport) exports = QTAILQ_HEAD_INITIALIZER(exports); @@ -644,6 +646,12 @@ static void blk_aio_detach(void *opaque) exp->ctx = NULL; } +static void nbd_eject_notifier(Notifier *n, void *data) +{ + NBDExport *exp = container_of(n, NBDExport, eject_notifier); + nbd_export_close(exp); +} + NBDExport *nbd_export_new(BlockBackend *blk, off_t dev_offset, off_t size, uint32_t nbdflags, void (*close)(NBDExport *), Error **errp) @@ -666,6 +674,10 @@ NBDExport *nbd_export_new(BlockBackend *blk, off_t dev_offset, off_t size, exp->ctx = blk_get_aio_context(blk); blk_ref(blk); blk_add_aio_context_notifier(blk, blk_aio_attached, blk_aio_detach, exp); + + exp->eject_notifier.notify = nbd_eject_notifier; + blk_add_remove_bs_notifier(blk, &exp->eject_notifier); + /* * NBD exports are used for non-shared storage migration. Make sure * that BDRV_O_INACTIVE is cleared and the image is ready for write @@ -747,6 +759,7 @@ void nbd_export_put(NBDExport *exp) } if (exp->blk) { + notifier_remove(&exp->eject_notifier); blk_remove_aio_context_notifier(exp->blk, blk_aio_attached, blk_aio_detach, exp); blk_unref(exp->blk); From 033cb5659a1dce15643d2d5123615c26e24298ce Mon Sep 17 00:00:00 2001 From: Max Reitz Date: Fri, 29 Jan 2016 16:36:07 +0100 Subject: [PATCH 22/50] block: Remove BDS close notifier It is unused now, so we can remove it. Signed-off-by: Max Reitz Reviewed-by: Fam Zheng Reviewed-by: Kevin Wolf Signed-off-by: Kevin Wolf --- block.c | 8 -------- block/block-backend.c | 7 ------- include/block/block.h | 1 - include/block/block_int.h | 2 -- include/sysemu/block-backend.h | 1 - 5 files changed, 19 deletions(-) diff --git a/block.c b/block.c index 41ab00efea..f4312d94f7 100644 --- a/block.c +++ b/block.c @@ -259,7 +259,6 @@ BlockDriverState *bdrv_new(void) for (i = 0; i < BLOCK_OP_TYPE_MAX; i++) { QLIST_INIT(&bs->op_blockers[i]); } - notifier_list_init(&bs->close_notifiers); notifier_with_return_list_init(&bs->before_write_notifiers); qemu_co_queue_init(&bs->throttled_reqs[0]); qemu_co_queue_init(&bs->throttled_reqs[1]); @@ -269,11 +268,6 @@ BlockDriverState *bdrv_new(void) return bs; } -void bdrv_add_close_notifier(BlockDriverState *bs, Notifier *notify) -{ - notifier_list_add(&bs->close_notifiers, notify); -} - BlockDriver *bdrv_find_format(const char *format_name) { BlockDriver *drv1; @@ -2157,8 +2151,6 @@ void bdrv_close(BlockDriverState *bs) bdrv_flush(bs); bdrv_drain(bs); /* in case flush left pending I/O */ - notifier_list_notify(&bs->close_notifiers, bs); - bdrv_release_named_dirty_bitmaps(bs); assert(QLIST_EMPTY(&bs->dirty_bitmaps)); diff --git a/block/block-backend.c b/block/block-backend.c index 1872191478..621787cdb0 100644 --- a/block/block-backend.c +++ b/block/block-backend.c @@ -1146,13 +1146,6 @@ void blk_add_insert_bs_notifier(BlockBackend *blk, Notifier *notify) notifier_list_add(&blk->insert_bs_notifiers, notify); } -void blk_add_close_notifier(BlockBackend *blk, Notifier *notify) -{ - if (blk->bs) { - bdrv_add_close_notifier(blk->bs, notify); - } -} - void blk_io_plug(BlockBackend *blk) { if (blk->bs) { diff --git a/include/block/block.h b/include/block/block.h index 25f36dcc74..c7345dea3b 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -226,7 +226,6 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, void bdrv_reopen_commit(BDRVReopenState *reopen_state); void bdrv_reopen_abort(BDRVReopenState *reopen_state); void bdrv_close(BlockDriverState *bs); -void bdrv_add_close_notifier(BlockDriverState *bs, Notifier *notify); int bdrv_read(BlockDriverState *bs, int64_t sector_num, uint8_t *buf, int nb_sectors); int bdrv_read_unthrottled(BlockDriverState *bs, int64_t sector_num, diff --git a/include/block/block_int.h b/include/block/block_int.h index ec31df1106..8730cf63c7 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -403,8 +403,6 @@ struct BlockDriverState { BdrvChild *backing; BdrvChild *file; - NotifierList close_notifiers; - /* Callback before write request is processed */ NotifierWithReturnList before_write_notifiers; diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h index e12be6759a..ae4efb4138 100644 --- a/include/sysemu/block-backend.h +++ b/include/sysemu/block-backend.h @@ -166,7 +166,6 @@ void blk_remove_aio_context_notifier(BlockBackend *blk, void *opaque); void blk_add_remove_bs_notifier(BlockBackend *blk, Notifier *notify); void blk_add_insert_bs_notifier(BlockBackend *blk, Notifier *notify); -void blk_add_close_notifier(BlockBackend *blk, Notifier *notify); void blk_io_plug(BlockBackend *blk); void blk_io_unplug(BlockBackend *blk); BlockAcctStats *blk_get_stats(BlockBackend *blk); From 13855c6b9fa7f9d9e6d1f90377be0f678671073a Mon Sep 17 00:00:00 2001 From: Max Reitz Date: Fri, 29 Jan 2016 16:36:08 +0100 Subject: [PATCH 23/50] block: Use blk_remove_bs() in blk_delete() Signed-off-by: Max Reitz Reviewed-by: Kevin Wolf Reviewed-by: Fam Zheng Signed-off-by: Kevin Wolf --- block/block-backend.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/block/block-backend.c b/block/block-backend.c index 621787cdb0..7f5ad59858 100644 --- a/block/block-backend.c +++ b/block/block-backend.c @@ -166,10 +166,7 @@ static void blk_delete(BlockBackend *blk) assert(!blk->refcnt); assert(!blk->dev); if (blk->bs) { - assert(blk->bs->blk == blk); - blk->bs->blk = NULL; - bdrv_unref(blk->bs); - blk->bs = NULL; + blk_remove_bs(blk); } assert(QLIST_EMPTY(&blk->remove_bs_notifiers.notifiers)); assert(QLIST_EMPTY(&blk->insert_bs_notifiers.notifiers)); @@ -351,6 +348,8 @@ void blk_hide_on_behalf_of_hmp_drive_del(BlockBackend *blk) */ void blk_remove_bs(BlockBackend *blk) { + assert(blk->bs->blk == blk); + notifier_list_notify(&blk->remove_bs_notifiers, blk); blk_update_root_state(blk); From 938abd4325951f94e87765b7daea61bf0bf71a5e Mon Sep 17 00:00:00 2001 From: Max Reitz Date: Fri, 29 Jan 2016 16:36:09 +0100 Subject: [PATCH 24/50] blockdev: Use blk_remove_bs() in do_drive_del() Signed-off-by: Max Reitz Reviewed-by: Kevin Wolf Reviewed-by: Fam Zheng Signed-off-by: Kevin Wolf --- blockdev.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/blockdev.c b/blockdev.c index 1044a6acad..09d4621cd2 100644 --- a/blockdev.c +++ b/blockdev.c @@ -2792,7 +2792,7 @@ void hmp_drive_del(Monitor *mon, const QDict *qdict) return; } - bdrv_close(bs); + blk_remove_bs(blk); } /* if we have a device attached to this BlockDriverState From 64dff52019367194699a772e3cc31941fd9752a8 Mon Sep 17 00:00:00 2001 From: Max Reitz Date: Fri, 29 Jan 2016 16:36:10 +0100 Subject: [PATCH 25/50] block: Make bdrv_close() static There are no users of bdrv_close() left, except for one of bdrv_open()'s failure paths, bdrv_close_all() and bdrv_delete(), and that is good. Make bdrv_close() static so nobody makes the mistake of directly using bdrv_close() again. Signed-off-by: Max Reitz Reviewed-by: Eric Blake Reviewed-by: Alberto Garcia Reviewed-by: Kevin Wolf Reviewed-by: Fam Zheng Signed-off-by: Kevin Wolf --- block.c | 4 +++- include/block/block.h | 1 - 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/block.c b/block.c index f4312d94f7..e076f1004a 100644 --- a/block.c +++ b/block.c @@ -93,6 +93,8 @@ static void bdrv_release_named_dirty_bitmaps(BlockDriverState *bs); /* If non-zero, use only whitelisted block drivers */ static int use_bdrv_whitelist; +static void bdrv_close(BlockDriverState *bs); + #ifdef _WIN32 static int is_windows_drive_prefix(const char *filename) { @@ -2134,7 +2136,7 @@ void bdrv_reopen_abort(BDRVReopenState *reopen_state) } -void bdrv_close(BlockDriverState *bs) +static void bdrv_close(BlockDriverState *bs) { BdrvAioNotifier *ban, *ban_next; diff --git a/include/block/block.h b/include/block/block.h index c7345dea3b..4452b79dd4 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -225,7 +225,6 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue, Error **errp); void bdrv_reopen_commit(BDRVReopenState *reopen_state); void bdrv_reopen_abort(BDRVReopenState *reopen_state); -void bdrv_close(BlockDriverState *bs); int bdrv_read(BlockDriverState *bs, int64_t sector_num, uint8_t *buf, int nb_sectors); int bdrv_read_unthrottled(BlockDriverState *bs, int64_t sector_num, From 2c1d04e002dc91a6f34e8e683ea6b84f61a0b9fd Mon Sep 17 00:00:00 2001 From: Max Reitz Date: Fri, 29 Jan 2016 16:36:11 +0100 Subject: [PATCH 26/50] block: Add list of all BlockDriverStates We need this list so that bdrv_close_all() can keep track of which BDSs are still open after having removed the BDSs from all of the BBs and having released all monitor BDS references. Signed-off-by: Max Reitz Reviewed-by: Kevin Wolf Reviewed-by: Fam Zheng Signed-off-by: Kevin Wolf --- block.c | 7 +++++++ include/block/block_int.h | 2 ++ 2 files changed, 9 insertions(+) diff --git a/block.c b/block.c index e076f1004a..d687d2c2b6 100644 --- a/block.c +++ b/block.c @@ -79,6 +79,9 @@ struct BdrvStates bdrv_states = QTAILQ_HEAD_INITIALIZER(bdrv_states); static QTAILQ_HEAD(, BlockDriverState) graph_bdrv_states = QTAILQ_HEAD_INITIALIZER(graph_bdrv_states); +static QTAILQ_HEAD(, BlockDriverState) all_bdrv_states = + QTAILQ_HEAD_INITIALIZER(all_bdrv_states); + static QLIST_HEAD(, BlockDriver) bdrv_drivers = QLIST_HEAD_INITIALIZER(bdrv_drivers); @@ -267,6 +270,8 @@ BlockDriverState *bdrv_new(void) bs->refcnt = 1; bs->aio_context = qemu_get_aio_context(); + QTAILQ_INSERT_TAIL(&all_bdrv_states, bs, bs_list); + return bs; } @@ -2371,6 +2376,8 @@ static void bdrv_delete(BlockDriverState *bs) /* remove from list, if necessary */ bdrv_make_anon(bs); + QTAILQ_REMOVE(&all_bdrv_states, bs, bs_list); + g_free(bs); } diff --git a/include/block/block_int.h b/include/block/block_int.h index 8730cf63c7..1e4c51875e 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -443,6 +443,8 @@ struct BlockDriverState { QTAILQ_ENTRY(BlockDriverState) node_list; /* element of the list of "drives" the guest sees */ QTAILQ_ENTRY(BlockDriverState) device_list; + /* element of the list of all BlockDriverStates (all_bdrv_states) */ + QTAILQ_ENTRY(BlockDriverState) bs_list; QLIST_HEAD(, BdrvDirtyBitmap) dirty_bitmaps; int refcnt; From 9c4218e957331e1ba0ba7565730b0b71c49b8d70 Mon Sep 17 00:00:00 2001 From: Max Reitz Date: Fri, 29 Jan 2016 16:36:12 +0100 Subject: [PATCH 27/50] blockdev: Keep track of monitor-owned BDS As a side effect, we can now make x-blockdev-del's check whether a BDS is actually owned by the monitor explicit. Signed-off-by: Max Reitz Signed-off-by: Kevin Wolf --- blockdev.c | 28 +++++++++++++++++++++++++- include/block/block_int.h | 4 ++++ stubs/Makefile.objs | 1 + stubs/blockdev-close-all-bdrv-states.c | 5 +++++ 4 files changed, 37 insertions(+), 1 deletion(-) create mode 100644 stubs/blockdev-close-all-bdrv-states.c diff --git a/blockdev.c b/blockdev.c index 09d4621cd2..35e1e5c58f 100644 --- a/blockdev.c +++ b/blockdev.c @@ -50,6 +50,9 @@ #include "trace.h" #include "sysemu/arch_init.h" +static QTAILQ_HEAD(, BlockDriverState) monitor_bdrv_states = + QTAILQ_HEAD_INITIALIZER(monitor_bdrv_states); + static const char *const if_name[IF_COUNT] = { [IF_NONE] = "none", [IF_IDE] = "ide", @@ -702,6 +705,19 @@ fail: return NULL; } +void blockdev_close_all_bdrv_states(void) +{ + BlockDriverState *bs, *next_bs; + + QTAILQ_FOREACH_SAFE(bs, &monitor_bdrv_states, monitor_list, next_bs) { + AioContext *ctx = bdrv_get_aio_context(bs); + + aio_context_acquire(ctx); + bdrv_unref(bs); + aio_context_release(ctx); + } +} + static void qemu_opt_rename(QemuOpts *opts, const char *from, const char *to, Error **errp) { @@ -3875,12 +3891,15 @@ void qmp_blockdev_add(BlockdevOptions *options, Error **errp) if (!bs) { goto fail; } + + QTAILQ_INSERT_TAIL(&monitor_bdrv_states, bs, monitor_list); } if (bs && bdrv_key_required(bs)) { if (blk) { blk_unref(blk); } else { + QTAILQ_REMOVE(&monitor_bdrv_states, bs, monitor_list); bdrv_unref(bs); } error_setg(errp, "blockdev-add doesn't support encrypted devices"); @@ -3940,7 +3959,13 @@ void qmp_x_blockdev_del(bool has_id, const char *id, goto out; } - if (bs->refcnt > 1 || !QLIST_EMPTY(&bs->parents)) { + if (!blk && !bs->monitor_list.tqe_prev) { + error_setg(errp, "Node %s is not owned by the monitor", + bs->node_name); + goto out; + } + + if (bs->refcnt > 1) { error_setg(errp, "Block device %s is in use", bdrv_get_device_or_node_name(bs)); goto out; @@ -3950,6 +3975,7 @@ void qmp_x_blockdev_del(bool has_id, const char *id, if (blk) { blk_unref(blk); } else { + QTAILQ_REMOVE(&monitor_bdrv_states, bs, monitor_list); bdrv_unref(bs); } diff --git a/include/block/block_int.h b/include/block/block_int.h index 1e4c51875e..dd00d12a6f 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -445,6 +445,8 @@ struct BlockDriverState { QTAILQ_ENTRY(BlockDriverState) device_list; /* element of the list of all BlockDriverStates (all_bdrv_states) */ QTAILQ_ENTRY(BlockDriverState) bs_list; + /* element of the list of monitor-owned BDS */ + QTAILQ_ENTRY(BlockDriverState) monitor_list; QLIST_HEAD(, BdrvDirtyBitmap) dirty_bitmaps; int refcnt; @@ -707,4 +709,6 @@ bool bdrv_requests_pending(BlockDriverState *bs); void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap **out); void bdrv_undo_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap *in); +void blockdev_close_all_bdrv_states(void); + #endif /* BLOCK_INT_H */ diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs index d7898a0c4a..e922de982f 100644 --- a/stubs/Makefile.objs +++ b/stubs/Makefile.objs @@ -1,5 +1,6 @@ stub-obj-y += arch-query-cpu-def.o stub-obj-y += bdrv-commit-all.o +stub-obj-y += blockdev-close-all-bdrv-states.o stub-obj-y += clock-warp.o stub-obj-y += cpu-get-clock.o stub-obj-y += cpu-get-icount.o diff --git a/stubs/blockdev-close-all-bdrv-states.c b/stubs/blockdev-close-all-bdrv-states.c new file mode 100644 index 0000000000..12d2442362 --- /dev/null +++ b/stubs/blockdev-close-all-bdrv-states.c @@ -0,0 +1,5 @@ +#include "block/block_int.h" + +void blockdev_close_all_bdrv_states(void) +{ +} From d8da3cef3bc649492d190e029343293df1386027 Mon Sep 17 00:00:00 2001 From: Max Reitz Date: Fri, 29 Jan 2016 16:36:13 +0100 Subject: [PATCH 28/50] block: Add blk_remove_all_bs() When bdrv_close_all() is called, instead of force-closing all root BlockDriverStates, it is better to just drop the reference from all BlockBackends and let them be closed automatically. This prevents BDS from getting closed that are still referenced by other BDS, which may result in loss of cached data. This patch adds a function for doing that, but does not yet incorporate it in bdrv_close_all(). Signed-off-by: Max Reitz Reviewed-by: Kevin Wolf Signed-off-by: Kevin Wolf --- block/block-backend.c | 15 +++++++++++++++ include/sysemu/block-backend.h | 1 + 2 files changed, 16 insertions(+) diff --git a/block/block-backend.c b/block/block-backend.c index 7f5ad59858..ebdf78a11c 100644 --- a/block/block-backend.c +++ b/block/block-backend.c @@ -223,6 +223,21 @@ void blk_unref(BlockBackend *blk) } } +void blk_remove_all_bs(void) +{ + BlockBackend *blk; + + QTAILQ_FOREACH(blk, &blk_backends, link) { + AioContext *ctx = blk_get_aio_context(blk); + + aio_context_acquire(ctx); + if (blk->bs) { + blk_remove_bs(blk); + } + aio_context_release(ctx); + } +} + /* * Return the BlockBackend after @blk. * If @blk is null, return the first one. diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h index ae4efb4138..ec303316bb 100644 --- a/include/sysemu/block-backend.h +++ b/include/sysemu/block-backend.h @@ -68,6 +68,7 @@ BlockBackend *blk_new_open(const char *name, const char *filename, int blk_get_refcnt(BlockBackend *blk); void blk_ref(BlockBackend *blk); void blk_unref(BlockBackend *blk); +void blk_remove_all_bs(void); const char *blk_name(BlockBackend *blk); BlockBackend *blk_by_name(const char *name); BlockBackend *blk_next(BlockBackend *blk); From ca9bd24cf1d53775169ba9adc17e265554d1afed Mon Sep 17 00:00:00 2001 From: Max Reitz Date: Fri, 29 Jan 2016 16:36:14 +0100 Subject: [PATCH 29/50] block: Rewrite bdrv_close_all() This patch rewrites bdrv_close_all(): Until now, all root BDSs have been force-closed. This is bad because it can lead to cached data not being flushed to disk. Instead, try to make all reference holders relinquish their reference voluntarily: 1. All BlockBackend users are handled by making all BBs simply eject their BDS tree. Since a BDS can never be on top of a BB, this will not cause any of the issues as seen with the force-closing of BDSs. The references will be relinquished and any further access to the BB will fail gracefully. 2. All BDSs which are owned by the monitor itself (because they do not have a BB) are relinquished next. 3. Besides BBs and the monitor, block jobs and other BDSs are the only things left that can hold a reference to BDSs. After every remaining block job has been canceled, there should not be any BDSs left (and the loop added here will always terminate (as long as NDEBUG is not defined), because either all_bdrv_states will be empty or there will not be any block job left to cancel, failing the assertion). Signed-off-by: Max Reitz Signed-off-by: Kevin Wolf --- block.c | 34 ++++++++++++++++++++++++++-------- 1 file changed, 26 insertions(+), 8 deletions(-) diff --git a/block.c b/block.c index d687d2c2b6..ff1aafc85f 100644 --- a/block.c +++ b/block.c @@ -2145,9 +2145,7 @@ static void bdrv_close(BlockDriverState *bs) { BdrvAioNotifier *ban, *ban_next; - if (bs->job) { - block_job_cancel_sync(bs->job); - } + assert(!bs->job); /* Disable I/O limits and drain all pending throttled requests */ if (bs->throttle_state) { @@ -2214,13 +2212,33 @@ static void bdrv_close(BlockDriverState *bs) void bdrv_close_all(void) { BlockDriverState *bs; + AioContext *aio_context; - QTAILQ_FOREACH(bs, &bdrv_states, device_list) { - AioContext *aio_context = bdrv_get_aio_context(bs); + /* Drop references from requests still in flight, such as canceled block + * jobs whose AIO context has not been polled yet */ + bdrv_drain_all(); - aio_context_acquire(aio_context); - bdrv_close(bs); - aio_context_release(aio_context); + blk_remove_all_bs(); + blockdev_close_all_bdrv_states(); + + /* Cancel all block jobs */ + while (!QTAILQ_EMPTY(&all_bdrv_states)) { + QTAILQ_FOREACH(bs, &all_bdrv_states, bs_list) { + aio_context = bdrv_get_aio_context(bs); + + aio_context_acquire(aio_context); + if (bs->job) { + block_job_cancel_sync(bs->job); + aio_context_release(aio_context); + break; + } + aio_context_release(aio_context); + } + + /* All the remaining BlockDriverStates are referenced directly or + * indirectly from block jobs, so there needs to be at least one BDS + * directly used by a block job */ + assert(bs); } } From 15a2b18fe5824388debe958e21c5dfa51457c7b6 Mon Sep 17 00:00:00 2001 From: Max Reitz Date: Fri, 29 Jan 2016 16:36:15 +0100 Subject: [PATCH 30/50] iotests: Add test for multiple BB on BDS tree This adds a test for having multiple BlockBackends in one BDS tree. In this case, there is one BB for the protocol BDS and one BB for the format BDS in a simple two-BDS tree (with the protocol BDS and BB added first). When bdrv_close_all() is executed, no cached data from any BDS should be lost; the protocol BDS may not be closed until the format BDS is closed. Otherwise, metadata updates may be lost. Signed-off-by: Max Reitz Reviewed-by: Kevin Wolf Signed-off-by: Kevin Wolf --- tests/qemu-iotests/117 | 86 ++++++++++++++++++++++++++++++++++++++ tests/qemu-iotests/117.out | 14 +++++++ tests/qemu-iotests/group | 1 + 3 files changed, 101 insertions(+) create mode 100755 tests/qemu-iotests/117 create mode 100644 tests/qemu-iotests/117.out diff --git a/tests/qemu-iotests/117 b/tests/qemu-iotests/117 new file mode 100755 index 0000000000..969750d137 --- /dev/null +++ b/tests/qemu-iotests/117 @@ -0,0 +1,86 @@ +#!/bin/bash +# +# Test case for shared BDS between backend trees +# +# Copyright (C) 2016 Red Hat, Inc. +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 2 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see . +# + +# creator +owner=mreitz@redhat.com + +seq="$(basename $0)" +echo "QA output created by $seq" + +here="$PWD" +tmp=/tmp/$$ +status=1 # failure is the default! + +_cleanup() +{ + _cleanup_test_img +} +trap "_cleanup; exit \$status" 0 1 2 3 15 + +# get standard environment, filters and checks +. ./common.rc +. ./common.filter +. ./common.qemu + +_supported_fmt qcow2 +_supported_proto file +_supported_os Linux + +_make_test_img 64k + +_launch_qemu + +_send_qemu_cmd $QEMU_HANDLE \ + "{ 'execute': 'qmp_capabilities' }" \ + 'return' + +_send_qemu_cmd $QEMU_HANDLE \ + "{ 'execute': 'blockdev-add', + 'arguments': { 'options': { 'id': 'protocol', + 'driver': 'file', + 'filename': '$TEST_IMG' } } }" \ + 'return' + +_send_qemu_cmd $QEMU_HANDLE \ + "{ 'execute': 'blockdev-add', + 'arguments': { 'options': { 'id': 'format', + 'driver': '$IMGFMT', + 'file': 'protocol' } } }" \ + 'return' + +_send_qemu_cmd $QEMU_HANDLE \ + "{ 'execute': 'human-monitor-command', + 'arguments': { 'command-line': 'qemu-io format \"write -P 42 0 64k\"' } }" \ + 'return' + +_send_qemu_cmd $QEMU_HANDLE \ + "{ 'execute': 'quit' }" \ + 'return' + +wait=1 _cleanup_qemu + +_check_test_img + +$QEMU_IO -c 'read -P 42 0 64k' "$TEST_IMG" | _filter_qemu_io + +# success, all done +echo '*** done' +rm -f $seq.full +status=0 diff --git a/tests/qemu-iotests/117.out b/tests/qemu-iotests/117.out new file mode 100644 index 0000000000..f52dc1a357 --- /dev/null +++ b/tests/qemu-iotests/117.out @@ -0,0 +1,14 @@ +QA output created by 117 +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=65536 +{"return": {}} +{"return": {}} +{"return": {}} +wrote 65536/65536 bytes at offset 0 +64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +{"return": ""} +{"return": {}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN"} +No errors were found on the image. +read 65536/65536 bytes at offset 0 +64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +*** done diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group index ff1ff0d06b..e89f076ea7 100644 --- a/tests/qemu-iotests/group +++ b/tests/qemu-iotests/group @@ -122,6 +122,7 @@ 114 rw auto quick 115 rw auto 116 rw auto quick +117 rw auto 118 rw auto 119 rw auto quick 120 rw auto quick From c78dc18295cb79c36d8993f245537f997cd9f2bb Mon Sep 17 00:00:00 2001 From: Max Reitz Date: Fri, 29 Jan 2016 16:36:16 +0100 Subject: [PATCH 31/50] iotests: Add test for block jobs and BDS ejection Suggested-by: Paolo Bonzini Signed-off-by: Max Reitz Reviewed-by: Kevin Wolf Signed-off-by: Kevin Wolf --- tests/qemu-iotests/141 | 186 +++++++++++++++++++++++++++++++++++++ tests/qemu-iotests/141.out | 59 ++++++++++++ tests/qemu-iotests/group | 1 + 3 files changed, 246 insertions(+) create mode 100755 tests/qemu-iotests/141 create mode 100644 tests/qemu-iotests/141.out diff --git a/tests/qemu-iotests/141 b/tests/qemu-iotests/141 new file mode 100755 index 0000000000..f7c28b4463 --- /dev/null +++ b/tests/qemu-iotests/141 @@ -0,0 +1,186 @@ +#!/bin/bash +# +# Test case for ejecting BDSs with block jobs still running on them +# +# Copyright (C) 2016 Red Hat, Inc. +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 2 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see . +# + +# creator +owner=mreitz@redhat.com + +seq="$(basename $0)" +echo "QA output created by $seq" + +here="$PWD" +tmp=/tmp/$$ +status=1 # failure is the default! + +_cleanup() +{ + _cleanup_test_img + rm -f "$TEST_DIR/{b,m,o}.$IMGFMT" +} +trap "_cleanup; exit \$status" 0 1 2 3 15 + +# get standard environment, filters and checks +. ./common.rc +. ./common.filter +. ./common.qemu + +# Needs backing file and backing format support +_supported_fmt qcow2 qed +_supported_proto file +_supported_os Linux + + +test_blockjob() +{ + _send_qemu_cmd $QEMU_HANDLE \ + "{'execute': 'blockdev-add', + 'arguments': { + 'options': { + 'id': 'drv0', + 'driver': '$IMGFMT', + 'file': { + 'driver': 'file', + 'filename': '$TEST_IMG' + }}}}" \ + 'return' + + _send_qemu_cmd $QEMU_HANDLE \ + "$1" \ + "$2" \ + | _filter_img_create + + # We want this to return an error because the block job is still running + _send_qemu_cmd $QEMU_HANDLE \ + "{'execute': 'x-blockdev-remove-medium', + 'arguments': {'device': 'drv0'}}" \ + 'error' + + _send_qemu_cmd $QEMU_HANDLE \ + "{'execute': 'block-job-cancel', + 'arguments': {'device': 'drv0'}}" \ + "$3" + + _send_qemu_cmd $QEMU_HANDLE \ + "{'execute': 'x-blockdev-del', + 'arguments': {'id': 'drv0'}}" \ + 'return' +} + + +TEST_IMG="$TEST_DIR/b.$IMGFMT" _make_test_img 1M +TEST_IMG="$TEST_DIR/m.$IMGFMT" _make_test_img -b "$TEST_DIR/b.$IMGFMT" 1M +_make_test_img -b "$TEST_DIR/m.$IMGFMT" 1M + +_launch_qemu -nodefaults + +_send_qemu_cmd $QEMU_HANDLE \ + "{'execute': 'qmp_capabilities'}" \ + 'return' + +echo +echo '=== Testing drive-backup ===' +echo + +# drive-backup will not send BLOCK_JOB_READY by itself, and cancelling the job +# will consequently result in BLOCK_JOB_CANCELLED being emitted. + +test_blockjob \ + "{'execute': 'drive-backup', + 'arguments': {'device': 'drv0', + 'target': '$TEST_DIR/o.$IMGFMT', + 'format': '$IMGFMT', + 'sync': 'none'}}" \ + 'return' \ + 'BLOCK_JOB_CANCELLED' + +echo +echo '=== Testing drive-mirror ===' +echo + +# drive-mirror will send BLOCK_JOB_READY basically immediately, and cancelling +# the job will consequently result in BLOCK_JOB_COMPLETED being emitted. + +test_blockjob \ + "{'execute': 'drive-mirror', + 'arguments': {'device': 'drv0', + 'target': '$TEST_DIR/o.$IMGFMT', + 'format': '$IMGFMT', + 'sync': 'none'}}" \ + 'BLOCK_JOB_READY' \ + 'BLOCK_JOB_COMPLETED' + +echo +echo '=== Testing active block-commit ===' +echo + +# An active block-commit will send BLOCK_JOB_READY basically immediately, and +# cancelling the job will consequently result in BLOCK_JOB_COMPLETED being +# emitted. + +test_blockjob \ + "{'execute': 'block-commit', + 'arguments': {'device': 'drv0'}}" \ + 'BLOCK_JOB_READY' \ + 'BLOCK_JOB_COMPLETED' + +echo +echo '=== Testing non-active block-commit ===' +echo + +# Give block-commit something to work on, otherwise it would be done +# immediately, send a BLOCK_JOB_COMPLETED and ejecting the BDS would work just +# fine without the block job still running. + +$QEMU_IO -c 'write 0 1M' "$TEST_DIR/m.$IMGFMT" | _filter_qemu_io + +test_blockjob \ + "{'execute': 'block-commit', + 'arguments': {'device': 'drv0', + 'top': '$TEST_DIR/m.$IMGFMT', + 'speed': 1}}" \ + 'return' \ + 'BLOCK_JOB_CANCELLED' + +echo +echo '=== Testing block-stream ===' +echo + +# Give block-stream something to work on, otherwise it would be done +# immediately, send a BLOCK_JOB_COMPLETED and ejecting the BDS would work just +# fine without the block job still running. + +$QEMU_IO -c 'write 0 1M' "$TEST_DIR/b.$IMGFMT" | _filter_qemu_io + +# With some data to stream (and @speed set to 1), block-stream will not complete +# until we send the block-job-cancel command. Therefore, no event other than +# BLOCK_JOB_CANCELLED will be emitted. + +test_blockjob \ + "{'execute': 'block-stream', + 'arguments': {'device': 'drv0', + 'speed': 1}}" \ + 'return' \ + 'BLOCK_JOB_CANCELLED' + +_cleanup_qemu + +# success, all done +echo "*** done" +rm -f $seq.full +status=0 diff --git a/tests/qemu-iotests/141.out b/tests/qemu-iotests/141.out new file mode 100644 index 0000000000..adceac1817 --- /dev/null +++ b/tests/qemu-iotests/141.out @@ -0,0 +1,59 @@ +QA output created by 141 +Formatting 'TEST_DIR/b.IMGFMT', fmt=IMGFMT size=1048576 +Formatting 'TEST_DIR/m.IMGFMT', fmt=IMGFMT size=1048576 backing_file=TEST_DIR/b.IMGFMT +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576 backing_file=TEST_DIR/m.IMGFMT +{"return": {}} + +=== Testing drive-backup === + +{"return": {}} +Formatting 'TEST_DIR/o.IMGFMT', fmt=IMGFMT size=1048576 backing_file=TEST_DIR/t.IMGFMT backing_fmt=IMGFMT +{"return": {}} +{"error": {"class": "GenericError", "desc": "Node 'drv0' is busy: block device is in use by block job: backup"}} +{"return": {}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_CANCELLED", "data": {"device": "drv0", "len": 1048576, "offset": 0, "speed": 0, "type": "backup"}} +{"return": {}} + +=== Testing drive-mirror === + +{"return": {}} +Formatting 'TEST_DIR/o.IMGFMT', fmt=IMGFMT size=1048576 backing_file=TEST_DIR/t.IMGFMT backing_fmt=IMGFMT +{"return": {}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "drv0", "len": 0, "offset": 0, "speed": 0, "type": "mirror"}} +{"error": {"class": "GenericError", "desc": "Node 'drv0' is busy: block device is in use by block job: mirror"}} +{"return": {}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "drv0", "len": 0, "offset": 0, "speed": 0, "type": "mirror"}} +{"return": {}} + +=== Testing active block-commit === + +{"return": {}} +{"return": {}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "drv0", "len": 0, "offset": 0, "speed": 0, "type": "commit"}} +{"error": {"class": "GenericError", "desc": "Node 'drv0' is busy: block device is in use by block job: commit"}} +{"return": {}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "drv0", "len": 0, "offset": 0, "speed": 0, "type": "commit"}} +{"return": {}} + +=== Testing non-active block-commit === + +wrote 1048576/1048576 bytes at offset 0 +1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +{"return": {}} +{"return": {}} +{"error": {"class": "GenericError", "desc": "Node 'drv0' is busy: block device is in use by block job: commit"}} +{"return": {}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_CANCELLED", "data": {"device": "drv0", "len": 1048576, "offset": 524288, "speed": 1, "type": "commit"}} +{"return": {}} + +=== Testing block-stream === + +wrote 1048576/1048576 bytes at offset 0 +1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +{"return": {}} +{"return": {}} +{"error": {"class": "GenericError", "desc": "Node 'drv0' is busy: block device is in use by block job: stream"}} +{"return": {}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_CANCELLED", "data": {"device": "drv0", "len": 1048576, "offset": 524288, "speed": 1, "type": "stream"}} +{"return": {}} +*** done diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group index e89f076ea7..cbc970a471 100644 --- a/tests/qemu-iotests/group +++ b/tests/qemu-iotests/group @@ -143,5 +143,6 @@ 138 rw auto quick 139 rw auto quick 140 rw auto quick +141 rw auto quick 142 auto 143 auto quick From 1963f8d52e04a8f8b213e34e6a76fb286fb23ec1 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Wed, 23 Dec 2015 11:48:23 +0100 Subject: [PATCH 32/50] block: acquire in bdrv_query_image_info NFS calls aio_poll inside bdrv_get_allocated_size. This requires acquiring the AioContext. Signed-off-by: Paolo Bonzini Message-id: 1450867706-19860-1-git-send-email-pbonzini@redhat.com Reviewed-by: Fam Zheng Signed-off-by: Max Reitz --- block/qapi.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/block/qapi.c b/block/qapi.c index bbe0c9dddb..2e8310591d 100644 --- a/block/qapi.c +++ b/block/qapi.c @@ -211,11 +211,13 @@ void bdrv_query_image_info(BlockDriverState *bs, Error *err = NULL; ImageInfo *info; + aio_context_acquire(bdrv_get_aio_context(bs)); + size = bdrv_getlength(bs); if (size < 0) { error_setg_errno(errp, -size, "Can't get size of device '%s'", bdrv_get_device_name(bs)); - return; + goto out; } info = g_new0(ImageInfo, 1); @@ -283,10 +285,13 @@ void bdrv_query_image_info(BlockDriverState *bs, default: error_propagate(errp, err); qapi_free_ImageInfo(info); - return; + goto out; } *p_info = info; + +out: + aio_context_release(bdrv_get_aio_context(bs)); } /* @p_info will be set only on success. */ From 67a0fd2a9bca204d2b39f910a97c7137636a0715 Mon Sep 17 00:00:00 2001 From: Fam Zheng Date: Tue, 26 Jan 2016 11:58:48 +0800 Subject: [PATCH 33/50] block: Add "file" output parameter to block status query functions The added parameter can be used to return the BDS pointer which the valid offset is referring to. Its value should be ignored unless BDRV_BLOCK_OFFSET_VALID in ret is set. Until block drivers fill in the right value, let's clear it explicitly right before calling .bdrv_get_block_status. The "bs->file" condition in bdrv_co_get_block_status is kept now to keep iotest case 102 passing, and will be fixed once all drivers return the right file pointer. Signed-off-by: Fam Zheng Message-id: 1453780743-16806-2-git-send-email-famz@redhat.com Reviewed-by: Max Reitz Signed-off-by: Max Reitz --- block/io.c | 40 +++++++++++++++++++++++++++------------ block/iscsi.c | 6 ++++-- block/mirror.c | 3 ++- block/parallels.c | 2 +- block/qcow.c | 2 +- block/qcow2.c | 2 +- block/qed.c | 3 ++- block/raw-posix.c | 3 ++- block/raw_bsd.c | 3 ++- block/sheepdog.c | 2 +- block/vdi.c | 2 +- block/vmdk.c | 2 +- block/vpc.c | 2 +- block/vvfat.c | 2 +- include/block/block.h | 11 +++++++---- include/block/block_int.h | 3 ++- qemu-img.c | 13 +++++++++---- 17 files changed, 66 insertions(+), 35 deletions(-) diff --git a/block/io.c b/block/io.c index 5bb353a8ca..ea040bef22 100644 --- a/block/io.c +++ b/block/io.c @@ -664,6 +664,7 @@ int bdrv_write_zeroes(BlockDriverState *bs, int64_t sector_num, int bdrv_make_zero(BlockDriverState *bs, BdrvRequestFlags flags) { int64_t target_sectors, ret, nb_sectors, sector_num = 0; + BlockDriverState *file; int n; target_sectors = bdrv_nb_sectors(bs); @@ -676,7 +677,7 @@ int bdrv_make_zero(BlockDriverState *bs, BdrvRequestFlags flags) if (nb_sectors <= 0) { return 0; } - ret = bdrv_get_block_status(bs, sector_num, nb_sectors, &n); + ret = bdrv_get_block_status(bs, sector_num, nb_sectors, &n, &file); if (ret < 0) { error_report("error getting block status at sector %" PRId64 ": %s", sector_num, strerror(-ret)); @@ -1466,6 +1467,7 @@ int bdrv_flush_all(void) typedef struct BdrvCoGetBlockStatusData { BlockDriverState *bs; BlockDriverState *base; + BlockDriverState **file; int64_t sector_num; int nb_sectors; int *pnum; @@ -1487,10 +1489,14 @@ typedef struct BdrvCoGetBlockStatusData { * * 'nb_sectors' is the max value 'pnum' should be set to. If nb_sectors goes * beyond the end of the disk image it will be clamped. + * + * If returned value is positive and BDRV_BLOCK_OFFSET_VALID bit is set, 'file' + * points to the BDS which the sector range is allocated in. */ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs, int64_t sector_num, - int nb_sectors, int *pnum) + int nb_sectors, int *pnum, + BlockDriverState **file) { int64_t total_sectors; int64_t n; @@ -1520,7 +1526,9 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs, return ret; } - ret = bs->drv->bdrv_co_get_block_status(bs, sector_num, nb_sectors, pnum); + *file = NULL; + ret = bs->drv->bdrv_co_get_block_status(bs, sector_num, nb_sectors, pnum, + file); if (ret < 0) { *pnum = 0; return ret; @@ -1529,7 +1537,7 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs, if (ret & BDRV_BLOCK_RAW) { assert(ret & BDRV_BLOCK_OFFSET_VALID); return bdrv_get_block_status(bs->file->bs, ret >> BDRV_SECTOR_BITS, - *pnum, pnum); + *pnum, pnum, file); } if (ret & (BDRV_BLOCK_DATA | BDRV_BLOCK_ZERO)) { @@ -1549,10 +1557,11 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs, if (bs->file && (ret & BDRV_BLOCK_DATA) && !(ret & BDRV_BLOCK_ZERO) && (ret & BDRV_BLOCK_OFFSET_VALID)) { + BlockDriverState *file2; int file_pnum; ret2 = bdrv_co_get_block_status(bs->file->bs, ret >> BDRV_SECTOR_BITS, - *pnum, &file_pnum); + *pnum, &file_pnum, &file2); if (ret2 >= 0) { /* Ignore errors. This is just providing extra information, it * is useful but not necessary. @@ -1577,14 +1586,15 @@ static int64_t coroutine_fn bdrv_co_get_block_status_above(BlockDriverState *bs, BlockDriverState *base, int64_t sector_num, int nb_sectors, - int *pnum) + int *pnum, + BlockDriverState **file) { BlockDriverState *p; int64_t ret = 0; assert(bs != base); for (p = bs; p != base; p = backing_bs(p)) { - ret = bdrv_co_get_block_status(p, sector_num, nb_sectors, pnum); + ret = bdrv_co_get_block_status(p, sector_num, nb_sectors, pnum, file); if (ret < 0 || ret & BDRV_BLOCK_ALLOCATED) { break; } @@ -1603,7 +1613,8 @@ static void coroutine_fn bdrv_get_block_status_above_co_entry(void *opaque) data->ret = bdrv_co_get_block_status_above(data->bs, data->base, data->sector_num, data->nb_sectors, - data->pnum); + data->pnum, + data->file); data->done = true; } @@ -1615,12 +1626,14 @@ static void coroutine_fn bdrv_get_block_status_above_co_entry(void *opaque) int64_t bdrv_get_block_status_above(BlockDriverState *bs, BlockDriverState *base, int64_t sector_num, - int nb_sectors, int *pnum) + int nb_sectors, int *pnum, + BlockDriverState **file) { Coroutine *co; BdrvCoGetBlockStatusData data = { .bs = bs, .base = base, + .file = file, .sector_num = sector_num, .nb_sectors = nb_sectors, .pnum = pnum, @@ -1644,16 +1657,19 @@ int64_t bdrv_get_block_status_above(BlockDriverState *bs, int64_t bdrv_get_block_status(BlockDriverState *bs, int64_t sector_num, - int nb_sectors, int *pnum) + int nb_sectors, int *pnum, + BlockDriverState **file) { return bdrv_get_block_status_above(bs, backing_bs(bs), - sector_num, nb_sectors, pnum); + sector_num, nb_sectors, pnum, file); } int coroutine_fn bdrv_is_allocated(BlockDriverState *bs, int64_t sector_num, int nb_sectors, int *pnum) { - int64_t ret = bdrv_get_block_status(bs, sector_num, nb_sectors, pnum); + BlockDriverState *file; + int64_t ret = bdrv_get_block_status(bs, sector_num, nb_sectors, pnum, + &file); if (ret < 0) { return ret; } diff --git a/block/iscsi.c b/block/iscsi.c index bffd707b8b..e182557cfc 100644 --- a/block/iscsi.c +++ b/block/iscsi.c @@ -532,7 +532,8 @@ static bool iscsi_allocationmap_is_allocated(IscsiLun *iscsilun, static int64_t coroutine_fn iscsi_co_get_block_status(BlockDriverState *bs, int64_t sector_num, - int nb_sectors, int *pnum) + int nb_sectors, int *pnum, + BlockDriverState **file) { IscsiLun *iscsilun = bs->opaque; struct scsi_get_lba_status *lbas = NULL; @@ -650,7 +651,8 @@ static int coroutine_fn iscsi_co_readv(BlockDriverState *bs, !iscsi_allocationmap_is_allocated(iscsilun, sector_num, nb_sectors)) { int64_t ret; int pnum; - ret = iscsi_co_get_block_status(bs, sector_num, INT_MAX, &pnum); + BlockDriverState *file; + ret = iscsi_co_get_block_status(bs, sector_num, INT_MAX, &pnum, &file); if (ret < 0) { return ret; } diff --git a/block/mirror.c b/block/mirror.c index e9e151c341..2c0edfaf48 100644 --- a/block/mirror.c +++ b/block/mirror.c @@ -168,6 +168,7 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s) MirrorOp *op; int pnum; int64_t ret; + BlockDriverState *file; max_iov = MIN(source->bl.max_iov, s->target->bl.max_iov); @@ -306,7 +307,7 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s) trace_mirror_one_iteration(s, sector_num, nb_sectors); ret = bdrv_get_block_status_above(source, NULL, sector_num, - nb_sectors, &pnum); + nb_sectors, &pnum, &file); if (ret < 0 || pnum < nb_sectors || (ret & BDRV_BLOCK_DATA && !(ret & BDRV_BLOCK_ZERO))) { bdrv_aio_readv(source, sector_num, &op->qiov, nb_sectors, diff --git a/block/parallels.c b/block/parallels.c index ee390815dc..e2de308efa 100644 --- a/block/parallels.c +++ b/block/parallels.c @@ -261,7 +261,7 @@ static coroutine_fn int parallels_co_flush_to_os(BlockDriverState *bs) static int64_t coroutine_fn parallels_co_get_block_status(BlockDriverState *bs, - int64_t sector_num, int nb_sectors, int *pnum) + int64_t sector_num, int nb_sectors, int *pnum, BlockDriverState **file) { BDRVParallelsState *s = bs->opaque; int64_t offset; diff --git a/block/qcow.c b/block/qcow.c index afed18fe98..4202797a11 100644 --- a/block/qcow.c +++ b/block/qcow.c @@ -489,7 +489,7 @@ static uint64_t get_cluster_offset(BlockDriverState *bs, } static int64_t coroutine_fn qcow_co_get_block_status(BlockDriverState *bs, - int64_t sector_num, int nb_sectors, int *pnum) + int64_t sector_num, int nb_sectors, int *pnum, BlockDriverState **file) { BDRVQcowState *s = bs->opaque; int index_in_cluster, n; diff --git a/block/qcow2.c b/block/qcow2.c index fd8436c5f8..d4ea6b416d 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -1330,7 +1330,7 @@ static void qcow2_join_options(QDict *options, QDict *old_options) } static int64_t coroutine_fn qcow2_co_get_block_status(BlockDriverState *bs, - int64_t sector_num, int nb_sectors, int *pnum) + int64_t sector_num, int nb_sectors, int *pnum, BlockDriverState **file) { BDRVQcow2State *s = bs->opaque; uint64_t cluster_offset; diff --git a/block/qed.c b/block/qed.c index 0c870cd90f..8f6f8415d6 100644 --- a/block/qed.c +++ b/block/qed.c @@ -725,7 +725,8 @@ static void qed_is_allocated_cb(void *opaque, int ret, uint64_t offset, size_t l static int64_t coroutine_fn bdrv_qed_co_get_block_status(BlockDriverState *bs, int64_t sector_num, - int nb_sectors, int *pnum) + int nb_sectors, int *pnum, + BlockDriverState **file) { BDRVQEDState *s = bs->opaque; size_t len = (size_t)nb_sectors * BDRV_SECTOR_SIZE; diff --git a/block/raw-posix.c b/block/raw-posix.c index 6df3067ddf..3ef9b256f8 100644 --- a/block/raw-posix.c +++ b/block/raw-posix.c @@ -1818,7 +1818,8 @@ static int find_allocation(BlockDriverState *bs, off_t start, */ static int64_t coroutine_fn raw_co_get_block_status(BlockDriverState *bs, int64_t sector_num, - int nb_sectors, int *pnum) + int nb_sectors, int *pnum, + BlockDriverState **file) { off_t start, data = 0, hole = 0; int64_t total_size; diff --git a/block/raw_bsd.c b/block/raw_bsd.c index bcaee115e1..9a8933b211 100644 --- a/block/raw_bsd.c +++ b/block/raw_bsd.c @@ -115,7 +115,8 @@ fail: static int64_t coroutine_fn raw_co_get_block_status(BlockDriverState *bs, int64_t sector_num, - int nb_sectors, int *pnum) + int nb_sectors, int *pnum, + BlockDriverState **file) { *pnum = nb_sectors; return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | BDRV_BLOCK_DATA | diff --git a/block/sheepdog.c b/block/sheepdog.c index ff89298b13..2ea05a6e28 100644 --- a/block/sheepdog.c +++ b/block/sheepdog.c @@ -2708,7 +2708,7 @@ retry: static coroutine_fn int64_t sd_co_get_block_status(BlockDriverState *bs, int64_t sector_num, int nb_sectors, - int *pnum) + int *pnum, BlockDriverState **file) { BDRVSheepdogState *s = bs->opaque; SheepdogInode *inode = &s->inode; diff --git a/block/vdi.c b/block/vdi.c index 61bcd54575..294c43877a 100644 --- a/block/vdi.c +++ b/block/vdi.c @@ -527,7 +527,7 @@ static int vdi_reopen_prepare(BDRVReopenState *state, } static int64_t coroutine_fn vdi_co_get_block_status(BlockDriverState *bs, - int64_t sector_num, int nb_sectors, int *pnum) + int64_t sector_num, int nb_sectors, int *pnum, BlockDriverState **file) { /* TODO: Check for too large sector_num (in bdrv_is_allocated or here). */ BDRVVdiState *s = (BDRVVdiState *)bs->opaque; diff --git a/block/vmdk.c b/block/vmdk.c index 4a5850bcf0..109fd5f729 100644 --- a/block/vmdk.c +++ b/block/vmdk.c @@ -1261,7 +1261,7 @@ static inline uint64_t vmdk_find_index_in_cluster(VmdkExtent *extent, } static int64_t coroutine_fn vmdk_co_get_block_status(BlockDriverState *bs, - int64_t sector_num, int nb_sectors, int *pnum) + int64_t sector_num, int nb_sectors, int *pnum, BlockDriverState **file) { BDRVVmdkState *s = bs->opaque; int64_t index_in_cluster, n, ret; diff --git a/block/vpc.c b/block/vpc.c index d852f966a0..a0703074f8 100644 --- a/block/vpc.c +++ b/block/vpc.c @@ -579,7 +579,7 @@ static coroutine_fn int vpc_co_write(BlockDriverState *bs, int64_t sector_num, } static int64_t coroutine_fn vpc_co_get_block_status(BlockDriverState *bs, - int64_t sector_num, int nb_sectors, int *pnum) + int64_t sector_num, int nb_sectors, int *pnum, BlockDriverState **file) { BDRVVPCState *s = bs->opaque; VHDFooter *footer = (VHDFooter*) s->footer_buf; diff --git a/block/vvfat.c b/block/vvfat.c index 2ea5a4ab0b..b8d29e17cd 100644 --- a/block/vvfat.c +++ b/block/vvfat.c @@ -2884,7 +2884,7 @@ static coroutine_fn int vvfat_co_write(BlockDriverState *bs, int64_t sector_num, } static int64_t coroutine_fn vvfat_co_get_block_status(BlockDriverState *bs, - int64_t sector_num, int nb_sectors, int* n) + int64_t sector_num, int nb_sectors, int *n, BlockDriverState **file) { BDRVVVFATState* s = bs->opaque; *n = s->sector_count - sector_num; diff --git a/include/block/block.h b/include/block/block.h index 4452b79dd4..0035ad8ea8 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -111,9 +111,10 @@ typedef struct HDGeometry { /* * Allocation status flags - * BDRV_BLOCK_DATA: data is read from bs->file or another file + * BDRV_BLOCK_DATA: data is read from a file returned by bdrv_get_block_status. * BDRV_BLOCK_ZERO: sectors read as zero - * BDRV_BLOCK_OFFSET_VALID: sector stored in bs->file as raw data + * BDRV_BLOCK_OFFSET_VALID: sector stored as raw data in a file returned by + * bdrv_get_block_status. * BDRV_BLOCK_ALLOCATED: the content of the block is determined by this * layer (as opposed to the backing file) * BDRV_BLOCK_RAW: used internally to indicate that the request @@ -384,11 +385,13 @@ int bdrv_has_zero_init(BlockDriverState *bs); bool bdrv_unallocated_blocks_are_zero(BlockDriverState *bs); bool bdrv_can_write_zeroes_with_unmap(BlockDriverState *bs); int64_t bdrv_get_block_status(BlockDriverState *bs, int64_t sector_num, - int nb_sectors, int *pnum); + int nb_sectors, int *pnum, + BlockDriverState **file); int64_t bdrv_get_block_status_above(BlockDriverState *bs, BlockDriverState *base, int64_t sector_num, - int nb_sectors, int *pnum); + int nb_sectors, int *pnum, + BlockDriverState **file); int bdrv_is_allocated(BlockDriverState *bs, int64_t sector_num, int nb_sectors, int *pnum); int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base, diff --git a/include/block/block_int.h b/include/block/block_int.h index dd00d12a6f..9ef823a660 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -166,7 +166,8 @@ struct BlockDriver { int coroutine_fn (*bdrv_co_discard)(BlockDriverState *bs, int64_t sector_num, int nb_sectors); int64_t coroutine_fn (*bdrv_co_get_block_status)(BlockDriverState *bs, - int64_t sector_num, int nb_sectors, int *pnum); + int64_t sector_num, int nb_sectors, int *pnum, + BlockDriverState **file); /* * Invalidate any cached meta-data. diff --git a/qemu-img.c b/qemu-img.c index 33e451c101..e653b2f304 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -1072,13 +1072,15 @@ static int img_compare(int argc, char **argv) for (;;) { int64_t status1, status2; + BlockDriverState *file; + nb_sectors = sectors_to_process(total_sectors, sector_num); if (nb_sectors <= 0) { break; } status1 = bdrv_get_block_status_above(bs1, NULL, sector_num, total_sectors1 - sector_num, - &pnum1); + &pnum1, &file); if (status1 < 0) { ret = 3; error_report("Sector allocation test failed for %s", filename1); @@ -1088,7 +1090,7 @@ static int img_compare(int argc, char **argv) status2 = bdrv_get_block_status_above(bs2, NULL, sector_num, total_sectors2 - sector_num, - &pnum2); + &pnum2, &file); if (status2 < 0) { ret = 3; error_report("Sector allocation test failed for %s", filename2); @@ -1271,9 +1273,10 @@ static int convert_iteration_sectors(ImgConvertState *s, int64_t sector_num) n = MIN(s->total_sectors - sector_num, BDRV_REQUEST_MAX_SECTORS); if (s->sector_next_status <= sector_num) { + BlockDriverState *file; ret = bdrv_get_block_status(blk_bs(s->src[s->src_cur]), sector_num - s->src_cur_offset, - n, &n); + n, &n, &file); if (ret < 0) { return ret; } @@ -2201,6 +2204,7 @@ static int get_block_status(BlockDriverState *bs, int64_t sector_num, { int64_t ret; int depth; + BlockDriverState *file; /* As an optimization, we could cache the current range of unallocated * clusters in each file of the chain, and avoid querying the same @@ -2209,7 +2213,8 @@ static int get_block_status(BlockDriverState *bs, int64_t sector_num, depth = 0; for (;;) { - ret = bdrv_get_block_status(bs, sector_num, nb_sectors, &nb_sectors); + ret = bdrv_get_block_status(bs, sector_num, nb_sectors, &nb_sectors, + &file); if (ret < 0) { return ret; } From 3064bf6fffe4705d983b8cecfbbe3de3e5a75312 Mon Sep 17 00:00:00 2001 From: Fam Zheng Date: Tue, 26 Jan 2016 11:58:49 +0800 Subject: [PATCH 34/50] qcow: Assign bs->file->bs to file in qcow_co_get_block_status Reviewed-by: Eric Blake Reviewed-by: Stefan Hajnoczi Signed-off-by: Fam Zheng Message-id: 1453780743-16806-3-git-send-email-famz@redhat.com Signed-off-by: Max Reitz --- block/qcow.c | 1 + 1 file changed, 1 insertion(+) diff --git a/block/qcow.c b/block/qcow.c index 4202797a11..251910cc9d 100644 --- a/block/qcow.c +++ b/block/qcow.c @@ -510,6 +510,7 @@ static int64_t coroutine_fn qcow_co_get_block_status(BlockDriverState *bs, return BDRV_BLOCK_DATA; } cluster_offset |= (index_in_cluster << BDRV_SECTOR_BITS); + *file = bs->file->bs; return BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID | cluster_offset; } From 178b4db7e5dc6c5bbaa24a72fdb232ec259c26eb Mon Sep 17 00:00:00 2001 From: Fam Zheng Date: Tue, 26 Jan 2016 11:58:50 +0800 Subject: [PATCH 35/50] qcow2: Assign bs->file->bs to file in qcow2_co_get_block_status Reviewed-by: Stefan Hajnoczi Signed-off-by: Fam Zheng Message-id: 1453780743-16806-4-git-send-email-famz@redhat.com Signed-off-by: Max Reitz --- block/qcow2.c | 1 + 1 file changed, 1 insertion(+) diff --git a/block/qcow2.c b/block/qcow2.c index d4ea6b416d..8babecdab2 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -1349,6 +1349,7 @@ static int64_t coroutine_fn qcow2_co_get_block_status(BlockDriverState *bs, !s->cipher) { index_in_cluster = sector_num & (s->cluster_sectors - 1); cluster_offset |= (index_in_cluster << BDRV_SECTOR_BITS); + *file = bs->file->bs; status |= BDRV_BLOCK_OFFSET_VALID | cluster_offset; } if (ret == QCOW2_CLUSTER_ZERO) { From 02650acbc622b51b3428630b2f60ca1966caa1e8 Mon Sep 17 00:00:00 2001 From: Fam Zheng Date: Tue, 26 Jan 2016 11:58:51 +0800 Subject: [PATCH 36/50] raw: Assign bs to file in raw_co_get_block_status Signed-off-by: Fam Zheng Message-id: 1453780743-16806-5-git-send-email-famz@redhat.com Reviewed-by: Max Reitz Signed-off-by: Max Reitz --- block/raw-posix.c | 1 + block/raw_bsd.c | 1 + 2 files changed, 2 insertions(+) diff --git a/block/raw-posix.c b/block/raw-posix.c index 3ef9b256f8..8866121cf7 100644 --- a/block/raw-posix.c +++ b/block/raw-posix.c @@ -1861,6 +1861,7 @@ static int64_t coroutine_fn raw_co_get_block_status(BlockDriverState *bs, *pnum = MIN(nb_sectors, (data - start) / BDRV_SECTOR_SIZE); ret = BDRV_BLOCK_ZERO; } + *file = bs; return ret | BDRV_BLOCK_OFFSET_VALID | start; } diff --git a/block/raw_bsd.c b/block/raw_bsd.c index 9a8933b211..ea16a231c0 100644 --- a/block/raw_bsd.c +++ b/block/raw_bsd.c @@ -119,6 +119,7 @@ static int64_t coroutine_fn raw_co_get_block_status(BlockDriverState *bs, BlockDriverState **file) { *pnum = nb_sectors; + *file = bs->file->bs; return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | BDRV_BLOCK_DATA | (sector_num << BDRV_SECTOR_BITS); } From 3399833f1412e72e6f9e6997775dfdf12d624eaf Mon Sep 17 00:00:00 2001 From: Fam Zheng Date: Tue, 26 Jan 2016 11:58:52 +0800 Subject: [PATCH 37/50] iscsi: Assign bs to file in iscsi_co_get_block_status Reviewed-by: Eric Blake Reviewed-by: Stefan Hajnoczi Signed-off-by: Fam Zheng Message-id: 1453780743-16806-6-git-send-email-famz@redhat.com Signed-off-by: Max Reitz --- block/iscsi.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/block/iscsi.c b/block/iscsi.c index e182557cfc..9fe76f48ec 100644 --- a/block/iscsi.c +++ b/block/iscsi.c @@ -625,6 +625,9 @@ out: if (iTask.task != NULL) { scsi_free_scsi_task(iTask.task); } + if (ret > 0 && ret & BDRV_BLOCK_OFFSET_VALID) { + *file = bs; + } return ret; } From ddf4987d76ebc356da96f6901c1af970ef421da6 Mon Sep 17 00:00:00 2001 From: Fam Zheng Date: Tue, 26 Jan 2016 11:58:53 +0800 Subject: [PATCH 38/50] parallels: Assign bs->file->bs to file in parallels_co_get_block_status Reviewed-by: Eric Blake Reviewed-by: Stefan Hajnoczi Signed-off-by: Fam Zheng Message-id: 1453780743-16806-7-git-send-email-famz@redhat.com Signed-off-by: Max Reitz --- block/parallels.c | 1 + 1 file changed, 1 insertion(+) diff --git a/block/parallels.c b/block/parallels.c index e2de308efa..645521d783 100644 --- a/block/parallels.c +++ b/block/parallels.c @@ -274,6 +274,7 @@ static int64_t coroutine_fn parallels_co_get_block_status(BlockDriverState *bs, return 0; } + *file = bs->file->bs; return (offset << BDRV_SECTOR_BITS) | BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID; } From 53f1dfd1ff5a603ea1bd67b925758d22f54e1f8a Mon Sep 17 00:00:00 2001 From: Fam Zheng Date: Tue, 26 Jan 2016 11:58:54 +0800 Subject: [PATCH 39/50] qed: Assign bs->file->bs to file in bdrv_qed_co_get_block_status Reviewed-by: Eric Blake Reviewed-by: Stefan Hajnoczi Signed-off-by: Fam Zheng Message-id: 1453780743-16806-8-git-send-email-famz@redhat.com Signed-off-by: Max Reitz --- block/qed.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/block/qed.c b/block/qed.c index 8f6f8415d6..404be1e9b9 100644 --- a/block/qed.c +++ b/block/qed.c @@ -693,6 +693,7 @@ typedef struct { uint64_t pos; int64_t status; int *pnum; + BlockDriverState **file; } QEDIsAllocatedCB; static void qed_is_allocated_cb(void *opaque, int ret, uint64_t offset, size_t len) @@ -704,6 +705,7 @@ static void qed_is_allocated_cb(void *opaque, int ret, uint64_t offset, size_t l case QED_CLUSTER_FOUND: offset |= qed_offset_into_cluster(s, cb->pos); cb->status = BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID | offset; + *cb->file = cb->bs->file->bs; break; case QED_CLUSTER_ZERO: cb->status = BDRV_BLOCK_ZERO; @@ -735,6 +737,7 @@ static int64_t coroutine_fn bdrv_qed_co_get_block_status(BlockDriverState *bs, .pos = (uint64_t)sector_num * BDRV_SECTOR_SIZE, .status = BDRV_BLOCK_OFFSET_MASK, .pnum = pnum, + .file = file, }; QEDRequest request = { .l2_table = NULL }; From d234c929310a322357ed4323c7014605449f5802 Mon Sep 17 00:00:00 2001 From: Fam Zheng Date: Tue, 26 Jan 2016 11:58:55 +0800 Subject: [PATCH 40/50] sheepdog: Assign bs to file in sd_co_get_block_status Reviewed-by: Stefan Hajnoczi Signed-off-by: Fam Zheng Message-id: 1453780743-16806-9-git-send-email-famz@redhat.com Signed-off-by: Max Reitz --- block/sheepdog.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/block/sheepdog.c b/block/sheepdog.c index 2ea05a6e28..a0098c1165 100644 --- a/block/sheepdog.c +++ b/block/sheepdog.c @@ -2739,6 +2739,9 @@ sd_co_get_block_status(BlockDriverState *bs, int64_t sector_num, int nb_sectors, if (*pnum > nb_sectors) { *pnum = nb_sectors; } + if (ret > 0 && ret & BDRV_BLOCK_OFFSET_VALID) { + *file = bs; + } return ret; } From 8bfb137152d2195c70e99ed3269a3f4825ffc614 Mon Sep 17 00:00:00 2001 From: Fam Zheng Date: Tue, 26 Jan 2016 11:58:56 +0800 Subject: [PATCH 41/50] vdi: Assign bs->file->bs to file in vdi_co_get_block_status Reviewed-by: Stefan Hajnoczi Signed-off-by: Fam Zheng Message-id: 1453780743-16806-10-git-send-email-famz@redhat.com Signed-off-by: Max Reitz --- block/vdi.c | 1 + 1 file changed, 1 insertion(+) diff --git a/block/vdi.c b/block/vdi.c index 294c43877a..b403243604 100644 --- a/block/vdi.c +++ b/block/vdi.c @@ -551,6 +551,7 @@ static int64_t coroutine_fn vdi_co_get_block_status(BlockDriverState *bs, offset = s->header.offset_data + (uint64_t)bmap_entry * s->block_size + sector_in_block * SECTOR_SIZE; + *file = bs->file->bs; return BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID | offset; } From 7429e207883fb6f6bd46cefee4c79771e50f35f4 Mon Sep 17 00:00:00 2001 From: Fam Zheng Date: Tue, 26 Jan 2016 11:58:57 +0800 Subject: [PATCH 42/50] vpc: Assign bs->file->bs to file in vpc_co_get_block_status Reviewed-by: Stefan Hajnoczi Signed-off-by: Fam Zheng Message-id: 1453780743-16806-11-git-send-email-famz@redhat.com Signed-off-by: Max Reitz --- block/vpc.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/block/vpc.c b/block/vpc.c index a0703074f8..f504536d1c 100644 --- a/block/vpc.c +++ b/block/vpc.c @@ -589,6 +589,7 @@ static int64_t coroutine_fn vpc_co_get_block_status(BlockDriverState *bs, if (be32_to_cpu(footer->type) == VHD_FIXED) { *pnum = nb_sectors; + *file = bs->file->bs; return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | BDRV_BLOCK_DATA | (sector_num << BDRV_SECTOR_BITS); } @@ -610,6 +611,7 @@ static int64_t coroutine_fn vpc_co_get_block_status(BlockDriverState *bs, /* *pnum can't be greater than one block for allocated * sectors since there is always a bitmap in between. */ if (allocated) { + *file = bs->file->bs; return BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID | start; } if (nb_sectors == 0) { From d0a18f10251f515c86dcaec5bdf979a4e07fafc5 Mon Sep 17 00:00:00 2001 From: Fam Zheng Date: Tue, 26 Jan 2016 11:58:58 +0800 Subject: [PATCH 43/50] vmdk: Fix calculation of block status's offset "offset" is the offset of cluster and sector_num doesn't necessarily refer to the start of it, it should add index_in_cluster. Signed-off-by: Fam Zheng Message-id: 1453780743-16806-12-git-send-email-famz@redhat.com Reviewed-by: Max Reitz Signed-off-by: Max Reitz --- block/vmdk.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/block/vmdk.c b/block/vmdk.c index 109fd5f729..9d5a18ac05 100644 --- a/block/vmdk.c +++ b/block/vmdk.c @@ -1278,6 +1278,7 @@ static int64_t coroutine_fn vmdk_co_get_block_status(BlockDriverState *bs, 0, 0); qemu_co_mutex_unlock(&s->lock); + index_in_cluster = vmdk_find_index_in_cluster(extent, sector_num); switch (ret) { case VMDK_ERROR: ret = -EIO; @@ -1291,13 +1292,14 @@ static int64_t coroutine_fn vmdk_co_get_block_status(BlockDriverState *bs, case VMDK_OK: ret = BDRV_BLOCK_DATA; if (extent->file == bs->file && !extent->compressed) { - ret |= BDRV_BLOCK_OFFSET_VALID | offset; + ret |= BDRV_BLOCK_OFFSET_VALID; + ret |= (offset + (index_in_cluster << BDRV_SECTOR_BITS)) + & BDRV_BLOCK_OFFSET_MASK; } break; } - index_in_cluster = vmdk_find_index_in_cluster(extent, sector_num); n = extent->cluster_sectors - index_in_cluster; if (n > nb_sectors) { n = nb_sectors; From e0f100f57ceee919a7df7af316961b175ffef4e6 Mon Sep 17 00:00:00 2001 From: Fam Zheng Date: Tue, 26 Jan 2016 11:58:59 +0800 Subject: [PATCH 44/50] vmdk: Return extent's file in bdrv_get_block_status Signed-off-by: Fam Zheng Message-id: 1453780743-16806-13-git-send-email-famz@redhat.com Reviewed-by: Max Reitz Signed-off-by: Max Reitz --- block/vmdk.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/block/vmdk.c b/block/vmdk.c index 9d5a18ac05..a8db5d9ec2 100644 --- a/block/vmdk.c +++ b/block/vmdk.c @@ -1291,12 +1291,12 @@ static int64_t coroutine_fn vmdk_co_get_block_status(BlockDriverState *bs, break; case VMDK_OK: ret = BDRV_BLOCK_DATA; - if (extent->file == bs->file && !extent->compressed) { + if (!extent->compressed) { ret |= BDRV_BLOCK_OFFSET_VALID; ret |= (offset + (index_in_cluster << BDRV_SECTOR_BITS)) & BDRV_BLOCK_OFFSET_MASK; } - + *file = extent->file->bs; break; } From ac987b30d0a97f14b82584921fc5ab3cd88c431b Mon Sep 17 00:00:00 2001 From: Fam Zheng Date: Tue, 26 Jan 2016 11:59:00 +0800 Subject: [PATCH 45/50] block: Use returned *file in bdrv_co_get_block_status Now that all drivers return the right "file" pointer, we can use it. Signed-off-by: Fam Zheng Reviewed-by: Max Reitz Message-id: 1453780743-16806-14-git-send-email-famz@redhat.com Signed-off-by: Max Reitz --- block/io.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/block/io.c b/block/io.c index ea040bef22..343ff1f233 100644 --- a/block/io.c +++ b/block/io.c @@ -1554,13 +1554,13 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs, } } - if (bs->file && + if (*file && *file != bs && (ret & BDRV_BLOCK_DATA) && !(ret & BDRV_BLOCK_ZERO) && (ret & BDRV_BLOCK_OFFSET_VALID)) { BlockDriverState *file2; int file_pnum; - ret2 = bdrv_co_get_block_status(bs->file->bs, ret >> BDRV_SECTOR_BITS, + ret2 = bdrv_co_get_block_status(*file, ret >> BDRV_SECTOR_BITS, *pnum, &file_pnum, &file2); if (ret2 >= 0) { /* Ignore errors. This is just providing extra information, it From 9e4303400801e62e8e357decdb487834582459e8 Mon Sep 17 00:00:00 2001 From: Fam Zheng Date: Tue, 26 Jan 2016 11:59:01 +0800 Subject: [PATCH 46/50] qemu-img: In "map", use the returned "file" from bdrv_get_block_status Now all drivers should return a correct "file", we can make use of it, even with the recursion into backing chain above. Reviewed-by: Eric Blake Reviewed-by: Stefan Hajnoczi Signed-off-by: Fam Zheng Message-id: 1453780743-16806-15-git-send-email-famz@redhat.com Signed-off-by: Max Reitz --- qemu-img.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/qemu-img.c b/qemu-img.c index e653b2f304..c8bc63f0a0 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -2236,7 +2236,7 @@ static int get_block_status(BlockDriverState *bs, int64_t sector_num, e->flags = ret & ~BDRV_BLOCK_OFFSET_MASK; e->offset = ret & BDRV_BLOCK_OFFSET_MASK; e->depth = depth; - e->bs = bs; + e->bs = file; return 0; } From 16b0d555861b732c1dd76a986697214de1d774d5 Mon Sep 17 00:00:00 2001 From: Fam Zheng Date: Tue, 26 Jan 2016 11:59:02 +0800 Subject: [PATCH 47/50] qemu-img: Make MapEntry a QAPI struct The "flags" bit mask is expanded to two booleans, "data" and "zero"; "bs" is replaced with "filename" string. Refactor the merge conditions in img_map() into entry_mergeable(). Reviewed-by: Eric Blake Reviewed-by: Stefan Hajnoczi Signed-off-by: Fam Zheng Message-id: 1453780743-16806-16-git-send-email-famz@redhat.com Signed-off-by: Max Reitz --- qapi/block-core.json | 27 +++++++++++++++++ qemu-img.c | 71 ++++++++++++++++++++++++++------------------ 2 files changed, 69 insertions(+), 29 deletions(-) diff --git a/qapi/block-core.json b/qapi/block-core.json index 628a41dfca..33012b86c1 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -185,6 +185,33 @@ '*total-clusters': 'int', '*allocated-clusters': 'int', '*fragmented-clusters': 'int', '*compressed-clusters': 'int' } } +## +# @MapEntry: +# +# Mapping information from a virtual block range to a host file range +# +# @start: the start byte of the mapped virtual range +# +# @length: the number of bytes of the mapped virtual range +# +# @data: whether the mapped range has data +# +# @zero: whether the virtual blocks are zeroed +# +# @depth: the depth of the mapping +# +# @offset: #optional the offset in file that the virtual sectors are mapped to +# +# @filename: #optional filename that is referred to by @offset +# +# Since: 2.6 +# +## +{ 'struct': 'MapEntry', + 'data': {'start': 'int', 'length': 'int', 'data': 'bool', + 'zero': 'bool', 'depth': 'int', '*offset': 'int', + '*filename': 'str' } } + ## # @BlockdevCacheInfo # diff --git a/qemu-img.c b/qemu-img.c index c8bc63f0a0..f121980707 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -2147,47 +2147,37 @@ static int img_info(int argc, char **argv) return 0; } - -typedef struct MapEntry { - int flags; - int depth; - int64_t start; - int64_t length; - int64_t offset; - BlockDriverState *bs; -} MapEntry; - static void dump_map_entry(OutputFormat output_format, MapEntry *e, MapEntry *next) { switch (output_format) { case OFORMAT_HUMAN: - if ((e->flags & BDRV_BLOCK_DATA) && - !(e->flags & BDRV_BLOCK_OFFSET_VALID)) { + if (e->data && !e->has_offset) { error_report("File contains external, encrypted or compressed clusters."); exit(1); } - if ((e->flags & (BDRV_BLOCK_DATA|BDRV_BLOCK_ZERO)) == BDRV_BLOCK_DATA) { + if (e->data && !e->zero) { printf("%#-16"PRIx64"%#-16"PRIx64"%#-16"PRIx64"%s\n", - e->start, e->length, e->offset, e->bs->filename); + e->start, e->length, + e->has_offset ? e->offset : 0, + e->has_filename ? e->filename : ""); } /* This format ignores the distinction between 0, ZERO and ZERO|DATA. * Modify the flags here to allow more coalescing. */ - if (next && - (next->flags & (BDRV_BLOCK_DATA|BDRV_BLOCK_ZERO)) != BDRV_BLOCK_DATA) { - next->flags &= ~BDRV_BLOCK_DATA; - next->flags |= BDRV_BLOCK_ZERO; + if (next && (!next->data || next->zero)) { + next->data = false; + next->zero = true; } break; case OFORMAT_JSON: - printf("%s{ \"start\": %"PRId64", \"length\": %"PRId64", \"depth\": %d," - " \"zero\": %s, \"data\": %s", + printf("%s{ \"start\": %"PRId64", \"length\": %"PRId64"," + " \"depth\": %"PRId64", \"zero\": %s, \"data\": %s", (e->start == 0 ? "[" : ",\n"), e->start, e->length, e->depth, - (e->flags & BDRV_BLOCK_ZERO) ? "true" : "false", - (e->flags & BDRV_BLOCK_DATA) ? "true" : "false"); - if (e->flags & BDRV_BLOCK_OFFSET_VALID) { + e->zero ? "true" : "false", + e->data ? "true" : "false"); + if (e->has_offset) { printf(", \"offset\": %"PRId64"", e->offset); } putchar('}'); @@ -2233,13 +2223,39 @@ static int get_block_status(BlockDriverState *bs, int64_t sector_num, e->start = sector_num * BDRV_SECTOR_SIZE; e->length = nb_sectors * BDRV_SECTOR_SIZE; - e->flags = ret & ~BDRV_BLOCK_OFFSET_MASK; + e->data = !!(ret & BDRV_BLOCK_DATA); + e->zero = !!(ret & BDRV_BLOCK_ZERO); e->offset = ret & BDRV_BLOCK_OFFSET_MASK; + e->has_offset = !!(ret & BDRV_BLOCK_OFFSET_VALID); e->depth = depth; - e->bs = file; + if (file && e->has_offset) { + e->has_filename = true; + e->filename = file->filename; + } return 0; } +static inline bool entry_mergeable(const MapEntry *curr, const MapEntry *next) +{ + if (curr->length == 0) { + return false; + } + if (curr->zero != next->zero || + curr->data != next->data || + curr->depth != next->depth || + curr->has_filename != next->has_filename || + curr->has_offset != next->has_offset) { + return false; + } + if (curr->has_filename && strcmp(curr->filename, next->filename)) { + return false; + } + if (curr->has_offset && curr->offset + curr->length != next->offset) { + return false; + } + return true; +} + static int img_map(int argc, char **argv) { int c; @@ -2321,10 +2337,7 @@ static int img_map(int argc, char **argv) goto out; } - if (curr.length != 0 && curr.flags == next.flags && - curr.depth == next.depth && - ((curr.flags & BDRV_BLOCK_OFFSET_VALID) == 0 || - curr.offset + curr.length == next.offset)) { + if (entry_mergeable(&curr, &next)) { curr.length += next.length; continue; } From c7fc50d37620550c52854ca15c2a18da1b8155e0 Mon Sep 17 00:00:00 2001 From: Fam Zheng Date: Tue, 26 Jan 2016 11:59:03 +0800 Subject: [PATCH 48/50] iotests: Add "qemu-img map" test for VMDK extents Signed-off-by: Fam Zheng Message-id: 1453780743-16806-17-git-send-email-famz@redhat.com Reviewed-by: Max Reitz Signed-off-by: Max Reitz --- tests/qemu-iotests/059 | 10 ++++++++++ tests/qemu-iotests/059.out | 25 +++++++++++++++++++++++++ 2 files changed, 35 insertions(+) diff --git a/tests/qemu-iotests/059 b/tests/qemu-iotests/059 index 0ded0c3da4..0332bbb348 100755 --- a/tests/qemu-iotests/059 +++ b/tests/qemu-iotests/059 @@ -132,6 +132,16 @@ _img_info $QEMU_IO -c "write -P 0xa 900G 512" "$TEST_IMG" | _filter_qemu_io $QEMU_IO -c "read -v 900G 1024" "$TEST_IMG" | _filter_qemu_io +echo +echo "=== Testing qemu-img map on extents ===" +for fmt in monolithicSparse twoGbMaxExtentSparse; do + IMGOPTS="subformat=$fmt" _make_test_img 31G + $QEMU_IO -c "write 65024 1k" "$TEST_IMG" | _filter_qemu_io + $QEMU_IO -c "write 2147483136 1k" "$TEST_IMG" | _filter_qemu_io + $QEMU_IO -c "write 5G 1k" "$TEST_IMG" | _filter_qemu_io + $QEMU_IMG map "$TEST_IMG" | _filter_testdir +done + echo echo "=== Testing afl image with a very large capacity ===" _use_sample_img afl9.vmdk.bz2 diff --git a/tests/qemu-iotests/059.out b/tests/qemu-iotests/059.out index 9d506cb80c..678adb4379 100644 --- a/tests/qemu-iotests/059.out +++ b/tests/qemu-iotests/059.out @@ -2335,6 +2335,31 @@ e1000003f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ read 1024/1024 bytes at offset 966367641600 1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +=== Testing qemu-img map on extents === +Formatting 'TEST_DIR/iotest-version3.IMGFMT', fmt=IMGFMT size=33285996544 subformat=monolithicSparse +wrote 1024/1024 bytes at offset 65024 +1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +wrote 1024/1024 bytes at offset 2147483136 +1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +wrote 1024/1024 bytes at offset 5368709120 +1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +Offset Length Mapped to File +0 0x20000 0x3f0000 TEST_DIR/iotest-version3.vmdk +0x7fff0000 0x20000 0x410000 TEST_DIR/iotest-version3.vmdk +0x140000000 0x10000 0x430000 TEST_DIR/iotest-version3.vmdk +Formatting 'TEST_DIR/iotest-version3.IMGFMT', fmt=IMGFMT size=33285996544 subformat=twoGbMaxExtentSparse +wrote 1024/1024 bytes at offset 65024 +1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +wrote 1024/1024 bytes at offset 2147483136 +1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +wrote 1024/1024 bytes at offset 5368709120 +1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +Offset Length Mapped to File +0 0x20000 0x50000 TEST_DIR/iotest-version3-s001.vmdk +0x7fff0000 0x10000 0x70000 TEST_DIR/iotest-version3-s001.vmdk +0x80000000 0x10000 0x50000 TEST_DIR/iotest-version3-s002.vmdk +0x140000000 0x10000 0x50000 TEST_DIR/iotest-version3-s003.vmdk + === Testing afl image with a very large capacity === qemu-img: Can't get size of device 'image': File too large *** done From f8aa905a4fec89863c82de4186352447d851871e Mon Sep 17 00:00:00 2001 From: Jeff Cody Date: Mon, 1 Feb 2016 20:33:10 -0500 Subject: [PATCH 49/50] block: set device_list.tqe_prev to NULL on BDS removal This fixes a regression introduced with commit 3f09bfbc7. Multiple bugs arise in conjunction with live snapshots and mirroring operations (which include active layer commit). After a live snapshot occurs, the active layer and the base layer both have a non-NULL tqe_prev field in the device_list, although the base node's tqe_prev field points to a NULL entry. This non-NULL tqe_prev field occurs after the bdrv_append() in the external snapshot calls change_parent_backing_link(). In change_parent_backing_link(), when the previous active layer is removed from device_list, the device_list.tqe_prev pointer is not set to NULL. The operating scheme in the block layer is to indicate that a BDS belongs in the bdrv_states device_list iff the device_list.tqe_prev pointer is non-NULL. This patch does two things: 1.) Introduces a new block layer helper bdrv_device_remove() to remove a BDS from the device_list, and 2.) uses that new API, which also fixes the regression once used in change_parent_backing_link(). Signed-off-by: Jeff Cody Message-id: 0cd51e11c0666c04ddb7c05293fe94afeb551e89.1454376655.git.jcody@redhat.com Reviewed-by: Max Reitz Signed-off-by: Max Reitz --- block.c | 24 ++++++++++++++---------- blockdev.c | 3 +-- include/block/block.h | 1 + 3 files changed, 16 insertions(+), 12 deletions(-) diff --git a/block.c b/block.c index ff1aafc85f..dff3a3a3f9 100644 --- a/block.c +++ b/block.c @@ -2242,21 +2242,25 @@ void bdrv_close_all(void) } } +/* Note that bs->device_list.tqe_prev is initially null, + * and gets set to non-null by QTAILQ_INSERT_TAIL(). Establish + * the useful invariant "bs in bdrv_states iff bs->tqe_prev" by + * resetting it to null on remove. */ +void bdrv_device_remove(BlockDriverState *bs) +{ + QTAILQ_REMOVE(&bdrv_states, bs, device_list); + bs->device_list.tqe_prev = NULL; +} + /* make a BlockDriverState anonymous by removing from bdrv_state and * graph_bdrv_state list. Also, NULL terminate the device_name to prevent double remove */ void bdrv_make_anon(BlockDriverState *bs) { - /* - * Take care to remove bs from bdrv_states only when it's actually - * in it. Note that bs->device_list.tqe_prev is initially null, - * and gets set to non-null by QTAILQ_INSERT_TAIL(). Establish - * the useful invariant "bs in bdrv_states iff bs->tqe_prev" by - * resetting it to null on remove. - */ + /* Take care to remove bs from bdrv_states only when it's actually + * in it. */ if (bs->device_list.tqe_prev) { - QTAILQ_REMOVE(&bdrv_states, bs, device_list); - bs->device_list.tqe_prev = NULL; + bdrv_device_remove(bs); } if (bs->node_name[0] != '\0') { QTAILQ_REMOVE(&graph_bdrv_states, bs, node_list); @@ -2297,7 +2301,7 @@ static void change_parent_backing_link(BlockDriverState *from, if (!to->device_list.tqe_prev) { QTAILQ_INSERT_BEFORE(from, to, device_list); } - QTAILQ_REMOVE(&bdrv_states, from, device_list); + bdrv_device_remove(from); } } diff --git a/blockdev.c b/blockdev.c index 35e1e5c58f..be4ca44ab2 100644 --- a/blockdev.c +++ b/blockdev.c @@ -2408,8 +2408,7 @@ void qmp_x_blockdev_remove_medium(const char *device, Error **errp) /* This follows the convention established by bdrv_make_anon() */ if (bs->device_list.tqe_prev) { - QTAILQ_REMOVE(&bdrv_states, bs, device_list); - bs->device_list.tqe_prev = NULL; + bdrv_device_remove(bs); } blk_remove_bs(blk); diff --git a/include/block/block.h b/include/block/block.h index 0035ad8ea8..1c4f4d8141 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -199,6 +199,7 @@ int bdrv_create(BlockDriver *drv, const char* filename, int bdrv_create_file(const char *filename, QemuOpts *opts, Error **errp); BlockDriverState *bdrv_new_root(void); BlockDriverState *bdrv_new(void); +void bdrv_device_remove(BlockDriverState *bs); void bdrv_make_anon(BlockDriverState *bs); void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top); void bdrv_replace_in_backing_chain(BlockDriverState *old, From 8983b670f62ab5e5e8dd2690bf8304123651bfe5 Mon Sep 17 00:00:00 2001 From: Jeff Cody Date: Mon, 1 Feb 2016 20:33:11 -0500 Subject: [PATCH 50/50] block: qemu-iotests - add test for snapshot, commit, snapshot bug Signed-off-by: Jeff Cody Message-id: 2dbc05efba2f683cb3aaf71aaa9b776ebf7ec57c.1454376655.git.jcody@redhat.com Reviewed-by: Max Reitz [Moved test number from 143 to 144] Signed-off-by: Max Reitz --- tests/qemu-iotests/144 | 114 +++++++++++++++++++++++++++++++++++++ tests/qemu-iotests/144.out | 24 ++++++++ tests/qemu-iotests/group | 1 + 3 files changed, 139 insertions(+) create mode 100755 tests/qemu-iotests/144 create mode 100644 tests/qemu-iotests/144.out diff --git a/tests/qemu-iotests/144 b/tests/qemu-iotests/144 new file mode 100755 index 0000000000..00de3c33cf --- /dev/null +++ b/tests/qemu-iotests/144 @@ -0,0 +1,114 @@ +#!/bin/bash +# Check live snapshot, followed by active commit, and another snapshot. +# +# This test is to catch the error case of BZ #1300209: +# https://bugzilla.redhat.com/show_bug.cgi?id=1300209 +# +# Copyright (C) 2016 Red Hat, Inc. +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 2 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see . +# + +# creator +owner=jcody@redhat.com + +seq=`basename $0` +echo "QA output created by $seq" + +here=`pwd` +status=1 # failure is the default! + +TMP_SNAP1=${TEST_DIR}/tmp.qcow2 +TMP_SNAP2=${TEST_DIR}/tmp2.qcow2 + +_cleanup() +{ + _cleanup_qemu + rm -f "${TEST_IMG}" "${TMP_SNAP1}" "${TMP_SNAP2}" +} + +trap "_cleanup; exit \$status" 0 1 2 3 15 + +# get standard environment, filters and checks +. ./common.rc +. ./common.filter +. ./common.qemu + +_supported_fmt qcow2 +_supported_proto file +_supported_os Linux + +size=512M + +_make_test_img $size + +echo +echo === Launching QEMU === +echo + +qemu_comm_method="qmp" +_launch_qemu -drive file="${TEST_IMG}",if=virtio +h=$QEMU_HANDLE + + +echo +echo === Performing Live Snapshot 1 === +echo + +_send_qemu_cmd $h "{ 'execute': 'qmp_capabilities' }" "return" + + +# First live snapshot, new overlay as active layer +_send_qemu_cmd $h "{ 'execute': 'blockdev-snapshot-sync', + 'arguments': { + 'device': 'virtio0', + 'snapshot-file':'${TMP_SNAP1}', + 'format': 'qcow2' + } + }" "return" + +echo +echo === Performing block-commit on active layer === +echo + +# Block commit on active layer, push the new overlay into base +_send_qemu_cmd $h "{ 'execute': 'block-commit', + 'arguments': { + 'device': 'virtio0' + } + }" "READY" + +_send_qemu_cmd $h "{ 'execute': 'block-job-complete', + 'arguments': { + 'device': 'virtio0' + } + }" "COMPLETED" + +echo +echo === Performing Live Snapshot 2 === +echo + +# New live snapshot, new overlays as active layer +_send_qemu_cmd $h "{ 'execute': 'blockdev-snapshot-sync', + 'arguments': { + 'device': 'virtio0', + 'snapshot-file':'${TMP_SNAP2}', + 'format': 'qcow2' + } + }" "return" + +# success, all done +echo "*** done" +rm -f $seq.full +status=0 diff --git a/tests/qemu-iotests/144.out b/tests/qemu-iotests/144.out new file mode 100644 index 0000000000..410d74180a --- /dev/null +++ b/tests/qemu-iotests/144.out @@ -0,0 +1,24 @@ +QA output created by 144 +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=536870912 + +=== Launching QEMU === + + +=== Performing Live Snapshot 1 === + +{"return": {}} +Formatting 'TEST_DIR/tmp.qcow2', fmt=qcow2 size=536870912 backing_file=TEST_DIR/t.qcow2 backing_fmt=qcow2 encryption=off cluster_size=65536 lazy_refcounts=off refcount_bits=16 +{"return": {}} + +=== Performing block-commit on active layer === + +{"return": {}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "virtio0", "len": 0, "offset": 0, "speed": 0, "type": "commit"}} +{"return": {}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "virtio0", "len": 0, "offset": 0, "speed": 0, "type": "commit"}} + +=== Performing Live Snapshot 2 === + +Formatting 'TEST_DIR/tmp2.qcow2', fmt=qcow2 size=536870912 backing_file=TEST_DIR/t.qcow2 backing_fmt=qcow2 encryption=off cluster_size=65536 lazy_refcounts=off refcount_bits=16 +{"return": {}} +*** done diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group index cbc970a471..65df029d7d 100644 --- a/tests/qemu-iotests/group +++ b/tests/qemu-iotests/group @@ -146,3 +146,4 @@ 141 rw auto quick 142 auto 143 auto quick +144 rw auto quick