Re: [Qemu-devel] [PATCH] iscsi: add support for iovectors

[Peter Lieven]

Am 12.12.2012 14:57, schrieb Paolo Bonzini:
> Il 03/12/2012 20:35, Peter Lieven ha scritto:
>> This patch adds support for directly passing the iovec
>> array from QEMUIOVector if libiscsi supports it.
> 
> Thanks, one question below.
> 
>> Signed-off-by: Peter Lieven <address@hidden>
>> ---
>>  block/iscsi.c |   31 +++++++++++++++++++++++++++++++
>>  1 file changed, 31 insertions(+)
>>
>> diff --git a/block/iscsi.c b/block/iscsi.c
>> index c0b70b3..6f3ee4a 100644
>> --- a/block/iscsi.c
>> +++ b/block/iscsi.c
>> @@ -55,7 +55,9 @@ typedef struct IscsiAIOCB {
>>      QEMUBH *bh;
>>      IscsiLun *iscsilun;
>>      struct scsi_task *task;
>> +#if !defined(LIBISCSI_FEATURE_IOVECTOR)
>>      uint8_t *buf;
>> +#endif
>>      int status;
>>      int canceled;
>>      size_t read_size;
>> @@ -192,7 +194,9 @@ iscsi_aio_write16_cb(struct iscsi_context *iscsi, int 
>> status,
>>
>>      trace_iscsi_aio_write16_cb(iscsi, status, acb, acb->canceled);
>>
>> +#if !defined(LIBISCSI_FEATURE_IOVECTOR)
>>      g_free(acb->buf);
>> +#endif
>>
>>      if (acb->canceled != 0) {
>>          return;
>> @@ -225,7 +229,9 @@ iscsi_aio_writev(BlockDriverState *bs, int64_t 
>> sector_num,
>>      size_t size;
>>      uint32_t num_sectors;
>>      uint64_t lba;
>> +#if !defined(LIBISCSI_FEATURE_IOVECTOR)
>>      struct iscsi_data data;
>> +#endif
>>
>>      acb = qemu_aio_get(&iscsi_aiocb_info, bs, cb, opaque);
>>      trace_iscsi_aio_writev(iscsi, sector_num, nb_sectors, opaque, acb);
>> @@ -240,8 +246,11 @@ iscsi_aio_writev(BlockDriverState *bs, int64_t 
>> sector_num,
>>      /* XXX we should pass the iovec to write16 to avoid the extra copy */
>>      /* this will allow us to get rid of 'buf' completely */
>>      size = nb_sectors * BDRV_SECTOR_SIZE;
>> +
>> +#if !defined(LIBISCSI_FEATURE_IOVECTOR)
>>      acb->buf = g_malloc(size);
>>      qemu_iovec_to_buf(acb->qiov, 0, acb->buf, size);
>> +#endif
>>
>>      acb->task = malloc(sizeof(struct scsi_task));
>>      if (acb->task == NULL) {
>> @@ -262,6 +271,17 @@ iscsi_aio_writev(BlockDriverState *bs, int64_t 
>> sector_num,
>>      *(uint32_t *)&acb->task->cdb[10] = htonl(num_sectors);
>>      acb->task->expxferlen = size;
>>
>> +#if defined(LIBISCSI_FEATURE_IOVECTOR)
>> +    if (iscsi_scsi_command_async(iscsi, iscsilun->lun, acb->task,
>> +                                 iscsi_aio_write16_cb,
>> +                                 NULL,
>> +                                 acb) != 0) {
>> +        scsi_free_scsi_task(acb->task);
>> +        qemu_aio_release(acb);
>> +        return NULL;
>> +    }
>> +    scsi_task_set_iov_out(acb->task, (struct scsi_iovec*) acb->qiov->iov, 
>> acb->qiov->niov);
> 
> Are you sure that scsi_task_set_iov_out must be called _after_
> submitting the command?

This does not matter since iscsi_scsi_command_async() will only queue
the command pdu. The iov has to be set when iscsi_service() is called
which will actually send out the PDUs + payload to the socket.

Peter

> 
>> +#else
>>      data.data = acb->buf;
>>      data.size = size;
>>
>> @@ -274,6 +294,7 @@ iscsi_aio_writev(BlockDriverState *bs, int64_t 
>> sector_num,
>>          qemu_aio_release(acb);
>>          return NULL;
>>      }
>> +#endif
>>
>>      iscsi_set_events(iscsilun);
>>
>> @@ -312,7 +333,9 @@ iscsi_aio_readv(BlockDriverState *bs, int64_t sector_num,
>>      struct iscsi_context *iscsi = iscsilun->iscsi;
>>      IscsiAIOCB *acb;
>>      size_t qemu_read_size;
>> +#if !defined(LIBISCSI_FEATURE_IOVECTOR)
>>      int i;
>> +#endif
>>      uint64_t lba;
>>      uint32_t num_sectors;
>>
>> @@ -328,7 +351,9 @@ iscsi_aio_readv(BlockDriverState *bs, int64_t sector_num,
>>      acb->bh          = NULL;
>>      acb->status      = -EINPROGRESS;
>>      acb->read_size   = qemu_read_size;
>> +#if !defined(LIBISCSI_FEATURE_IOVECTOR)
>>      acb->buf         = NULL;
>> +#endif
>>
>>      /* If LUN blocksize is bigger than BDRV_BLOCK_SIZE a read from QEMU
>>       * may be misaligned to the LUN, so we may need to read some extra
>> @@ -383,11 +408,15 @@ iscsi_aio_readv(BlockDriverState *bs, int64_t 
>> sector_num,
>>          return NULL;
>>      }
>>
>> +#if defined(LIBISCSI_FEATURE_IOVECTOR)
>> +    scsi_task_set_iov_in(acb->task, (struct scsi_iovec*) acb->qiov->iov, 
>> acb->qiov->niov);
>> +#else
>>      for (i = 0; i < acb->qiov->niov; i++) {
>>          scsi_task_add_data_in_buffer(acb->task,
>>                  acb->qiov->iov[i].iov_len,
>>                  acb->qiov->iov[i].iov_base);
>>      }
>> +#endif
>>
>>      iscsi_set_events(iscsilun);
>>
>> @@ -557,7 +586,9 @@ static BlockDriverAIOCB 
>> *iscsi_aio_ioctl(BlockDriverState *bs,
>>      acb->canceled    = 0;
>>      acb->bh          = NULL;
>>      acb->status      = -EINPROGRESS;
>> +#if !defined(LIBISCSI_FEATURE_IOVECTOR)
>>      acb->buf         = NULL;
>> +#endif
>>      acb->ioh         = buf;
>>
>>      acb->task = malloc(sizeof(struct scsi_task));
>>
>