Skip to contentSkip to conversation

QED block conversion

PATCH In review Devin Nakamura 4 participants 10 messages Sep 12, 2011 – Sep 21, 2011 Block layer

lore
Share & tools
lore

Devin Nakamura

author
#0 lore raw

cover QED block conversion

This patch series adds support for block conversion to the qed driver. This depends on my precivious block conversion api series Devin Nakamura (6):
qed: add qed_find_cluster_sync()
qed: add bdrv_qed_get_conversion_options()
qed: add open_conversion_target()
qed: add qed_bdrv_get_mapping()
qed: add bdrv_qed_map()
qed: add bdrv_qed_copy_header()

 block/qed-cluster.c |   33 +++++++++++
 block/qed.c         |  161 +++++++++++++++++++++++++++++++++++++++++++++++++++
 block/qed.h         |    4 +
 3 files changed, 198 insertions(+), 0 deletions(-)
Signature · 1 line
1.7.6.rc1
author
#1 lore raw

PATCH 1/6 qed: add qed_find_cluster_sync()

Signed-off-by Devin Nakamura <[email protected]>
---
 block/qed-cluster.c |   33 +++++++++++++++++++++++++++++++++
 block/qed.h         |    4 ++++
 2 files changed, 37 insertions(+), 0 deletions(-)
block/qed-cluster.c +33 -0
index f64b2af..6e68ba7 100644
@@ -163,3 +163,36 @@ void qed_find_cluster(BDRVQEDState *s, QEDRequest *request, uint64_t pos,
     qed_read_l2_table(s, request, l2_offset,
                       qed_find_cluster_cb, find_cluster_cb);
 }
+
+typedef struct {
+    int ret;
+    uint64_t *offset;
+    size_t *len;
+} QEDFindClusterSyncCB;
+
+static void qed_find_cluster_sync_cb(void *opaque, int ret, uint64_t offset,
+                              size_t len)
+{
+    QEDFindClusterSyncCB *find_cluster_sync_cb = opaque;
+    *find_cluster_sync_cb->offset = offset;
+    *find_cluster_sync_cb->len = len;
+    find_cluster_sync_cb->ret = ret;
+}
+
+int qed_find_cluster_sync(BDRVQEDState *s, QEDRequest *request, uint64_t pos,
+                          size_t len, uint64_t *offset,
+                          size_t *contiguous_bytes)
+{
+    QEDFindClusterSyncCB find_cluster_cb;
+    find_cluster_cb.ret = -EINPROGRESS;
+    find_cluster_cb.offset = offset;
+    find_cluster_cb.len = contiguous_bytes;
+
+    qed_find_cluster(s, request, pos, len, &qed_find_cluster_sync_cb,
+                     &find_cluster_cb);
+    while (find_cluster_cb.ret == -EINPROGRESS) {
+        qemu_aio_wait();
+    }
+
+    return find_cluster_cb.ret;
+}
block/qed.h +4 -0
index 388fdb3..c899c15 100644
@@ -239,6 +239,10 @@ int qed_write_l2_table_sync(BDRVQEDState *s, QEDRequest *request,
 void qed_find_cluster(BDRVQEDState *s, QEDRequest *request, uint64_t pos,
                       size_t len, QEDFindClusterFunc *cb, void *opaque);
 
+int qed_find_cluster_sync(BDRVQEDState *s, QEDRequest *request, uint64_t pos,
+                          size_t len, uint64_t *offset,
+                          size_t *contiguous_bytes);
+
 /**
  * Consistency check
  */
Signature · 1 line
1.7.6.rc1
#2 lore raw

Re: [Qemu-devel] [PATCH 1/6] qed: add qed_find_cluster_sync()

On Mon, Sep 12, 2011 at 3:47 PM, Devin Nakamura <[email protected]> wrote:
/* See qed_find_cluster(), especially how request->l2_table reference count works */

Quoted 3 lines

+int qed_find_cluster_sync(BDRVQEDState *s, QEDRequest *request, uint64_t pos,
+                          size_t len, uint64_t *offset,
+                          size_t *contiguous_bytes)

If you respin please add a comment like the one above. It's easy to forget that this function grabs the l2_table and gives you a reference. Let's put a warning in place.

Stefan

#3 lore raw

Re: [Qemu-devel] [PATCH 1/6] qed: add qed_find_cluster_sync()

Am 21.09.2011 14:45, schrieb Stefan Hajnoczi:

Quoted 10 lines · Devin Nakamura

/* See qed_find_cluster(), especially how request->l2_table reference count works */

block/qed-cluster.c void qed_find_cluster(BDRVQEDState *s, QEDRequest *request, uint64_t pos, · line 184

Quoted 3 lines
block/qed-cluster.c +3 -0
+int qed_find_cluster_sync(BDRVQEDState *s, QEDRequest *request, uint64_t pos,
+                          size_t len, uint64_t *offset,
+                          size_t *contiguous_bytes)

If you respin please add a comment like the one above. It's easy to forget that this function grabs the l2_table and gives you a reference. Let's put a warning in place.

Fixing the name of the function is probably better than adding a comment to each user.

Kevin

author
#4 lore raw

PATCH 2/6 qed: add bdrv_qed_get_conversion_options()

In reply to Devin Nakamura
Signed-off-by Devin Nakamura <[email protected]>
---
 block/qed.c |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)
block/qed.c +13 -0
index 624e261..16320f5 100644
@@ -1444,6 +1444,18 @@ static int bdrv_qed_check(BlockDriverState *bs, BdrvCheckResult *result)
     return qed_check(s, result, false);
 }
 
