Re: [Qemu-devel] [PATCH v2] qcow2: add update refcount table realization for update_refcount

["Benoît Canet" <benoit.canet@irqsave.net>]

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
> > > 
> > > 
>