iscsi/qemu-img/block-migration enhancements

Quoted 4 lines

block/iscsi.c +0 -0

@@ -1130,6 +1137,46 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags)
        bs->sg = 1;
    }
 

block/iscsi.c static int iscsi_open(BlockDriverState *bs, QDict *options, int flags) · line 1146

Quoted 7 lines

block/iscsi.c +7 -0

+    if (iscsilun->lbpme) {
+        struct scsi_inquiry_logical_block_provisioning *inq_lbp;
+        int full_size;
+
+        task = iscsi_inquiry_sync(iscsi, iscsilun->lun, 1,
+                                  SCSI_INQUIRY_PAGECODE_LOGICAL_BLOCK_PROVISIONING,
+                                  64);

Does this leak the previous task from line 1101?

Signed-off-by Peter Lieven <[email protected]>

Reviewed-by Paolo Bonzini <[email protected]>

---
 block/iscsi.c |   47 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 47 insertions(+)

block/iscsi.c +7 -0

index 0bbf0b1..a38a1bf 100644
@@ -49,6 +49,11 @@ typedef struct IscsiLun {
     uint64_t num_blocks;
     int events;
     QEMUTimer *nop_timer;
+    uint8_t lbpme;
+    uint8_t lbprz;
+    uint8_t lbpu;
+    uint8_t lbpws;
+    uint8_t lbpws10;
 } IscsiLun;
 
 typedef struct IscsiAIOCB {

@@ -948,6 +953,8 @@ static int iscsi_readcapacity_sync(IscsiLun *iscsilun)
                 } else {
                     iscsilun->block_size = rc16->block_length;
                     iscsilun->num_blocks = rc16->returned_lba + 1;
+                    iscsilun->lbpme = rc16->lbpme;
+                    iscsilun->lbprz = rc16->lbprz;
                 }
             }
             break;

@@ -1130,6 +1137,46 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags)
         bs->sg = 1;
     }
 

block/iscsi.c +19 -0

+            ret = -EINVAL;
+            goto out;
+        }
+        full_size = scsi_datain_getfullsize(task);
+        if (full_size > task->datain.size) {
+            scsi_free_scsi_task(task);
+
+            /* we need more data for the full list */
+            task = iscsi_inquiry_sync(iscsi, iscsilun->lun, 1,
+                                      SCSI_INQUIRY_PAGECODE_LOGICAL_BLOCK_PROVISIONING,
+                                      full_size);
+            if (task == NULL || task->status != SCSI_STATUS_GOOD) {
+                error_report("iSCSI: Inquiry command failed : %s",
+                             iscsi_get_error(iscsilun->iscsi));
+                ret = -EINVAL;
+                goto out;
+            }
+        }
+

Signed-off-by Peter Lieven <[email protected]>

---
 block/iscsi.c |   80 ++++++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 56 insertions(+), 24 deletions(-)

block/iscsi.c +56 -25

index a38a1bf..2e2455d 100644
@@ -54,6 +54,8 @@ typedef struct IscsiLun {
     uint8_t lbpu;
     uint8_t lbpws;
     uint8_t lbpws10;
+    uint32_t max_unmap;
+    uint32_t max_unmap_bdc;
 } IscsiLun;
 
 typedef struct IscsiAIOCB {

@@ -1007,6 +1009,37 @@ static QemuOptsList runtime_opts = {
     },
 };
 
+static struct scsi_task *iscsi_do_inquiry(struct iscsi_context *iscsi,
+                                          int lun, int evpd, int pc) {
+        int full_size;
+        struct scsi_task *task = NULL;
+        task = iscsi_inquiry_sync(iscsi, lun, evpd, pc, 64);
+        if (task == NULL || task->status != SCSI_STATUS_GOOD) {
+            goto fail;
+        }
+        full_size = scsi_datain_getfullsize(task);
+        if (full_size > task->datain.size) {
+            scsi_free_scsi_task(task);
+
+            /* we need more data for the full list */
+            task = iscsi_inquiry_sync(iscsi, lun, evpd, pc, full_size);
+            if (task == NULL || task->status != SCSI_STATUS_GOOD) {
+                goto fail;
+            }
+        }
+
+        return task;
+
+fail:
+        error_report("iSCSI: Inquiry command failed : %s",
+                     iscsi_get_error(iscsi));
+        if (task) {
+            scsi_free_scsi_task(task);
+            return NULL;
+        }
+        return NULL;
+}
+
 /*
  * We support iscsi url's on the form
  * iscsi://[<username>%<password>@]<host>[:<port>]/<targetname>/<lun>

@@ -1139,33 +1172,12 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags)
 
     if (iscsilun->lbpme) {
         struct scsi_inquiry_logical_block_provisioning *inq_lbp;
-        int full_size;
-
-        task = iscsi_inquiry_sync(iscsi, iscsilun->lun, 1,
-                                  SCSI_INQUIRY_PAGECODE_LOGICAL_BLOCK_PROVISIONING,
-                                  64);
-        if (task == NULL || task->status != SCSI_STATUS_GOOD) {
-            error_report("iSCSI: Inquiry command failed : %s",
-                   iscsi_get_error(iscsilun->iscsi));
+        task = iscsi_do_inquiry(iscsilun->iscsi, iscsilun->lun, 1,
+                                SCSI_INQUIRY_PAGECODE_LOGICAL_BLOCK_PROVISIONING);
+        if (task == NULL) {
             ret = -EINVAL;
             goto out;
         }
-        full_size = scsi_datain_getfullsize(task);
-        if (full_size > task->datain.size) {
-            scsi_free_scsi_task(task);
-
-            /* we need more data for the full list */
-            task = iscsi_inquiry_sync(iscsi, iscsilun->lun, 1,
-                                      SCSI_INQUIRY_PAGECODE_LOGICAL_BLOCK_PROVISIONING,
-                                      full_size);
-            if (task == NULL || task->status != SCSI_STATUS_GOOD) {
-                error_report("iSCSI: Inquiry command failed : %s",
-                             iscsi_get_error(iscsilun->iscsi));
-                ret = -EINVAL;
-                goto out;
-            }
-        }
-
         inq_lbp = scsi_datain_unmarshall(task);
         if (inq_lbp == NULL) {
             error_report("iSCSI: failed to unmarshall inquiry datain blob");

@@ -1175,6 +1187,26 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags)
         iscsilun->lbpu = inq_lbp->lbpu;
         iscsilun->lbpws = inq_lbp->lbpws;
         iscsilun->lbpws10 = inq_lbp->lbpws10;
+        scsi_free_scsi_task(task);
+        task = NULL;
+    }
+
+    if (iscsilun->lbpu) {
+        struct scsi_inquiry_block_limits *inq_bl;
+        task = iscsi_do_inquiry(iscsilun->iscsi, iscsilun->lun, 1,
+                                SCSI_INQUIRY_PAGECODE_BLOCK_LIMITS);
+        if (task == NULL) {
+            ret = -EINVAL;
+            goto out;
+        }
+        inq_bl = scsi_datain_unmarshall(task);
+        if (inq_bl == NULL) {
+            error_report("iSCSI: failed to unmarshall inquiry datain blob");
+            ret = -EINVAL;
+            goto out;
+        }
+        iscsilun->max_unmap = inq_bl->max_unmap;
+        iscsilun->max_unmap_bdc = inq_bl->max_unmap_bdc;
     }
 
 #if defined(LIBISCSI_FEATURE_NOP_COUNTER)
--

1.7.9.5

max_unmap :

If the target does not return an explicit limit for max_unmap it will return 0xffffffff which means "no limit".

I think you should add a check for max_unmap and clamp it down to something sane.
Maybe a maximum of 128M ?

Same for bdc, that should also be checked and clamped down to sane values.

Quoted 123 lines

Signed-off-by Peter Lieven <[email protected]>

---
block/iscsi.c | 80 ++++++++++++++++++++++++++++++++++++++++-----------------

1 file changed, 56 insertions(+), 24 deletions(-)

block/iscsi.c +56 -25

index a38a1bf..2e2455d 100644
@@ -54,6 +54,8 @@ typedef struct IscsiLun {
    uint8_t lbpu;
    uint8_t lbpws;
    uint8_t lbpws10;
+    uint32_t max_unmap;
+    uint32_t max_unmap_bdc;
 } IscsiLun;
 
 typedef struct IscsiAIOCB {

@@ -1007,6 +1009,37 @@ static QemuOptsList runtime_opts = {
    },
 };
 
+static struct scsi_task *iscsi_do_inquiry(struct iscsi_context *iscsi,
+                                          int lun, int evpd, int pc) {
+        int full_size;
+        struct scsi_task *task = NULL;
+        task = iscsi_inquiry_sync(iscsi, lun, evpd, pc, 64);
+        if (task == NULL || task->status != SCSI_STATUS_GOOD) {
+            goto fail;
+        }
+        full_size = scsi_datain_getfullsize(task);
+        if (full_size > task->datain.size) {
+            scsi_free_scsi_task(task);
+
+            /* we need more data for the full list */
+            task = iscsi_inquiry_sync(iscsi, lun, evpd, pc, full_size);
+            if (task == NULL || task->status != SCSI_STATUS_GOOD) {
+                goto fail;
+            }
+        }
+
+        return task;
+
+fail:
+        error_report("iSCSI: Inquiry command failed : %s",
+                     iscsi_get_error(iscsi));
+        if (task) {
+            scsi_free_scsi_task(task);
+            return NULL;
+        }
+        return NULL;
+}
+
 /*
 * We support iscsi url's on the form
 * iscsi://[<username>%<password>@]<host>[:<port>]/<targetname>/<lun>

@@ -1139,33 +1172,12 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags)
 
    if (iscsilun->lbpme) {
        struct scsi_inquiry_logical_block_provisioning *inq_lbp;
-        int full_size;
-
-        task = iscsi_inquiry_sync(iscsi, iscsilun->lun, 1,
-                                  SCSI_INQUIRY_PAGECODE_LOGICAL_BLOCK_PROVISIONING,
-                                  64);
-        if (task == NULL || task->status != SCSI_STATUS_GOOD) {
-            error_report("iSCSI: Inquiry command failed : %s",
-                   iscsi_get_error(iscsilun->iscsi));
+        task = iscsi_do_inquiry(iscsilun->iscsi, iscsilun->lun, 1,
+                                SCSI_INQUIRY_PAGECODE_LOGICAL_BLOCK_PROVISIONING);
+        if (task == NULL) {
            ret = -EINVAL;
            goto out;
        }
-        full_size = scsi_datain_getfullsize(task);
-        if (full_size > task->datain.size) {
-            scsi_free_scsi_task(task);
-
-            /* we need more data for the full list */
-            task = iscsi_inquiry_sync(iscsi, iscsilun->lun, 1,
-                                      SCSI_INQUIRY_PAGECODE_LOGICAL_BLOCK_PROVISIONING,
-                                      full_size);
-            if (task == NULL || task->status != SCSI_STATUS_GOOD) {
-                error_report("iSCSI: Inquiry command failed : %s",
-                             iscsi_get_error(iscsilun->iscsi));
-                ret = -EINVAL;
-                goto out;
-            }
-        }
-
        inq_lbp = scsi_datain_unmarshall(task);
        if (inq_lbp == NULL) {
            error_report("iSCSI: failed to unmarshall inquiry datain blob");

@@ -1175,6 +1187,26 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags)
        iscsilun->lbpu = inq_lbp->lbpu;
        iscsilun->lbpws = inq_lbp->lbpws;
        iscsilun->lbpws10 = inq_lbp->lbpws10;
