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

[Peter Lieven]

Am 04.06.2014 16:00, schrieb ronnie sahlberg:
> Looks good.
>
> As an alternative, you could do the 10 vs 16 decision based on the LBA
> instead of the size of the device :
>
> -    if (use_16_for_ws) {
> +   if (lba >= 0x100000000) {
>         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);
>     }
>
> That would mean you get to use the 10 version of the cdb even for very
> large devices (as long as the IO is for blocks at the beginning of the
> device) and thus provide partial avoidance of this issue for those
> large devices.

I like that idea, however I fear that this would introduce additional bugs.
- Using 10 Byte CDBs where the target might expect 16 Byte CDBs?!
- What if lba + num_blocks > 2^32-1 ?

The switch I added is like Linux does it - as Paolo pointed out earlier.

In my case the number of >2TB Targets is not that big so I can work with
the switch based on the capacity. Until the bug is fixed I just can't move
those 2TB volumes around on the storage array.

Peter

>
>
> ronnie shalberg
>
>
> On Wed, Jun 4, 2014 at 6:47 AM, Peter Lieven <address@hidden> wrote:
>> 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 <address@hidden>
>> ---
>>  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;
>> --
>> 1.7.9.5
>>