Re: [Qemu-devel] [PATCHv2 03/11] iscsi: add bdrv_co_is_allocated

[Kevin Wolf]

Am 10.07.2013 um 16:49 hat Peter Lieven geschrieben:
> Am 10.07.2013 16:43, schrieb Kevin Wolf:
> > Am 10.07.2013 um 15:49 hat Peter Lieven geschrieben:
> >> Am 10.07.2013 11:41, schrieb Kevin Wolf:
> >>> Am 27.06.2013 um 15:11 hat Peter Lieven geschrieben:
> >>>> this patch adds a coroutine for .bdrv_co_is_allocated as well as
> >>>> a generic framework that can be used to build coroutines in block/iscsi.
> >>>>
> >>>> Signed-off-by: Peter Lieven <address@hidden>
> >>>> ---
> >>>>  block/iscsi.c |  123 
> >>>> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >>>>  1 file changed, 123 insertions(+)
> >>>>
> >>>> diff --git a/block/iscsi.c b/block/iscsi.c
> >>>> index 2e2455d..d31ee95 100644
> >>>> --- a/block/iscsi.c
> >>>> +++ b/block/iscsi.c
> >>>> @@ -58,6 +58,15 @@ typedef struct IscsiLun {
> >>>>      uint32_t max_unmap_bdc;
> >>>>  } IscsiLun;
> >>>>  
> >>>> +typedef struct IscsiTask {
> >>>> +    int status;
> >>>> +    int complete;
> >>>> +    int retries;
> >>>> +    int do_retry;
> >>>> +    struct scsi_task *task;
> >>>> +    Coroutine *co;
> >>>> +} IscsiTask;
> >>>> +
> >>>>  typedef struct IscsiAIOCB {
> >>>>      BlockDriverAIOCB common;
> >>>>      QEMUIOVector *qiov;
> >>>> @@ -113,6 +122,45 @@ iscsi_schedule_bh(IscsiAIOCB *acb)
> >>>>      qemu_bh_schedule(acb->bh);
> >>>>  }
> >>>>  
> >>>> +static void
> >>>> +iscsi_co_generic_cb(struct iscsi_context *iscsi, int status,
> >>>> +                        void *command_data, void *opaque)
> >>>> +{
> >>>> +    struct IscsiTask *iTask = opaque;
> >>>> +    struct scsi_task *task = command_data;
> >>>> +
> >>>> +    iTask->complete = 1;
> >>>> +    iTask->status = status;
> >>>> +    iTask->do_retry = 0;
> >>>> +    iTask->task = task;
> >>>> +
> >>>> +    if (iTask->retries-- > 0 && status == SCSI_STATUS_CHECK_CONDITION
> >>>> +        && task->sense.key == SCSI_SENSE_UNIT_ATTENTION) {
> >>>> +        iTask->do_retry = 1;
> >>>> +        goto out;
> >>>> +    }
> >>>> +
> >>>> +    if (status != SCSI_STATUS_GOOD) {
> >>>> +        error_report("iSCSI: Failure. %s", iscsi_get_error(iscsi));
> >>>> +        goto out;
> >>>> +    }
> >>>> +
> >>>> +out:
> >>>> +    if (iTask->status != SCSI_STATUS_GOOD) {
> >>>> +        scsi_free_scsi_task(iTask->task);
> >>>> +        iTask->task = NULL;
> >>>> +    }
> >>>> +    if (iTask->co) {
> >>>> +        qemu_coroutine_enter(iTask->co, NULL);
> >>>> +    }
> >>>> +}
> >>> In which context does this callback run? If this is from a separate
> >>> thread, you may not do all of this (without even holding the QBL). You
> >>> should probably use a BH to switch to the I/O thread.
> >> good question. the callbacks can only be fired when iscsi_service() is 
> >> invoked.
> >> this is currently done by registereing iscsi_process_read/write/flush via 
> >> qemu_aio_set_fd_handler().
> >>
> >> so the callbacks are invoked in the aio context it seems. can this be 
> >> right?
> > Yes, I took another look and I think it's fine.
> Would this still be true if all aio routines are replaced by coroutines?

Yes, AIO callbacks and coroutines are mostly equivalent from the point
of view of anything external to the block driver itself.

Kevin