+        scsi_free_scsi_task(task);
+        task = NULL;
+    }
+
+    if (iscsilun->lbpu) {
+        struct scsi_inquiry_block_limits *inq_bl;
+        task = iscsi_do_inquiry(iscsilun->iscsi, iscsilun->lun, 1,
+                                SCSI_INQUIRY_PAGECODE_BLOCK_LIMITS);
+        if (task == NULL) {
+            ret = -EINVAL;
+            goto out;
+        }
+        inq_bl = scsi_datain_unmarshall(task);
+        if (inq_bl == NULL) {
+            error_report("iSCSI: failed to unmarshall inquiry datain blob");
+            ret = -EINVAL;
+            goto out;
+        }
+        iscsilun->max_unmap = inq_bl->max_unmap;
+        iscsilun->max_unmap_bdc = inq_bl->max_unmap_bdc;
    }
 
 #if defined(LIBISCSI_FEATURE_NOP_COUNTER)
--

1.7.9.5

BDC is not used. I had an implementation that sent multiple descriptors out, but at least for my storage the maximum unmap counts not for each descriptors, but for all together. So in this case we do not need the field at all. I forgot to remove it.

discard and write_zeroes will both only send one request up to max_unmap in size.

apropos write_zeroes: do you know if UNMAP is guaranteed to unmap data if lbprz == 1?

I have read in the specs something that the target might unmap the blocks or not touch them at all. Maybe you have more information.

Il 03/07/2013 23:23, Peter Lieven ha scritto:

Quoted 7 lines

BDC is not used. I had an implementation that sent multiple descriptors out, but at least for my storage the maximum unmap counts not for each descriptors, but for all together. So in this case we do not need the field at all. I forgot to remove it.

discard and write_zeroes will both only send one request up to max_unmap in size.

apropos write_zeroes: do you know if UNMAP is guaranteed to unmap data if lbprz == 1?

Yes. On the other hand note that WRITE_SAME should be guaranteed _not_ to unmap if lbprz == 0 and you do WRITE_SAME with UNMAP and a zero payload, but I suspect there may be buggy targets here.

Quoted 2 lines

I have read in the specs something that the target might unmap the blocks or not touch them at all. Maybe you have more information.

That's even true of UNMAP itself, actually. :)

The storage can always "upgrade" a block from unmapped to anchored and from anchored to allocated, so UNMAP can be a no-op and still comply with the standard.

Am 04.07.2013 um 14:37 schrieb Paolo Bonzini <[email protected]>:

Quoted 18 lines

Il 03/07/2013 23:23, Peter Lieven ha scritto:

Quoted 7 lines

BDC is not used. I had an implementation that sent multiple descriptors out, but at least for my storage the maximum unmap counts not for each descriptors, but for all together. So in this case we do not need the field at all. I forgot to remove it.

discard and write_zeroes will both only send one request up to max_unmap in size.

apropos write_zeroes: do you know if UNMAP is guaranteed to unmap data if lbprz == 1?

Yes. On the other hand note that WRITE_SAME should be guaranteed _not_ to unmap if lbprz == 0 and you do WRITE_SAME with UNMAP and a zero payload, but I suspect there may be buggy targets here.

Quoted 2 lines

I have read in the specs something that the target might unmap the blocks or not touch them at all. Maybe you have more information.

That's even true of UNMAP itself, actually. :)

Quoted 3 lines

The storage can always "upgrade" a block from unmapped to anchored and from anchored to allocated, so UNMAP can be a no-op and still comply with the standard.

My concern was, if I UNMAP a block and lbprz == 1 is it guaranteed that it reads as zero afterwards? Regardless if the target decides to "upgrade" the block or do not unmap the block?

Am 04.07.2013 um 14:37 schrieb Paolo Bonzini <[email protected]>:

Quoted 13 lines

Il 03/07/2013 23:23, Peter Lieven ha scritto:

Quoted 7 lines

BDC is not used. I had an implementation that sent multiple descriptors out, but at least for my storage the maximum unmap counts not for each descriptors, but for all together. So in this case we do not need the field at all. I forgot to remove it.

discard and write_zeroes will both only send one request up to max_unmap in size.

apropos write_zeroes: do you know if UNMAP is guaranteed to unmap data if lbprz == 1?

Yes. On the other hand note that WRITE_SAME should be guaranteed _not_ to unmap if lbprz == 0 and you do WRITE_SAME with UNMAP and a zero payload, but I suspect there may be buggy targets here.

Quoted 8 lines

Quoted 2 lines

I have read in the specs something that the target might unmap the blocks or not touch them at all. Maybe you have more information.

That's even true of UNMAP itself, actually. :)

The storage can always "upgrade" a block from unmapped to anchored and from anchored to allocated, so UNMAP can be a no-op and still comply with the standard.

My concern was, if I UNMAP a block and lbprz == 1 is it guaranteed that it reads as zero afterwards? Regardless if the target decides to "upgrade" the block or do not unmap the block?

Peter

The device MIGHT map or anchor the blocks after the unmap but it may only do so if the blocks that become mapped are all zero.

So I think you can safely assume that if lbprz==1 then it will always read back as zero no matter what happens internally in the target.

Either the block becomes unmapped/deallocated and then it will read back as zero,
or the blocks is automatically re-mapped to anchored/mapped again but this can only happen if the mapped block is all zero so once again it will still read back as all zero.

===

4.7.3.5 Autonomous LBA transitions
A device server may perform the following actions at any time:
a) transition any deallocated LBA to mapped;
b) transition any anchored LBA to mapped; or
c) transition any deallocated LBA to anchored.
If the LBPRZ bit in the READ CAPACITY (16) parameter data (see 5.16.2) is set to one, and a mapped LBA
references a logical block that contains:
a) user data with all bits set to zero; and
Working Draft SCSI Block Commands – 3 (SBC-3)
27T10/BSR INCITS 514 Revision 35d
15 May 2013
b) protection information, if any, set to FFFF_FFFF_FFFF_FFFFh, then the device server may transition that mapped LBA to anchored or deallocated at any time.
The logical block provisioning st

Quoted 31 lines · Peter Lieven

Am 04.07.2013 um 14:37 schrieb Paolo Bonzini <[email protected]>:

Quoted 13 lines

Il 03/07/2013 23:23, Peter Lieven ha scritto:

Quoted 7 lines

BDC is not used. I had an implementation that sent multiple descriptors out, but at least for my storage the maximum unmap counts not for each descriptors, but for all together. So in this case we do not need the field at all. I forgot to remove it.

discard and write_zeroes will both only send one request up to max_unmap in size.

apropos write_zeroes: do you know if UNMAP is guaranteed to unmap data if lbprz == 1?

Yes. On the other hand note that WRITE_SAME should be guaranteed _not_ to unmap if lbprz == 0 and you do WRITE_SAME with UNMAP and a zero payload, but I suspect there may be buggy targets here.

Quoted 8 lines

Quoted 2 lines

I have read in the specs something that the target might unmap the blocks or not touch them at all. Maybe you have more information.

That's even true of UNMAP itself, actually. :)

The storage can always "upgrade" a block from unmapped to anchored and from anchored to allocated, so UNMAP can be a no-op and still comply with the standard.

My concern was, if I UNMAP a block and lbprz == 1 is it guaranteed that it reads as zero afterwards? Regardless if the target decides to "upgrade" the block or do not unmap the block?

Peter

ok, to sum up you see no potential problem with my patch to optimize write zeroes by unmap iff lbprz==1 and lbpme == 1 ?

the alternative would be to use writesame16 and sent a zero block. this would allow an optimization also if lbprz == 0. in this case i would not set the unmap bit.

peter

Am 05.07.2013 um 09:11 schrieb ronnie sahlberg <[email protected]>:

Quoted 63 lines

The device MIGHT map or anchor the blocks after the unmap but it may only do so if the blocks that become mapped are all zero.

So I think you can safely assume that if lbprz==1 then it will always read back as zero no matter what happens internally in the target.

Either the block becomes unmapped/deallocated and then it will read back as zero,
or the blocks is automatically re-mapped to anchored/mapped again but this can only happen if the mapped block is all zero so once again it will still read back as all zero.

===

4.7.3.5 Autonomous LBA transitions
A device server may perform the following actions at any time:
a) transition any deallocated LBA to mapped;
b) transition any anchored LBA to mapped; or
c) transition any deallocated LBA to anchored.
If the LBPRZ bit in the READ CAPACITY (16) parameter data (see 5.16.2) is set to one, and a mapped LBA
references a logical block that contains:
a) user data with all bits set to zero; and
Working Draft SCSI Block Commands – 3 (SBC-3)
27T10/BSR INCITS 514 Revision 35d
15 May 2013
b) protection information, if any, set to FFFF_FFFF_FFFF_FFFFh, then the device server may transition that mapped LBA to anchored or deallocated at any time.
The logical block provisioning st

Quoted 31 lines · Peter Lieven

Am 04.07.2013 um 14:37 schrieb Paolo Bonzini <[email protected]>:

Quoted 13 lines

Il 03/07/2013 23:23, Peter Lieven ha scritto:

Quoted 7 lines

BDC is not used. I had an implementation that sent multiple descriptors out, but at least for my storage the maximum unmap counts not for each descriptors, but for all together. So in this case we do not need the field at all. I forgot to remove it.

discard and write_zeroes will both only send one request up to max_unmap in size.

apropos write_zeroes: do you know if UNMAP is guaranteed to unmap data if lbprz == 1?

Yes. On the other hand note that WRITE_SAME should be guaranteed _not_ to unmap if lbprz == 0 and you do WRITE_SAME with UNMAP and a zero payload, but I suspect there may be buggy targets here.

Quoted 8 lines

Quoted 2 lines

I have read in the specs something that the target might unmap the blocks or not touch them at all. Maybe you have more information.

That's even true of UNMAP itself, actually. :)

The storage can always "upgrade" a block from unmapped to anchored and from anchored to allocated, so UNMAP can be a no-op and still comply with the standard.

My concern was, if I UNMAP a block and lbprz == 1 is it guaranteed that it reads as zero afterwards? Regardless if the target decides to "upgrade" the block or do not unmap the block?

Peter

Signed-off-by Peter Lieven <[email protected]>

---
 block/iscsi.c |   80 ++++++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 56 insertions(+), 24 deletions(-)

block/iscsi.c +11 -0

@@ -1175,6 +1187,26 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags)
         iscsilun->lbpu = inq_lbp->lbpu;
         iscsilun->lbpws = inq_lbp->lbpws;
         iscsilun->lbpws10 = inq_lbp->lbpws10;
+        scsi_free_scsi_task(task);
+        task = NULL;
+    }
+
+    if (iscsilun->lbpu) {
+        struct scsi_inquiry_block_limits *inq_bl;
+        task = iscsi_do_inquiry(iscsilun->iscsi, iscsilun->lun, 1,
+                                SCSI_INQUIRY_PAGECODE_BLOCK_LIMITS);
+        if (task == NULL) {
+            ret = -EINVAL;
+            goto out;

Signed-off-by Peter Lieven <[email protected]>

---
 block/iscsi.c |   80 ++++++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 56 insertions(+), 24 deletions(-)

block/iscsi.c +2 -0

index a38a1bf..2e2455d 100644
@@ -54,6 +54,8 @@ typedef struct IscsiLun {
     uint8_t lbpu;
     uint8_t lbpws;
     uint8_t lbpws10;
+    uint32_t max_unmap;
+    uint32_t max_unmap_bdc;
 } IscsiLun;
 
 typedef struct IscsiAIOCB {

block/iscsi.c +18 -0

@@ -113,6 +122,45 @@ iscsi_schedule_bh(IscsiAIOCB *acb)
     qemu_bh_schedule(acb->bh);
 }
 
+static void
+iscsi_co_generic_cb(struct iscsi_context *iscsi, int status,
+                        void *command_data, void *opaque)
+{
+    struct IscsiTask *iTask = opaque;
+    struct scsi_task *task = command_data;
+
+    iTask->complete = 1;
+    iTask->status = status;
+    iTask->do_retry = 0;
+    iTask->task = task;
+
+    if (iTask->retries-- > 0 && status == SCSI_STATUS_CHECK_CONDITION
+        && task->sense.key == SCSI_SENSE_UNIT_ATTENTION) {
+        iTask->do_retry = 1;
+        goto out;
+    }
+

Quoted 22 lines

block/iscsi.c +18 -0

@@ -113,6 +122,45 @@ iscsi_schedule_bh(IscsiAIOCB *acb)
    qemu_bh_schedule(acb->bh);
 }
 
+static void
+iscsi_co_generic_cb(struct iscsi_context *iscsi, int status,
+                        void *command_data, void *opaque)
+{
+    struct IscsiTask *iTask = opaque;
+    struct scsi_task *task = command_data;
+
+    iTask->complete = 1;
+    iTask->status = status;
+    iTask->do_retry = 0;
+    iTask->task = task;
+
+    if (iTask->retries-- > 0 && status == SCSI_STATUS_CHECK_CONDITION
+        && task->sense.key == SCSI_SENSE_UNIT_ATTENTION) {
+        iTask->do_retry = 1;
+        goto out;
+    }
+

block/iscsi.c iscsi_schedule_bh(IscsiAIOCB *acb) · line 152

Quoted 10 lines

block/iscsi.c +10 -0

+    if (status != SCSI_STATUS_GOOD) {
+        error_report("iSCSI: Failure. %s", iscsi_get_error(iscsi));
+        goto out;
+    }
+
+out:
+    if (iTask->status != SCSI_STATUS_GOOD) {
+        scsi_free_scsi_task(iTask->task);
+        iTask->task = NULL;
+    }

Not sure about freeing the task here since the caller is still responsible for freeing the task in the success case and higher-level error cases.

I suggest keeping iTask->task alive here so the caller is easier to maintain (it can use a single out/error path).

this patch adds a coroutine for .bdrv_co_is_allocated as well as a generic framework that can be used to build coroutines in block/iscsi.

Signed-off-by Peter Lieven <[email protected]>

---
 block/iscsi.c |  123 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 123 insertions(+)

block/iscsi.c +31 -0

index 2e2455d..d31ee95 100644
@@ -58,6 +58,15 @@ typedef struct IscsiLun {
     uint32_t max_unmap_bdc;
 } IscsiLun;
 
+typedef struct IscsiTask {
+    int status;
+    int complete;
+    int retries;
+    int do_retry;
+    struct scsi_task *task;
+    Coroutine *co;
+} IscsiTask;
+
 typedef struct IscsiAIOCB {
     BlockDriverAIOCB common;
     QEMUIOVector *qiov;

@@ -113,6 +122,45 @@ iscsi_schedule_bh(IscsiAIOCB *acb)
     qemu_bh_schedule(acb->bh);
 }
 
+static void
+iscsi_co_generic_cb(struct iscsi_context *iscsi, int status,
+                        void *command_data, void *opaque)
+{
+    struct IscsiTask *iTask = opaque;
+    struct scsi_task *task = command_data;
+
+    iTask->complete = 1;
+    iTask->status = status;
+    iTask->do_retry = 0;
+    iTask->task = task;
+
+    if (iTask->retries-- > 0 && status == SCSI_STATUS_CHECK_CONDITION
+        && task->sense.key == SCSI_SENSE_UNIT_ATTENTION) {
+        iTask->do_retry = 1;
+        goto out;
+    }
+
+    if (status != SCSI_STATUS_GOOD) {
+        error_report("iSCSI: Failure. %s", iscsi_get_error(iscsi));
+        goto out;
+    }

block/iscsi.c +51 -0

 static void
 iscsi_abort_task_cb(struct iscsi_context *iscsi, int status, void *command_data,

@@ -807,6 +855,79 @@ iscsi_getlength(BlockDriverState *bs)
     return len;
 }
 
+static int coroutine_fn iscsi_co_is_allocated(BlockDriverState *bs,
+                                              int64_t sector_num,
+                                              int nb_sectors, int *pnum)
+{
+    IscsiLun *iscsilun = bs->opaque;
+    struct scsi_get_lba_status *lbas = NULL;
+    struct scsi_lba_status_descriptor *lbasd = NULL;
+    struct IscsiTask iTask;
+    int ret;
+
+    /* LUN does not support logical block provisioning */
+    if (iscsilun->lbpme == 0) {
+        *pnum = nb_sectors;
+        return 1;
+    }
+
+    iscsi_co_init_iscsitask(iscsilun, &iTask);
+
+retry:
+    if (iscsi_get_lba_status_task(iscsilun->iscsi, iscsilun->lun,
+                                  sector_qemu2lun(sector_num, iscsilun),
+                                  8 + 16, iscsi_co_generic_cb,
+                                  &iTask) == NULL) {
+        goto fail;
+    }
+
+    while (!iTask.complete) {
+        iscsi_set_events(iscsilun);
+        qemu_coroutine_yield();
+    }
+
+    if (iTask.do_retry) {
+        goto retry;
+    }
+
+    if (iTask.status != SCSI_STATUS_GOOD) {
+        /* in case the get_lba_status_callout fails (i.e.
+         * because the device is busy or the cmd is not
+         * supported) we pretend all blocks are allocated
+         * for backwards compatiblity */
+        *pnum = nb_sectors;
+        return 1;
+    }
+
+    lbas = scsi_datain_unmarshall(iTask.task);
+    if (lbas == NULL) {
+        goto fail;
+    }
+
+    lbasd = &lbas->descriptors[0];
+

Am 27.06.2013 um 15:11 hat Peter Lieven geschrieben:

Quoted 59 lines

this patch adds a coroutine for .bdrv_co_is_allocated as well as a generic framework that can be used to build coroutines in block/iscsi.

Signed-off-by Peter Lieven <[email protected]>

---
 block/iscsi.c |  123 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 123 insertions(+)

block/iscsi.c +36 -0

index 2e2455d..d31ee95 100644
@@ -58,6 +58,15 @@ typedef struct IscsiLun {
     uint32_t max_unmap_bdc;
 } IscsiLun;
 
+typedef struct IscsiTask {
+    int status;
+    int complete;
+    int retries;
+    int do_retry;
+    struct scsi_task *task;
+    Coroutine *co;
+} IscsiTask;
+
 typedef struct IscsiAIOCB {
     BlockDriverAIOCB common;
     QEMUIOVector *qiov;

@@ -113,6 +122,45 @@ iscsi_schedule_bh(IscsiAIOCB *acb)
     qemu_bh_schedule(acb->bh);
 }
 
+static void
+iscsi_co_generic_cb(struct iscsi_context *iscsi, int status,
+                        void *command_data, void *opaque)
+{
+    struct IscsiTask *iTask = opaque;
+    struct scsi_task *task = command_data;
+
+    iTask->complete = 1;
+    iTask->status = status;
+    iTask->do_retry = 0;
+    iTask->task = task;
+
+    if (iTask->retries-- > 0 && status == SCSI_STATUS_CHECK_CONDITION
+        && task->sense.key == SCSI_SENSE_UNIT_ATTENTION) {
+        iTask->do_retry = 1;
+        goto out;
+    }
+
+    if (status != SCSI_STATUS_GOOD) {
+        error_report("iSCSI: Failure. %s", iscsi_get_error(iscsi));
+        goto out;
+    }
+
+out:
+    if (iTask->status != SCSI_STATUS_GOOD) {
+        scsi_free_scsi_task(iTask->task);
+        iTask->task = NULL;

block/iscsi.c iscsi_schedule_bh(IscsiAIOCB *acb) · line 156

Quoted 5 lines

block/iscsi.c +5 -0

+    }
+    if (iTask->co) {
+        qemu_coroutine_enter(iTask->co, NULL);
+    }
+}

In which context does this callback run? If this is from a separate thread, you may not do all of this (without even holding the QBL). You should probably use a BH to switch to the I/O thread.

block/iscsi.c iscsi_schedule_bh(IscsiAIOCB *acb) · line 163

Quoted 7 lines

block/iscsi.c +7 -0

+
+static void iscsi_co_init_iscsitask(IscsiLun *iscsilun, struct IscsiTask *iTask)
+{
+    memset(iTask, 0x00, sizeof(struct IscsiTask));
+    iTask->co = qemu_coroutine_self();
+    iTask->retries = ISCSI_CMD_RETRIES;
+}

Matter of taste, but in my own code I prefer not to have a memset:

*iTask = (struct IscsiTask) {
.co = qemu_coroutine_self(),
.retries = ISCSI_CMD_RETRIES,
};

Quoted 63 lines

block/iscsi.c +56 -0

 static void
 iscsi_abort_task_cb(struct iscsi_context *iscsi, int status, void *command_data,

@@ -807,6 +855,79 @@ iscsi_getlength(BlockDriverState *bs)
     return len;
 }
 
+static int coroutine_fn iscsi_co_is_allocated(BlockDriverState *bs,
+                                              int64_t sector_num,
+                                              int nb_sectors, int *pnum)
+{
+    IscsiLun *iscsilun = bs->opaque;
+    struct scsi_get_lba_status *lbas = NULL;
+    struct scsi_lba_status_descriptor *lbasd = NULL;
+    struct IscsiTask iTask;
+    int ret;
+
+    /* LUN does not support logical block provisioning */
+    if (iscsilun->lbpme == 0) {
+        *pnum = nb_sectors;
+        return 1;
+    }
+
+    iscsi_co_init_iscsitask(iscsilun, &iTask);
+
+retry:
+    if (iscsi_get_lba_status_task(iscsilun->iscsi, iscsilun->lun,
+                                  sector_qemu2lun(sector_num, iscsilun),
+                                  8 + 16, iscsi_co_generic_cb,
+                                  &iTask) == NULL) {
+        goto fail;
+    }
+
+    while (!iTask.complete) {
+        iscsi_set_events(iscsilun);
+        qemu_coroutine_yield();
+    }
+
+    if (iTask.do_retry) {
+        goto retry;
+    }
+
+    if (iTask.status != SCSI_STATUS_GOOD) {
+        /* in case the get_lba_status_callout fails (i.e.
+         * because the device is busy or the cmd is not
+         * supported) we pretend all blocks are allocated
+         * for backwards compatiblity */
+        *pnum = nb_sectors;
+        return 1;
+    }
+
+    lbas = scsi_datain_unmarshall(iTask.task);
+    if (lbas == NULL) {
+        goto fail;
+    }
+
+    lbasd = &lbas->descriptors[0];
+
+    if (sector_qemu2lun(sector_num, iscsilun) != lbasd->lba) {
+        goto fail;
+    }
+
+    *pnum = lbasd->num_blocks * (iscsilun->block_size / BDRV_SECTOR_SIZE);

block/iscsi.c iscsi_getlength(BlockDriverState *bs) · line 918

Quoted 5 lines

block/iscsi.c +5 -0

+    if (*pnum > nb_sectors) {
+        *pnum = nb_sectors;
+    }
+
+    ret = lbasd->provisioning == SCSI_PROVISIONING_TYPE_MAPPED ? 1 : 0;

Wouldn't it be safer to default to allocated, in case new values are added in a later protocol version? I agree with returning 0 for both unmapped and anchored, though.

block/iscsi.c iscsi_getlength(BlockDriverState *bs) · line 929

Quoted 11 lines

block/iscsi.c +10 -0

+    scsi_free_scsi_task(iTask.task);
+    return ret;
+
+fail:
+    if (iTask.task != NULL) {
+        scsi_free_scsi_task(iTask.task);
+    }
+    *pnum = 0;
+    return 0;
+}

Kevin

Am 10.07.2013 11:41, schrieb Kevin Wolf:

Quoted 60 lines

Am 27.06.2013 um 15:11 hat Peter Lieven geschrieben:

Quoted 59 lines

this patch adds a coroutine for .bdrv_co_is_allocated as well as a generic framework that can be used to build coroutines in block/iscsi.

Signed-off-by Peter Lieven <[email protected]>

---
 block/iscsi.c |  123 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 123 insertions(+)

block/iscsi.c +36 -0

index 2e2455d..d31ee95 100644
@@ -58,6 +58,15 @@ typedef struct IscsiLun {
     uint32_t max_unmap_bdc;
 } IscsiLun;
 
+typedef struct IscsiTask {
+    int status;
+    int complete;
+    int retries;
+    int do_retry;
+    struct scsi_task *task;
+    Coroutine *co;
+} IscsiTask;
+
 typedef struct IscsiAIOCB {
     BlockDriverAIOCB common;
     QEMUIOVector *qiov;

@@ -113,6 +122,45 @@ iscsi_schedule_bh(IscsiAIOCB *acb)
     qemu_bh_schedule(acb->bh);
 }
 
+static void
+iscsi_co_generic_cb(struct iscsi_context *iscsi, int status,
+                        void *command_data, void *opaque)
+{
+    struct IscsiTask *iTask = opaque;
+    struct scsi_task *task = command_data;
+
+    iTask->complete = 1;
+    iTask->status = status;
+    iTask->do_retry = 0;
+    iTask->task = task;
+
+    if (iTask->retries-- > 0 && status == SCSI_STATUS_CHECK_CONDITION
+        && task->sense.key == SCSI_SENSE_UNIT_ATTENTION) {
+        iTask->do_retry = 1;
+        goto out;
+    }
+
+    if (status != SCSI_STATUS_GOOD) {
+        error_report("iSCSI: Failure. %s", iscsi_get_error(iscsi));
+        goto out;
+    }
+
+out:
+    if (iTask->status != SCSI_STATUS_GOOD) {
+        scsi_free_scsi_task(iTask->task);
+        iTask->task = NULL;

Quoted 8 lines

block/iscsi.c iscsi_schedule_bh(IscsiAIOCB *acb) · line 156

Quoted 5 lines

block/iscsi.c +5 -0

+    }
+    if (iTask->co) {
+        qemu_coroutine_enter(iTask->co, NULL);
+    }
+}

In which context does this callback run? If this is from a separate thread, you may not do all of this (without even holding the QBL). You should probably use a BH to switch to the I/O thread.

good question. the callbacks can only be fired when iscsi_service() is invoked.
this is currently done by registereing iscsi_process_read/write/flush via qemu_aio_set_fd_handler().

so the callbacks are invoked in the aio context it seems. can this be right?

Am 10.07.2013 um 15:49 hat Peter Lieven geschrieben:

Quoted 69 lines

Am 10.07.2013 11:41, schrieb Kevin Wolf:

Quoted 68 lines

Am 27.06.2013 um 15:11 hat Peter Lieven geschrieben:

Quoted 54 lines

this patch adds a coroutine for .bdrv_co_is_allocated as well as a generic framework that can be used to build coroutines in block/iscsi.

Signed-off-by Peter Lieven <[email protected]>

---
 block/iscsi.c |  123 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 123 insertions(+)

block/iscsi.c +31 -0

index 2e2455d..d31ee95 100644
@@ -58,6 +58,15 @@ typedef struct IscsiLun {
     uint32_t max_unmap_bdc;
 } IscsiLun;
 
+typedef struct IscsiTask {
+    int status;
+    int complete;
+    int retries;
+    int do_retry;
+    struct scsi_task *task;
+    Coroutine *co;
+} IscsiTask;
+
 typedef struct IscsiAIOCB {
     BlockDriverAIOCB common;
     QEMUIOVector *qiov;

@@ -113,6 +122,45 @@ iscsi_schedule_bh(IscsiAIOCB *acb)
     qemu_bh_schedule(acb->bh);
 }
 
+static void
+iscsi_co_generic_cb(struct iscsi_context *iscsi, int status,
+                        void *command_data, void *opaque)
+{
+    struct IscsiTask *iTask = opaque;
+    struct scsi_task *task = command_data;
+
+    iTask->complete = 1;
+    iTask->status = status;
+    iTask->do_retry = 0;
+    iTask->task = task;
+
+    if (iTask->retries-- > 0 && status == SCSI_STATUS_CHECK_CONDITION
+        && task->sense.key == SCSI_SENSE_UNIT_ATTENTION) {
+        iTask->do_retry = 1;
+        goto out;
+    }
+
+    if (status != SCSI_STATUS_GOOD) {
+        error_report("iSCSI: Failure. %s", iscsi_get_error(iscsi));
+        goto out;
+    }

block/iscsi.c iscsi_schedule_bh(IscsiAIOCB *acb) · line 156

Quoted 10 lines

block/iscsi.c +10 -0

+
+out:
+    if (iTask->status != SCSI_STATUS_GOOD) {
+        scsi_free_scsi_task(iTask->task);
+        iTask->task = NULL;
+    }
+    if (iTask->co) {
+        qemu_coroutine_enter(iTask->co, NULL);
+    }
+}

In which context does this callback run? If this is from a separate thread, you may not do all of this (without even holding the QBL). You should probably use a BH to switch to the I/O thread.

Quoted 4 lines

good question. the callbacks can only be fired when iscsi_service() is invoked.
this is currently done by registereing iscsi_process_read/write/flush via qemu_aio_set_fd_handler().

so the callbacks are invoked in the aio context it seems. can this be right?

Yes, I took another look and I think it's fine.

Am 10.07.2013 16:43, schrieb Kevin Wolf:

Quoted 70 lines

Am 10.07.2013 um 15:49 hat Peter Lieven geschrieben:

Quoted 69 lines

Am 10.07.2013 11:41, schrieb Kevin Wolf:

Quoted 68 lines

Am 27.06.2013 um 15:11 hat Peter Lieven geschrieben:

Quoted 54 lines

this patch adds a coroutine for .bdrv_co_is_allocated as well as a generic framework that can be used to build coroutines in block/iscsi.

Signed-off-by Peter Lieven <[email protected]>

---
 block/iscsi.c |  123 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 123 insertions(+)

block/iscsi.c +31 -0

index 2e2455d..d31ee95 100644
@@ -58,6 +58,15 @@ typedef struct IscsiLun {
     uint32_t max_unmap_bdc;
 } IscsiLun;
 
+typedef struct IscsiTask {
+    int status;
+    int complete;
+    int retries;
+    int do_retry;
+    struct scsi_task *task;
+    Coroutine *co;
+} IscsiTask;
+
 typedef struct IscsiAIOCB {
     BlockDriverAIOCB common;
     QEMUIOVector *qiov;

@@ -113,6 +122,45 @@ iscsi_schedule_bh(IscsiAIOCB *acb)
     qemu_bh_schedule(acb->bh);
 }
 
+static void
+iscsi_co_generic_cb(struct iscsi_context *iscsi, int status,
+                        void *command_data, void *opaque)
+{
+    struct IscsiTask *iTask = opaque;
+    struct scsi_task *task = command_data;
+
+    iTask->complete = 1;
+    iTask->status = status;
+    iTask->do_retry = 0;
+    iTask->task = task;
+
+    if (iTask->retries-- > 0 && status == SCSI_STATUS_CHECK_CONDITION
+        && task->sense.key == SCSI_SENSE_UNIT_ATTENTION) {
+        iTask->do_retry = 1;
+        goto out;
+    }
+
+    if (status != SCSI_STATUS_GOOD) {
+        error_report("iSCSI: Failure. %s", iscsi_get_error(iscsi));
+        goto out;
+    }

block/iscsi.c iscsi_schedule_bh(IscsiAIOCB *acb) · line 156

Quoted 10 lines

block/iscsi.c +10 -0

+
+out:
+    if (iTask->status != SCSI_STATUS_GOOD) {
+        scsi_free_scsi_task(iTask->task);
+        iTask->task = NULL;
+    }
+    if (iTask->co) {
+        qemu_coroutine_enter(iTask->co, NULL);
+    }
+}

In which context does this callback run? If this is from a separate thread, you may not do all of this (without even holding the QBL). You should probably use a BH to switch to the I/O thread.

Quoted 5 lines

Quoted 4 lines

good question. the callbacks can only be fired when iscsi_service() is invoked.
this is currently done by registereing iscsi_process_read/write/flush via qemu_aio_set_fd_handler().

so the callbacks are invoked in the aio context it seems. can this be right?

Yes, I took another look and I think it's fine.

Would this still be true if all aio routines are replaced by coroutines?

write zeroes is emulated by unmap if the target supports unmapping an unmapped blocks read as zero. this emulation is only done if the device was opened with BDRV_O_UNMAP and the request can be handled within a single request. a failback to writev is performed otherwise.

Signed-off-by Peter Lieven <[email protected]>

---
 block/iscsi.c |   44 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

block/iscsi.c +0 -0

index d31ee95..92e66a6 100644

Signed-off-by Peter Lieven <[email protected]>

---
 block.c               |   27 +++++++++++++++++++--------
 include/block/block.h |    2 ++
 2 files changed, 21 insertions(+), 8 deletions(-)

Signed-off-by Peter Lieven <[email protected]>

---
 block/raw.c |    8 ++++++++
 1 file changed, 8 insertions(+)

if the device supports unmapping and unmapped blocks read as zero ensure that the whole device is unmapped and report .has_zero_init = 1 in this case to speed up qemu-img convert.

Signed-off-by Peter Lieven <[email protected]>

---
 block/iscsi.c |   72 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 71 insertions(+), 1 deletion(-)

block/iscsi.c +71 -1

index 92e66a6..621ca40 100644
@@ -1436,7 +1436,8 @@ static int iscsi_truncate(BlockDriverState *bs, int64_t offset)
 
 static int iscsi_has_zero_init(BlockDriverState *bs)
 {
-    return 0;
+    IscsiLun *iscsilun = bs->opaque;
+    return (iscsilun->lbpu && iscsilun->lbprz) ? 1 : 0;
 }
 
 static int iscsi_create(const char *filename, QEMUOptionParameter *options)

@@ -1446,6 +1447,7 @@ static int iscsi_create(const char *filename, QEMUOptionParameter *options)
     BlockDriverState bs;
     IscsiLun *iscsilun = NULL;
     QDict *bs_options;
+    struct scsi_task *task = NULL;
 
     memset(&bs, 0, sizeof(BlockDriverState));
 

@@ -1481,7 +1483,75 @@ static int iscsi_create(const char *filename, QEMUOptionParameter *options)
     }
 
     ret = 0;
+
+    if (iscsilun->lbpu && iscsilun->lbprz) {
+        uint64_t lba = 0;
+        while (lba < iscsilun->num_blocks) {
+            struct scsi_get_lba_status *lbas = NULL;
+            struct scsi_lba_status_descriptor *lbasd = NULL;
+            enum scsi_provisioning_type provisioning;
+            uint32_t nb_sectors;
+
+            task = iscsi_get_lba_status_sync(iscsilun->iscsi, iscsilun->lun,
+                                             lba, 8 + 16);
+            if (task == NULL || task->status != SCSI_STATUS_GOOD) {
+                error_report("iSCSI: Failed to get_lba_status on iSCSI lun. %s",
+                             iscsi_get_error(iscsilun->iscsi));
+                ret = -EINVAL;
+                goto out;
+            }
+
+            lbas = scsi_datain_unmarshall(task);
+            if (lbas == NULL) {
+                error_report("iSCSI: failed to unmarshall inquiry datain blob");
+                ret = -EINVAL;
+                goto out;
+            }
+            lbasd = &lbas->descriptors[0];
+            if (lbasd->lba != lba) {
+                ret = -EINVAL;
+                goto out;
+            }
+            nb_sectors = lbasd->num_blocks;
+            provisioning = lbasd->provisioning;
+            scsi_free_scsi_task(task);
+            task = NULL;
+
+            /* blocks from lba to lba + nb_sectors - 1 are not mapped
+             * and read as zero (lbprz==1) so we can skip them */
+            if (provisioning != SCSI_PROVISIONING_TYPE_MAPPED) {
+                lba += nb_sectors;
+                continue;
+            }
+
+            uint64_t lba2 = lba + nb_sectors;
+            while (lba < lba2) {
+                struct unmap_list list[1];
+                list[0].lba = lba;
+                list[0].num = iscsilun->max_unmap;
+                if (lba + list[0].num > iscsilun->num_blocks) {
+                    list[0].num = iscsilun->num_blocks - lba;
+                }
+                task = iscsi_unmap_sync(iscsilun->iscsi,
+                                        iscsilun->lun,
+                                        0, 0, &list[0], 1);
+                if (task == NULL || task->status != SCSI_STATUS_GOOD) {
+                    error_report("iSCSI: Failed to unmap data on iSCSI lun. %s",
+                                 iscsi_get_error(iscsilun->iscsi));
+                    ret = -EINVAL;
+                    goto out;
+                }
+                scsi_free_scsi_task(task);
+                task = NULL;
+                lba += list[0].num;
+            }
+        }
+    }
+
 out:
+    if (task) {
+        scsi_free_scsi_task(task);
+    }
     if (iscsilun->iscsi != NULL) {
         iscsi_destroy_context(iscsilun->iscsi);
     }

Il 27/06/2013 15:11, Peter Lieven ha scritto:

Quoted 3 lines

if the device supports unmapping and unmapped blocks read as zero ensure that the whole device is unmapped and report .has_zero_init = 1 in this case to speed up qemu-img convert.

This can still take several minutes. Do you need any special timeout in libiscsi?

Perhaps we can have a new "discard_zeroes" field in bdrv_get_info, and the unmap functionality can be moved up to qemu-img convert?

Am 01.07.2013 um 22:20 schrieb Paolo Bonzini <[email protected]>:

Quoted 5 lines

Il 27/06/2013 15:11, Peter Lieven ha scritto:

Quoted 3 lines

if the device supports unmapping and unmapped blocks read as zero ensure that the whole device is unmapped and report .has_zero_init = 1 in this case to speed up qemu-img convert.

Quoted 2 lines

This can still take several minutes. Do you need any special timeout in libiscsi?

Not as far as I know. The number of unmapped sectors is bound by iscsilun->max_unmap. This is what the storage will handle in one request. For my storage its the internal page size (15MB). Its very fast. Except for some broken storages I expect that unmapping on a thin-provisioned target means deleting of pointers or something similar not actually writing something to the disks. This it what the SANITIZE command does.

Do you have evidence that it is really taking minutes somewhere?

I tested it with a fully allocated 1TB volume. In my case it was a question of about 20 seconds. I think this was mainly due to the thousands of sync requests that had to be sent.

Quoted 2 lines

Perhaps we can have a new "discard_zeroes" field in bdrv_get_info, and the unmap functionality can be moved up to qemu-img convert?

Is there any other storage protocol out there that could benefit from it?

Il 01/07/2013 23:36, Peter Lieven ha scritto:

Quoted 8 lines

Am 01.07.2013 um 22:20 schrieb Paolo Bonzini <[email protected]>:

Quoted 5 lines

Il 27/06/2013 15:11, Peter Lieven ha scritto:

Quoted 3 lines

if the device supports unmapping and unmapped blocks read as zero ensure that the whole device is unmapped and report .has_zero_init = 1 in this case to speed up qemu-img convert.

Quoted 8 lines

Quoted 2 lines

This can still take several minutes. Do you need any special timeout in libiscsi?

Not as far as I know. The number of unmapped sectors is bound by iscsilun->max_unmap. This is what the storage will handle in one request. For my storage its the internal page size (15MB). Its very fast. Except for some broken storages I expect that unmapping on a thin-provisioned target means deleting of pointers or something similar not actually writing something to the disks. This it what the SANITIZE command does.

Ok, I remember someone reporting timeouts when doing a discard with virtio-scsi. But maybe the problem there was that Linux parallelized the requests excessively after splitting them.

Quoted 5 lines

Do you have evidence that it is really taking minutes somewhere?

I tested it with a fully allocated 1TB volume. In my case it was a question of about 20 seconds. I think this was mainly due to the thousands of sync requests that had to be sent.

Quoted 4 lines

Quoted 2 lines

Perhaps we can have a new "discard_zeroes" field in bdrv_get_info, and the unmap functionality can be moved up to qemu-img convert?

Is there any other storage protocol out there that could benefit from it?

Definitely LVM. Perhaps in the future gluster too, though right now it only supports discard on files, not block devices.

Quoted 7 lines

Quoted 4 lines

Quoted 2 lines

Perhaps we can have a new "discard_zeroes" field in bdrv_get_info, and the unmap functionality can be moved up to qemu-img convert?

Is there any other storage protocol out there that could benefit from it?

Definitely LVM. Perhaps in the future gluster too, though right now it only supports discard on files, not block devices.

Is discards on LVM sth that is already implemented in qemu?

Would you mind if we postpone the general approach to a later point. It seems that this is much more complex than the iSCSI approach.

Il 02/07/2013 12:36, Peter Lieven ha scritto:

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

Quoted 2 lines

Perhaps we can have a new "discard_zeroes" field in bdrv_get_info, and the unmap functionality can be moved up to qemu-img convert?

Is there any other storage protocol out there that could benefit from it?

Quoted 4 lines

Quoted 2 lines

Definitely LVM. Perhaps in the future gluster too, though right now it only supports discard on files, not block devices.

Is discards on LVM sth that is already implemented in qemu?

Yes, it supports BLKDISCARD (see handle_aiocb_discard in block/raw-posix.c). Of course there is no way to query the host discard_zeroes setting yet.

But even if it weren't implemented in QEMU, you should aim at making it easier (if it's not too much work, which it isn't), not harder. If you do it in block/iscsi.c, the next person who comes will have to basically undo your work and reimplement+retest it with the right API.

Quoted 2 lines

Would you mind if we postpone the general approach to a later point. It seems that this is much more complex than the iSCSI approach.

It shouldn't be more complex at all, actually. You just need to pass the maximum unmap sectors and lbprz parameters through bdrv_get_info.

I'm not asking you to add support for BLKDISCARDZEROES and all that. I'm asking you to do the work at the right level.

Am 02.07.2013 um 12:49 schrieb Paolo Bonzini <[email protected]>:

Quoted 9 lines

Il 02/07/2013 12:36, Peter Lieven ha scritto:

Quoted 8 lines

Quoted 7 lines

Quoted 4 lines

Quoted 2 lines

Perhaps we can have a new "discard_zeroes" field in bdrv_get_info, and the unmap functionality can be moved up to qemu-img convert?

Is there any other storage protocol out there that could benefit from it?

Definitely LVM. Perhaps in the future gluster too, though right now it only supports discard on files, not block devices.

Quoted 5 lines

Quoted 1 line

Is discards on LVM sth that is already implemented in qemu?

Yes, it supports BLKDISCARD (see handle_aiocb_discard in block/raw-posix.c). Of course there is no way to query the host discard_zeroes setting yet.

No way in qemu or no way at all?

Il 02/07/2013 12:56, Peter Lieven ha scritto:

Quoted 14 lines

Am 02.07.2013 um 12:49 schrieb Paolo Bonzini <[email protected]>:

Quoted 11 lines

Il 02/07/2013 12:36, Peter Lieven ha scritto:

Quoted 9 lines

Quoted 7 lines

Quoted 4 lines

Quoted 2 lines

Perhaps we can have a new "discard_zeroes" field in bdrv_get_info, and the unmap functionality can be moved up to qemu-img convert?

Is there any other storage protocol out there that could benefit from it?

Definitely LVM. Perhaps in the future gluster too, though right now it only supports discard on files, not block devices.

Is discards on LVM sth that is already implemented in qemu?

Quoted 5 lines

Quoted 3 lines

Yes, it supports BLKDISCARD (see handle_aiocb_discard in block/raw-posix.c). Of course there is no way to query the host discard_zeroes setting yet.

No way in qemu or no way at all?

No way in QEMU. Linux has the BLKDISCARDZEROES ioctl, it takes an int* as the third argument and puts 0/1 in it.

if the device supports unmapping and unmapped blocks read as zero ensure that the whole device is unmapped and report .has_zero_init = 1 in this case to speed up qemu-img convert.

Signed-off-by Peter Lieven <[email protected]>

---
 block/iscsi.c |   72 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 71 insertions(+), 1 deletion(-)

block/iscsi.c +3 -1

index 92e66a6..621ca40 100644
@@ -1436,7 +1436,8 @@ static int iscsi_truncate(BlockDriverState *bs, int64_t offset)
 
 static int iscsi_has_zero_init(BlockDriverState *bs)
 {
-    return 0;
+    IscsiLun *iscsilun = bs->opaque;
+    return (iscsilun->lbpu && iscsilun->lbprz) ? 1 : 0;
 }
 
 static int iscsi_create(const char *filename, QEMUOptionParameter *options)

@@ -1446,6 +1447,7 @@ static int iscsi_create(const char *filename, QEMUOptionParameter *options)
     BlockDriverState bs;
     IscsiLun *iscsilun = NULL;
     QDict *bs_options;
+    struct scsi_task *task = NULL;
 
     memset(&bs, 0, sizeof(BlockDriverState));
 

Am 27.06.2013 um 15:11 hat Peter Lieven geschrieben:

Quoted 32 lines

if the device supports unmapping and unmapped blocks read as zero ensure that the whole device is unmapped and report .has_zero_init = 1 in this case to speed up qemu-img convert.

Signed-off-by Peter Lieven <[email protected]>

---
 block/iscsi.c |   72 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 71 insertions(+), 1 deletion(-)

block/iscsi.c +3 -1

index 92e66a6..621ca40 100644
@@ -1436,7 +1436,8 @@ static int iscsi_truncate(BlockDriverState *bs, int64_t offset)
 
 static int iscsi_has_zero_init(BlockDriverState *bs)
 {
-    return 0;
+    IscsiLun *iscsilun = bs->opaque;
+    return (iscsilun->lbpu && iscsilun->lbprz) ? 1 : 0;
 }
 
 static int iscsi_create(const char *filename, QEMUOptionParameter *options)

@@ -1446,6 +1447,7 @@ static int iscsi_create(const char *filename, QEMUOptionParameter *options)
     BlockDriverState bs;
     IscsiLun *iscsilun = NULL;
     QDict *bs_options;
+    struct scsi_task *task = NULL;
 
     memset(&bs, 0, sizeof(BlockDriverState));
 

block/iscsi.c static int iscsi_create(const char *filename, QEMUOptionParameter *options) · line 1485

Quoted 3 lines

block/iscsi.c +0 -0

@@ -1481,7 +1483,75 @@ static int iscsi_create(const char *filename, QEMUOptionParameter *options)
     }
 
     ret = 0;

Noticed something independent from your patch, a bit more context:

    if (bs.total_sectors < total_size) {
        ret = -ENOSPC;
    }

    ret = 0;

The -ENOSPC case can't ever trigger, there's a 'goto out' missing. Maybe you can include another patch too fix this in the next version of your series?

block-migration.c +1 -0

index 2fd7699..99b3757 100644
@@ -29,6 +29,7 @@
 #define BLK_MIG_FLAG_DEVICE_BLOCK       0x01
 #define BLK_MIG_FLAG_EOS                0x02
 #define BLK_MIG_FLAG_PROGRESS           0x04
+#define BLK_MIG_FLAG_ZERO_BLOCK         0x08
 
 #define MAX_IS_ALLOCATED_SEARCH 65536
 

@@ -114,16 +115,29 @@ static void blk_mig_unlock(void)
 static void blk_send(QEMUFile *f, BlkMigBlock * blk)
 {
     int len;

This patch breaks cross-version blog migration. We need to control whether or not to use the new BLK_MIG_FLAG_ZERO_BLOCK flag.

Quoted 17 lines

block-migration.c +1 -0

index 2fd7699..99b3757 100644
@@ -29,6 +29,7 @@
 #define BLK_MIG_FLAG_DEVICE_BLOCK       0x01
 #define BLK_MIG_FLAG_EOS                0x02
 #define BLK_MIG_FLAG_PROGRESS           0x04
+#define BLK_MIG_FLAG_ZERO_BLOCK         0x08
 
 #define MAX_IS_ALLOCATED_SEARCH 65536
 

@@ -114,16 +115,29 @@ static void blk_mig_unlock(void)
 static void blk_send(QEMUFile *f, BlkMigBlock * blk)
 {
    int len;

block-migration.c static void blk_mig_unlock(void) · line 126

Quoted 10 lines

block-migration.c +6 -1

+    int flags = BLK_MIG_FLAG_DEVICE_BLOCK;
+    
+    if (buffer_is_zero(blk->buf, BLOCK_SIZE)) {
+        flags |= BLK_MIG_FLAG_ZERO_BLOCK;
+    }
 
    /* sector number and flags */
    qemu_put_be64(f, (blk->sector << BDRV_SECTOR_BITS)
-                     | BLK_MIG_FLAG_DEVICE_BLOCK);
+                     | flags);

blk->sector is int64_t and flags is signed int. This conversion will sign-extend from 32-bit flags up to 64-bits.

Flags should be uint64_t or at least unsigned so that we don't hit sign-extension when BLK_MIG_FLAG_x uses the top bit.

Am 01.07.2013 um 16:13 schrieb Stefan Hajnoczi <[email protected]>:

Quoted 4 lines · Peter Lieven

This patch breaks cross-version blog migration. We need to control whether or not to use the new BLK_MIG_FLAG_ZERO_BLOCK flag.

you are right the upgrade way works, but downgrade not. what is the proposed way to fix this?

Quoted 10 lines

Am 01.07.2013 um 16:13 schrieb Stefan Hajnoczi <[email protected]>:

Quoted 4 lines · Peter Lieven

This patch breaks cross-version blog migration. We need to control whether or not to use the new BLK_MIG_FLAG_ZERO_BLOCK flag.

you are right the upgrade way works, but downgrade not. what is the proposed way to fix this?

The safest way is to add a block migration flag that toggles the zero block. By default it would be off.

Maybe you would also add a MigrationCapability in qapi-schema.json to indicated that zero blocks are supported in block migration. That would allow management tools to ask QEMU on both sides if they support zero blocks.

Use migrate-set-capabilities to enable the zero block feature.

This patch breaks cross-version blog migration. We need to control whether or not to use the new BLK_MIG_FLAG_ZERO_BLOCK flag.

Quoted 16 lines

block-migration.c +1 -0

index 2fd7699..99b3757 100644
@@ -29,6 +29,7 @@
 #define BLK_MIG_FLAG_DEVICE_BLOCK       0x01
 #define BLK_MIG_FLAG_EOS                0x02
 #define BLK_MIG_FLAG_PROGRESS           0x04
+#define BLK_MIG_FLAG_ZERO_BLOCK         0x08
 
 #define MAX_IS_ALLOCATED_SEARCH 65536
 

@@ -114,16 +115,29 @@ static void blk_mig_unlock(void)
 static void blk_send(QEMUFile *f, BlkMigBlock * blk)
 {
    int len;

block-migration.c static void blk_mig_unlock(void) · line 126

Quoted 10 lines

block-migration.c +6 -1

+    int flags = BLK_MIG_FLAG_DEVICE_BLOCK;
+    
+    if (buffer_is_zero(blk->buf, BLOCK_SIZE)) {
+        flags |= BLK_MIG_FLAG_ZERO_BLOCK;
+    }
 
    /* sector number and flags */
    qemu_put_be64(f, (blk->sector << BDRV_SECTOR_BITS)
-                     | BLK_MIG_FLAG_DEVICE_BLOCK);
+                     | flags);

blk->sector is int64_t and flags is signed int. This conversion will sign-extend from 32-bit flags up to 64-bits.

Flags should be uint64_t or at least unsigned so that we don't hit sign-extension when BLK_MIG_FLAG_x uses the top bit.

block-migration.c static void blk_mig_unlock(void) · line 139

Quoted 13 lines

block-migration.c +7 -0

    /* device name */
    len = strlen(blk->bmds->bs->device_name);
    qemu_put_byte(f, len);
    qemu_put_buffer(f, (uint8_t *)blk->bmds->bs->device_name, len);
 
+    /* if a block is zero we need to flush here since the network
+     * bandwidth is now a lot higher than the storage device bandwidth.
+     * thus if we queue zero blocks we slow down the migration */
+    if (flags & BLK_MIG_FLAG_ZERO_BLOCK) {
+        qemu_fflush(f);
+        return;
+    }

Not sure I understand this. Is the problem that the other side may require an slow writev() to fill zeros? So you want to tell the destination about the zeroes ASAP.

Am 01.07.2013 um 16:13 schrieb Stefan Hajnoczi <[email protected]>:

Quoted 15 lines · Peter Lieven

Quoted 13 lines

    /* device name */
    len = strlen(blk->bmds->bs->device_name);
    qemu_put_byte(f, len);
    qemu_put_buffer(f, (uint8_t *)blk->bmds->bs->device_name, len);

block-migration.c +7 -0

+    /* if a block is zero we need to flush here since the network
+     * bandwidth is now a lot higher than the storage device bandwidth.
+     * thus if we queue zero blocks we slow down the migration */
+    if (flags & BLK_MIG_FLAG_ZERO_BLOCK) {
+        qemu_fflush(f);
+        return;
+    }

Quoted 3 lines

Not sure I understand this. Is the problem that the other side may require an slow writev() to fill zeros? So you want to tell the destination about the zeroes ASAP.

Sorry, missed this question. Yes. If a lot of zero blocks is queued it delays migration because the target is idle while the source is queuing zero blocks.

Signed-off-by Peter Lieven <[email protected]>

---
 block/iscsi.c |   18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

Am 27.06.2013 um 15:11 hat Peter Lieven geschrieben:

Quoted 4 lines

Signed-off-by Peter Lieven <[email protected]>

---
 block/iscsi.c |   18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

A bit confusing that you add only one function (because the other one is already there), but convert both directions. Looks correct, though.

Reviewed-by Kevin Wolf <[email protected]>

if the target does not support UNMAP or the request is too big silently ignore the discard request.

Signed-off-by Peter Lieven <[email protected]>

Am 27.06.2013 um 15:11 hat Peter Lieven geschrieben:

Quoted 4 lines

if the target does not support UNMAP or the request is too big silently ignore the discard request.

Signed-off-by Peter Lieven <[email protected]>

Why not loop for the "too big" case? You can probably use the same logic for unmapping the whole device in .bdrv_create and here.

Kevin

Am 10.07.2013 13:33, schrieb Kevin Wolf:

Quoted 7 lines

Am 27.06.2013 um 15:11 hat Peter Lieven geschrieben:

Quoted 4 lines

if the target does not support UNMAP or the request is too big silently ignore the discard request.

Signed-off-by Peter Lieven <[email protected]>

Why not loop for the "too big" case? You can probably use the same logic for unmapping the whole device in .bdrv_create and here.

right, but looping in an aio function seemed not so trivial to me. it seems more and more obvious to me that the best would be to change all the remaining aio routines to co routines.

in this case i could add the too big logic in iscsi_co_discard and simply call it from iscsi_co_write_zeroes.

Am 10.07.2013 um 16:04 hat Peter Lieven geschrieben:

Quoted 11 lines

Am 10.07.2013 13:33, schrieb Kevin Wolf:

Quoted 7 lines

Am 27.06.2013 um 15:11 hat Peter Lieven geschrieben:

Quoted 4 lines

if the target does not support UNMAP or the request is too big silently ignore the discard request.

Signed-off-by Peter Lieven <[email protected]>

Why not loop for the "too big" case? You can probably use the same logic for unmapping the whole device in .bdrv_create and here.

right, but looping in an aio function seemed not so trivial to me. it seems more and more obvious to me that the best would be to change all the remaining aio routines to co routines.

The pattern for AIO functions is that the real work of submitting requests is done in the AIO callback, and it submits new AIO requests calling back into the same callback as long as acb->remaining_secs > 0 (or something like that).

You can still see that kind of thing alive in qed_aio_next_io(), (most of?) the rest is converted to coroutines because it makes the code look nicer.

Quoted 3 lines

in this case i could add the too big logic in iscsi_co_discard and simply call it from iscsi_co_write_zeroes.

I think that would be the nicest solution.

Am 10.07.2013 16:28, schrieb Kevin Wolf:

Quoted 12 lines

Am 10.07.2013 um 16:04 hat Peter Lieven geschrieben:

Quoted 11 lines

Am 10.07.2013 13:33, schrieb Kevin Wolf:

Quoted 7 lines

Am 27.06.2013 um 15:11 hat Peter Lieven geschrieben:

Quoted 4 lines

if the target does not support UNMAP or the request is too big silently ignore the discard request.

Signed-off-by Peter Lieven <[email protected]>

Why not loop for the "too big" case? You can probably use the same logic for unmapping the whole device in .bdrv_create and here.

right, but looping in an aio function seemed not so trivial to me. it seems more and more obvious to me that the best would be to change all the remaining aio routines to co routines.

Quoted 8 lines

The pattern for AIO functions is that the real work of submitting requests is done in the AIO callback, and it submits new AIO requests calling back into the same callback as long as acb->remaining_secs > 0 (or something like that).

You can still see that kind of thing alive in qed_aio_next_io(), (most of?) the rest is converted to coroutines because it makes the code look nicer.

would you agree if I leave the easy version in just to fix the potential problems if iscsi_aio_discard is called with too high nb_sectors or on a storage where UNMAP is unsupported.

I will add a TODO with the comment that the limit of iscsi->max_unmap should be replaced by a loop once the routine is replaced by a coroutine?

Quoted 3 lines

Quoted 2 lines

in this case i could add the too big logic in iscsi_co_discard and simply call it from iscsi_co_write_zeroes.

I think that would be the nicest solution.

I promised to take care of this for 1.7.0 latest.

Am 10.07.2013 um 16:49 hat Peter Lieven geschrieben:

Quoted 25 lines

Am 10.07.2013 16:28, schrieb Kevin Wolf:

Quoted 12 lines

Am 10.07.2013 um 16:04 hat Peter Lieven geschrieben:

Quoted 11 lines

Am 10.07.2013 13:33, schrieb Kevin Wolf:

Quoted 7 lines

Am 27.06.2013 um 15:11 hat Peter Lieven geschrieben:

Quoted 4 lines

if the target does not support UNMAP or the request is too big silently ignore the discard request.

Signed-off-by Peter Lieven <[email protected]>

Why not loop for the "too big" case? You can probably use the same logic for unmapping the whole device in .bdrv_create and here.

right, but looping in an aio function seemed not so trivial to me. it seems more and more obvious to me that the best would be to change all the remaining aio routines to co routines.

Quoted 8 lines

The pattern for AIO functions is that the real work of submitting requests is done in the AIO callback, and it submits new AIO requests calling back into the same callback as long as acb->remaining_secs > 0 (or something like that).

You can still see that kind of thing alive in qed_aio_next_io(), (most of?) the rest is converted to coroutines because it makes the code look nicer.

would you agree if I leave the easy version in just to fix the potential problems if iscsi_aio_discard is called with too high nb_sectors or on a storage where UNMAP is unsupported.

Quoted 2 lines

I will add a TODO with the comment that the limit of iscsi->max_unmap should be replaced by a loop once the routine is replaced by a coroutine?

Meh, another pony I don't get... ;-)

Leaving a TODO comment for now is okay with me.

if the blocksize of an iSCSI LUN is bigger than the BDRV_SECTOR_SIZE it is possible that sector_num or nb_sectors are not correctly alligned.

to avoid corruption we fail requests which are misaligned.

Signed-off-by Peter Lieven <[email protected]>

---
 block/iscsi.c |   27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

block/iscsi.c +0 -0

index 0567b46..bff2e1f 100644

Quoted 19 lines

if the blocksize of an iSCSI LUN is bigger than the BDRV_SECTOR_SIZE it is possible that sector_num or nb_sectors are not correctly alligned.

to avoid corruption we fail requests which are misaligned.

Signed-off-by Peter Lieven <[email protected]>

---
block/iscsi.c | 27 +++++++++++++++++++++++++++

1 file changed, 27 insertions(+)

block/iscsi.c +0 -0

index 0567b46..bff2e1f 100644
@@ -298,6 +298,13 @@ static int64_t sector_lun2qemu(int64_t sector, IscsiLun *iscsilun)
    return sector * iscsilun->block_size / BDRV_SECTOR_SIZE;
 }
 

block/iscsi.c static int64_t sector_lun2qemu(int64_t sector, IscsiLun *iscsilun) · line 306

Quoted 6 lines

block/iscsi.c +6 -0

+static int64_t is_request_lun_aligned(int64_t sector_num, int nb_sectors,
+                                      IscsiLun *iscsilun)
+{
+    return ((sector_num * BDRV_SECTOR_SIZE) % iscsilun->block_size ||
+            (nb_sectors * BDRV_SECTOR_SIZE) % iscsilun->block_size) ? 0 : 1;
+}

When QEMU does geometry probing it reads 2KB. So if the LUN has 4KB block size then QEMU will fail to open it? This would also affect image formats on top of iSCSI LUNs.

AFAICT we have no way to passing I/O topology information up from the block driver.

Am 01.07.2013 um 16:35 schrieb Stefan Hajnoczi <[email protected]>:

Quoted 20 lines · Peter Lieven

Quoted 19 lines

if the blocksize of an iSCSI LUN is bigger than the BDRV_SECTOR_SIZE it is possible that sector_num or nb_sectors are not correctly alligned.

to avoid corruption we fail requests which are misaligned.

Signed-off-by Peter Lieven <[email protected]>

---
block/iscsi.c | 27 +++++++++++++++++++++++++++

1 file changed, 27 insertions(+)

block/iscsi.c +0 -0

index 0567b46..bff2e1f 100644
@@ -298,6 +298,13 @@ static int64_t sector_lun2qemu(int64_t sector, IscsiLun *iscsilun)
    return sector * iscsilun->block_size / BDRV_SECTOR_SIZE;
 }
 

Quoted 10 lines

block/iscsi.c static int64_t sector_lun2qemu(int64_t sector, IscsiLun *iscsilun) · line 306

Quoted 6 lines

block/iscsi.c +6 -0

+static int64_t is_request_lun_aligned(int64_t sector_num, int nb_sectors,
+                                      IscsiLun *iscsilun)
+{
+    return ((sector_num * BDRV_SECTOR_SIZE) % iscsilun->block_size ||
+            (nb_sectors * BDRV_SECTOR_SIZE) % iscsilun->block_size) ? 0 : 1;
+}

When QEMU does geometry probing it reads 2KB. So if the LUN has 4KB block size then QEMU will fail to open it? This would also affect image formats on top of iSCSI LUNs.

opening a 4K LUN does not fail with my target. So writing unaligned sectors will result in corruption. we should at least fail all those unaligned operations until we have a fix or workaround in place in place.

Quoted 2 lines

AFAICT we have no way to passing I/O topology information up from the block driver.

not at the moment. Paolo had a patch series back in Dec 2011, but it never went upstream. I asked him off list and he told me that 4K drives where not important enough and the Redhat bug for this was closed. Now with 4K iSCSI targets these old work could become important again.

It would be nice to find out if there was anything wrong with them or what has to be done to get them integrated.

Quoted 34 lines

Am 01.07.2013 um 16:35 schrieb Stefan Hajnoczi <[email protected]>:

Quoted 30 lines · Peter Lieven

Quoted 16 lines

if the blocksize of an iSCSI LUN is bigger than the BDRV_SECTOR_SIZE it is possible that sector_num or nb_sectors are not correctly alligned.

to avoid corruption we fail requests which are misaligned.

Signed-off-by Peter Lieven <[email protected]>

---
block/iscsi.c | 27 +++++++++++++++++++++++++++

1 file changed, 27 insertions(+)

block/iscsi.c +0 -0

index 0567b46..bff2e1f 100644

block/iscsi.c static int64_t sector_lun2qemu(int64_t sector, IscsiLun *iscsilun) · line 306

Quoted 9 lines

block/iscsi.c +6 -0

@@ -298,6 +298,13 @@ static int64_t sector_lun2qemu(int64_t sector, IscsiLun *iscsilun)
   return sector * iscsilun->block_size / BDRV_SECTOR_SIZE;
 }
 
+static int64_t is_request_lun_aligned(int64_t sector_num, int nb_sectors,
+                                      IscsiLun *iscsilun)
+{
+    return ((sector_num * BDRV_SECTOR_SIZE) % iscsilun->block_size ||
+            (nb_sectors * BDRV_SECTOR_SIZE) % iscsilun->block_size) ? 0 : 1;
+}

When QEMU does geometry probing it reads 2KB. So if the LUN has 4KB block size then QEMU will fail to open it? This would also affect image formats on top of iSCSI LUNs.

Quoted 3 lines

opening a 4K LUN does not fail with my target. So writing unaligned sectors will result in corruption. we should at least fail all those unaligned operations until we have a fix or workaround in place in place.

When QEMU tries to probe the master boot record or when an image format performs an unaligned read (say 512 bytes instead of 4 KB), won't we fail the read request? Therefore opening will fail.

I agree that it's better to emit an error than to corrupt the disk. I'm just trying to understand what needs to be done on top of this to make 4 KB LUNs work.

Quoted 3 lines

I agree that it's better to emit an error than to corrupt the disk. I'm just trying to understand what needs to be done on top of this to make 4 KB LUNs work.

Reading is quite easy, you just have to have a wrapper that is reading an aligned portion of sectors around the original request and then extracting what was requested. Writing is very complicated. Requests have to be serialized, Read-Modify-Write for unaligned write requests. Paolo had all this prepared already.

I wonder if it would be enough to have the block size of the host/iSCSI device propagated to the guest drivers and read 4K (or the protocol block size bytes) when probing the MBR.

Il 02/07/2013 10:28, Peter Lieven ha scritto:

Quoted 11 lines

Quoted 3 lines

I agree that it's better to emit an error than to corrupt the disk. I'm just trying to understand what needs to be done on top of this to make 4 KB LUNs work.

Reading is quite easy, you just have to have a wrapper that is reading an aligned portion of sectors around the original request and then extracting what was requested. Writing is very complicated. Requests have to be serialized, Read-Modify-Write for unaligned write requests. Paolo had all this prepared already.

I wonder if it would be enough to have the block size of the host/iSCSI device propagated to the guest drivers and read 4K (or the protocol block size bytes) when probing the MBR.

No, propagating the size of the device is only correct if we're doing SCSI passthrough (-device scsi-block) and in that case we're doing it already.

For emulation, the choices are:

1) manually set the logical_block_size of the disk to 4096 (which means the disk cannot be used as a boot disk, at least with BIOS; I don't know about UEFI);

2) do 512e/4KN as in my patches. You would still need to expose a 4K physical_sector_size to be precise, but it is just an optimization.