+static int bdrv_qed_get_conversion_options(BlockDriverState *bs,
+                                           BlockConversionOptions *options)
+{
+    BDRVQEDState* s = bs->opaque;
+
+    options->encryption_type = 0;
+    options->cluster_size = s->header.cluster_size;
+    options->image_size = s->header.image_size;
+    options->nb_snapshots = 0;
+    return 0;
+}
+
 static QEMUOptionParameter qed_create_options[] = {
     {
         .name = BLOCK_OPT_SIZE,

@@ -1490,6 +1502,7 @@ static BlockDriver bdrv_qed = {
     .bdrv_get_info            = bdrv_qed_get_info,
     .bdrv_change_backing_file = bdrv_qed_change_backing_file,
     .bdrv_check               = bdrv_qed_check,
+    .bdrv_get_conversion_options = bdrv_qed_get_conversion_options,
 };
 
 static void bdrv_qed_init(void)
Signature · 1 line
1.7.6.rc1
author
#5 lore raw

PATCH 3/6 qed: add open_conversion_target()

In reply to Devin Nakamura
Signed-off-by Devin Nakamura <[email protected]>
---
 block/qed.c |   57 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 57 insertions(+), 0 deletions(-)
block/qed.c +57 -0
index 16320f5..93827db 100644
@@ -1456,6 +1456,62 @@ static int bdrv_qed_get_conversion_options(BlockDriverState *bs,
     return 0;
 }
 
+static int bdrv_qed_open_conversion_target(BlockDriverState *bs,
+                                           BlockConversionOptions *drv_options,
+                                           QEMUOptionParameter *usr_options,
+                                           bool force)
+{
+    BDRVQEDState *s = bs->opaque;
+    s->bs = bs;
+    if (drv_options->encryption_type != BLOCK_CRYPT_NONE) {
+        error_report("Encryption not supported");
+        return -ENOTSUP;
+    }
+    if(drv_options->nb_snapshots && !force) {
+        error_report("Snapshots are not supported");
+        return -ENOTSUP;
+    }
+    s->header.magic = QED_MAGIC;
+    s->header.table_size = QED_DEFAULT_TABLE_SIZE;
+    if(qed_is_cluster_size_valid(drv_options->cluster_size)) {
+        s->header.cluster_size = drv_options->cluster_size;
+    } else {
+        error_report("Invalid cluster size");
+        return -EINVAL;
+    }
+    if(qed_is_image_size_valid(drv_options->image_size, s->header.cluster_size,
+                               s->header.table_size)) {
+        s->header.image_size = drv_options->image_size;
+    } else {
+        error_report("Invalid image size");
+        return -EINVAL;
+    }
+    s->file_size = qed_Start_of_cluster(s, bs->file->total_sectors +
+                                        drv_options->cluster_size -1);
+    s->l1_table = qed_alloc_table(s);
+    s->header.l1_table_offset = qed_alloc_clusters(s, s->header.table_size);
+    QSIMPLEQ_INIT(&s->allocating_write_reqs);
+
+
+    if (!qed_check_table_offset(s, s->header.l1_table_offset)) {
+        error_report("Invalid L1 table offset");
+        return -EINVAL;
+    }
+
+    s->table_nelems = (s->header.cluster_size * s->header.table_size) /
+                      sizeof(uint64_t);
+    s->l2_shift = ffs(s->header.cluster_size) - 1;
+    s->l2_mask = s->table_nelems - 1;
+    s->l1_shift = s->l2_shift + ffs(s->table_nelems) - 1;
+
+    qed_init_l2_cache(&s->l2_cache);
+
+    s->need_check_timer = qemu_new_timer_ns(vm_clock,
+                                            qed_need_check_timer_cb, s);
+    qed_write_l1_table_sync(s, 0, s->table_nelems);
+    return 0;
+}
+
 static QEMUOptionParameter qed_create_options[] = {
     {
         .name = BLOCK_OPT_SIZE,

@@ -1503,6 +1559,7 @@ static BlockDriver bdrv_qed = {
     .bdrv_change_backing_file = bdrv_qed_change_backing_file,
     .bdrv_check               = bdrv_qed_check,
     .bdrv_get_conversion_options = bdrv_qed_get_conversion_options,
+    .bdrv_open_conversion_target = bdrv_qed_open_conversion_target,
 };
 
 static void bdrv_qed_init(void)
Signature · 1 line
1.7.6.rc1
#6 lore raw

Re: [Qemu-devel] [PATCH 3/6] qed: add open_conversion_target()

Am 12.09.2011 16:47, schrieb Devin Nakamura:

Quoted 35 lines
Signed-off-by Devin Nakamura <[email protected]>
---
 block/qed.c |   57 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 57 insertions(+), 0 deletions(-)
block/qed.c +22 -0
index 16320f5..93827db 100644
@@ -1456,6 +1456,62 @@ static int bdrv_qed_get_conversion_options(BlockDriverState *bs,
     return 0;
 }
 
+static int bdrv_qed_open_conversion_target(BlockDriverState *bs,
+                                           BlockConversionOptions *drv_options,
+                                           QEMUOptionParameter *usr_options,
+                                           bool force)
+{
+    BDRVQEDState *s = bs->opaque;
+    s->bs = bs;
+    if (drv_options->encryption_type != BLOCK_CRYPT_NONE) {
+        error_report("Encryption not supported");
+        return -ENOTSUP;
+    }
+    if(drv_options->nb_snapshots && !force) {
+        error_report("Snapshots are not supported");
+        return -ENOTSUP;
+    }
+    s->header.magic = QED_MAGIC;
+    s->header.table_size = QED_DEFAULT_TABLE_SIZE;
+    if(qed_is_cluster_size_valid(drv_options->cluster_size)) {
+        s->header.cluster_size = drv_options->cluster_size;
+    } else {
+        error_report("Invalid cluster size");
+        return -EINVAL;

block/qed.c static int bdrv_qed_get_conversion_options(BlockDriverState *bs, · line 1490

Quoted 10 lines
block/qed.c +10 -0
+    }
+    if(qed_is_image_size_valid(drv_options->image_size, s->header.cluster_size,
+                               s->header.table_size)) {
+        s->header.image_size = drv_options->image_size;
+    } else {
+        error_report("Invalid image size");
+        return -EINVAL;
+    }
+    s->file_size = qed_Start_of_cluster(s, bs->file->total_sectors +
+                                        drv_options->cluster_size -1);

This doesn't look quite right. With the capital S in qemu_Start_of_cluster it actually shouldn't even compile.

drv->options->cluster_size is in bytes, but bs->file->total_sectors is in sectors. I don't think the sum of them makes a lot of sense.

block/qed.c static int bdrv_qed_get_conversion_options(BlockDriverState *bs, · line 1498

Quoted 8 lines
block/qed.c +8 -0
+    s->l1_table = qed_alloc_table(s);
+    s->header.l1_table_offset = qed_alloc_clusters(s, s->header.table_size);
+    QSIMPLEQ_INIT(&s->allocating_write_reqs);
+
+
+    if (!qed_check_table_offset(s, s->header.l1_table_offset)) {
+        error_report("Invalid L1 table offset");
+        return -EINVAL;

This leaks s->l1_table.

Quoted 20 lines
block/qed.c +16 -0
+    }
+
+    s->table_nelems = (s->header.cluster_size * s->header.table_size) /
+                      sizeof(uint64_t);
+    s->l2_shift = ffs(s->header.cluster_size) - 1;
+    s->l2_mask = s->table_nelems - 1;
+    s->l1_shift = s->l2_shift + ffs(s->table_nelems) - 1;
+
+    qed_init_l2_cache(&s->l2_cache);
+
+    s->need_check_timer = qemu_new_timer_ns(vm_clock,
+                                            qed_need_check_timer_cb, s);
+    qed_write_l1_table_sync(s, 0, s->table_nelems);
+    return 0;
+}
+
 static QEMUOptionParameter qed_create_options[] = {
     {
         .name = BLOCK_OPT_SIZE,

block/qed.c static BlockDriver bdrv_qed = { · line 1565

Quoted 7 lines
block/qed.c +1 -0
@@ -1503,6 +1559,7 @@ static BlockDriver bdrv_qed = {
     .bdrv_change_backing_file = bdrv_qed_change_backing_file,
     .bdrv_check               = bdrv_qed_check,
     .bdrv_get_conversion_options = bdrv_qed_get_conversion_options,
+    .bdrv_open_conversion_target = bdrv_qed_open_conversion_target,
 };
 
 static void bdrv_qed_init(void)

Another question that came to my mind while reviewing this - although mostly unrelated - is where we handle backing files. Is that completely contained in the generic conversion code?

Kevin

#7 lore raw

Re: [Qemu-devel] [PATCH 3/6] qed: add open_conversion_target()

In reply to Devin Nakamura
Quoted 7 lines · Devin Nakamura
block/qed.c +7 -0
+static int bdrv_qed_open_conversion_target(BlockDriverState *bs,
+                                           BlockConversionOptions *drv_options,
+                                           QEMUOptionParameter *usr_options,
+                                           bool force)
+{
+    BDRVQEDState *s = bs->opaque;
+    s->bs = bs;

block/qed.c static int bdrv_qed_get_conversion_options(BlockDriverState *bs, · line 1475

Quoted 10 lines
block/qed.c +10 -0
+    if (drv_options->encryption_type != BLOCK_CRYPT_NONE) {
+        error_report("Encryption not supported");
+        return -ENOTSUP;
+    }
+    if(drv_options->nb_snapshots && !force) {
+        error_report("Snapshots are not supported");
+        return -ENOTSUP;
+    }
+    s->header.magic = QED_MAGIC;
+    s->header.table_size = QED_DEFAULT_TABLE_SIZE;

Where does s->header.header_size get initialized?

Quoted 6 lines

+    if(qed_is_cluster_size_valid(drv_options->cluster_size)) {
+        s->header.cluster_size = drv_options->cluster_size;
+    } else {
+        error_report("Invalid cluster size");
+        return -EINVAL;
+    }

Quoted 8 lines

+    if(qed_is_image_size_valid(drv_options->image_size, s->header.cluster_size,
+                               s->header.table_size)) {
+        s->header.image_size = drv_options->image_size;
+    } else {
+        error_report("Invalid image size");
+        return -EINVAL;
+    }
+    s->file_size = qed_Start_of_cluster(s, bs->file->total_sectors +

Please use bdrv_getlength(bs->file) instead of directly accessing total_sectors on an unknown BlockDriverState. We cache size in the total_sectors field but if we want to modify the way caching works then it's easiest if users do not reach into the field directly but call the accessor function instead.

block/qed.c static int bdrv_qed_get_conversion_options(BlockDriverState *bs, · line 1499

Quoted 10 lines

+                                        drv_options->cluster_size -1);
+    s->l1_table = qed_alloc_table(s);
+    s->header.l1_table_offset = qed_alloc_clusters(s, s->header.table_size);
+    QSIMPLEQ_INIT(&s->allocating_write_reqs);

block/qed.c +6 -0
+
+
+    if (!qed_check_table_offset(s, s->header.l1_table_offset)) {
+        error_report("Invalid L1 table offset");
+        return -EINVAL;
+    }

Does this leak s->l1_table?

Why check l1_table_offset just a few lines after carefully creating it? Is there a specific case where this could fail?

block/qed.c static int bdrv_qed_get_conversion_options(BlockDriverState *bs, · line 1511

Quoted 11 lines

+    s->table_nelems = (s->header.cluster_size * s->header.table_size) /
+                      sizeof(uint64_t);
+    s->l2_shift = ffs(s->header.cluster_size) - 1;
+    s->l2_mask = s->table_nelems - 1;
+    s->l1_shift = s->l2_shift + ffs(s->table_nelems) - 1;

block/qed.c +6 -0
+
+    qed_init_l2_cache(&s->l2_cache);
+
+    s->need_check_timer = qemu_new_timer_ns(vm_clock,
+                                            qed_need_check_timer_cb, s);
+    qed_write_l1_table_sync(s, 0, s->table_nelems);

Did I miss where the L1 table elements are initialized to zero? At this point I think we're writing out undefined values from memory just allocated with qed_alloc_table().

Stefan

#8 lore raw
In reply to Devin Nakamura

Am 12.09.2011 16:47, schrieb Devin Nakamura:

Quoted 10 lines

This patch series adds support for block conversion to the qed driver. This depends on my precivious block conversion api series Devin Nakamura (6):
qed: add qed_find_cluster_sync()
qed: add bdrv_qed_get_conversion_options()
qed: add open_conversion_target()
qed: add qed_bdrv_get_mapping()
qed: add bdrv_qed_map()
qed: add bdrv_qed_copy_header()

Quoted 4 lines
 block/qed-cluster.c |   33 +++++++++++
 block/qed.c         |  161 +++++++++++++++++++++++++++++++++++++++++++++++++++
 block/qed.h         |    4 +
 3 files changed, 198 insertions(+), 0 deletions(-)

Stefan, can you have a look at this series? I'm reviewing it right now, but I'm not that familiar with the details of the QED implementation.

Devin, in the previous series there were two patches that I had commented on and that need a new version. For now I'll push everything that has been reviewed into a separate branch in my git repo [1], and flag patches with unadressed comments with "(NEEDS FIX)" in the subject. I hope this will help us with managing the whole set of patch series.

Kevin

[1]
http://repo.or.cz/w/qemu/kevin.git/shortlog/refs/heads/inplace-conversion

#9 lore raw
Quoted 11 lines · Kevin Wolf

Am 12.09.2011 16:47, schrieb Devin Nakamura:

Quoted 10 lines

This patch series adds support for block conversion to the qed driver. This depends on my precivious block conversion api series Devin Nakamura (6):
qed: add qed_find_cluster_sync()
qed: add bdrv_qed_get_conversion_options()
qed: add open_conversion_target()
qed: add qed_bdrv_get_mapping()
qed: add bdrv_qed_map()
qed: add bdrv_qed_copy_header()

Quoted 7 lines
Quoted 4 lines
 block/qed-cluster.c |   33 +++++++++++
 block/qed.c         |  161 +++++++++++++++++++++++++++++++++++++++++++++++++++
 block/qed.h         |    4 +
 3 files changed, 198 insertions(+), 0 deletions(-)

Stefan, can you have a look at this series? I'm reviewing it right now, but I'm not that familiar with the details of the QED implementation.

Sure it's in my review queue, will try to get around to it soon.

Stefan