Commit 4720cbee authored by Kevin Wolf's avatar Kevin Wolf
Browse files

block: Fix hangs in synchronous APIs with iothreads 



In the block layer, synchronous APIs are often implemented by creating a
coroutine that calls the asynchronous coroutine-based implementation and
then waiting for completion with BDRV_POLL_WHILE().

For this to work with iothreads (more specifically, when the synchronous
API is called in a thread that is not the home thread of the block
device, so that the coroutine will run in a different thread), we must
make sure to call aio_wait_kick() at the end of the operation. Many
places are missing this, so that BDRV_POLL_WHILE() keeps hanging even if
the condition has long become false.

Note that bdrv_dec_in_flight() involves an aio_wait_kick() call. This
corresponds to the BDRV_POLL_WHILE() in the drain functions, but it is
generally not enough for most other operations because they haven't set
the return value in the coroutine entry stub yet. To avoid race
conditions there, we need to kick after setting the return value.

The race window is small enough that the problem doesn't usually surface
in the common path. However, it does surface and causes easily
reproducible hangs if the operation can return early before even calling
bdrv_inc/dec_in_flight, which many of them do (trivial error or no-op
success paths).

The bug in bdrv_truncate(), bdrv_check() and bdrv_invalidate_cache() is
slightly different: These functions even neglected to schedule the
coroutine in the home thread of the node. This avoids the hang, but is
obviously wrong, too. Fix those to schedule the coroutine in the right
AioContext in addition to adding aio_wait_kick() calls.

Cc: qemu-stable@nongnu.org
Signed-off-by: default avatarKevin Wolf <kwolf@redhat.com>
Reviewed-by: default avatarStefan Hajnoczi <stefanha@redhat.com>
parent 4e20c1be

block.c

+4 −2
Original line number Diff line number Diff line
@@ -3725,6 +3725,7 @@ static void bdrv_check_co_entry(void *opaque) 
{

    CheckCo *cco = opaque;

    cco->ret = bdrv_co_check(cco->bs, cco->res, cco->fix);

    aio_wait_kick();

}



int bdrv_check(BlockDriverState *bs,
@@ -3743,7 +3744,7 @@ int bdrv_check(BlockDriverState *bs, 
        bdrv_check_co_entry(&cco);

    } else {

        co = qemu_coroutine_create(bdrv_check_co_entry, &cco);

        qemu_coroutine_enter(co);

        bdrv_coroutine_enter(bs, co);

        BDRV_POLL_WHILE(bs, cco.ret == -EINPROGRESS);

    }
@@ -4708,6 +4709,7 @@ static void coroutine_fn bdrv_invalidate_cache_co_entry(void *opaque) 
    InvalidateCacheCo *ico = opaque;

    bdrv_co_invalidate_cache(ico->bs, ico->errp);

    ico->done = true;

    aio_wait_kick();

}



void bdrv_invalidate_cache(BlockDriverState *bs, Error **errp)
@@ -4724,7 +4726,7 @@ void bdrv_invalidate_cache(BlockDriverState *bs, Error **errp) 
        bdrv_invalidate_cache_co_entry(&ico);

    } else {

        co = qemu_coroutine_create(bdrv_invalidate_cache_co_entry, &ico);

        qemu_coroutine_enter(co);

        bdrv_coroutine_enter(bs, co);

        BDRV_POLL_WHILE(bs, !ico.done);

    }

}

block/block-backend.c

+5 −0
Original line number Diff line number Diff line
@@ -1220,6 +1220,7 @@ static void blk_read_entry(void *opaque) 


    rwco->ret = blk_co_preadv(rwco->blk, rwco->offset, qiov->size,

                              qiov, rwco->flags);

    aio_wait_kick();

}



static void blk_write_entry(void *opaque)
@@ -1229,6 +1230,7 @@ static void blk_write_entry(void *opaque) 


    rwco->ret = blk_co_pwritev(rwco->blk, rwco->offset, qiov->size,

                               qiov, rwco->flags);

    aio_wait_kick();

}



static int blk_prw(BlockBackend *blk, int64_t offset, uint8_t *buf,
@@ -1540,6 +1542,7 @@ static void blk_ioctl_entry(void *opaque) 


    rwco->ret = blk_co_ioctl(rwco->blk, rwco->offset,

                             qiov->iov[0].iov_base);

    aio_wait_kick();

}



