| Message ID | 1408622216-9578-7-git-send-email-famz@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Thu, Aug 21, 2014 at 07:56:53PM +0800, Fam Zheng wrote: > @@ -446,12 +439,25 @@ static void error_callback_bh(void *opaque) > qemu_aio_release(acb); > } > > +static void blkdebug_aio_cancel_async(BlockDriverAIOCB *blockacb) > +{ > + BlkdebugAIOCB *acb = container_of(blockacb, BlkdebugAIOCB, common); > + blockacb->cb(blockacb->opaque, -ECANCELED); > + qemu_aio_release(acb); > +} > + > static void blkdebug_aio_cancel(BlockDriverAIOCB *blockacb) > { > BlkdebugAIOCB *acb = container_of(blockacb, BlkdebugAIOCB, common); > qemu_aio_release(acb); > } Both blkdebug_aio_cancel() and blkdebug_aio_cancel_async() look incorrect. It is not safe to release acb because the error_callback_bh() BH may still be scheduled. I guess we don't hit this problem because the error injection happens within the same event loop iteration. In practice no one ever calls blkdebug_aio_cancel()? Stefan
On Thu, 08/21 17:52, Stefan Hajnoczi wrote: > On Thu, Aug 21, 2014 at 07:56:53PM +0800, Fam Zheng wrote: > > @@ -446,12 +439,25 @@ static void error_callback_bh(void *opaque) > > qemu_aio_release(acb); > > } > > > > +static void blkdebug_aio_cancel_async(BlockDriverAIOCB *blockacb) > > +{ > > + BlkdebugAIOCB *acb = container_of(blockacb, BlkdebugAIOCB, common); > > + blockacb->cb(blockacb->opaque, -ECANCELED); > > + qemu_aio_release(acb); > > +} > > + > > static void blkdebug_aio_cancel(BlockDriverAIOCB *blockacb) > > { > > BlkdebugAIOCB *acb = container_of(blockacb, BlkdebugAIOCB, common); > > qemu_aio_release(acb); > > } > > Both blkdebug_aio_cancel() and blkdebug_aio_cancel_async() look > incorrect. It is not safe to release acb because the > error_callback_bh() BH may still be scheduled. > > I guess we don't hit this problem because the error injection happens > within the same event loop iteration. In practice no one ever calls > blkdebug_aio_cancel()? > I'll drop this patch and send a separate fix for blkdebug_aio_cancel. Fam
diff --git a/block/blkdebug.c b/block/blkdebug.c index 1586ed9..d269956 100644 --- a/block/blkdebug.c +++ b/block/blkdebug.c @@ -48,13 +48,6 @@ typedef struct BlkdebugSuspendedReq { QLIST_ENTRY(BlkdebugSuspendedReq) next; } BlkdebugSuspendedReq; -static void blkdebug_aio_cancel(BlockDriverAIOCB *blockacb); - -static const AIOCBInfo blkdebug_aiocb_info = { - .aiocb_size = sizeof(BlkdebugAIOCB), - .cancel = blkdebug_aio_cancel, -}; - enum { ACTION_INJECT_ERROR, ACTION_SET_STATE, @@ -446,12 +439,25 @@ static void error_callback_bh(void *opaque) qemu_aio_release(acb); } +static void blkdebug_aio_cancel_async(BlockDriverAIOCB *blockacb) +{ + BlkdebugAIOCB *acb = container_of(blockacb, BlkdebugAIOCB, common); + blockacb->cb(blockacb->opaque, -ECANCELED); + qemu_aio_release(acb); +} + static void blkdebug_aio_cancel(BlockDriverAIOCB *blockacb) { BlkdebugAIOCB *acb = container_of(blockacb, BlkdebugAIOCB, common); qemu_aio_release(acb); } +static const AIOCBInfo blkdebug_aiocb_info = { + .aiocb_size = sizeof(BlkdebugAIOCB), + .cancel = blkdebug_aio_cancel, + .cancel_async = blkdebug_aio_cancel_async, +}; + static BlockDriverAIOCB *inject_error(BlockDriverState *bs, BlockDriverCompletionFunc *cb, void *opaque, BlkdebugRule *rule) {
Very similar to .cancel, except that cb is called before releasing the aio. Signed-off-by: Fam Zheng <famz@redhat.com> --- block/blkdebug.c | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-)