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, due to the following (theoretical)
problem:
Consider write.
It's possible, that qemu_coroutine_enter only schedules execution,
assume such case.
Then we may possibly have the following:
1. Somehow check that we are not in drained section in outer code.
2. Call bdrv_pwritev(), assuming that it will increase in_flight, which
will protect us from starting drained section.
3. It calls bdrv_prwv_co() -> bdrv_coroutine_enter() (not yet increased
in_flight).
4. Assume coroutine not yet actually entered, only scheduled, and we go
to some code, which starts drained section (as in_flight is zero).
5. Scheduled coroutine starts, and blindly increases in_flight, and we
are in drained section with in_flight request.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com>
---
block/io.c | 161 +++++++++++++++++++++++++++++++++++++++++------------
1 file changed, 124 insertions(+), 37 deletions(-)
+int coroutine_fn bdrv_co_preadv_part(BdrvChild *child,
+ int64_t offset, unsigned int bytes,
+ QEMUIOVector *qiov, size_t qiov_offset,
+ BdrvRequestFlags flags)
+{
Doesn't seem to be the usual indentation in this file.
@@ -1922,7 +1934,8 @@ int coroutine_fn bdrv_co_pwritev(BdrvChild *child,
return bdrv_co_pwritev_part(child, offset, bytes, qiov, 0, flags);
}
-int coroutine_fn bdrv_co_pwritev_part(BdrvChild *child,
+/* To be called between exactly one pair of bdrv_inc/dec_in_flight() */
+static int coroutine_fn bdrv_do_pwritev_part(BdrvChild *child,
int64_t offset, unsigned int bytes, QEMUIOVector *qiov, size_t
qiov_offset,
BdrvRequestFlags flags)
{
then again, it was in use here, and saves reindenting the remaining
lines. I'll let the maintainer decide which style is preferred.
@@ -2014,17 +2038,18 @@ typedef struct RwCo {
BdrvRequestFlags flags;
} RwCo;
+/* To be called between exactly one pair of bdrv_inc/dec_in_flight() */
static void coroutine_fn bdrv_rw_co_entry(void *opaque)
{
RwCo *rwco = opaque;
if (!rwco->is_write) {
- rwco->ret = bdrv_co_preadv(rwco->child, rwco->offset,
- rwco->qiov->size, rwco->qiov,
+ rwco->ret = bdrv_do_preadv_part(rwco->child, rwco->offset,
+ rwco->qiov->size, rwco->qiov, 0,
rwco->flags);
Indentation is now off.
} else {
- rwco->ret = bdrv_co_pwritev(rwco->child, rwco->offset,
- rwco->qiov->size, rwco->qiov,
+ rwco->ret = bdrv_do_pwritev_part(rwco->child, rwco->offset,
+ rwco->qiov->size, rwco->qiov, 0,
rwco->flags);
and again
@@ -3411,9 +3478,12 @@ static void bdrv_parent_cb_resize(BlockDriverState *bs)
* If 'exact' is true, the file must be resized to exactly the given
* 'offset'. Otherwise, it is sufficient for the node to be at least
* 'offset' bytes in length.
+ *
+ * To be called between exactly one pair of bdrv_inc/dec_in_flight()
*/
-int coroutine_fn bdrv_co_truncate(BdrvChild *child, int64_t offset, bool exact,
- PreallocMode prealloc, Error **errp)
+static int coroutine_fn bdrv_do_truncate(BdrvChild *child,
+ int64_t offset, bool exact,
+ PreallocMode prealloc, Error **errp)
Needs to be rebased, now that master has Kevin's patches addeing a
'BdrvRequestFlags flags' parameter. But the rebase should be obvious.
Otherwise looks sane to me, but I may be missing one of the finer points
on which functions should be decorated with 'coroutine_fn'.
Reviewed-by: Eric Blake <ebl...@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
