[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 00/23] block: Lock the graph, part 2 (BlockDriver callbacks)
From: |
Stefan Hajnoczi |
Subject: |
Re: [PATCH 00/23] block: Lock the graph, part 2 (BlockDriver callbacks) |
Date: |
Tue, 21 Feb 2023 17:13:41 -0500 |
On Fri, Feb 03, 2023 at 04:21:39PM +0100, Kevin Wolf wrote:
> After introducing the graph lock in a previous series, this series
> actually starts making widespread use of it.
>
> Most of the BlockDriver callbacks access the children list in some way,
> so you need to hold the graph lock to call them. The patches in this
> series add the corresponding GRAPH_RDLOCK annotations and take the lock
> in places where it doesn't happen yet - all of the bdrv_*() co_wrappers
> are already covered, but in particular BlockBackend coroutine_fns still
> need it.
>
> There is no particularly good reason why exactly these patches and not
> others are included in the series. I couldn't find a self-contained part
> that could reasonable be addressed in a single series. So these just
> happen to be patches that are somewhat related (centered around the
> BlockDriver callback theme), are ready and their number looks
> manageable. You will still see some FIXMEs at the end of the series
> that will only be addressed in future patches.
Two things occurred to me:
1. The graph lock is becoming the new AioContext lock in the sense that
code using the block layer APIs needs to carefully acquire and release
the lock around operations. Why is it necessary to explicitly take the
rdlock in mirror_iteration()?
+ WITH_GRAPH_RDLOCK_GUARD() {
ret = bdrv_block_status_above(source, NULL, offset,
I guess because bdrv_*() APIs are unlocked? The equivalent blk_*() API
would have taken the graph lock internally. Do we want to continue using
bdrv APIs even though it spreads graph locking concerns into block jobs?
2. This series touches block drivers like qcow2. Luckily block drivers
just need to annotate their BlockDriver functions to indicate they run
under the rdlock, a lock that the block driver itself doesn't mess with.
It makes me wonder whether there is any point in annotating the
BlockDriver function pointers? It would be simpler if the block drivers
were unaware of the graph lock.
Stefan
signature.asc
Description: PGP signature
- [PATCH 14/23] block: Mark bdrv_co_copy_range() GRAPH_RDLOCK, (continued)
- [PATCH 14/23] block: Mark bdrv_co_copy_range() GRAPH_RDLOCK, Kevin Wolf, 2023/02/03
- [PATCH 17/23] block: Mark bdrv_co_io_(un)plug() and callers GRAPH_RDLOCK, Kevin Wolf, 2023/02/03
- [PATCH 18/23] block: Mark bdrv_co_is_inserted() and callers GRAPH_RDLOCK, Kevin Wolf, 2023/02/03
- [PATCH 16/23] block: Mark bdrv_co_create() and callers GRAPH_RDLOCK, Kevin Wolf, 2023/02/03
- [PATCH 19/23] block: Mark bdrv_co_eject/lock_medium() and callers GRAPH_RDLOCK, Kevin Wolf, 2023/02/03
- [PATCH 20/23] block: Mark bdrv_(un)register_buf() GRAPH_RDLOCK, Kevin Wolf, 2023/02/03
- [PATCH 21/23] block: Mark bdrv_co_delete_file() and callers GRAPH_RDLOCK, Kevin Wolf, 2023/02/03
- [PATCH 22/23] block: Mark bdrv_*_dirty_bitmap() and callers GRAPH_RDLOCK, Kevin Wolf, 2023/02/03
- [PATCH 23/23] block: Mark bdrv_co_refresh_total_sectors() and callers GRAPH_RDLOCK, Kevin Wolf, 2023/02/03
- Re: [PATCH 00/23] block: Lock the graph, part 2 (BlockDriver callbacks), Emanuele Giuseppe Esposito, 2023/02/17
- Re: [PATCH 00/23] block: Lock the graph, part 2 (BlockDriver callbacks),
Stefan Hajnoczi <=