[Qemu-devel] [PATCH 0/6] QED block conversion

[Qemu-devel] [PATCH 0/6] QED block conversion

[Devin Nakamura]

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(-)

-- 
1.7.6.rc1

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

[Devin Nakamura]

Signed-off-by: Devin Nakamura <devin122@gmail.com>
---
 block/qed-cluster.c |   33 +++++++++++++++++++++++++++++++++
 block/qed.h         |    4 ++++
 2 files changed, 37 insertions(+), 0 deletions(-)

diff --git a/block/qed-cluster.c b/block/qed-cluster.c
index f64b2af..6e68ba7 100644
--- a/block/qed-cluster.c
+++ b/block/qed-cluster.c
@@ -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;
+}
diff --git a/block/qed.h b/block/qed.h
index 388fdb3..c899c15 100644
--- a/block/qed.h
+++ b/block/qed.h
@@ -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
  */
-- 
1.7.6.rc1

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

[Stefan Hajnoczi]

On Mon, Sep 12, 2011 at 3:47 PM, Devin Nakamura <devin122@gmail.com> wrote:
/* See qed_find_cluster(), especially how request->l2_table reference
count works */
> +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

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

[Kevin Wolf]

Am 21.09.2011 14:45, schrieb Stefan Hajnoczi:
> On Mon, Sep 12, 2011 at 3:47 PM, Devin Nakamura <devin122@gmail.com> wrote:
> /* See qed_find_cluster(), especially how request->l2_table reference
> count works */
>> +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

[Qemu-devel] [PATCH 2/6] qed: add bdrv_qed_get_conversion_options()

[Devin Nakamura]

Signed-off-by: Devin Nakamura <devin122@gmail.com>
---
 block/qed.c |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/block/qed.c b/block/qed.c
index 624e261..16320f5 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -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)
-- 
1.7.6.rc1

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

[Devin Nakamura]

Signed-off-by: Devin Nakamura <devin122@gmail.com>
---
 block/qed.c |   57 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 57 insertions(+), 0 deletions(-)

diff --git a/block/qed.c b/block/qed.c
index 16320f5..93827db 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -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)
-- 
1.7.6.rc1

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

[Kevin Wolf]

Am 12.09.2011 16:47, schrieb Devin Nakamura:
> Signed-off-by: Devin Nakamura <devin122@gmail.com>
> ---
>  block/qed.c |   57 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 57 insertions(+), 0 deletions(-)
> 
> diff --git a/block/qed.c b/block/qed.c
> index 16320f5..93827db 100644
> --- a/block/qed.c
> +++ b/block/qed.c
> @@ -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);

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.

> +    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.

> +    }
> +
> +    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)

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

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

[Stefan Hajnoczi]

On Mon, Sep 12, 2011 at 3:47 PM, Devin Nakamura <devin122@gmail.com> wrote:
> +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;

Where does s->header.header_size get initialized?

> +    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 +

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.

> +                                        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;
> +    }

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?

> +    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);

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

Re: [Qemu-devel] [PATCH 0/6] QED block conversion

[Kevin Wolf]

Am 12.09.2011 16:47, schrieb Devin Nakamura:
> 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(-)

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

Re: [Qemu-devel] [PATCH 0/6] QED block conversion

[Stefan Hajnoczi]

On Tue, Sep 20, 2011 at 10:38:05AM +0200, Kevin Wolf wrote:
> Am 12.09.2011 16:47, schrieb Devin Nakamura:
> > 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(-)
> 
> 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