On 4/27/20 9:39 AM, Vladimir Sementsov-Ogievskiy wrote:
It's safer to expand in_flight request to start before enter to
coroutine in synchronous wrappers and end after BDRV_POLL_WHILE loop.
Note that qemu_coroutine_enter may only schedule the coroutine in some
circumstances.
Wording suggestion:
It's safer to expand the region protected by an in_flight request to
begin in the synchronous wrapper and end after the BDRV_POLL_WHILE loop.
Leaving the in_flight request in the coroutine itself risks a race
where calling qemu_coroutine_enter() may have only scheduled, rather
than started, the coroutine, allowing some other thread a chance to not
realize an operation is in flight.
block-status requests are complex, they involve querying different
block driver states across backing chain. Let's expand only in_flight
section for the top bs, keeping other sections as is.
block-status requests are complex, involving a query of different block
driver states across the backing chain. Let's expand only the in_flight
section for the top bs, and keep the other sections as-is.
I'd welcome Kevin's review on my next comment, but if I'm correct, I
think we can further add the following justification to the commit message:
Gathering block status only requires reads from the block device, and
backing devices are typically read-only, so losing any in_flight race on
a backing device is less likely to cause problems with concurrent
modifications on the overall backing chain.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com>
---
block/io.c | 65 ++++++++++++++++++++++++++++++++++++++++++------------
1 file changed, 51 insertions(+), 14 deletions(-)
diff --git a/block/io.c b/block/io.c
index a91d8c1e21..1cb6f433e5 100644
--- a/block/io.c
@@ -2624,15 +2646,19 @@ int coroutine_fn bdrv_is_allocated(BlockDriverState
*bs, int64_t offset,
* words, the result is not necessarily the maximum possible range);
* but 'pnum' will only be 0 when end of file is reached.
*
+ * To be called between exactly one pair of bdrv_inc/dec_in_flight() for top
bs.
+ * bdrv_do_is_allocated_above takes care of increasing in_fligth for other
block
in_flight
+ * driver states from bs backing chain.
*/
static int coroutine_fn
-bdrv_co_is_allocated_above(BlockDriverState *top, BlockDriverState *base,
+bdrv_do_is_allocated_above(BlockDriverState *top, BlockDriverState *base,
bool include_base, int64_t offset, int64_t bytes,
int64_t *pnum)
@@ -2682,11 +2710,16 @@ typedef struct BdrvCoIsAllocatedAboveData {
bool done;
} BdrvCoIsAllocatedAboveData;
+/*
+ * To be called between exactly one pair of bdrv_inc/dec_in_flight() for top
bs.
+ * bdrv_do_is_allocated_above takes care of increasing in_fligth for other
block
+ * driver states from the backing chain.
+ */
static void coroutine_fn bdrv_is_allocated_above_co_entry(void *opaque)
and again
Otherwise looks reasonable to me. Fixing typos is trivial, so:
Reviewed-by: Eric Blake <ebl...@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