Am 02.07.2013 um 12:44 schrieb Paolo Bonzini <[email protected]>:

Quoted 23 lines

Il 02/07/2013 10:28, Peter Lieven ha scritto:

Quoted 11 lines

Quoted 3 lines

I agree that it's better to emit an error than to corrupt the disk. I'm just trying to understand what needs to be done on top of this to make 4 KB LUNs work.

Reading is quite easy, you just have to have a wrapper that is reading an aligned portion of sectors around the original request and then extracting what was requested. Writing is very complicated. Requests have to be serialized, Read-Modify-Write for unaligned write requests. Paolo had all this prepared already.

I wonder if it would be enough to have the block size of the host/iSCSI device propagated to the guest drivers and read 4K (or the protocol block size bytes) when probing the MBR.

No, propagating the size of the device is only correct if we're doing SCSI passthrough (-device scsi-block) and in that case we're doing it already.

For emulation, the choices are:

1) manually set the logical_block_size of the disk to 4096 (which means the disk cannot be used as a boot disk, at least with BIOS; I don't know about UEFI);

Quoted 2 lines

2) do 512e/4KN as in my patches. You would still need to expose a 4K physical_sector_size to be precise, but it is just an optimization.

This seems the more reasonable for me. Can you check what needs to be done to get your last version of your old series ready for upstream?

