[00/15] block: AioContext management, part 2
mbox series

Message ID 20190523160104.21258-1-kwolf@redhat.com
Headers show
Series
  • block: AioContext management, part 2
Related show

Message

Kevin Wolf May 23, 2019, 4 p.m. UTC
Recently, a few bugs were reported that resulted from an inconsistent
state regarding AioContexts. Block nodes can end up in different
contexts than their users expect - the AioContext of a node can even
change under the feet of a device with no way for the device to forbid
this. We recently added a few basic checks to scsi-disk and virtio-blk,
but they are by far not enough.

Part 1 solved the first half of the problem and made sure that
AioContext changes are propagated through the graph as necessary, so
that a bdrv_set_aio_context() correctly pulls everything that uses the
node into the right context.

This is part 2 that fixes the second half: Attaching new child nodes
where the parent and child are already in different AioContexts. This
operation may only succeed if we can move one of the node into the
context of the other node.

After applying this series, unchecked bdrv_set_aio_context() doesn't
exist any more and all users call functions that make sure that the
AioContext assignments across the graph stays consistent.

Kevin Wolf (15):
  test-block-iothread: Check filter node in test_propagate_mirror
  block: Add Error to blk_set_aio_context()
  block: Add BlockBackend.ctx
  block: Add qdev_prop_drive_iothread property type
  scsi-disk: Use qdev_prop_drive_iothread
  block: Adjust AioContexts when attaching nodes
  test-block-iothread: Test adding parent to iothread node
  test-block-iothread: BlockBackend AioContext across root node change
  block: Move node without parents to main AioContext
  blockdev: Use bdrv_try_set_aio_context() for monitor commands
  block: Remove wrong bdrv_set_aio_context() calls
  virtio-scsi-test: Test attaching new overlay with iothreads
  iotests: Attach new devices to node in non-default iothread
  test-bdrv-drain: Use bdrv_try_set_aio_context()
  block: Remove bdrv_set_aio_context()

 docs/devel/multiple-iothreads.txt |   4 +-
 include/block/block.h             |   9 ---
 include/block/block_int.h         |   1 +
 include/hw/block/block.h          |   7 +-
 include/hw/qdev-properties.h      |   3 +
 include/hw/scsi/scsi.h            |   1 +
 include/sysemu/block-backend.h    |   5 +-
 tests/libqtest.h                  |  11 ++++
 block.c                           |  70 ++++++++++++++------
 block/backup.c                    |   3 +-
 block/block-backend.c             |  47 +++++++++-----
 block/commit.c                    |  13 ++--
 block/crypto.c                    |   3 +-
 block/mirror.c                    |   4 +-
 block/parallels.c                 |   3 +-
 block/qcow.c                      |   3 +-
 block/qcow2.c                     |   6 +-
 block/qed.c                       |   3 +-
 block/sheepdog.c                  |   3 +-
 block/vdi.c                       |   3 +-
 block/vhdx.c                      |   3 +-
 block/vmdk.c                      |   3 +-
 block/vpc.c                       |   3 +-
 blockdev.c                        |  48 ++++++++------
 blockjob.c                        |  12 +++-
 hmp.c                             |   3 +-
 hw/block/dataplane/virtio-blk.c   |  12 +++-
 hw/block/dataplane/xen-block.c    |   6 +-
 hw/block/fdc.c                    |   2 +-
 hw/block/xen-block.c              |   2 +-
 hw/core/qdev-properties-system.c  |  41 +++++++++++-
 hw/ide/qdev.c                     |   2 +-
 hw/scsi/scsi-disk.c               |  24 ++++---
 hw/scsi/virtio-scsi.c             |  25 ++++---
 migration/block.c                 |   3 +-
 nbd/server.c                      |   5 +-
 tests/libqtest.c                  |  19 ++++++
 tests/test-bdrv-drain.c           |  50 +++++++-------
 tests/test-bdrv-graph-mod.c       |   5 +-
 tests/test-block-backend.c        |   4 +-
 tests/test-block-iothread.c       | 104 +++++++++++++++++++++++++-----
 tests/test-blockjob.c             |   2 +-
 tests/test-throttle.c             |   6 +-
 tests/virtio-scsi-test.c          |  62 ++++++++++++++++++
 tests/qemu-iotests/051            |  24 +++++++
 tests/qemu-iotests/051.out        |   3 +
 tests/qemu-iotests/051.pc.out     |  27 ++++++++
 tests/qemu-iotests/240.out        |   2 +-
 48 files changed, 531 insertions(+), 173 deletions(-)

