Re: [Qemu-devel] [PATCH] This patch adds a new block driver : iSCSI

[Stefan Hajnoczi <stefanha@gmail.com>]

On Wed, Sep 14, 2011 at 3:36 PM, Christoph Hellwig <hch@lst.de> wrote:
> On Mon, Sep 12, 2011 at 10:14:08AM +0100, Stefan Hajnoczi wrote:
>> > +static void
>> > +iscsi_aio_cancel(BlockDriverAIOCB *blockacb)
>> > +{
>> > +    IscsiAIOCB *acb = (IscsiAIOCB *)blockacb;
>> > +    IscsiLun *iscsilun = acb->iscsilun;
>> > +
>> > +    acb->status = -ECANCELED;
>> > +    acb->common.cb(acb->common.opaque, acb->status);
>> > +    acb->canceled = 1;
>> > +
>> > +    iscsi_task_mgmt_abort_task_async(iscsilun->iscsi, acb->task,
>> > +                                     iscsi_abort_task_cb, NULL);
>> > +}
>>
>> The asynchronous abort task call looks odd.  If a caller allocates a
>> buffer and issues a read request, then we need to make sure that the
>> request is really aborted by the time .bdrv_aio_cancel() returns.
>
> Shouldn't new drivers use coroutines instead of aio instead?
>
> (just poking around, I don't fully understand the new scheme myself yet
>  either)

I think in this case it will not make the code nicer.  Since the
external iSCSI library is based on callbacks it would be necessary to
write the coroutines<->callbacks adapter functions.  So for example,
the READ10 command would need a function that can be called in
coroutine context and yields while libiscsi does the I/O.  When the
callback is invoked it will re-enter the coroutine.

The area where coroutines are useful in the block layer is for image
formats.  We already have common coroutines<->callback adapter
functions in block.c so it's possible to write sequential code for
image formats.  They only need access to block layer functions which
have already been adapted.  But as soon as you interact with a
callback-based API from the coroutine, then you need to write an
adapter yourself.

Stefan