Re: [Qemu-devel] [PATCH 1/2] iscsi: add support for bdrv_co_is_allocated()

[Peter Lieven <pl@kamp.de>]

Am 21.06.2013 13:07, schrieb Kevin Wolf:
> Am 21.06.2013 um 11:45 hat Peter Lieven geschrieben:
>> Am 21.06.2013 11:18, schrieb Kevin Wolf:
>>> Am 20.06.2013 um 20:20 hat Peter Lieven geschrieben:
>>>> Signed-off-by: Peter Lieven <pl@kamp.de>
>>>> ---
>>>>  block/iscsi.c |   57 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>  1 file changed, 57 insertions(+)
>>>>
>>>> diff --git a/block/iscsi.c b/block/iscsi.c
>>>> index 0bbf0b1..e6b966d 100644
>>>> --- a/block/iscsi.c
>>>> +++ b/block/iscsi.c
>>>> @@ -49,6 +49,7 @@ typedef struct IscsiLun {
>>>>      uint64_t num_blocks;
>>>>      int events;
>>>>      QEMUTimer *nop_timer;
>>>> +    uint8_t lbpme;
>>>>  } IscsiLun;
>>>>  
>>>>  typedef struct IscsiAIOCB {
>>>> @@ -800,6 +801,60 @@ iscsi_getlength(BlockDriverState *bs)
>>>>      return len;
>>>>  }
>>>>  
>>>> +static int coroutine_fn iscsi_co_is_allocated(BlockDriverState *bs,
>>>> +                                              int64_t sector_num,
>>>> +                                              int nb_sectors, int *pnum)
>>>> +{
>>>> +    IscsiLun *iscsilun = bs->opaque;
>>>> +    struct scsi_task *task = NULL;
>>>> +    struct scsi_get_lba_status *lbas = NULL;
>>>> +    struct scsi_lba_status_descriptor *lbasd = NULL;
>>>> +    int ret;
>>>> +
>>>> +    *pnum = nb_sectors;
>>>> +
>>>> +    if (iscsilun->lbpme == 0) {
>>>> +        return 1;
>>>> +    }
>>>> +
>>>> +    /* in-flight requests could invalidate the lba status result */
>>>> +    while (iscsi_process_flush(iscsilun)) {
>>>> +        qemu_aio_wait();
>>>> +    }
>>> Note that you're blocking here. The preferred way would be something
>>> involving a yield from the coroutine and a reenter as soon as all
>>> requests are done. Maybe a CoRwLock does what you need?
>> Is there a document how to use it? Or can you help here?
> The idea would be to take a read lock while any request is in flight
> (i.e. qemu_co_rwlock_rdlock() before it's started and
> qemu_co_rwlock_unlock() when it completes), and to take a write lock
> (qemu_co_rwlock_wrlock) for the part of iscsi_co_is_allocated() that
> requires that no other request runs in parallel.
Wouldn't this require that all the other operations in iscsi.c would
also take these lock? Currently there are only aio requests
implemented as it seems.

Would it help here to add sth like this?

while (iscsi_process_flush(iscsilun)) {
    if (qemu_in_coroutine()) {
        qemu_coroutine_yield();
    } else {
        qemu_aio_wait();
    }       
}
>>>> +    if (task == NULL || task->status != SCSI_STATUS_GOOD) {
>>>> +        scsi_free_scsi_task(task);
>>>> +        return 1;
>>> Error cases should set *pnum = 0 and return 0.
>> In this special case, the target might not implement get_lba_status
>> or it might return SCSI_STATUS_BUSY. We shouldn't generate an
>> error or should we?
> If you have reasons to not return an error, do so. Maybe adding a
> comment wouldn't hurt.
Will do ;-)
>
> I just saw more than one of these return 1; lines which looked like
> error handling to me. If you don't want any of them to result in an
> error, that's okay with me, I just wanted to mention how you would
> correctly signal an error.
Ah okay. Thank you.

Peter
>
> Kevin