iscsi/qemu-img/block-migration enhancements

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

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

block/iscsi.c +40 -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;
     }
 
+    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));
+            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");
+            ret = -EINVAL;

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

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

block/iscsi.c +101 -0

index a38a1bf..98be71e 100644
@@ -56,6 +56,15 @@ typedef struct IscsiLun {
     uint8_t lbpws10;
 } IscsiLun;
 
+typedef struct IscsiTask {
+    IscsiLun *iscsilun;
+    BlockDriverState *bs;
+    int status;
+    int complete;
+    Coroutine *co;
+    struct scsi_lba_status_descriptor lbasd;
+} IscsiTask;
+
 typedef struct IscsiAIOCB {
     BlockDriverAIOCB common;
     QEMUIOVector *qiov;

@@ -805,6 +814,98 @@ iscsi_getlength(BlockDriverState *bs)
     return len;
 }
 
+static void
+iscsi_co_is_allocated_cb(struct iscsi_context *iscsi, int status,
+                        void *command_data, void *opaque)
+{
+    struct IscsiTask *iTask = opaque;
+    struct scsi_task *task = command_data;
+    struct scsi_get_lba_status *lbas = NULL;
+
+    iTask->complete = 1;
+
+    if (status != 0) {
+        error_report("iSCSI: Failed to get_lba_status on iSCSI lun. %s",
+                     iscsi_get_error(iscsi));
+        iTask->status   = 1;
+        goto out;
+    }
+
+    lbas = scsi_datain_unmarshall(task);
+    if (lbas == NULL) {
+        iTask->status   = 1;
+        goto out;
+    }
+
+    memcpy(&iTask->lbasd, &lbas->descriptors[0],
+           sizeof(struct scsi_lba_status_descriptor));
+
+    iTask->status   = 0;
+
+out:
+    scsi_free_scsi_task(task);
+
+    if (iTask->co) {
+        qemu_coroutine_enter(iTask->co, NULL);
+    }
+}
+
+static int coroutine_fn iscsi_co_is_allocated(BlockDriverState *bs,
+                                              int64_t sector_num,
+                                              int nb_sectors, int *pnum)
+{
+    IscsiLun *iscsilun = bs->opaque;
+    struct IscsiTask iTask;
+    int ret;
+
+    *pnum = nb_sectors;
+
+    if (iscsilun->lbpme == 0) {
+        return 1;
+    }
+
+    iTask.iscsilun = iscsilun;
+    iTask.status = 0;
+    iTask.complete = 0;
+    iTask.bs = bs;
+
+    if (iscsi_get_lba_status_task(iscsilun->iscsi, iscsilun->lun,
+                                  sector_qemu2lun(sector_num, iscsilun),
+                                  8 + 16, iscsi_co_is_allocated_cb,
+                                  &iTask) == NULL) {
+        *pnum = 0;
+        return 0;
+    }
+
+    while (!iTask.complete) {
+        iscsi_set_events(iscsilun);
+        iTask.co = qemu_coroutine_self();
+        qemu_coroutine_yield();
+    }
+
+    if (iTask.status != 0) {
+        /* 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 */
+        return 1;
+    }
+
+    if (sector_qemu2lun(sector_num, iscsilun) != iTask.lbasd.lba) {
+        *pnum = 0;
+        return 0;
+    }
+
+    *pnum = iTask.lbasd.num_blocks * (iscsilun->block_size / BDRV_SECTOR_SIZE);
+    if (*pnum > nb_sectors) {
+        *pnum = nb_sectors;
+    }
+
+    return (iTask.lbasd.provisioning == SCSI_PROVISIONING_TYPE_MAPPED) ? 1 : 0;
+
+    return ret;
+}
+
 static int parse_chap(struct iscsi_context *iscsi, const char *target)
 {
     QemuOptsList *list;

write zeroes is emulated by unmap if the
target supports unmapping an unmapped blocks
read as zero.

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

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

block/iscsi.c +63 -0

index 98be71e..683692a 100644
@@ -906,6 +906,68 @@ static int coroutine_fn iscsi_co_is_allocated(BlockDriverState *bs,
     return ret;
 }
 
+static void
+iscsi_co_write_zeroes_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   = 0;
+
+    if (status != 0) {
+        error_report("iSCSI: Failed to unmap data on iSCSI lun. %s",
+                     iscsi_get_error(iscsi));
+        iTask->status   = 1;
+    }
+
+    scsi_free_scsi_task(task);
+
+    if (iTask->co) {
+        qemu_coroutine_enter(iTask->co, NULL);
+    }
+}
+
+static int
+coroutine_fn iscsi_co_write_zeroes(BlockDriverState *bs, int64_t sector_num,
+                                   int nb_sectors)
+{
+    IscsiLun *iscsilun = bs->opaque;
+    struct IscsiTask iTask;
+    struct unmap_list list[1];
+
+    if (!iscsilun->lbprz || !iscsilun->lbpu) {
+        /* fall back to writev */
+        return -ENOTSUP;
+    }
+
+    iTask.iscsilun = iscsilun;
+    iTask.status = 0;
+    iTask.complete = 0;
+    iTask.bs = bs;
+
+    list[0].lba = sector_qemu2lun(sector_num, iscsilun);
+    list[0].num = nb_sectors * BDRV_SECTOR_SIZE / iscsilun->block_size;
+
+    if (iscsi_unmap_task(iscsilun->iscsi, iscsilun->lun, 0, 0, &list[0], 1,
+                         iscsi_co_write_zeroes_cb, &iTask) == NULL) {
+        return -EIO;
+    }
+
+    while (!iTask.complete) {
+        iscsi_set_events(iscsilun);
+        iTask.co = qemu_coroutine_self();
+        qemu_coroutine_yield();
+    }
+
+    if (iTask.status != 0) {
+        return -EIO;
+    }
+
+    return 0;
+}
+
 static int parse_chap(struct iscsi_context *iscsi, const char *target)
 {
     QemuOptsList *list;

@@ -1417,6 +1479,7 @@ static BlockDriver bdrv_iscsi = {
     .bdrv_truncate   = iscsi_truncate,
 
     .bdrv_co_is_allocated = iscsi_co_is_allocated,
+    .bdrv_co_write_zeroes = iscsi_co_write_zeroes,
 
     .bdrv_aio_readv  = iscsi_aio_readv,
     .bdrv_aio_writev = iscsi_aio_writev,

Il 22/06/2013 22:58, Peter Lieven ha scritto:

Quoted 3 lines

write zeroes is emulated by unmap if the
target supports unmapping an unmapped blocks
read as zero.

I think you should check BDRV_O_UNMAP in bs->open_flags, and not do this optimization if it is not set.

Paolo

Quoted 87 lines

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

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

block/iscsi.c +63 -0

index 98be71e..683692a 100644
@@ -906,6 +906,68 @@ static int coroutine_fn iscsi_co_is_allocated(BlockDriverState *bs,
     return ret;
 }
 
+static void
+iscsi_co_write_zeroes_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   = 0;
+
+    if (status != 0) {
+        error_report("iSCSI: Failed to unmap data on iSCSI lun. %s",
+                     iscsi_get_error(iscsi));
+        iTask->status   = 1;
+    }
+
+    scsi_free_scsi_task(task);
+
+    if (iTask->co) {
+        qemu_coroutine_enter(iTask->co, NULL);
+    }
+}
+
+static int
+coroutine_fn iscsi_co_write_zeroes(BlockDriverState *bs, int64_t sector_num,
+                                   int nb_sectors)
+{
+    IscsiLun *iscsilun = bs->opaque;
+    struct IscsiTask iTask;
+    struct unmap_list list[1];
+
+    if (!iscsilun->lbprz || !iscsilun->lbpu) {
+        /* fall back to writev */
+        return -ENOTSUP;
+    }
+
+    iTask.iscsilun = iscsilun;
+    iTask.status = 0;
+    iTask.complete = 0;
+    iTask.bs = bs;
+
+    list[0].lba = sector_qemu2lun(sector_num, iscsilun);
+    list[0].num = nb_sectors * BDRV_SECTOR_SIZE / iscsilun->block_size;
+
+    if (iscsi_unmap_task(iscsilun->iscsi, iscsilun->lun, 0, 0, &list[0], 1,
+                         iscsi_co_write_zeroes_cb, &iTask) == NULL) {
+        return -EIO;
+    }
+
+    while (!iTask.complete) {
+        iscsi_set_events(iscsilun);
+        iTask.co = qemu_coroutine_self();
+        qemu_coroutine_yield();
+    }
+
+    if (iTask.status != 0) {
+        return -EIO;
+    }
+
+    return 0;
+}
+
 static int parse_chap(struct iscsi_context *iscsi, const char *target)
 {
     QemuOptsList *list;

@@ -1417,6 +1479,7 @@ static BlockDriver bdrv_iscsi = {
     .bdrv_truncate   = iscsi_truncate,
 
     .bdrv_co_is_allocated = iscsi_co_is_allocated,
+    .bdrv_co_write_zeroes = iscsi_co_write_zeroes,
 
     .bdrv_aio_readv  = iscsi_aio_readv,
     .bdrv_aio_writev = iscsi_aio_writev,

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

---
 qemu-img.c |   10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

qemu-img.c +0 -0

index 809b4f1..5aa53ab 100644
@@ -1513,9 +1513,13 @@ static int img_convert(int argc, char **argv)
                    If the output is to a host device, we also write out
                    sectors that are entirely 0, since whatever data was

qemu-img.c +1 -0

+                    }
                     if (ret < 0) {
                         error_report("error while writing sector %" PRId64
                                      ": %s", sector_num, strerror(-ret));

Il 22/06/2013 22:58, Peter Lieven ha scritto:

Quoted 15 lines

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

---
 qemu-img.c |   10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

qemu-img.c +0 -2

index 809b4f1..5aa53ab 100644
@@ -1513,9 +1513,13 @@ static int img_convert(int argc, char **argv)
                    If the output is to a host device, we also write out
                    sectors that are entirely 0, since whatever data was
                    already there is garbage, not 0s. */
-                if (!has_zero_init || out_baseimg ||
-                    is_allocated_sectors_min(buf1, n, &n1, min_sparse)) {

qemu-img.c static int img_convert(int argc, char **argv) · line 1521

Quoted 7 lines

qemu-img.c +6 -1

-                    ret = bdrv_write(out_bs, sector_num, buf1, n1);
+                int allocated = is_allocated_sectors_min(buf1, n, &n1, min_sparse);
+                if (!has_zero_init || out_baseimg || allocated) {
+                    if (allocated || out_baseimg) {
+                        ret = bdrv_write(out_bs, sector_num, buf1, n1);
+                    } else {
+                        ret = bdrv_write_zeroes(out_bs, sector_num, n1);

I think it should still do the write only if !has_zero_init.

Paolo

Quoted 5 lines

qemu-img.c +1 -0

+                    }
                     if (ret < 0) {
                         error_report("error while writing sector %" PRId64
                                      ": %s", sector_num, strerror(-ret));

Am 24.06.2013 16:33, schrieb Paolo Bonzini:

Quoted 16 lines

Il 22/06/2013 22:58, Peter Lieven ha scritto:

Quoted 15 lines

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

---
 qemu-img.c |   10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

qemu-img.c +0 -2

index 809b4f1..5aa53ab 100644
@@ -1513,9 +1513,13 @@ static int img_convert(int argc, char **argv)
                    If the output is to a host device, we also write out
                    sectors that are entirely 0, since whatever data was
                    already there is garbage, not 0s. */
-                if (!has_zero_init || out_baseimg ||
-                    is_allocated_sectors_min(buf1, n, &n1, min_sparse)) {

Quoted 8 lines

qemu-img.c static int img_convert(int argc, char **argv) · line 1521

Quoted 7 lines

qemu-img.c +6 -1

-                    ret = bdrv_write(out_bs, sector_num, buf1, n1);
+                int allocated = is_allocated_sectors_min(buf1, n, &n1, min_sparse);
+                if (!has_zero_init || out_baseimg || allocated) {
+                    if (allocated || out_baseimg) {
+                        ret = bdrv_write(out_bs, sector_num, buf1, n1);
+                    } else {
+                        ret = bdrv_write_zeroes(out_bs, sector_num, n1);

I think it should still do the write only if !has_zero_init.

With this I am still in trouble with iSCSI, but Kevin pointed out another possible solution for the allocating everything problem.

a) let iscsi_create discard the whole device if lbpz && lbprz
b) return 1 for has_zero_init if lbprz.

What do you think?

Il 24/06/2013 18:17, Peter Lieven ha scritto:

Quoted 25 lines

Am 24.06.2013 16:33, schrieb Paolo Bonzini:

Quoted 24 lines

Il 22/06/2013 22:58, Peter Lieven ha scritto:

Quoted 12 lines

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

---
 qemu-img.c |   10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

qemu-img.c +0 -0

index 809b4f1..5aa53ab 100644
@@ -1513,9 +1513,13 @@ static int img_convert(int argc, char **argv)
                    If the output is to a host device, we also write out
                    sectors that are entirely 0, since whatever data was

qemu-img.c static int img_convert(int argc, char **argv) · line 1521

Quoted 10 lines

qemu-img.c +6 -3

                    already there is garbage, not 0s. */
-                if (!has_zero_init || out_baseimg ||
-                    is_allocated_sectors_min(buf1, n, &n1, min_sparse)) {
-                    ret = bdrv_write(out_bs, sector_num, buf1, n1);
+                int allocated = is_allocated_sectors_min(buf1, n, &n1, min_sparse);
+                if (!has_zero_init || out_baseimg || allocated) {
+                    if (allocated || out_baseimg) {
+                        ret = bdrv_write(out_bs, sector_num, buf1, n1);
+                    } else {
+                        ret = bdrv_write_zeroes(out_bs, sector_num, n1);

I think it should still do the write only if !has_zero_init.

Quoted 7 lines

With this I am still in trouble with iSCSI, but Kevin pointed out another possible solution for the allocating everything problem.

a) let iscsi_create discard the whole device if lbpz && lbprz
b) return 1 for has_zero_init if lbprz.

What do you think?

Fine by me (but the discard may take a looong time! :)).

Paolo

Il 24/06/2013 18:17, Peter Lieven ha scritto:

Quoted 25 lines

Am 24.06.2013 16:33, schrieb Paolo Bonzini:

Quoted 24 lines

Il 22/06/2013 22:58, Peter Lieven ha scritto:

Quoted 12 lines

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

---
 qemu-img.c |   10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

qemu-img.c +0 -0

index 809b4f1..5aa53ab 100644
@@ -1513,9 +1513,13 @@ static int img_convert(int argc, char **argv)
                    If the output is to a host device, we also write out
                    sectors that are entirely 0, since whatever data was

qemu-img.c static int img_convert(int argc, char **argv) · line 1521

Quoted 10 lines

qemu-img.c +6 -3

                    already there is garbage, not 0s. */
-                if (!has_zero_init || out_baseimg ||
-                    is_allocated_sectors_min(buf1, n, &n1, min_sparse)) {
-                    ret = bdrv_write(out_bs, sector_num, buf1, n1);
+                int allocated = is_allocated_sectors_min(buf1, n, &n1, min_sparse);
+                if (!has_zero_init || out_baseimg || allocated) {
+                    if (allocated || out_baseimg) {
+                        ret = bdrv_write(out_bs, sector_num, buf1, n1);
+                    } else {
+                        ret = bdrv_write_zeroes(out_bs, sector_num, n1);

I think it should still do the write only if !has_zero_init.

Quoted 7 lines

With this I am still in trouble with iSCSI, but Kevin pointed out another possible solution for the allocating everything problem.

a) let iscsi_create discard the whole device if lbpz && lbprz
b) return 1 for has_zero_init if lbprz.

What do you think?

Fine by me (but the discard may take a looong time! :)).

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.

for now assert that there is no misalignment to avoid data corruption.

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

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

block/iscsi.c +7 -0

index 683692a..2c410b1 100644
@@ -249,6 +249,7 @@ iscsi_aio_write16_cb(struct iscsi_context *iscsi, int status,
 
 static int64_t sector_qemu2lun(int64_t sector, IscsiLun *iscsilun)
 {
+    assert((sector * BDRV_SECTOR_SIZE) % iscsilun->block_size == 0);
     return sector * BDRV_SECTOR_SIZE / iscsilun->block_size;
 }
 

@@ -269,6 +270,8 @@ iscsi_aio_writev_acb(IscsiAIOCB *acb)
     acb->status     = -EINPROGRESS;
     acb->buf        = NULL;
 
+    assert((acb->nb_sectors * BDRV_SECTOR_SIZE) % acb->iscsilun->block_size == 0);
+
     /* this will allow us to get rid of 'buf' completely */
     size = acb->nb_sectors * BDRV_SECTOR_SIZE;
 

@@ -618,6 +621,8 @@ static int iscsi_aio_discard_acb(IscsiAIOCB *acb) {
     acb->buf        = NULL;
 
     list[0].lba = sector_qemu2lun(acb->sector_num, acb->iscsilun);
+
+    assert((acb->nb_sectors * BDRV_SECTOR_SIZE) % acb->iscsilun->block_size == 0);
     list[0].num = acb->nb_sectors * BDRV_SECTOR_SIZE / acb->iscsilun->block_size;
 
     acb->task = iscsi_unmap_task(iscsi, acb->iscsilun->lun,

@@ -947,6 +952,8 @@ coroutine_fn iscsi_co_write_zeroes(BlockDriverState *bs, int64_t sector_num,
     iTask.complete = 0;
     iTask.bs = bs;
 
+    assert((nb_sectors * BDRV_SECTOR_SIZE) % iscsilun->block_size == 0);
+
     list[0].lba = sector_qemu2lun(sector_num, iscsilun);
     list[0].num = nb_sectors * BDRV_SECTOR_SIZE / iscsilun->block_size;
 

Il 22/06/2013 22:58, Peter Lieven ha scritto:

Quoted 6 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.

for now assert that there is no misalignment to avoid data corruption.

You should just fail to open the LUN instead.

Paolo

Quoted 45 lines

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

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

block/iscsi.c +7 -0

index 683692a..2c410b1 100644
@@ -249,6 +249,7 @@ iscsi_aio_write16_cb(struct iscsi_context *iscsi, int status,
 
 static int64_t sector_qemu2lun(int64_t sector, IscsiLun *iscsilun)
 {
+    assert((sector * BDRV_SECTOR_SIZE) % iscsilun->block_size == 0);
     return sector * BDRV_SECTOR_SIZE / iscsilun->block_size;
 }
 

@@ -269,6 +270,8 @@ iscsi_aio_writev_acb(IscsiAIOCB *acb)
     acb->status     = -EINPROGRESS;
     acb->buf        = NULL;
 
+    assert((acb->nb_sectors * BDRV_SECTOR_SIZE) % acb->iscsilun->block_size == 0);
+
     /* this will allow us to get rid of 'buf' completely */
     size = acb->nb_sectors * BDRV_SECTOR_SIZE;
 

@@ -618,6 +621,8 @@ static int iscsi_aio_discard_acb(IscsiAIOCB *acb) {
     acb->buf        = NULL;
 
     list[0].lba = sector_qemu2lun(acb->sector_num, acb->iscsilun);
+
+    assert((acb->nb_sectors * BDRV_SECTOR_SIZE) % acb->iscsilun->block_size == 0);
     list[0].num = acb->nb_sectors * BDRV_SECTOR_SIZE / acb->iscsilun->block_size;
 
     acb->task = iscsi_unmap_task(iscsi, acb->iscsilun->lun,

@@ -947,6 +952,8 @@ coroutine_fn iscsi_co_write_zeroes(BlockDriverState *bs, int64_t sector_num,
     iTask.complete = 0;
     iTask.bs = bs;
 
+    assert((nb_sectors * BDRV_SECTOR_SIZE) % iscsilun->block_size == 0);
+
     list[0].lba = sector_qemu2lun(sector_num, iscsilun);
     list[0].num = nb_sectors * BDRV_SECTOR_SIZE / iscsilun->block_size;
 

Am 24.06.2013 16:30, schrieb Paolo Bonzini:

Quoted 7 lines

Il 22/06/2013 22:58, Peter Lieven ha scritto:

Quoted 6 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.

for now assert that there is no misalignment to avoid data corruption.

Quoted 1 line

You should just fail to open the LUN instead.

Ronnie added support for reading fragments of blocksize if the offset is aligned. I don't know what the idea was, maybe for qemu reading the boot sector. If the OS does only read multiple of blocksize at aligned offsets everything should work. I now that our storages support 4K blocksize. I can check if it is usable. I could also just fail the operations instead of asserting.

Il 24/06/2013 18:10, Peter Lieven ha scritto:

Quoted 8 lines

Am 24.06.2013 16:30, schrieb Paolo Bonzini:

Quoted 7 lines

Il 22/06/2013 22:58, Peter Lieven ha scritto:

Quoted 6 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.

for now assert that there is no misalignment to avoid data corruption.

Quoted 7 lines

Quoted 1 line

You should just fail to open the LUN instead.

Ronnie added support for reading fragments of blocksize if the offset is aligned. I don't know what the idea was, maybe for qemu reading the boot sector. If the OS does only read multiple of blocksize at aligned offsets everything should work. I now that our storages support 4K blocksize. I can check if it is usable. I could also just fail the operations instead of asserting.

So far, 4K blocksize is usable if you also specify the same block size for the guest device. I have posted once the patches to do read-modify-write, but I never really pursued inclusion of those.

In any case, the right place to fix this is the block layer; driver-specific hacks are... hacks. :)

Quoted 6 lines

Quoted 6 lines

So far, 4K blocksize is usable if you also specify the same block size for the guest device. I have posted once the patches to do read-modify-write, but I never really pursued inclusion of those.

In any case, the right place to fix this is the block layer; driver-specific hacks are... hacks. :)

Where do I find the sector size if not in BDRV_SECTOR_SIZE? Is there a dynamic field or is the answer qemu only support 512 Byte sector size at the moment?

So you would go for fail to open a device if the LUN blocksize is not equal to BDRV_SECTOR_SIZE?

Il 24/06/2013 18:24, Peter Lieven ha scritto:

Quoted 9 lines

Quoted 6 lines

Quoted 6 lines

So far, 4K blocksize is usable if you also specify the same block size for the guest device. I have posted once the patches to do read-modify-write, but I never really pursued inclusion of those.

In any case, the right place to fix this is the block layer; driver-specific hacks are... hacks. :)

Where do I find the sector size if not in BDRV_SECTOR_SIZE? Is there a dynamic field or is the answer qemu only support 512 Byte sector size at the moment?

bdrv_read/write and friends are always in 512-byte units.

There is bs->buffer_alignment for the guest sector size, but it is not set at bdrv_open, only later. So I think QEMU support for bigger sectors is too sparse to be useful.

Quoted 2 lines

So you would go for fail to open a device if the LUN blocksize is not equal to BDRV_SECTOR_SIZE?

Either that, or fails reads/writes that are not aligned.

Paolo

this patch adds a efficient encoding for zero blocks by adding a new flag indiciating a block is completly zero.

additionally bdrv_write_zeros() is used at the destination to efficiently write these zeroes. if the driver supports it this avoids blindly allocating all sectors consumed by zero blocks effectively re-thinning the device.

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

---
 block-migration.c             |   29 +++++++++++++++++++++++------
 include/migration/qemu-file.h |    1 +
 savevm.c                      |    2 +-
 3 files changed, 25 insertions(+), 7 deletions(-)

block-migration.c +23 -6

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;
+    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);
 
     /* 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;
+    }
+
     qemu_put_buffer(f, blk->buf, BLOCK_SIZE);
 }
 

@@ -762,12 +776,15 @@ static int block_load(QEMUFile *f, void *opaque, int version_id)
                 nr_sectors = BDRV_SECTORS_PER_DIRTY_CHUNK;
             }
 
-            buf = g_malloc(BLOCK_SIZE);
-
-            qemu_get_buffer(f, buf, BLOCK_SIZE);
-            ret = bdrv_write(bs, addr, buf, nr_sectors);
+            if (flags & BLK_MIG_FLAG_ZERO_BLOCK) {
+                ret = bdrv_write_zeroes(bs, addr, nr_sectors);
+            } else {
+                buf = g_malloc(BLOCK_SIZE);
+                qemu_get_buffer(f, buf, BLOCK_SIZE);
+                ret = bdrv_write(bs, addr, buf, nr_sectors);
+                g_free(buf);
+            }
 
-            g_free(buf);
             if (ret < 0) {
                 return ret;
             }

include/migration/qemu-file.h +1 -0

index 7519464..b73298d 100644
@@ -71,6 +71,7 @@ QEMUFile *qemu_fdopen(int fd, const char *mode);
 QEMUFile *qemu_fopen_socket(int fd, const char *mode);
 QEMUFile *qemu_popen_cmd(const char *command, const char *mode);
 int qemu_get_fd(QEMUFile *f);
+void qemu_fflush(QEMUFile *f);
 int qemu_fclose(QEMUFile *f);
 int64_t qemu_ftell(QEMUFile *f);
 void qemu_put_buffer(QEMUFile *f, const uint8_t *buf, int size);

savevm.c +1 -1

index ff5ece6..4d12d92 100644
@@ -610,7 +610,7 @@ static inline bool qemu_file_is_writable(QEMUFile *f)
  * If there is writev_buffer QEMUFileOps it uses it otherwise uses
  * put_buffer ops.
  */
-static void qemu_fflush(QEMUFile *f)
+void qemu_fflush(QEMUFile *f)
 {
     ssize_t ret = 0;
 

Il 22/06/2013 22:58, Peter Lieven ha scritto:

Quoted 3 lines

this patch adds a efficient encoding for zero blocks by adding a new flag indiciating a block is completly zero.

Quoted 6 lines

additionally bdrv_write_zeros() is used at the destination to efficiently write these zeroes. if the driver supports it this avoids blindly allocating all sectors consumed by zero blocks effectively re-thinning the device.

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

This is a bit ugly because it doesn't work with drive-mirror. Perhaps we can add a write-zeroes opcode to NBD, too.

Paolo

Quoted 96 lines

---
 block-migration.c             |   29 +++++++++++++++++++++++------
 include/migration/qemu-file.h |    1 +
 savevm.c                      |    2 +-
 3 files changed, 25 insertions(+), 7 deletions(-)

block-migration.c +23 -6

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;
+    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);
 
     /* 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;
+    }
+
     qemu_put_buffer(f, blk->buf, BLOCK_SIZE);
 }
 

@@ -762,12 +776,15 @@ static int block_load(QEMUFile *f, void *opaque, int version_id)
                 nr_sectors = BDRV_SECTORS_PER_DIRTY_CHUNK;
             }
 
-            buf = g_malloc(BLOCK_SIZE);
-
-            qemu_get_buffer(f, buf, BLOCK_SIZE);
-            ret = bdrv_write(bs, addr, buf, nr_sectors);
+            if (flags & BLK_MIG_FLAG_ZERO_BLOCK) {
+                ret = bdrv_write_zeroes(bs, addr, nr_sectors);
+            } else {
+                buf = g_malloc(BLOCK_SIZE);
+                qemu_get_buffer(f, buf, BLOCK_SIZE);
+                ret = bdrv_write(bs, addr, buf, nr_sectors);
+                g_free(buf);
+            }
 
-            g_free(buf);
             if (ret < 0) {
                 return ret;
             }

include/migration/qemu-file.h +1 -0

index 7519464..b73298d 100644
@@ -71,6 +71,7 @@ QEMUFile *qemu_fdopen(int fd, const char *mode);
 QEMUFile *qemu_fopen_socket(int fd, const char *mode);
 QEMUFile *qemu_popen_cmd(const char *command, const char *mode);
 int qemu_get_fd(QEMUFile *f);
+void qemu_fflush(QEMUFile *f);
 int qemu_fclose(QEMUFile *f);
 int64_t qemu_ftell(QEMUFile *f);
 void qemu_put_buffer(QEMUFile *f, const uint8_t *buf, int size);

savevm.c +1 -1

index ff5ece6..4d12d92 100644
@@ -610,7 +610,7 @@ static inline bool qemu_file_is_writable(QEMUFile *f)
  * If there is writev_buffer QEMUFileOps it uses it otherwise uses
  * put_buffer ops.
  */
-static void qemu_fflush(QEMUFile *f)
+void qemu_fflush(QEMUFile *f)
 {
     ssize_t ret = 0;