* Re: [Qemu-devel] [PATCH v2] qcow2: add update refcount table realization for update_refcount
2014-09-01 10:52 [Qemu-devel] [PATCH v2] qcow2: add update refcount table realization for update_refcount Jun Li
@ 2014-09-01 11:11 ` Benoît Canet
2014-09-01 16:04 ` Jun Li
2014-09-02 17:12 ` Greg Kurz
` (3 subsequent siblings)
4 siblings, 1 reply; 14+ messages in thread
From: Benoît Canet @ 2014-09-01 11:11 UTC (permalink / raw)
To: Jun Li; +Cc: kwolf, juli, famz, qemu-devel, stefanha
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
>
>
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [Qemu-devel] [PATCH v2] qcow2: add update refcount table realization for update_refcount
2014-09-01 11:11 ` Benoît Canet
@ 2014-09-01 16:04 ` Jun Li
2014-09-02 13:38 ` Benoît Canet
0 siblings, 1 reply; 14+ messages in thread
From: Jun Li @ 2014-09-01 16:04 UTC (permalink / raw)
To: Benoît Canet; +Cc: kwolf, juli, famz, qemu-devel, stefanha
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
> >
> >
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [Qemu-devel] [PATCH v2] qcow2: add update refcount table realization for update_refcount
2014-09-01 16:04 ` Jun Li
@ 2014-09-02 13:38 ` Benoît Canet
0 siblings, 0 replies; 14+ messages in thread
From: Benoît Canet @ 2014-09-02 13:38 UTC (permalink / raw)
To: Jun Li; +Cc: Benoît Canet, kwolf, famz, juli, qemu-devel, stefanha
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
> > >
> > >
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH v2] qcow2: add update refcount table realization for update_refcount
2014-09-01 10:52 [Qemu-devel] [PATCH v2] qcow2: add update refcount table realization for update_refcount Jun Li
2014-09-01 11:11 ` Benoît Canet
@ 2014-09-02 17:12 ` Greg Kurz
2014-09-22 1:45 ` jun muzi
2014-09-05 10:21 ` Kevin Wolf
` (2 subsequent siblings)
4 siblings, 1 reply; 14+ messages in thread
From: Greg Kurz @ 2014-09-02 17:12 UTC (permalink / raw)
To: Jun Li; +Cc: kwolf, juli, famz, qemu-devel, stefanha
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.
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [Qemu-devel] [PATCH v2] qcow2: add update refcount table realization for update_refcount
2014-09-02 17:12 ` Greg Kurz
@ 2014-09-22 1:45 ` jun muzi
0 siblings, 0 replies; 14+ messages in thread
From: jun muzi @ 2014-09-22 1:45 UTC (permalink / raw)
To: Greg Kurz; +Cc: kwolf, Fam Zheng, qemu-devel, stefanha
[-- Attachment #1: Type: text/plain, Size: 3568 bytes --]
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.
>
>
[-- Attachment #2: Type: text/html, Size: 4937 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH v2] qcow2: add update refcount table realization for update_refcount
2014-09-01 10:52 [Qemu-devel] [PATCH v2] qcow2: add update refcount table realization for update_refcount Jun Li
2014-09-01 11:11 ` Benoît Canet
2014-09-02 17:12 ` Greg Kurz
@ 2014-09-05 10:21 ` Kevin Wolf
2014-09-09 2:52 ` Jun Li
2014-09-05 15:33 ` Stefan Hajnoczi
2014-09-05 15:34 ` Stefan Hajnoczi
4 siblings, 1 reply; 14+ messages in thread
From: Kevin Wolf @ 2014-09-05 10:21 UTC (permalink / raw)
To: Jun Li; +Cc: famz, juli, qemu-devel, stefanha
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH v2] qcow2: add update refcount table realization for update_refcount
2014-09-05 10:21 ` Kevin Wolf
@ 2014-09-09 2:52 ` Jun Li
2014-09-09 8:21 ` Kevin Wolf
0 siblings, 1 reply; 14+ messages in thread
From: Jun Li @ 2014-09-09 2:52 UTC (permalink / raw)
To: Kevin Wolf; +Cc: famz, juli, qemu-devel, stefanha
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH v2] qcow2: add update refcount table realization for update_refcount
2014-09-09 2:52 ` Jun Li
@ 2014-09-09 8:21 ` Kevin Wolf
2014-09-09 14:04 ` Jun Li
0 siblings, 1 reply; 14+ messages in thread
From: Kevin Wolf @ 2014-09-09 8:21 UTC (permalink / raw)
To: Jun Li; +Cc: famz, juli, qemu-devel, stefanha
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH v2] qcow2: add update refcount table realization for update_refcount
2014-09-09 8:21 ` Kevin Wolf
@ 2014-09-09 14:04 ` Jun Li
0 siblings, 0 replies; 14+ messages in thread
From: Jun Li @ 2014-09-09 14:04 UTC (permalink / raw)
To: Kevin Wolf; +Cc: famz, juli, qemu-devel, stefanha
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH v2] qcow2: add update refcount table realization for update_refcount
2014-09-01 10:52 [Qemu-devel] [PATCH v2] qcow2: add update refcount table realization for update_refcount Jun Li
` (2 preceding siblings ...)
2014-09-05 10:21 ` Kevin Wolf
@ 2014-09-05 15:33 ` Stefan Hajnoczi
2014-09-13 15:53 ` Jun Li
2014-09-05 15:34 ` Stefan Hajnoczi
4 siblings, 1 reply; 14+ messages in thread
From: Stefan Hajnoczi @ 2014-09-05 15:33 UTC (permalink / raw)
To: Jun Li; +Cc: kwolf, juli, famz, qemu-devel, stefanha
[-- Attachment #1: Type: text/plain, Size: 2187 bytes --]
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().
[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [Qemu-devel] [PATCH v2] qcow2: add update refcount table realization for update_refcount
2014-09-05 15:33 ` Stefan Hajnoczi
@ 2014-09-13 15:53 ` Jun Li
2014-09-15 9:27 ` Stefan Hajnoczi
0 siblings, 1 reply; 14+ messages in thread
From: Jun Li @ 2014-09-13 15:53 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: kwolf, juli, famz, qemu-devel, stefanha
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
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [Qemu-devel] [PATCH v2] qcow2: add update refcount table realization for update_refcount
2014-09-13 15:53 ` Jun Li
@ 2014-09-15 9:27 ` Stefan Hajnoczi
0 siblings, 0 replies; 14+ messages in thread
From: Stefan Hajnoczi @ 2014-09-15 9:27 UTC (permalink / raw)
To: Jun Li; +Cc: kwolf, Stefan Hajnoczi, juli, famz, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 1618 bytes --]
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
[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH v2] qcow2: add update refcount table realization for update_refcount
2014-09-01 10:52 [Qemu-devel] [PATCH v2] qcow2: add update refcount table realization for update_refcount Jun Li
` (3 preceding siblings ...)
2014-09-05 15:33 ` Stefan Hajnoczi
@ 2014-09-05 15:34 ` Stefan Hajnoczi
4 siblings, 0 replies; 14+ messages in thread
From: Stefan Hajnoczi @ 2014-09-05 15:34 UTC (permalink / raw)
To: Jun Li; +Cc: kwolf, juli, famz, qemu-devel, stefanha
[-- Attachment #1: Type: text/plain, Size: 392 bytes --]
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
[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread