Re: [Qemu-devel] [PATCH v6 08/11] qcow2: Rebuild refcount structure during check

[Kevin Wolf]

Am 21.10.2014 um 11:52 hat Max Reitz geschrieben:
> On 2014-10-21 at 11:31, Kevin Wolf wrote:
> >Am 20.10.2014 um 16:35 hat Max Reitz geschrieben:
> >>The previous commit introduced the "rebuild" variable to qcow2's
> >>implementation of the image consistency check. Now make use of this by
> >>adding a function which creates a completely new refcount structure
> >>based solely on the in-memory information gathered before.
> >>
> >>The old refcount structure will be leaked, however. This leak will be
> >>dealt with in a follow-up commit.
> >>
> >>Signed-off-by: Max Reitz <address@hidden>
> >>---
> >>  block/qcow2-refcount.c | 296 
> >> ++++++++++++++++++++++++++++++++++++++++++++++++-
> >>  1 file changed, 293 insertions(+), 3 deletions(-)

> >>+/*
> >>+ * Creates a new refcount structure based solely on the in-memory 
> >>information
> >>+ * given through *refcount_table. All necessary allocations will be 
> >>reflected
> >>+ * in that array.
> >>+ *
> >>+ * On success, the old refcount structure is leaked (it will be covered by 
> >>the
> >>+ * new refcount structure).
> >>+ */
> >>+static int rebuild_refcount_structure(BlockDriverState *bs,
> >>+                                      BdrvCheckResult *res,
> >>+                                      uint16_t **refcount_table,
> >>+                                      int64_t *nb_clusters)
> >>+{
> >>+    BDRVQcowState *s = bs->opaque;
> >>+    int64_t first_free_cluster = 0, reftable_offset = -1, cluster = 0;
> >>+    int64_t refblock_offset, refblock_start, refblock_index;
> >>+    uint32_t reftable_size = 0;
> >>+    uint64_t *reftable = NULL;
> >refcount_table and reftable? Seriously?
> 
> Reviewing would have been too easy otherwise. *cough*
> 
> One option is s/refcount_table/imrt/. I like it more than the second
> option, but it simply goes against the current naming convention.
> 
> The other option would be s/reftable/on_disk_reftable/, following
> the convention used in the next line.

The second option is probably more consistent with the rest (and less
cryptic).

> >>+        ret = qcow2_pre_write_overlap_check(bs, 0, refblock_offset,
> >>+                                            s->cluster_size);
> >>+        if (ret < 0) {
> >>+            fprintf(stderr, "ERROR writing refblock: %s\n", 
> >>strerror(-ret));
> >>+            goto fail;
> >>+        }
> >>+
> >>+        on_disk_refblock = g_malloc0(s->cluster_size);
> >qemu_blockalign?
> 
> Meh, I think I have to write a qemu_blockalign0() some time...
> 
> If I do so, it's one more patch you have to review; if I just use
> qemu_blockalign(), I find the memset() directly afterwards rather
> ugly, but would be fine with it. I guess it's up to you, then.

A qemu_blockalign0() sounds easy enough to review. It's not the number
of patches that makes review hard, it's their content.

Anyway, you're the patch author, it's your choice.

> >>+    ret = qcow2_pre_write_overlap_check(bs, 0, reftable_offset,
> >>+                                        reftable_size * sizeof(uint64_t));
> >>+    if (ret < 0) {
> >>+        fprintf(stderr, "ERROR writing reftable: %s\n", strerror(-ret));
> >>+        goto fail;
> >>+    }
> >>+
> >>+    ret = bdrv_write(bs->file, reftable_offset / BDRV_SECTOR_SIZE,
> >>+                     (void *)reftable,
> >>+                     reftable_size * sizeof(uint64_t) / BDRV_SECTOR_SIZE);
> >Why not bdrv_pwrite when you only have byte offset and length?
> 
> I don't like pwrite probably only because it takes an int byte
> length. reftable_size * sizeof(uint64_t) should be well below
> INT_MAX, but I don't see why we should use pwrite if we are writing
> full sectors to a sector-aligned offset.

Good point, we should look into fixing the type of the length argument
sometime.

Other than that, I'd prefer bdrv_pread/pwrite because it doesn't require
unit conversion here, and later in block.c back (bdrv_co_do_pwritev is
the native block layer interface, and it's byte based). bdrv_write()
goes through more emulation layers in block.c.

Kevin