| Message ID | 1409568798-2292-1-git-send-email-junmuzi@gmail.com |
|---|---|
| State | New |
| Headers | show |
The Monday 01 Sep 2014 à 18:52:48 (+0800), Jun Li wrote : > When every item of refcount block is NULL, free refcount block and reset the > corresponding item of refcount table with NULL. > > Signed-off-by: Jun Li <address@hidden> > --- > > The v2 do following change to modify some potential issue. > > +------- Here should start from "0". > | > for (k = 0; k < refcount_block_entries; k++) { > if (refcount_block[k] != cpu_to_be16(0)) { > ... | | > } | | > } | +---- Using "0" is more safe. > | > +-------- This should be "k" not "++k". > --- > block/qcow2-refcount.c | 31 +++++++++++++++++++++++++++++++ > 1 file changed, 31 insertions(+) > > diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c > index 43665b8..63f36e6 100644 > --- a/block/qcow2-refcount.c > +++ b/block/qcow2-refcount.c > @@ -586,6 +586,37 @@ static int QEMU_WARN_UNUSED_RESULT update_refcount(BlockDriverState *bs, > if (refcount == 0 && s->discard_passthrough[type]) { > update_refcount_discard(bs, cluster_offset, s->cluster_size); > } > + > + /* When refcount block is NULL, update refcount table */ > + if (block_index == 0) { > + int k = block_index; > + int refcount_block_entries = s->cluster_size / sizeof(uint16_t); > + for (k = 0; k < refcount_block_entries; k++) { > + if (refcount_block[k] != cpu_to_be16(0)) { > + break; > + } > + } > + > + if (k == refcount_block_entries) { > + qemu_vfree(refcount_block); > + /* update refcount table */ > + unsigned int refcount_table_index; > + uint64_t data64 = cpu_to_be64(0); > + refcount_table_index = cluster_index >> (s->cluster_bits - > + REFCOUNT_SHIFT); > + ret = bdrv_pwrite_sync(bs->file, > + s->refcount_table_offset + > + refcount_table_index * > + sizeof(uint64_t), > + &data64, sizeof(data64)); > + if (ret < 0) { > + goto fail; > + } > + > + s->refcount_table[refcount_table_index] = data64; Shouldn't the in memory version be be in cpu order ? like s->refcount_table[refcount_table_index] = 0; Best regards Benoît > + > + } > + } > } > > ret = 0; > -- > 1.9.3 > >
On Mon, 09/01 13:11, Benoît Canet wrote: > The Monday 01 Sep 2014 à 18:52:48 (+0800), Jun Li wrote : > > When every item of refcount block is NULL, free refcount block and reset the > > corresponding item of refcount table with NULL. > > > > Signed-off-by: Jun Li <address@hidden> > > --- > > > > The v2 do following change to modify some potential issue. > > > > +------- Here should start from "0". > > | > > for (k = 0; k < refcount_block_entries; k++) { > > if (refcount_block[k] != cpu_to_be16(0)) { > > ... | | > > } | | > > } | +---- Using "0" is more safe. > > | > > +-------- This should be "k" not "++k". > > --- > > block/qcow2-refcount.c | 31 +++++++++++++++++++++++++++++++ > > 1 file changed, 31 insertions(+) > > > > diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c > > index 43665b8..63f36e6 100644 > > --- a/block/qcow2-refcount.c > > +++ b/block/qcow2-refcount.c > > @@ -586,6 +586,37 @@ static int QEMU_WARN_UNUSED_RESULT update_refcount(BlockDriverState *bs, > > if (refcount == 0 && s->discard_passthrough[type]) { > > update_refcount_discard(bs, cluster_offset, s->cluster_size); > > } > > + > > + /* When refcount block is NULL, update refcount table */ > > + if (block_index == 0) { > > + int k = block_index; > > + int refcount_block_entries = s->cluster_size / sizeof(uint16_t); > > + for (k = 0; k < refcount_block_entries; k++) { > > + if (refcount_block[k] != cpu_to_be16(0)) { > > + break; > > + } > > + } > > + > > + if (k == refcount_block_entries) { > > + qemu_vfree(refcount_block); > > + /* update refcount table */ > > + unsigned int refcount_table_index; > > + uint64_t data64 = cpu_to_be64(0); > > + refcount_table_index = cluster_index >> (s->cluster_bits - > > + REFCOUNT_SHIFT); > > + ret = bdrv_pwrite_sync(bs->file, > > + s->refcount_table_offset + > > + refcount_table_index * > > + sizeof(uint64_t), > > + &data64, sizeof(data64)); > > + if (ret < 0) { > > + goto fail; > > + } > > + > > > + s->refcount_table[refcount_table_index] = data64; > > Shouldn't the in memory version be be in cpu order ? like > s->refcount_table[refcount_table_index] = 0; I don't think so. See following: (gdb) p sizeof(s->refcount_table[0]) $5 = 8 (gdb) p sizeof(s->refcount_table[1]) $6 = 8 (gdb) p sizeof(0) $7 = 4 So I think here is right. Thank you for sharing Max's patch(qcow2: Drop REFCOUNT_SHIFT) with me. I find this patch has been reviewed, but it has not been merged. Maybe I will modify my realization after this patch merged. Thanks again. Jun Li > > Best regards > > Benoît > > + > > + } > > + } > > } > > > > ret = 0; > > -- > > 1.9.3 > > > >
The Tuesday 02 Sep 2014 à 00:04:08 (+0800), Jun Li wrote : > On Mon, 09/01 13:11, Benoît Canet wrote: > > The Monday 01 Sep 2014 à 18:52:48 (+0800), Jun Li wrote : > > > When every item of refcount block is NULL, free refcount block and reset the > > > corresponding item of refcount table with NULL. > > > > > > Signed-off-by: Jun Li <address@hidden> > > > --- > > > > > > The v2 do following change to modify some potential issue. > > > > > > +------- Here should start from "0". > > > | > > > for (k = 0; k < refcount_block_entries; k++) { > > > if (refcount_block[k] != cpu_to_be16(0)) { > > > ... | | > > > } | | > > > } | +---- Using "0" is more safe. > > > | > > > +-------- This should be "k" not "++k". > > > --- > > > block/qcow2-refcount.c | 31 +++++++++++++++++++++++++++++++ > > > 1 file changed, 31 insertions(+) > > > > > > diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c > > > index 43665b8..63f36e6 100644 > > > --- a/block/qcow2-refcount.c > > > +++ b/block/qcow2-refcount.c > > > @@ -586,6 +586,37 @@ static int QEMU_WARN_UNUSED_RESULT update_refcount(BlockDriverState *bs, > > > if (refcount == 0 && s->discard_passthrough[type]) { > > > update_refcount_discard(bs, cluster_offset, s->cluster_size); > > > } > > > + > > > + /* When refcount block is NULL, update refcount table */ > > > + if (block_index == 0) { > > > + int k = block_index; > > > + int refcount_block_entries = s->cluster_size / sizeof(uint16_t); > > > + for (k = 0; k < refcount_block_entries; k++) { > > > + if (refcount_block[k] != cpu_to_be16(0)) { > > > + break; > > > + } > > > + } > > > + > > > + if (k == refcount_block_entries) { > > > + qemu_vfree(refcount_block); > > > + /* update refcount table */ > > > + unsigned int refcount_table_index; > > > + uint64_t data64 = cpu_to_be64(0); > > > + refcount_table_index = cluster_index >> (s->cluster_bits - > > > + REFCOUNT_SHIFT); > > > + ret = bdrv_pwrite_sync(bs->file, > > > + s->refcount_table_offset + > > > + refcount_table_index * > > > + sizeof(uint64_t), > > > + &data64, sizeof(data64)); > > > + if (ret < 0) { > > > + goto fail; > > > + } > > > + > > > > > + s->refcount_table[refcount_table_index] = data64; > > > > Shouldn't the in memory version be be in cpu order ? like > > s->refcount_table[refcount_table_index] = 0; > > I don't think so. See following: > > (gdb) p sizeof(s->refcount_table[0]) > $5 = 8 > (gdb) p sizeof(s->refcount_table[1]) > $6 = 8 > (gdb) p sizeof(0) > $7 = 4 There is two different thing here: endianness and type. For the endianess you can look at qcow2_refcount_init. The endianness of this in memory table is the one of the CPU. Here data64 is big endian and this is wrong. For the type integer promotion will take care of it. See http://www.tutorialspoint.com/cprogramming/c_type_casting.htm assigning zero means that the compiler will silently perform a cast to int64_t. Best regards Benoît > > So I think here is right. Thank you for sharing Max's patch(qcow2: Drop > REFCOUNT_SHIFT) with me. I find this patch has been reviewed, but it has not > been merged. Maybe I will modify my realization after this patch merged. > > Thanks again. > > Jun Li > > > > > > Best regards > > > > Benoît > > > + > > > + } > > > + } > > > } > > > > > > ret = 0; > > > -- > > > 1.9.3 > > > > > > >
On Mon, 1 Sep 2014 18:52:48 +0800 Jun Li <junmuzi@gmail.com> wrote: > When every item of refcount block is NULL, free refcount block and reset the > corresponding item of refcount table with NULL. > > Signed-off-by: Jun Li <address@hidden> > --- > > The v2 do following change to modify some potential issue. > > +------- Here should start from "0". > | > for (k = 0; k < refcount_block_entries; k++) { > if (refcount_block[k] != cpu_to_be16(0)) { > ... | | > } | | > } | +---- Using "0" is more safe. > | > +-------- This should be "k" not "++k". > --- > block/qcow2-refcount.c | 31 +++++++++++++++++++++++++++++++ > 1 file changed, 31 insertions(+) > > diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c > index 43665b8..63f36e6 100644 > --- a/block/qcow2-refcount.c > +++ b/block/qcow2-refcount.c > @@ -586,6 +586,37 @@ static int QEMU_WARN_UNUSED_RESULT update_refcount(BlockDriverState *bs, > if (refcount == 0 && s->discard_passthrough[type]) { > update_refcount_discard(bs, cluster_offset, s->cluster_size); > } > + > + /* When refcount block is NULL, update refcount table */ > + if (block_index == 0) { > + int k = block_index; Hi, k = 0 is also done in the for block below... > + int refcount_block_entries = s->cluster_size / sizeof(uint16_t); It's better for maintainance to count elements in an array this way: int refcount_block_entries = s->cluster_size / sizeof(refcount_block[0]); > + for (k = 0; k < refcount_block_entries; k++) { > + if (refcount_block[k] != cpu_to_be16(0)) { > + break; > + } > + } > + > + if (k == refcount_block_entries) { > + qemu_vfree(refcount_block); > + /* update refcount table */ > + unsigned int refcount_table_index; > + uint64_t data64 = cpu_to_be64(0); > + refcount_table_index = cluster_index >> (s->cluster_bits - > + REFCOUNT_SHIFT); > + ret = bdrv_pwrite_sync(bs->file, > + s->refcount_table_offset + > + refcount_table_index * > + sizeof(uint64_t), > + &data64, sizeof(data64)); > + if (ret < 0) { > + goto fail; > + } > + > + s->refcount_table[refcount_table_index] = data64; > + > + } > + } > } > > ret = 0; Cheers.
Am 01.09.2014 um 12:52 hat Jun Li geschrieben: > When every item of refcount block is NULL, free refcount block and reset the > corresponding item of refcount table with NULL. > > Signed-off-by: Jun Li <address@hidden> The commit message should also describe why this is a relevant improvement for some use case. My gut feeling is that it complicates the code for a very minimal gain. Kevin
On Mon, Sep 01, 2014 at 06:52:48PM +0800, Jun Li wrote: How does this patch handle self-describing refcount blocks? I think they will keep the refcount block alive forever because your code will not decide to free them. This patch should also discard the refcount block if we decide to free it (in the same way that we discard at cluster_offset). > diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c > index 43665b8..63f36e6 100644 > --- a/block/qcow2-refcount.c > +++ b/block/qcow2-refcount.c > @@ -586,6 +586,37 @@ static int QEMU_WARN_UNUSED_RESULT update_refcount(BlockDriverState *bs, > if (refcount == 0 && s->discard_passthrough[type]) { > update_refcount_discard(bs, cluster_offset, s->cluster_size); > } > + > + /* When refcount block is NULL, update refcount table */ > + if (block_index == 0) { What is the purpose of block_index == 0? > + int k = block_index; > + int refcount_block_entries = s->cluster_size / sizeof(uint16_t); > + for (k = 0; k < refcount_block_entries; k++) { > + if (refcount_block[k] != cpu_to_be16(0)) { > + break; > + } > + } > + > + if (k == refcount_block_entries) { > + qemu_vfree(refcount_block); You can't do this, the buffer belongs to the refcount block cache. Please look at the cache get/put as well as qcow2_cache_create/destroy. > + /* update refcount table */ > + unsigned int refcount_table_index; > + uint64_t data64 = cpu_to_be64(0); > + refcount_table_index = cluster_index >> (s->cluster_bits - > + REFCOUNT_SHIFT); > + ret = bdrv_pwrite_sync(bs->file, > + s->refcount_table_offset + > + refcount_table_index * > + sizeof(uint64_t), > + &data64, sizeof(data64)); > + if (ret < 0) { > + goto fail; > + } Plase use write_reftable_entry().
On Mon, Sep 01, 2014 at 06:52:48PM +0800, Jun Li wrote: > When every item of refcount block is NULL, free refcount block and reset the > corresponding item of refcount table with NULL. > > Signed-off-by: Jun Li <address@hidden> > --- By the way, test cases are definitely needed for this change. See tests/qemu-iotests/qcow2.py and the test cases in tests/qemu-iotests. Stefan
On Fri, 09/05 12:21, Kevin Wolf wrote: > Am 01.09.2014 um 12:52 hat Jun Li geschrieben: > > When every item of refcount block is NULL, free refcount block and reset the > > corresponding item of refcount table with NULL. > > > > Signed-off-by: Jun Li <address@hidden> > > The commit message should also describe why this is a relevant > improvement for some use case. My gut feeling is that it complicates the > code for a very minimal gain. Hi Kevin, "Add update refcount table realization for update_refcount" is nesseary for qcow2 shrinking. I will submit v3 of "qcow2: Patch for shrinking qcow2 disk image". When check the code of update_refcount, I find it lacks of this patch. Best Regards, Jun Li
Am 09.09.2014 um 04:52 hat Jun Li geschrieben: > On Fri, 09/05 12:21, Kevin Wolf wrote: > > Am 01.09.2014 um 12:52 hat Jun Li geschrieben: > > > When every item of refcount block is NULL, free refcount block and reset the > > > corresponding item of refcount table with NULL. > > > > > > Signed-off-by: Jun Li <address@hidden> > > > > The commit message should also describe why this is a relevant > > improvement for some use case. My gut feeling is that it complicates the > > code for a very minimal gain. > > Hi Kevin, > > "Add update refcount table realization for update_refcount" is nesseary for > qcow2 shrinking. I will submit v3 of "qcow2: Patch for shrinking qcow2 disk > image". When check the code of update_refcount, I find it lacks of this patch. Why is it necessary? Can't you just leave the refcount blocks allocated? They shouldn't take a lot of space. Kevin
On Tue, 09/09 10:21, Kevin Wolf wrote: > Am 09.09.2014 um 04:52 hat Jun Li geschrieben: > > On Fri, 09/05 12:21, Kevin Wolf wrote: > > > Am 01.09.2014 um 12:52 hat Jun Li geschrieben: > > > > When every item of refcount block is NULL, free refcount block and reset the > > > > corresponding item of refcount table with NULL. > > > > > > > > Signed-off-by: Jun Li <address@hidden> > > > > > > The commit message should also describe why this is a relevant > > > improvement for some use case. My gut feeling is that it complicates the > > > code for a very minimal gain. > > > > Hi Kevin, > > > > "Add update refcount table realization for update_refcount" is nesseary for > > qcow2 shrinking. I will submit v3 of "qcow2: Patch for shrinking qcow2 disk > > image". When check the code of update_refcount, I find it lacks of this patch. > > Why is it necessary? Can't you just leave the refcount blocks allocated? > They shouldn't take a lot of space. > For example: cluster_size: 64k We want to shrink a disk from 2T to 1T. one refcount block which in one cluster size can show 64k / 2B = 32k clusters. As 32k * 64k = 2G, so one refcount block will show 2G space. And (2T - 1T) / 2G = 512. So 512 refcount block will take 512 * 64k = 32M space. So when we shrink a disk from 2T to 1T, host cluster will leak at least 32M space(refcount block leak) without this patch. Above is just an example. For usual test case, we can not hit this host cluster leak, but when we do qcow2 shrinking, this will lead host cluster leak. Best Regards, Jun Li
On Fri, 09/05 16:33, Stefan Hajnoczi wrote: > On Mon, Sep 01, 2014 at 06:52:48PM +0800, Jun Li wrote: > > How does this patch handle self-describing refcount blocks? I think > they will keep the refcount block alive forever because your code will > not decide to free them. > Sorry, I have ignored self-describing refcount blocks. :) > This patch should also discard the refcount block if we decide to free > it (in the same way that we discard at cluster_offset). > > > diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c > > index 43665b8..63f36e6 100644 > > --- a/block/qcow2-refcount.c > > +++ b/block/qcow2-refcount.c > > @@ -586,6 +586,37 @@ static int QEMU_WARN_UNUSED_RESULT update_refcount(BlockDriverState *bs, > > if (refcount == 0 && s->discard_passthrough[type]) { > > update_refcount_discard(bs, cluster_offset, s->cluster_size); > > } > > + > > + /* When refcount block is NULL, update refcount table */ > > + if (block_index == 0) { > > What is the purpose of block_index == 0? Here is want to reduce the probability of running the following code. Only when block_index == 0, we will run the following code to free refcount block. > > > + int k = block_index; > > + int refcount_block_entries = s->cluster_size / sizeof(uint16_t); > > + for (k = 0; k < refcount_block_entries; k++) { > > + if (refcount_block[k] != cpu_to_be16(0)) { > > + break; > > + } > > + } > > + > > + if (k == refcount_block_entries) { > > + qemu_vfree(refcount_block); > > You can't do this, the buffer belongs to the refcount block cache. > Please look at the cache get/put as well as qcow2_cache_create/destroy. ok, thx. Should add qcow2_cache_put(bs, s->refcount_block_cache, (void**) refcount_block); > > > + /* update refcount table */ > > + unsigned int refcount_table_index; > > + uint64_t data64 = cpu_to_be64(0); > > + refcount_table_index = cluster_index >> (s->cluster_bits - > > + REFCOUNT_SHIFT); > > + ret = bdrv_pwrite_sync(bs->file, > > + s->refcount_table_offset + > > + refcount_table_index * > > + sizeof(uint64_t), > > + &data64, sizeof(data64)); > > + if (ret < 0) { > > + goto fail; > > + } > > Plase use write_reftable_entry(). ok, got it. I will submit a new version when I submit v3 of qcow2 shrinking. Best Regards, Jun Li
On Sat, Sep 13, 2014 at 11:53:58PM +0800, Jun Li wrote: > On Fri, 09/05 16:33, Stefan Hajnoczi wrote: > > On Mon, Sep 01, 2014 at 06:52:48PM +0800, Jun Li wrote: > > > > How does this patch handle self-describing refcount blocks? I think > > they will keep the refcount block alive forever because your code will > > not decide to free them. > > > > Sorry, I have ignored self-describing refcount blocks. :) For this... > > This patch should also discard the refcount block if we decide to free > > it (in the same way that we discard at cluster_offset). > > > > > diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c > > > index 43665b8..63f36e6 100644 > > > --- a/block/qcow2-refcount.c > > > +++ b/block/qcow2-refcount.c > > > @@ -586,6 +586,37 @@ static int QEMU_WARN_UNUSED_RESULT update_refcount(BlockDriverState *bs, > > > if (refcount == 0 && s->discard_passthrough[type]) { > > > update_refcount_discard(bs, cluster_offset, s->cluster_size); > > > } > > > + > > > + /* When refcount block is NULL, update refcount table */ > > > + if (block_index == 0) { > > > > What is the purpose of block_index == 0? > > Here is want to reduce the probability of running the following code. Only > when block_index == 0, we will run the following code to free refcount block. ...and this reason, I consider this approach incomplete. The approach is unreliable because a change to refcount update ordering could change leak behavior. Either free refcount blocks to avoid leaks in all cases, or don't bother. Stefan
Thanks. I will give a new version in v3 of qcow2 shrink. Jun Li 2014-9-3 上午1:12于 "Greg Kurz" <gkurz@linux.vnet.ibm.com>写道： > On Mon, 1 Sep 2014 18:52:48 +0800 > Jun Li <junmuzi@gmail.com> wrote: > > > When every item of refcount block is NULL, free refcount block and reset > the > > corresponding item of refcount table with NULL. > > > > Signed-off-by: Jun Li <address@hidden> > > --- > > > > The v2 do following change to modify some potential issue. > > > > +------- Here should start from "0". > > | > > for (k = 0; k < refcount_block_entries; k++) { > > if (refcount_block[k] != cpu_to_be16(0)) { > > ... | | > > } | | > > } | +---- Using "0" is more > safe. > > | > > +-------- This should be "k" not "++k". > > --- > > block/qcow2-refcount.c | 31 +++++++++++++++++++++++++++++++ > > 1 file changed, 31 insertions(+) > > > > diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c > > index 43665b8..63f36e6 100644 > > --- a/block/qcow2-refcount.c > > +++ b/block/qcow2-refcount.c > > @@ -586,6 +586,37 @@ static int QEMU_WARN_UNUSED_RESULT > update_refcount(BlockDriverState *bs, > > if (refcount == 0 && s->discard_passthrough[type]) { > > update_refcount_discard(bs, cluster_offset, > s->cluster_size); > > } > > + > > + /* When refcount block is NULL, update refcount table */ > > + if (block_index == 0) { > > + int k = block_index; > > Hi, > > k = 0 is also done in the for block below... > > > + int refcount_block_entries = s->cluster_size / > sizeof(uint16_t); > > It's better for maintainance to count elements in an array this way: > > int refcount_block_entries = s->cluster_size / sizeof(refcount_block[0]); > > > + for (k = 0; k < refcount_block_entries; k++) { > > + if (refcount_block[k] != cpu_to_be16(0)) { > > + break; > > + } > > + } > > + > > + if (k == refcount_block_entries) { > > + qemu_vfree(refcount_block); > > + /* update refcount table */ > > + unsigned int refcount_table_index; > > + uint64_t data64 = cpu_to_be64(0); > > + refcount_table_index = cluster_index >> > (s->cluster_bits - > > + REFCOUNT_SHIFT); > > + ret = bdrv_pwrite_sync(bs->file, > > + s->refcount_table_offset + > > + refcount_table_index * > > + sizeof(uint64_t), > > + &data64, sizeof(data64)); > > + if (ret < 0) { > > + goto fail; > > + } > > + > > + s->refcount_table[refcount_table_index] = data64; > > + > > + } > > + } > > } > > > > ret = 0; > > Cheers. > > -- > Gregory Kurz kurzgreg@fr.ibm.com > gkurz@linux.vnet.ibm.com > Software Engineer @ IBM/Meiosys http://www.ibm.com > Tel +33 (0)562 165 496 > > "Anarchy is about taking complete responsibility for yourself." > Alan Moore. > >
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c index 43665b8..63f36e6 100644 --- a/block/qcow2-refcount.c +++ b/block/qcow2-refcount.c @@ -586,6 +586,37 @@ static int QEMU_WARN_UNUSED_RESULT update_refcount(BlockDriverState *bs, if (refcount == 0 && s->discard_passthrough[type]) { update_refcount_discard(bs, cluster_offset, s->cluster_size); } + + /* When refcount block is NULL, update refcount table */ + if (block_index == 0) { + int k = block_index; + int refcount_block_entries = s->cluster_size / sizeof(uint16_t); + for (k = 0; k < refcount_block_entries; k++) { + if (refcount_block[k] != cpu_to_be16(0)) { + break; + } + } + + if (k == refcount_block_entries) { + qemu_vfree(refcount_block); + /* update refcount table */ + unsigned int refcount_table_index; + uint64_t data64 = cpu_to_be64(0); + refcount_table_index = cluster_index >> (s->cluster_bits - + REFCOUNT_SHIFT); + ret = bdrv_pwrite_sync(bs->file, + s->refcount_table_offset + + refcount_table_index * + sizeof(uint64_t), + &data64, sizeof(data64)); + if (ret < 0) { + goto fail; + } + + s->refcount_table[refcount_table_index] = data64; + + } + } } ret = 0;
When every item of refcount block is NULL, free refcount block and reset the corresponding item of refcount table with NULL. Signed-off-by: Jun Li <address@hidden> --- The v2 do following change to modify some potential issue. +------- Here should start from "0". | for (k = 0; k < refcount_block_entries; k++) { if (refcount_block[k] != cpu_to_be16(0)) { ... | | } | | } | +---- Using "0" is more safe. | +-------- This should be "k" not "++k". --- block/qcow2-refcount.c | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+)