Is the exposing of a sector size > 512 byte complicated or is this something that is already possible with IDE, Virtio-BLK, Virtio-SCSI etc.?

Peter

if the blocksize of an iSCSI LUN is bigger than the BDRV_SECTOR_SIZE it is possible that sector_num or nb_sectors are not correctly alligned.

to avoid corruption we fail requests which are misaligned.

Signed-off-by Peter Lieven <[email protected]>

---
 block/iscsi.c |   27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

block/iscsi.c +0 -0

index 0567b46..bff2e1f 100644

Am 27.06.2013 um 15:11 hat Peter Lieven geschrieben:

Quoted 19 lines

if the blocksize of an iSCSI LUN is bigger than the BDRV_SECTOR_SIZE it is possible that sector_num or nb_sectors are not correctly alligned.

to avoid corruption we fail requests which are misaligned.

Signed-off-by Peter Lieven <[email protected]>

---
 block/iscsi.c |   27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

block/iscsi.c +0 -0

index 0567b46..bff2e1f 100644
@@ -298,6 +298,13 @@ static int64_t sector_lun2qemu(int64_t sector, IscsiLun *iscsilun)
     return sector * iscsilun->block_size / BDRV_SECTOR_SIZE;
 }
 

block/iscsi.c static int64_t sector_lun2qemu(int64_t sector, IscsiLun *iscsilun) · line 302

Quoted 2 lines

block/iscsi.c +2 -0

+static int64_t is_request_lun_aligned(int64_t sector_num, int nb_sectors,
+                                      IscsiLun *iscsilun)

This should certainly return bool instead of int64_t?

block/iscsi.c static int64_t sector_lun2qemu(int64_t sector, IscsiLun *iscsilun) · line 305

Quoted 4 lines

block/iscsi.c +3 -0

+{
+    return ((sector_num * BDRV_SECTOR_SIZE) % iscsilun->block_size ||
+            (nb_sectors * BDRV_SECTOR_SIZE) % iscsilun->block_size) ? 0 : 1;

'x ? 0 : 1' is usually written '!x'.

Kevin