Re: [Qemu-devel] [PATCH] iSCSI: Add support for SG_IO in bdrv_ioctl()

[Paolo Bonzini <pbonzini@redhat.com>]

Il 21/08/2012 01:59, Ronnie Sahlberg ha scritto:
> We need to support SG_IO in the synchronous bdrv_ioctl() since this
> is used by scsi-block
> 
> Signed-off-by: Ronnie Sahlberg <ronniesahlberg@gmail.com>
> ---
>  block/iscsi.c |  109 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 files changed, 108 insertions(+), 1 deletions(-)
> 
> diff --git a/block/iscsi.c b/block/iscsi.c
> index 993a86d..9e98bfe 100644
> --- a/block/iscsi.c
> +++ b/block/iscsi.c
> @@ -548,7 +548,8 @@ iscsi_aio_ioctl_cb(struct iscsi_context *iscsi, int status,
>  
>  #define SG_ERR_DRIVER_SENSE    0x08
>  
> -    if (status == SCSI_STATUS_CHECK_CONDITION && acb->task->datain.size >= 2) {
> +    if (status == SCSI_STATUS_CHECK_CONDITION
> +    && acb->task->datain.size >= 2) {
>          int ss;
>  
>          acb->ioh->driver_status |= SG_ERR_DRIVER_SENSE;
> @@ -633,9 +634,53 @@ static BlockDriverAIOCB *iscsi_aio_ioctl(BlockDriverState *bs,
>      return &acb->common;
>  }
>  
> +struct IoctlTask {
> +    int status;
> +    int complete;
> +    sg_io_hdr_t *ioh;
> +    struct scsi_task *task;
> +};
> +
> +static void
> +iscsi_ioctl_cb(struct iscsi_context *iscsi, int status,
> +                     void *command_data, void *opaque)
> +{
> +    struct IoctlTask *itask = opaque;
> +
> +    if (status < 0) {
> +        error_report("Failed to ioctl(SG_IO) to iSCSI lun. %s",
> +                     iscsi_get_error(iscsi));
> +        itask->status = -EIO;
> +    }
> +
> +    itask->ioh->driver_status = 0;
> +    itask->ioh->host_status   = 0;
> +    itask->ioh->resid         = 0;
> +
> +#define SG_ERR_DRIVER_SENSE    0x08
> +
> +    if (status == SCSI_STATUS_CHECK_CONDITION
> +    && itask->task->datain.size >= 2) {
> +        int ss;
> +
> +        itask->ioh->driver_status |= SG_ERR_DRIVER_SENSE;
> +
> +        itask->ioh->sb_len_wr = itask->task->datain.size - 2;
> +        ss = (itask->ioh->mx_sb_len >= itask->ioh->sb_len_wr) ?
> +             itask->ioh->mx_sb_len : itask->ioh->sb_len_wr;
> +        memcpy(itask->ioh->sbp, &itask->task->datain.data[2], ss);
> +    }
> +
> +    itask->complete = 1;
> +}
> +
>  static int iscsi_ioctl(BlockDriverState *bs, unsigned long int req, void *buf)
>  {
>      IscsiLun *iscsilun = bs->opaque;
> +    struct iscsi_context *iscsi = iscsilun->iscsi;
> +    struct IoctlTask itask;
> +    struct scsi_task *task;
> +    struct iscsi_data data;
>  
>      switch (req) {
>      case SG_GET_VERSION_NUM:
> @@ -644,6 +689,68 @@ static int iscsi_ioctl(BlockDriverState *bs, unsigned long int req, void *buf)
>      case SG_GET_SCSI_ID:
>          ((struct sg_scsi_id *)buf)->scsi_type = iscsilun->type;
>          break;
> +    case SG_IO:
> +        itask.ioh = buf;
> +        task = malloc(sizeof(struct scsi_task));
> +        if (task == NULL) {
> +            error_report("iSCSI: Failed to allocate task for scsi command. %s",
> +                         iscsi_get_error(iscsi));
> +            return -1;
> +        }
> +        memset(task, 0, sizeof(struct scsi_task));
> +
> +        switch (itask.ioh->dxfer_direction) {
> +        case SG_DXFER_TO_DEV:
> +            task->xfer_dir = SCSI_XFER_WRITE;
> +            break;
> +        case SG_DXFER_FROM_DEV:
> +            task->xfer_dir = SCSI_XFER_READ;
> +            break;
> +        default:
> +            task->xfer_dir = SCSI_XFER_NONE;
> +            break;
> +        }
> +        task->cdb_size = itask.ioh->cmd_len;
> +        memcpy(&task->cdb[0], itask.ioh->cmdp, itask.ioh->cmd_len);
> +        task->expxferlen = itask.ioh->dxfer_len;
> +
> +        if (task->xfer_dir == SCSI_XFER_WRITE) {
> +            data.data = itask.ioh->dxferp;
> +            data.size = itask.ioh->dxfer_len;
> +        }
> +
> +        if (iscsi_scsi_command_async(iscsi, iscsilun->lun, task,
> +                                     iscsi_ioctl_cb,
> +                                     (task->xfer_dir == SCSI_XFER_WRITE) ?
> +                                         &data : NULL,
> +                                     &itask) != 0) {
> +            scsi_free_scsi_task(task);
> +            return -1;
> +        }
> +
> +        /* tell libiscsi to read straight into the buffer we got from ioctl */
> +        if (task->xfer_dir == SCSI_XFER_READ) {
> +            scsi_task_add_data_in_buffer(task,
> +                                         itask.ioh->dxfer_len,
> +                                         itask.ioh->dxferp);
> +        }
> +
> +        itask.complete = 0;
> +        itask.status   = 0;
> +        itask.task     = task;
> +        while (!itask.complete) {
> +            iscsi_set_events(iscsilun);
> +            qemu_aio_wait();
> +        }
> +        scsi_free_scsi_task(task);
> +
> +        if (itask.status != 0) {
> +            error_report("iSCSI: Failed to send async command to target : %s",
> +                         iscsi_get_error(iscsi));
> +            return -1;
> +        }
> +
> +        return 0;
>      default:
>          return -1;
>      }
> 

Lots of duplicate code, can you just call bdrv_aio_ioctl with a callback
that is as simple as

    int *p_status = opaque;
    *p_status = status;

and then

    status = -EINPROGRESS;
    bdrv_aio_ioctl(..., ioctl_cb, &status);
    while (status == -EINPROGRESS) {
        qemu_aio_wait();
    }

?

The iscsi_set_events should not be needed because bdrv_aio_ioctl calls it.

Paolo