int blk_ioctl(BlockBackend *blk, unsigned long int req, void *buf)
@@ -1586,6 +1589,7 @@ static void blk_flush_entry(void *opaque) 
{

    BlkRwCo *rwco = opaque;

    rwco->ret = blk_co_flush(rwco->blk);

    aio_wait_kick();

}



int blk_flush(BlockBackend *blk)
@@ -2018,6 +2022,7 @@ static void blk_pdiscard_entry(void *opaque) 
    QEMUIOVector *qiov = rwco->iobuf;



    rwco->ret = blk_co_pdiscard(rwco->blk, rwco->offset, qiov->size);

    aio_wait_kick();

}



int blk_pdiscard(BlockBackend *blk, int64_t offset, int bytes)

block/io.c

+7 −1
Original line number Diff line number Diff line
@@ -806,6 +806,7 @@ static void coroutine_fn bdrv_rw_co_entry(void *opaque) 
                                    rwco->qiov->size, rwco->qiov,

                                    rwco->flags);

    }

    aio_wait_kick();

}



/*
@@ -2279,6 +2280,7 @@ static void coroutine_fn bdrv_block_status_above_co_entry(void *opaque) 
                                           data->offset, data->bytes,

                                           data->pnum, data->map, data->file);

    data->done = true;

    aio_wait_kick();

}



/*
@@ -2438,6 +2440,7 @@ static void coroutine_fn bdrv_co_rw_vmstate_entry(void *opaque) 
{

    BdrvVmstateCo *co = opaque;

    co->ret = bdrv_co_rw_vmstate(co->bs, co->qiov, co->pos, co->is_read);

    aio_wait_kick();

}



static inline int
@@ -2559,6 +2562,7 @@ static void coroutine_fn bdrv_flush_co_entry(void *opaque) 
    FlushCo *rwco = opaque;



    rwco->ret = bdrv_co_flush(rwco->bs);

    aio_wait_kick();

}



int coroutine_fn bdrv_co_flush(BlockDriverState *bs)
@@ -2704,6 +2708,7 @@ static void coroutine_fn bdrv_pdiscard_co_entry(void *opaque) 
    DiscardCo *rwco = opaque;



    rwco->ret = bdrv_co_pdiscard(rwco->child, rwco->offset, rwco->bytes);

    aio_wait_kick();

}



int coroutine_fn bdrv_co_pdiscard(BdrvChild *child, int64_t offset, int bytes)
@@ -3217,6 +3222,7 @@ static void coroutine_fn bdrv_truncate_co_entry(void *opaque) 
    TruncateCo *tco = opaque;

    tco->ret = bdrv_co_truncate(tco->child, tco->offset, tco->prealloc,

                                tco->errp);

    aio_wait_kick();

}



int bdrv_truncate(BdrvChild *child, int64_t offset, PreallocMode prealloc,
@@ -3236,7 +3242,7 @@ int bdrv_truncate(BdrvChild *child, int64_t offset, PreallocMode prealloc, 
        bdrv_truncate_co_entry(&tco);

    } else {

        co = qemu_coroutine_create(bdrv_truncate_co_entry, &tco);

        qemu_coroutine_enter(co);

        bdrv_coroutine_enter(child->bs, co);

        BDRV_POLL_WHILE(child->bs, tco.ret == NOT_DONE);

    }

block/nbd-client.c

+1 −0
Original line number Diff line number Diff line
@@ -119,6 +119,7 @@ static coroutine_fn void nbd_read_reply_entry(void *opaque) 
    s->quit = true;

    nbd_recv_coroutines_wake_all(s);

    s->read_reply_co = NULL;

    aio_wait_kick();

}



static int nbd_co_send_request(BlockDriverState *bs,

block/nvme.c

+1 −0
Original line number Diff line number Diff line
@@ -390,6 +390,7 @@ static void nvme_cmd_sync_cb(void *opaque, int ret) 
{

    int *pret = opaque;

    *pret = ret;

    aio_wait_kick();

}



static int nvme_cmd_sync(BlockDriverState *bs, NVMeQueuePair *q,