| Message ID | 1377771363-5198-1-git-send-email-mreitz@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 08/29/2013 04:16 AM, Max Reitz wrote: > Do not try to update the refcount for zero clusters in > qcow2_update_snapshot_refcount. Why? What does this fix? (Hint - your commit message could use more details, and you should add a testsuite addition that exposes the case that you are fixing)
Am 29.08.2013 um 12:16 hat Max Reitz geschrieben: > Do not try to update the refcount for zero clusters in > qcow2_update_snapshot_refcount. > > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > block/qcow2-refcount.c | 16 +++++++++++++--- > 1 file changed, 13 insertions(+), 3 deletions(-) Please don't forget to add a test case for v2. > diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c > index 1244693..7555242 100644 > --- a/block/qcow2-refcount.c > +++ b/block/qcow2-refcount.c > @@ -863,9 +863,13 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs, > for(j = 0; j < s->l2_size; j++) { > offset = be64_to_cpu(l2_table[j]); > if (offset != 0) { > + uint64_t cluster_index; > + > old_offset = offset; > offset &= ~QCOW_OFLAG_COPIED; > - if (offset & QCOW_OFLAG_COMPRESSED) { > + > + switch (qcow2_get_cluster_type(offset)) { > + case QCOW2_CLUSTER_COMPRESSED: > nb_csectors = ((offset >> s->csize_shift) & > s->csize_mask) + 1; > if (addend != 0) { > @@ -880,8 +884,10 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs, > } > /* compressed clusters are never modified */ > refcount = 2; > - } else { > - uint64_t cluster_index = (offset & L2E_OFFSET_MASK) >> s->cluster_bits; > + break; > + > + case QCOW2_CLUSTER_NORMAL: > + cluster_index = (offset & L2E_OFFSET_MASK) >> s->cluster_bits; > if (addend != 0) { > refcount = update_cluster_refcount(bs, cluster_index, addend, > QCOW2_DISCARD_SNAPSHOT); > @@ -893,6 +899,10 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs, > ret = refcount; > goto fail; > } > + break; I think this part isn't quite right. Even zero clusters can have an allocated offset, so I think what's really needed is that the same code runs from QCOW2_CLUSTER_ZERO. However, add an if (cluster_index == 0) case for the unallocated case. > + > + default: > + refcount = 0; Other places enumerate the enum values explicitly and have a default label that simply abort()s. This will help catching forgotten places when a new value is added. > } > > if (refcount == 1) { Kevin
Am 29.08.2013 14:30, schrieb Eric Blake: > On 08/29/2013 04:16 AM, Max Reitz wrote: >> Do not try to update the refcount for zero clusters in >> qcow2_update_snapshot_refcount. > Why? What does this fix? > > (Hint - your commit message could use more details, and you should add a > testsuite addition that exposes the case that you are fixing) > Okay, I'll put more detail into the message. The test case is actually kind of contained in my series "block/qcow2: Image file option amendment", but, of course, you're right, a dedicated test case won't hurt. Max
Am 29.08.2013 14:33, schrieb Kevin Wolf: > Am 29.08.2013 um 12:16 hat Max Reitz geschrieben: >> Do not try to update the refcount for zero clusters in >> qcow2_update_snapshot_refcount. >> >> Signed-off-by: Max Reitz <mreitz@redhat.com> >> --- >> block/qcow2-refcount.c | 16 +++++++++++++--- >> 1 file changed, 13 insertions(+), 3 deletions(-) > Please don't forget to add a test case for v2. Okay. >> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c >> index 1244693..7555242 100644 >> --- a/block/qcow2-refcount.c >> +++ b/block/qcow2-refcount.c >> @@ -863,9 +863,13 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs, >> for(j = 0; j < s->l2_size; j++) { >> offset = be64_to_cpu(l2_table[j]); >> if (offset != 0) { >> + uint64_t cluster_index; >> + >> old_offset = offset; >> offset &= ~QCOW_OFLAG_COPIED; >> - if (offset & QCOW_OFLAG_COMPRESSED) { >> + >> + switch (qcow2_get_cluster_type(offset)) { >> + case QCOW2_CLUSTER_COMPRESSED: >> nb_csectors = ((offset >> s->csize_shift) & >> s->csize_mask) + 1; >> if (addend != 0) { >> @@ -880,8 +884,10 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs, >> } >> /* compressed clusters are never modified */ >> refcount = 2; >> - } else { >> - uint64_t cluster_index = (offset & L2E_OFFSET_MASK) >> s->cluster_bits; >> + break; >> + >> + case QCOW2_CLUSTER_NORMAL: >> + cluster_index = (offset & L2E_OFFSET_MASK) >> s->cluster_bits; >> if (addend != 0) { >> refcount = update_cluster_refcount(bs, cluster_index, addend, >> QCOW2_DISCARD_SNAPSHOT); >> @@ -893,6 +899,10 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs, >> ret = refcount; >> goto fail; >> } >> + break; > I think this part isn't quite right. Even zero clusters can have an > allocated offset, so I think what's really needed is that the same code > runs from QCOW2_CLUSTER_ZERO. However, add an if (cluster_index == 0) > case for the unallocated case. So then it would be enough to change: - if (offset != 0) { + if ((offset & L2E_OFFSET_MASK) != 0) { and just execute the same code for QCOW2_CLUSTER_NORMAL and QCOW2_CLUSTER_ZERO? >> + >> + default: >> + refcount = 0; > Other places enumerate the enum values explicitly and have a default > label that simply abort()s. This will help catching forgotten places > when a new value is added. Okay. >> } >> >> if (refcount == 1) { > Kevin Max
Am 29.08.2013 um 14:38 hat Max Reitz geschrieben: > Am 29.08.2013 14:33, schrieb Kevin Wolf: > >Am 29.08.2013 um 12:16 hat Max Reitz geschrieben: > >>Do not try to update the refcount for zero clusters in > >>qcow2_update_snapshot_refcount. > >> > >>Signed-off-by: Max Reitz <mreitz@redhat.com> > >>--- > >> block/qcow2-refcount.c | 16 +++++++++++++--- > >> 1 file changed, 13 insertions(+), 3 deletions(-) > >Please don't forget to add a test case for v2. > Okay. > > >>diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c > >>index 1244693..7555242 100644 > >>--- a/block/qcow2-refcount.c > >>+++ b/block/qcow2-refcount.c > >>@@ -863,9 +863,13 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs, > >> for(j = 0; j < s->l2_size; j++) { > >> offset = be64_to_cpu(l2_table[j]); > >> if (offset != 0) { > >>+ uint64_t cluster_index; > >>+ > >> old_offset = offset; > >> offset &= ~QCOW_OFLAG_COPIED; > >>- if (offset & QCOW_OFLAG_COMPRESSED) { > >>+ > >>+ switch (qcow2_get_cluster_type(offset)) { > >>+ case QCOW2_CLUSTER_COMPRESSED: > >> nb_csectors = ((offset >> s->csize_shift) & > >> s->csize_mask) + 1; > >> if (addend != 0) { > >>@@ -880,8 +884,10 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs, > >> } > >> /* compressed clusters are never modified */ > >> refcount = 2; > >>- } else { > >>- uint64_t cluster_index = (offset & L2E_OFFSET_MASK) >> s->cluster_bits; > >>+ break; > >>+ > >>+ case QCOW2_CLUSTER_NORMAL: > >>+ cluster_index = (offset & L2E_OFFSET_MASK) >> s->cluster_bits; > >> if (addend != 0) { > >> refcount = update_cluster_refcount(bs, cluster_index, addend, > >> QCOW2_DISCARD_SNAPSHOT); > >>@@ -893,6 +899,10 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs, > >> ret = refcount; > >> goto fail; > >> } > >>+ break; > >I think this part isn't quite right. Even zero clusters can have an > >allocated offset, so I think what's really needed is that the same code > >runs from QCOW2_CLUSTER_ZERO. However, add an if (cluster_index == 0) > >case for the unallocated case. > So then it would be enough to change: > > - if (offset != 0) { > + if ((offset & L2E_OFFSET_MASK) != 0) { > > and just execute the same code for QCOW2_CLUSTER_NORMAL and > QCOW2_CLUSTER_ZERO? Doing only this change in the original code might happen to work, but it's not really clean as the L2 entry format is different for compressed clusters (L2E_OFFSET_MASK isn't valid for it). So having the switch for the cluster type is a good thing, but the implementation for normal and zero clusters should be shared indeed. The offset != 0 check ends up redundant when you introduce the switch, so in theory it could as well be removed. Kevin
Am 29.08.2013 14:53, schrieb Kevin Wolf: > Am 29.08.2013 um 14:38 hat Max Reitz geschrieben: >> Am 29.08.2013 14:33, schrieb Kevin Wolf: >>> Am 29.08.2013 um 12:16 hat Max Reitz geschrieben: >>>> Do not try to update the refcount for zero clusters in >>>> qcow2_update_snapshot_refcount. >>>> >>>> Signed-off-by: Max Reitz <mreitz@redhat.com> >>>> --- >>>> block/qcow2-refcount.c | 16 +++++++++++++--- >>>> 1 file changed, 13 insertions(+), 3 deletions(-) >>> Please don't forget to add a test case for v2. >> Okay. >> >>>> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c >>>> index 1244693..7555242 100644 >>>> --- a/block/qcow2-refcount.c >>>> +++ b/block/qcow2-refcount.c >>>> @@ -863,9 +863,13 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs, >>>> for(j = 0; j < s->l2_size; j++) { >>>> offset = be64_to_cpu(l2_table[j]); >>>> if (offset != 0) { >>>> + uint64_t cluster_index; >>>> + >>>> old_offset = offset; >>>> offset &= ~QCOW_OFLAG_COPIED; >>>> - if (offset & QCOW_OFLAG_COMPRESSED) { >>>> + >>>> + switch (qcow2_get_cluster_type(offset)) { >>>> + case QCOW2_CLUSTER_COMPRESSED: >>>> nb_csectors = ((offset >> s->csize_shift) & >>>> s->csize_mask) + 1; >>>> if (addend != 0) { >>>> @@ -880,8 +884,10 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs, >>>> } >>>> /* compressed clusters are never modified */ >>>> refcount = 2; >>>> - } else { >>>> - uint64_t cluster_index = (offset & L2E_OFFSET_MASK) >> s->cluster_bits; >>>> + break; >>>> + >>>> + case QCOW2_CLUSTER_NORMAL: >>>> + cluster_index = (offset & L2E_OFFSET_MASK) >> s->cluster_bits; >>>> if (addend != 0) { >>>> refcount = update_cluster_refcount(bs, cluster_index, addend, >>>> QCOW2_DISCARD_SNAPSHOT); >>>> @@ -893,6 +899,10 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs, >>>> ret = refcount; >>>> goto fail; >>>> } >>>> + break; >>> I think this part isn't quite right. Even zero clusters can have an >>> allocated offset, so I think what's really needed is that the same code >>> runs from QCOW2_CLUSTER_ZERO. However, add an if (cluster_index == 0) >>> case for the unallocated case. >> So then it would be enough to change: >> >> - if (offset != 0) { >> + if ((offset & L2E_OFFSET_MASK) != 0) { >> >> and just execute the same code for QCOW2_CLUSTER_NORMAL and >> QCOW2_CLUSTER_ZERO? > Doing only this change in the original code might happen to work, but > it's not really clean as the L2 entry format is different for compressed > clusters (L2E_OFFSET_MASK isn't valid for it). So having the switch for > the cluster type is a good thing, but the implementation for normal and > zero clusters should be shared indeed. Yes, of course I meant to include the switch as well. > The offset != 0 check ends up redundant when you introduce the switch, > so in theory it could as well be removed. > Well, I don't know; for zero clusters, the if statement is required anyway (since qcow2_get_cluster_type does not distinguish between allocated and non-allocated zero clusters), so I though we might as well just leave it where it is (just slightly adjusted in the way I proposed). Max
Am 29.08.2013 um 14:56 hat Max Reitz geschrieben: > Am 29.08.2013 14:53, schrieb Kevin Wolf: > >Am 29.08.2013 um 14:38 hat Max Reitz geschrieben: > >>Am 29.08.2013 14:33, schrieb Kevin Wolf: > >>>Am 29.08.2013 um 12:16 hat Max Reitz geschrieben: > >>>>Do not try to update the refcount for zero clusters in > >>>>qcow2_update_snapshot_refcount. > >>>> > >>>>Signed-off-by: Max Reitz <mreitz@redhat.com> > >>>>--- > >>>> block/qcow2-refcount.c | 16 +++++++++++++--- > >>>> 1 file changed, 13 insertions(+), 3 deletions(-) > >>>Please don't forget to add a test case for v2. > >>Okay. > >> > >>>>diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c > >>>>index 1244693..7555242 100644 > >>>>--- a/block/qcow2-refcount.c > >>>>+++ b/block/qcow2-refcount.c > >>>>@@ -863,9 +863,13 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs, > >>>> for(j = 0; j < s->l2_size; j++) { > >>>> offset = be64_to_cpu(l2_table[j]); > >>>> if (offset != 0) { > >>>>+ uint64_t cluster_index; > >>>>+ > >>>> old_offset = offset; > >>>> offset &= ~QCOW_OFLAG_COPIED; > >>>>- if (offset & QCOW_OFLAG_COMPRESSED) { > >>>>+ > >>>>+ switch (qcow2_get_cluster_type(offset)) { > >>>>+ case QCOW2_CLUSTER_COMPRESSED: > >>>> nb_csectors = ((offset >> s->csize_shift) & > >>>> s->csize_mask) + 1; > >>>> if (addend != 0) { > >>>>@@ -880,8 +884,10 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs, > >>>> } > >>>> /* compressed clusters are never modified */ > >>>> refcount = 2; > >>>>- } else { > >>>>- uint64_t cluster_index = (offset & L2E_OFFSET_MASK) >> s->cluster_bits; > >>>>+ break; > >>>>+ > >>>>+ case QCOW2_CLUSTER_NORMAL: > >>>>+ cluster_index = (offset & L2E_OFFSET_MASK) >> s->cluster_bits; > >>>> if (addend != 0) { > >>>> refcount = update_cluster_refcount(bs, cluster_index, addend, > >>>> QCOW2_DISCARD_SNAPSHOT); > >>>>@@ -893,6 +899,10 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs, > >>>> ret = refcount; > >>>> goto fail; > >>>> } > >>>>+ break; > >>>I think this part isn't quite right. Even zero clusters can have an > >>>allocated offset, so I think what's really needed is that the same code > >>>runs from QCOW2_CLUSTER_ZERO. However, add an if (cluster_index == 0) > >>>case for the unallocated case. > >>So then it would be enough to change: > >> > >>- if (offset != 0) { > >>+ if ((offset & L2E_OFFSET_MASK) != 0) { > >> > >>and just execute the same code for QCOW2_CLUSTER_NORMAL and > >>QCOW2_CLUSTER_ZERO? > >Doing only this change in the original code might happen to work, but > >it's not really clean as the L2 entry format is different for compressed > >clusters (L2E_OFFSET_MASK isn't valid for it). So having the switch for > >the cluster type is a good thing, but the implementation for normal and > >zero clusters should be shared indeed. > Yes, of course I meant to include the switch as well. > > >The offset != 0 check ends up redundant when you introduce the switch, > >so in theory it could as well be removed. > > > Well, I don't know; for zero clusters, the if statement is required > anyway (since qcow2_get_cluster_type does not distinguish between > allocated and non-allocated zero clusters), so I though we might as > well just leave it where it is (just slightly adjusted in the way I > proposed). You do need a zero check, but it's only for normal/zero clusters, not for compressed ones; and it must check offset & L2E_OFFSET_MASK (or cluster_index) rather than offset itself. So I think you end up removing the outer if (offset != 0) and adding a new inner check in case QCOW2_CLUSTER_NORMAL/ZERO. Kevin
Am 29.08.2013 15:04, schrieb Kevin Wolf: > Am 29.08.2013 um 14:56 hat Max Reitz geschrieben: >> Am 29.08.2013 14:53, schrieb Kevin Wolf: >>> Am 29.08.2013 um 14:38 hat Max Reitz geschrieben: >>>> Am 29.08.2013 14:33, schrieb Kevin Wolf: >>>>> Am 29.08.2013 um 12:16 hat Max Reitz geschrieben: >>>>>> Do not try to update the refcount for zero clusters in >>>>>> qcow2_update_snapshot_refcount. >>>>>> >>>>>> Signed-off-by: Max Reitz <mreitz@redhat.com> >>>>>> --- >>>>>> block/qcow2-refcount.c | 16 +++++++++++++--- >>>>>> 1 file changed, 13 insertions(+), 3 deletions(-) >>>>> Please don't forget to add a test case for v2. >>>> Okay. >>>> >>>>>> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c >>>>>> index 1244693..7555242 100644 >>>>>> --- a/block/qcow2-refcount.c >>>>>> +++ b/block/qcow2-refcount.c >>>>>> @@ -863,9 +863,13 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs, >>>>>> for(j = 0; j < s->l2_size; j++) { >>>>>> offset = be64_to_cpu(l2_table[j]); >>>>>> if (offset != 0) { >>>>>> + uint64_t cluster_index; >>>>>> + >>>>>> old_offset = offset; >>>>>> offset &= ~QCOW_OFLAG_COPIED; >>>>>> - if (offset & QCOW_OFLAG_COMPRESSED) { >>>>>> + >>>>>> + switch (qcow2_get_cluster_type(offset)) { >>>>>> + case QCOW2_CLUSTER_COMPRESSED: >>>>>> nb_csectors = ((offset >> s->csize_shift) & >>>>>> s->csize_mask) + 1; >>>>>> if (addend != 0) { >>>>>> @@ -880,8 +884,10 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs, >>>>>> } >>>>>> /* compressed clusters are never modified */ >>>>>> refcount = 2; >>>>>> - } else { >>>>>> - uint64_t cluster_index = (offset & L2E_OFFSET_MASK) >> s->cluster_bits; >>>>>> + break; >>>>>> + >>>>>> + case QCOW2_CLUSTER_NORMAL: >>>>>> + cluster_index = (offset & L2E_OFFSET_MASK) >> s->cluster_bits; >>>>>> if (addend != 0) { >>>>>> refcount = update_cluster_refcount(bs, cluster_index, addend, >>>>>> QCOW2_DISCARD_SNAPSHOT); >>>>>> @@ -893,6 +899,10 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs, >>>>>> ret = refcount; >>>>>> goto fail; >>>>>> } >>>>>> + break; >>>>> I think this part isn't quite right. Even zero clusters can have an >>>>> allocated offset, so I think what's really needed is that the same code >>>>> runs from QCOW2_CLUSTER_ZERO. However, add an if (cluster_index == 0) >>>>> case for the unallocated case. >>>> So then it would be enough to change: >>>> >>>> - if (offset != 0) { >>>> + if ((offset & L2E_OFFSET_MASK) != 0) { >>>> >>>> and just execute the same code for QCOW2_CLUSTER_NORMAL and >>>> QCOW2_CLUSTER_ZERO? >>> Doing only this change in the original code might happen to work, but >>> it's not really clean as the L2 entry format is different for compressed >>> clusters (L2E_OFFSET_MASK isn't valid for it). So having the switch for >>> the cluster type is a good thing, but the implementation for normal and >>> zero clusters should be shared indeed. >> Yes, of course I meant to include the switch as well. >> >>> The offset != 0 check ends up redundant when you introduce the switch, >>> so in theory it could as well be removed. >>> >> Well, I don't know; for zero clusters, the if statement is required >> anyway (since qcow2_get_cluster_type does not distinguish between >> allocated and non-allocated zero clusters), so I though we might as >> well just leave it where it is (just slightly adjusted in the way I >> proposed). > You do need a zero check, but it's only for normal/zero clusters, not > for compressed ones; and it must check offset & L2E_OFFSET_MASK (or > cluster_index) rather than offset itself. > > So I think you end up removing the outer if (offset != 0) and adding a > new inner check in case QCOW2_CLUSTER_NORMAL/ZERO. > > Kevin Ah, yes, you're right. Max
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c index 1244693..7555242 100644 --- a/block/qcow2-refcount.c +++ b/block/qcow2-refcount.c @@ -863,9 +863,13 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs, for(j = 0; j < s->l2_size; j++) { offset = be64_to_cpu(l2_table[j]); if (offset != 0) { + uint64_t cluster_index; + old_offset = offset; offset &= ~QCOW_OFLAG_COPIED; - if (offset & QCOW_OFLAG_COMPRESSED) { + + switch (qcow2_get_cluster_type(offset)) { + case QCOW2_CLUSTER_COMPRESSED: nb_csectors = ((offset >> s->csize_shift) & s->csize_mask) + 1; if (addend != 0) { @@ -880,8 +884,10 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs, } /* compressed clusters are never modified */ refcount = 2; - } else { - uint64_t cluster_index = (offset & L2E_OFFSET_MASK) >> s->cluster_bits; + break; + + case QCOW2_CLUSTER_NORMAL: + cluster_index = (offset & L2E_OFFSET_MASK) >> s->cluster_bits; if (addend != 0) { refcount = update_cluster_refcount(bs, cluster_index, addend, QCOW2_DISCARD_SNAPSHOT); @@ -893,6 +899,10 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs, ret = refcount; goto fail; } + break; + + default: + refcount = 0; } if (refcount == 1) {
Do not try to update the refcount for zero clusters in qcow2_update_snapshot_refcount. Signed-off-by: Max Reitz <mreitz@redhat.com> --- block/qcow2-refcount.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-)