Re: [Qemu-devel] [PATCH v3 0/8] block: Asynchronous request cancellation

[Fam Zheng]

On Tue, 09/02 16:09, Stefan Hajnoczi wrote:
> On Wed, Aug 27, 2014 at 10:49:08AM +0800, Fam Zheng wrote:
> > v3: Drop "RFC".
> >     Improvements according to Paolo's comments:
> >     05: Just use THREAD_DONE and ret = -ECANCELED in thread-pool.c
> >     06: Don't check dbs->cancelled for twice.
> >         Don't set dbs->acb to NULL.
> > 
> > v2: Drop the unfinished scsi part, which was broken in v1. (Paolo)
> >     Add refcnt in BlockDriverAIOCB to maintain invariant of acb 
> > availability in
> >     bdrv_aio_cancel_async. (Paolo)
> >     Drop blkdebug change. (Stefan)
> > 
> > This series adds a new block layer API:
> > 
> >   void bdrv_aio_cancel_async(BlockDriverAIOCB *acb);
> > 
> > which is similar to existing bdrv_aio_cancel in that it cancels an AIO 
> > request,
> > but different that it doesn't block until the request is completely 
> > cancelled
> > or done.
> > 
> > More importantly, the completion callback, BlockDriverAIOCB.cb, is 
> > guaranteed
> > to be called, so that the cb can take care of resource releasing and status
> > reporting to guest, etc.
> > 
> > In the following work, scsi emulation code will be shifted to use the async
> > cancelling.
> > 
> > One major benefit would be that when guest tries to cancel a request, where 
> > the
> > request cannot be cancelled easily, (due to throttled BlockDriverState, a 
> > lost
> > connection, or a large request queue), we don't need to block the whole vm 
> > with
> > a busy loop, which is how bdrv_aio_cancel is implemented now.
> > 
> > A test case that is easy to reproduce is, throttle a scsi-disk to a very low
> > limit, for example 50 bps, then stress the guest block device with dd or 
> > fio.
> > 
> > Currently, the vm will quickly hang when it loses patience and send a tmf
> > command to cancel the request, at which point we will busy wait in
> > bdrv_aio_cancel, until the request is slowly spit out from throttled_reqs.
> > 
> > Later, we will change scsi device code to make this asynchronous, on top of
> > bdrv_aio_cancel_async.
> 
> We need to get rid of .bdrv_aio_cancel().  Keeping both
> .bdrv_aio_cancel() and .bdrv_aio_cancel_async() around is problematic
> because they have slightly different semantics.
> 
> This patch series makes block driver cancellation more complex because
> we support both approaches :(.
> 
> Could we do something like:
> 
> void bdrv_aio_cancel(BdrvAIOCB *acb)
> {
>     bdrv_aiocb_ref(acb);
>     bdrv_aio_cancel_async(acb);
>     while (acb->ref > 1) {
>         aio_poll(bdrv_get_aio_context(acb->bs), true);
>     }
>     bdrv_aiocb_release(acb);
> }
> 
> (pseudo-code)
> 
> Then .bdrv_aio_cancel() should be deleted.
> 

OK, I will drop .io_cancel field from AIOCBInfo, and convert all AIO
implementations to only support .io_cancel_async. That way we can emulate
bdrv_aio_cancel with bdrv_aio_cancel_async.

Thanks for reviewing!

Fam