Comments

no-reply@patchew.org May 23, 2019, 7:44 p.m. UTC | #1
Patchew URL: https://patchew.org/QEMU/20190523160104.21258-1-kwolf@redhat.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Subject: [Qemu-devel] [PATCH 00/15] block: AioContext management, part 2
Type: series
Message-id: 20190523160104.21258-1-kwolf@redhat.com

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
6ca27a5 block: Remove bdrv_set_aio_context()
984ba6d test-bdrv-drain: Use bdrv_try_set_aio_context()
b054561 iotests: Attach new devices to node in non-default iothread
98252b3 virtio-scsi-test: Test attaching new overlay with iothreads
7f6cd93 block: Remove wrong bdrv_set_aio_context() calls
0419720 blockdev: Use bdrv_try_set_aio_context() for monitor commands
26a1acf block: Move node without parents to main AioContext
cd2b0d7 test-block-iothread: BlockBackend AioContext across root node change
5a4c60c test-block-iothread: Test adding parent to iothread node
1767a79 block: Adjust AioContexts when attaching nodes
16a9345 scsi-disk: Use qdev_prop_drive_iothread
cc6b17c block: Add qdev_prop_drive_iothread property type
2e30704 block: Add BlockBackend.ctx
15a3e30 block: Add Error to blk_set_aio_context()
88715f0 test-block-iothread: Check filter node in test_propagate_mirror

=== OUTPUT BEGIN ===
1/15 Checking commit 88715f0ee897 (test-block-iothread: Check filter node in test_propagate_mirror)
2/15 Checking commit 15a3e308457c (block: Add Error to blk_set_aio_context())
WARNING: Block comments use a leading /* on a separate line
#104: FILE: hw/block/dataplane/virtio-blk.c:289:
+    /* Drain and try to switch bs back to the QEMU main loop. If other users

WARNING: Block comments use a trailing */ on a separate line
#105: FILE: hw/block/dataplane/virtio-blk.c:290:
+     * keep the BlockBackend in the iothread, that's ok */

total: 0 errors, 2 warnings, 259 lines checked

Patch 2/15 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
3/15 Checking commit 2e30704b8e57 (block: Add BlockBackend.ctx)
WARNING: Block comments use a leading /* on a separate line
#96: FILE: block/block-backend.c:1865:
+    /* FIXME The AioContext of bs and blk can be inconsistent. For the moment,

WARNING: Block comments use a trailing */ on a separate line
#97: FILE: block/block-backend.c:1866:
+     * we prefer the one of bs for compatibility. */

WARNING: Block comments use a leading /* on a separate line
#405: FILE: hw/core/qdev-properties-system.c:83:
+            /* BlockBackends of devices start in the main context and are only

WARNING: Block comments use a trailing */ on a separate line
#406: FILE: hw/core/qdev-properties-system.c:84:
+             * later moved into another context if the device supports that. */

WARNING: line over 80 characters
#647: FILE: tests/test-block-backend.c:40:
+    BlockBackend *blk = blk_new(qemu_get_aio_context(), BLK_PERM_ALL, BLK_PERM_ALL);

WARNING: line over 80 characters
#656: FILE: tests/test-block-backend.c:56:
+    BlockBackend *blk = blk_new(qemu_get_aio_context(), BLK_PERM_ALL, BLK_PERM_ALL);

total: 0 errors, 6 warnings, 533 lines checked

Patch 3/15 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
4/15 Checking commit cc6b17c2c35e (block: Add qdev_prop_drive_iothread property type)
ERROR: Macros with complex values should be enclosed in parenthesis
#133: FILE: include/hw/block/block.h:61:
+#define DEFINE_BLOCK_PROPERTIES(_state, _conf)                          \
+    DEFINE_PROP_DRIVE("drive", _state, _conf.blk),                      \
+    DEFINE_BLOCK_PROPERTIES_BASE(_state, _conf)

total: 1 errors, 0 warnings, 107 lines checked

Patch 4/15 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

