Re: [Qemu-devel] [PATCH] qcow2-refcount: Snapshot update for zero clusters

[Max Reitz]

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 <address@hidden>
> > > > ---
> > > >   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