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

[Peter Lieven]

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