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

[Kevin Wolf <kwolf@redhat.com>]

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.

> >> +    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.

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.

Kevin