[PATCH v2 00/11] block: Re-enable the graph lock

Kevin Wolf posted 11 patches 11 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20230605085711.21261-1-kwolf@redhat.com
Maintainers: Kevin Wolf <kwolf@redhat.com>, Hanna Reitz <hreitz@redhat.com>, John Snow <jsnow@redhat.com>, Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>, Paolo Bonzini <pbonzini@redhat.com>, "Daniel P. Berrangé" <berrange@redhat.com>, Eduardo Habkost <eduardo@habkost.net>
include/block/graph-lock.h                    |   6 +-
block.c                                       | 103 ++++++++++++++++--
block/graph-lock.c                            |  42 ++++---
blockjob.c                                    |  17 ++-
hw/core/qdev-properties-system.c              |   8 +-
tests/unit/test-block-iothread.c              |   7 +-
.../tests/iothreads-commit-active             |  85 +++++++++++++++
.../tests/iothreads-commit-active.out         |  23 ++++
8 files changed, 250 insertions(+), 41 deletions(-)
create mode 100755 tests/qemu-iotests/tests/iothreads-commit-active
create mode 100644 tests/qemu-iotests/tests/iothreads-commit-active.out
[PATCH v2 00/11] block: Re-enable the graph lock
Posted by Kevin Wolf 11 months ago
This series fixes the deadlock that was observed before commit ad128dff
('graph-lock: Disable locking for now'), which just disabled the graph
lock completely as a workaround to get 8.0.1 stable.

In theory the problem is simple: We can't poll while still holding the
lock of a different AioContext. So bdrv_graph_wrlock() just needs to
drop that lock before it polls. However, there are a number of callers
that don't even hold the AioContext lock they are supposed to hold, so
temporarily unlocking tries to unlock a mutex that isn't locked,
resulting in assertion failures.

Therefore, much of this series is just for fixing AioContext locking
correctness. It is only the last two patches that actually fix the
deadlock and reenable the graph locking.

v2:
- Fixed patch 2 to actually lock the correct AioContext even if the
  device doesn't support iothreads
- Improved the commit message for patch 7 [Eric]
- Fixed mismerge in patch 11 (v1 incorrectly left an #if 0 around)

Kevin Wolf (11):
  iotests: Test active commit with iothread and background I/O
  qdev-properties-system: Lock AioContext for blk_insert_bs()
  test-block-iothread: Lock AioContext for blk_insert_bs()
  block: Fix AioContext locking in bdrv_open_child()
  block: Fix AioContext locking in bdrv_attach_child_common()
  block: Fix AioContext locking in bdrv_reopen_parse_file_or_backing()
  block: Fix AioContext locking in bdrv_open_inherit()
  block: Fix AioContext locking in bdrv_open_backing_file()
  blockjob: Fix AioContext locking in block_job_add_bdrv()
  graph-lock: Unlock the AioContext while polling
  Revert "graph-lock: Disable locking for now"

 include/block/graph-lock.h                    |   6 +-
 block.c                                       | 103 ++++++++++++++++--
 block/graph-lock.c                            |  42 ++++---
 blockjob.c                                    |  17 ++-
 hw/core/qdev-properties-system.c              |   8 +-
 tests/unit/test-block-iothread.c              |   7 +-
 .../tests/iothreads-commit-active             |  85 +++++++++++++++
 .../tests/iothreads-commit-active.out         |  23 ++++
 8 files changed, 250 insertions(+), 41 deletions(-)
 create mode 100755 tests/qemu-iotests/tests/iothreads-commit-active
 create mode 100644 tests/qemu-iotests/tests/iothreads-commit-active.out

-- 
2.40.1
Re: [PATCH v2 00/11] block: Re-enable the graph lock
Posted by Stefan Hajnoczi 11 months ago
On Mon, Jun 05, 2023 at 10:57:00AM +0200, Kevin Wolf wrote:
> This series fixes the deadlock that was observed before commit ad128dff
> ('graph-lock: Disable locking for now'), which just disabled the graph
> lock completely as a workaround to get 8.0.1 stable.
> 
> In theory the problem is simple: We can't poll while still holding the
> lock of a different AioContext. So bdrv_graph_wrlock() just needs to
> drop that lock before it polls. However, there are a number of callers
> that don't even hold the AioContext lock they are supposed to hold, so
> temporarily unlocking tries to unlock a mutex that isn't locked,
> resulting in assertion failures.
> 
> Therefore, much of this series is just for fixing AioContext locking
> correctness. It is only the last two patches that actually fix the
> deadlock and reenable the graph locking.
> 
> v2:
> - Fixed patch 2 to actually lock the correct AioContext even if the
>   device doesn't support iothreads
> - Improved the commit message for patch 7 [Eric]
> - Fixed mismerge in patch 11 (v1 incorrectly left an #if 0 around)
> 
> Kevin Wolf (11):
>   iotests: Test active commit with iothread and background I/O
>   qdev-properties-system: Lock AioContext for blk_insert_bs()
>   test-block-iothread: Lock AioContext for blk_insert_bs()
>   block: Fix AioContext locking in bdrv_open_child()
>   block: Fix AioContext locking in bdrv_attach_child_common()
>   block: Fix AioContext locking in bdrv_reopen_parse_file_or_backing()
>   block: Fix AioContext locking in bdrv_open_inherit()
>   block: Fix AioContext locking in bdrv_open_backing_file()
>   blockjob: Fix AioContext locking in block_job_add_bdrv()
>   graph-lock: Unlock the AioContext while polling
>   Revert "graph-lock: Disable locking for now"
> 
>  include/block/graph-lock.h                    |   6 +-
>  block.c                                       | 103 ++++++++++++++++--
>  block/graph-lock.c                            |  42 ++++---
>  blockjob.c                                    |  17 ++-
>  hw/core/qdev-properties-system.c              |   8 +-
>  tests/unit/test-block-iothread.c              |   7 +-
>  .../tests/iothreads-commit-active             |  85 +++++++++++++++
>  .../tests/iothreads-commit-active.out         |  23 ++++
>  8 files changed, 250 insertions(+), 41 deletions(-)
>  create mode 100755 tests/qemu-iotests/tests/iothreads-commit-active
>  create mode 100644 tests/qemu-iotests/tests/iothreads-commit-active.out
> 
> -- 
> 2.40.1
> 

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