5/15 Checking commit 16a934533950 (scsi-disk: Use qdev_prop_drive_iothread)
ERROR: Macros with complex values should be enclosed in parenthesis
#49: FILE: hw/scsi/scsi-disk.c:2939:
+#define DEFINE_SCSI_DISK_PROPERTIES()                                   \
+    DEFINE_PROP_DRIVE_IOTHREAD("drive", SCSIDiskState, qdev.conf.blk),  \
+    DEFINE_BLOCK_PROPERTIES_BASE(SCSIDiskState, qdev.conf),             \
+    DEFINE_BLOCK_ERROR_PROPERTIES(SCSIDiskState, qdev.conf),            \
+    DEFINE_PROP_STRING("ver", SCSIDiskState, version),                  \
+    DEFINE_PROP_STRING("serial", SCSIDiskState, serial),                \
+    DEFINE_PROP_STRING("vendor", SCSIDiskState, vendor),                \
+    DEFINE_PROP_STRING("product", SCSIDiskState, product),              \
     DEFINE_PROP_STRING("device_id", SCSIDiskState, device_id)

total: 1 errors, 0 warnings, 85 lines checked

Patch 5/15 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

6/15 Checking commit 1767a7917e97 (block: Adjust AioContexts when attaching nodes)
WARNING: Block comments use a leading /* on a separate line
#27: FILE: block.c:2246:
+/* The caller must hold the AioContext lock @child_bs, but not that of @ctx

WARNING: Block comments use a trailing */ on a separate line
#28: FILE: block.c:2247:
+ * (unless @child_bs is already in @ctx). */

WARNING: Block comments use a leading /* on a separate line
#45: FILE: block.c:2275:
+    /* If the AioContexts don't match, first try to move the subtree of

WARNING: Block comments use a trailing */ on a separate line
#47: FILE: block.c:2277:
+     * try moving the parent into the AioContext of child_bs instead. */

WARNING: Block comments use a leading /* on a separate line
#76: FILE: block.c:2306:
+/* If @parent_bs and @child_bs are in different AioContexts, the caller must

WARNING: Block comments use a trailing */ on a separate line
#77: FILE: block.c:2307:
+ * hold the AioContext lock for @child_bs, but not for @parent_bs. */

total: 0 errors, 6 warnings, 149 lines checked

Patch 6/15 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
7/15 Checking commit 5a4c60c72bfc (test-block-iothread: Test adding parent to iothread node)
8/15 Checking commit cd2b0d787e1b (test-block-iothread: BlockBackend AioContext across root node change)
9/15 Checking commit 26a1acffbe16 (block: Move node without parents to main AioContext)
WARNING: Block comments use a leading /* on a separate line
#23: FILE: block.c:2239:
+        /* When the parent requiring a non-default AioContext is removed, the

WARNING: Block comments use a trailing */ on a separate line
#24: FILE: block.c:2240:
+         * node moves back to the main AioContext */

total: 0 errors, 2 warnings, 28 lines checked

Patch 9/15 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
10/15 Checking commit 0419720e7eb2 (blockdev: Use bdrv_try_set_aio_context() for monitor commands)
11/15 Checking commit 7f6cd93a5e95 (block: Remove wrong bdrv_set_aio_context() calls)
12/15 Checking commit 98252b3fe05c (virtio-scsi-test: Test attaching new overlay with iothreads)
13/15 Checking commit b0545619fcbb (iotests: Attach new devices to node in non-default iothread)
14/15 Checking commit 984ba6d265e1 (test-bdrv-drain: Use bdrv_try_set_aio_context())
15/15 Checking commit 6ca27a5484fa (block: Remove bdrv_set_aio_context())
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20190523160104.21258-1-kwolf@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
Kevin Wolf June 3, 2019, 1:19 p.m. UTC | #2
Am 23.05.2019 um 18:00 hat Kevin Wolf geschrieben:
> Recently, a few bugs were reported that resulted from an inconsistent
> state regarding AioContexts. Block nodes can end up in different
> contexts than their users expect - the AioContext of a node can even
> change under the feet of a device with no way for the device to forbid
> this. We recently added a few basic checks to scsi-disk and virtio-blk,
> but they are by far not enough.
> 
> Part 1 solved the first half of the problem and made sure that
> AioContext changes are propagated through the graph as necessary, so
> that a bdrv_set_aio_context() correctly pulls everything that uses the
> node into the right context.
> 
> This is part 2 that fixes the second half: Attaching new child nodes
> where the parent and child are already in different AioContexts. This
> operation may only succeed if we can move one of the node into the
> context of the other node.
> 
> After applying this series, unchecked bdrv_set_aio_context() doesn't
> exist any more and all users call functions that make sure that the
> AioContext assignments across the graph stays consistent.

Applied to the block branch.

Kevin