Re: [Qemu-devel] [PATCH] block/iscsi: use 16 byte CDBs only when necessary

[Paolo Bonzini <pbonzini@redhat.com>]

Il 04/06/2014 15:47, Peter Lieven ha scritto:
> this patch changes the driver to uses 16 Byte CDBs for
> READ/WRITE only if the target requires 64bit lba addressing.
>
> On one hand this saves 6 bytes in each PDU on the other
> hand it seems that 10 Byte CDBs seems to be much better
> supported and tested as a recent issue I had with a
> major storage supplier lined out.
>
> For WRITESAME the logic is a bit more tricky as WRITESAME10
> with UNMAP was added really late. Thus a fallback to WRITESAME16
> is possible if it supports UNMAP and WRITESAME10 not.
>
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>  block/iscsi.c |   58 +++++++++++++++++++++++++++++++++++++++------------------
>  1 file changed, 40 insertions(+), 18 deletions(-)
>
> diff --git a/block/iscsi.c b/block/iscsi.c
> index d241e83..019b324 100644
> --- a/block/iscsi.c
> +++ b/block/iscsi.c
> @@ -65,6 +65,7 @@ typedef struct IscsiLun {
>      unsigned char *zeroblock;
>      unsigned long *allocationmap;
>      int cluster_sectors;
> +    bool use_16_for_rw;
>  } IscsiLun;
>
>  typedef struct IscsiTask {
> @@ -368,10 +369,17 @@ static int coroutine_fn iscsi_co_writev(BlockDriverState *bs,
>      num_sectors = sector_qemu2lun(nb_sectors, iscsilun);
>      iscsi_co_init_iscsitask(iscsilun, &iTask);
>  retry:
> -    iTask.task = iscsi_write16_task(iscsilun->iscsi, iscsilun->lun, lba,
> -                                    data, num_sectors * iscsilun->block_size,
> -                                    iscsilun->block_size, 0, 0, 0, 0, 0,
> -                                    iscsi_co_generic_cb, &iTask);
> +    if (iscsilun->use_16_for_rw) {
> +        iTask.task = iscsi_write16_task(iscsilun->iscsi, iscsilun->lun, lba,
> +                                        data, num_sectors * iscsilun->block_size,
> +                                        iscsilun->block_size, 0, 0, 0, 0, 0,
> +                                        iscsi_co_generic_cb, &iTask);
> +    } else {
> +        iTask.task = iscsi_write10_task(iscsilun->iscsi, iscsilun->lun, lba,
> +                                        data, num_sectors * iscsilun->block_size,
> +                                        iscsilun->block_size, 0, 0, 0, 0, 0,
> +                                        iscsi_co_generic_cb, &iTask);
> +    }
>      if (iTask.task == NULL) {
>          g_free(buf);
>          return -ENOMEM;
> @@ -545,20 +553,17 @@ static int coroutine_fn iscsi_co_readv(BlockDriverState *bs,
>
>      iscsi_co_init_iscsitask(iscsilun, &iTask);
>  retry:
> -    switch (iscsilun->type) {
> -    case TYPE_DISK:
> +    if (iscsilun->use_16_for_rw) {
>          iTask.task = iscsi_read16_task(iscsilun->iscsi, iscsilun->lun, lba,
>                                         num_sectors * iscsilun->block_size,
>                                         iscsilun->block_size, 0, 0, 0, 0, 0,
>                                         iscsi_co_generic_cb, &iTask);
> -        break;
> -    default:
> +    } else {
>          iTask.task = iscsi_read10_task(iscsilun->iscsi, iscsilun->lun, lba,
>                                         num_sectors * iscsilun->block_size,
>                                         iscsilun->block_size,
>                                         0, 0, 0, 0, 0,
>                                         iscsi_co_generic_cb, &iTask);
> -        break;
>      }
>      if (iTask.task == NULL) {
>          return -ENOMEM;
> @@ -864,19 +869,27 @@ coroutine_fn iscsi_co_write_zeroes(BlockDriverState *bs, int64_t sector_num,
>      struct IscsiTask iTask;
>      uint64_t lba;
>      uint32_t nb_blocks;
> +    bool use_16_for_ws = iscsilun->use_16_for_rw;
>
>      if (!is_request_lun_aligned(sector_num, nb_sectors, iscsilun)) {
>          return -EINVAL;
>      }
>
> -    if ((flags & BDRV_REQ_MAY_UNMAP) && !iscsilun->lbp.lbpws) {
> -        /* WRITE SAME with UNMAP is not supported by the target,
> -         * fall back and try WRITE SAME without UNMAP */
> -        flags &= ~BDRV_REQ_MAY_UNMAP;
> +    if (flags & BDRV_REQ_MAY_UNMAP) {
> +        if (!use_16_for_ws && !iscsilun->lbp.lbpws10) {
> +            /* WRITESAME10 with UNMAP is unsupported try WRITESAME16 */
> +            use_16_for_ws = true;
> +        }
> +        if (use_16_for_ws && !iscsilun->lbp.lbpws) {
> +            /* WRITESAME16 with UNMAP is not supported by the target,
> +             * fall back and try WRITESAME10/16 without UNMAP */
> +            flags &= ~BDRV_REQ_MAY_UNMAP;
> +            use_16_for_ws = iscsilun->use_16_for_rw;
> +        }
>      }
>
>      if (!(flags & BDRV_REQ_MAY_UNMAP) && !iscsilun->has_write_same) {
> -        /* WRITE SAME without UNMAP is not supported by the target */
> +        /* WRITESAME without UNMAP is not supported by the target */
>          return -ENOTSUP;
>      }
>
> @@ -889,10 +902,18 @@ coroutine_fn iscsi_co_write_zeroes(BlockDriverState *bs, int64_t sector_num,
>
>      iscsi_co_init_iscsitask(iscsilun, &iTask);
>  retry:
> -    if (iscsi_writesame16_task(iscsilun->iscsi, iscsilun->lun, lba,
> -                               iscsilun->zeroblock, iscsilun->block_size,
> -                               nb_blocks, 0, !!(flags & BDRV_REQ_MAY_UNMAP),
> -                               0, 0, iscsi_co_generic_cb, &iTask) == NULL) {
> +    if (use_16_for_ws) {
> +        iTask.task = iscsi_writesame16_task(iscsilun->iscsi, iscsilun->lun, lba,
> +                                            iscsilun->zeroblock, iscsilun->block_size,
> +                                            nb_blocks, 0, !!(flags & BDRV_REQ_MAY_UNMAP),
> +                                            0, 0, iscsi_co_generic_cb, &iTask);
> +    } else {
> +        iTask.task = iscsi_writesame10_task(iscsilun->iscsi, iscsilun->lun, lba,
> +                                            iscsilun->zeroblock, iscsilun->block_size,
> +                                            nb_blocks, 0, !!(flags & BDRV_REQ_MAY_UNMAP),
> +                                            0, 0, iscsi_co_generic_cb, &iTask);
> +    }
> +    if (iTask.task == NULL) {
>          return -ENOMEM;
>      }
>
> @@ -1087,6 +1108,7 @@ static void iscsi_readcapacity_sync(IscsiLun *iscsilun, Error **errp)
>                      iscsilun->num_blocks = rc16->returned_lba + 1;
>                      iscsilun->lbpme = rc16->lbpme;
>                      iscsilun->lbprz = rc16->lbprz;
> +                    iscsilun->use_16_for_rw = (rc16->returned_lba > 0xffffffff);
>                  }
>              }
>              break;
>

Thanks, applying this to scsi-next (once Stefan pushes his patches).

Paolo