Re: [Qemu-devel] [PATCH 2/4] qcow2: Allow update_cluster_refcount() to update several clusters refcount.

[Laurent Vivier <Laurent.Vivier@bull.net>]

Le jeudi 06 novembre 2008 à 19:11 +0100, Kevin Wolf a écrit :
> Laurent Vivier schrieb:
> > pièce jointe document texte brut
> > (0002-Allow-update_cluster_refcount-to-update-several-cl.patch)
> > To improve performance when the qcow2 file is empty, this patch
> > allows update_cluster_refcount() to update refcount of
> > several clusters.
> > 
> > Signed-off-by: Laurent Vivier <Laurent.Vivier@bull.net>
> > ---
> >  block-qcow2.c |  107 ++++++++++++++++++++++++++++++++++++----------------------
> >  1 file changed, 68 insertions(+), 39 deletions(-)
> > 
> > Index: qemu/block-qcow2.c
> > ===================================================================
> > --- qemu.orig/block-qcow2.c	2008-11-06 16:40:44.000000000 +0100
> > +++ qemu/block-qcow2.c	2008-11-06 16:40:45.000000000 +0100
> > @@ -159,6 +159,7 @@ static void refcount_close(BlockDriverSt
> >  static int get_refcount(BlockDriverState *bs, int64_t cluster_index);
> >  static int update_cluster_refcount(BlockDriverState *bs,
> >                                     int64_t cluster_index,
> > +                                   int nb_clusters,
> >                                     int addend);
> >  static void update_refcount(BlockDriverState *bs,
> >                              int64_t offset, int64_t length,
> > @@ -1711,7 +1712,7 @@ static int update_snapshot_refcount(Bloc
> >                          refcount = 2;
> >                      } else {
> >                          if (addend != 0) {
> > -                            refcount = update_cluster_refcount(bs, offset >> s->cluster_bits, addend);
> > +                            refcount = update_cluster_refcount(bs, offset >> s->cluster_bits, 1, addend);
> >                          } else {
> >                              refcount = get_refcount(bs, offset >> s->cluster_bits);
> >                          }
> > @@ -1733,7 +1734,7 @@ static int update_snapshot_refcount(Bloc
> >              }
> >  
> >              if (addend != 0) {
> > -                refcount = update_cluster_refcount(bs, l2_offset >> s->cluster_bits, addend);
> > +                refcount = update_cluster_refcount(bs, l2_offset >> s->cluster_bits, 1, addend);
> >              } else {
> >                  refcount = get_refcount(bs, l2_offset >> s->cluster_bits);
> >              }
> > @@ -2292,14 +2293,14 @@ static int64_t alloc_bytes(BlockDriverSt
> >          if (free_in_cluster == 0)
> >              s->free_byte_offset = 0;
> >          if ((offset & (s->cluster_size - 1)) != 0)
> > -            update_cluster_refcount(bs, offset >> s->cluster_bits, 1);
> > +            update_cluster_refcount(bs, offset >> s->cluster_bits, 1, 1);
> >      } else {
> >          offset = alloc_clusters(bs, s->cluster_size);
> >          cluster_offset = s->free_byte_offset & ~(s->cluster_size - 1);
> >          if ((cluster_offset + s->cluster_size) == offset) {
> >              /* we are lucky: contiguous data */
> >              offset = s->free_byte_offset;
> > -            update_cluster_refcount(bs, offset >> s->cluster_bits, 1);
> > +            update_cluster_refcount(bs, offset >> s->cluster_bits, 1, 1);
> >              s->free_byte_offset += size;
> >          } else {
> >              s->free_byte_offset = offset;
> > @@ -2389,46 +2390,77 @@ static int grow_refcount_table(BlockDriv
> >  /* XXX: cache several refcount block clusters ? */
> >  static int update_cluster_refcount(BlockDriverState *bs,
> >                                     int64_t cluster_index,
> > +                                   int nb_clusters,
> >                                     int addend)
> >  {
> >      BDRVQcowState *s = bs->opaque;
> >      int64_t refcount_block_offset;
> > -    int ret, refcount_table_index, block_index, refcount;
> > +    int ret, refcount_table_index, refcount_table_last_index, block_index, refcount;
> > +    int nb_block_index;
> > +    int refcount_cache_size;
> >  
> > -    refcount_table_index = cluster_index >> (s->cluster_bits - REFCOUNT_SHIFT);
> > -    if (refcount_table_index >= s->refcount_table_size) {
> > +    if (nb_clusters == 0)
> > +        return 0;
> > +
> > +    refcount_table_last_index = (cluster_index + nb_clusters - 1) >>
> > +                                (s->cluster_bits - REFCOUNT_SHIFT);
> > +
> > +    /* grow the refcount table if needed */
> > +
> > +    if (refcount_table_last_index >= s->refcount_table_size) {
> >          if (addend < 0)
> >              return -EINVAL;
> > -        ret = grow_refcount_table(bs, refcount_table_index + 1);
> > +        ret = grow_refcount_table(bs, refcount_table_last_index + 1);
> >          if (ret < 0)
> >              return ret;
> >      }
> > -    refcount_block_offset = s->refcount_table[refcount_table_index];
> > -    if (!refcount_block_offset) {
> > -        if (addend < 0)
> > -            return -EINVAL;
> > -        refcount_block_offset = alloc_refcount_block(bs, refcount_table_index);
> > -        if (refcount_block_offset < 0)
> > -            return -EINVAL;
> > -    } else {
> > -        if (load_refcount_block(bs, refcount_block_offset) < 0)
> > +
> > +    while (nb_clusters) {
> > +        refcount_table_index = cluster_index >>
> > +                               (s->cluster_bits - REFCOUNT_SHIFT);
> > +        refcount_block_offset = s->refcount_table[refcount_table_index];
> 
> I guess (comment? ;-)) this outer loop is for handling requests which
> span multiple refcount blocks? If so, am I missing something or is

Yes,

> refcount_block_offset the same for each iteration because cluster_index
> never changes?

You're right, there is a missing "cluster_index++" at the end of this
loop.

> > +
> > +        if (!refcount_block_offset) {
> > +            if (addend < 0)
> > +                return -EINVAL;
> > +            refcount_block_offset = alloc_refcount_block(bs, refcount_table_index);
> > +            if (refcount_block_offset < 0)
> > +		    return -EINVAL;
> > +        } else {
> > +            if (load_refcount_block(bs, refcount_block_offset) < 0)
> > +                return -EIO;
> > +        }
> > +
> > +        /* we can update the count and save it */
> > +
> > +        refcount_cache_size = 1 << (s->cluster_bits - REFCOUNT_SHIFT);
> > +        nb_block_index = 0;
> 
> I have to admit that nb_block_index is a name where I can't image what
> the variable might be for. Seems to be a counter for the changed
> refcounts in the current refcount block, and block_index seems to be the
> first index to be touched in this block. What about first_index and
> cur_refcount or something like that? (I don't like these suggestions too
> much, either. Maybe someone has better names.)
> 
> > +        block_index = cluster_index & (refcount_cache_size - 1);
> > +        refcount = 0;
> > +        while (nb_clusters &&
> > +               block_index + nb_block_index < refcount_cache_size) {
> > +
> > +            refcount = be16_to_cpu(
> > +                         s->refcount_block_cache[block_index + nb_block_index]);
> > +            refcount += addend;
> > +            if (refcount < 0 || refcount > 0xffff)
> > +                return -EINVAL;
> 
> Here we possibly abort in the middle of the operation. If it fails
> somewhere in the fifth refcount block, what happens with the four
> already updated blocks?

Yes, you're right. I think we have at least to save refcount we have
updated.

> 
> > +            if (refcount == 0 &&
> > +                cluster_index + nb_block_index < s->free_cluster_index) {
> > +                s->free_cluster_index = cluster_index + nb_block_index;
> > +            }
> > +            s->refcount_block_cache[block_index + nb_block_index] =
> > +                                                          cpu_to_be16(refcount);
> > +            nb_block_index++;
> > +            nb_clusters--;
> > +        }
> > +        if (bdrv_pwrite(s->hd,
> > +                        refcount_block_offset + (block_index << REFCOUNT_SHIFT),
> > +                        s->refcount_block_cache + block_index,
> > +                        nb_block_index * sizeof(uint16_t)) !=
> > +                        nb_block_index * sizeof(uint16_t))
> >              return -EIO;
> 
> Same here.

I think we are not worst than the original behavior. I don't know how to
manage this better.

> >      }
> > -    /* we can update the count and save it */
> > -    block_index = cluster_index &
> > -        ((1 << (s->cluster_bits - REFCOUNT_SHIFT)) - 1);
> > -    refcount = be16_to_cpu(s->refcount_block_cache[block_index]);
> > -    refcount += addend;
> > -    if (refcount < 0 || refcount > 0xffff)
> > -        return -EINVAL;
> > -    if (refcount == 0 && cluster_index < s->free_cluster_index) {
> > -        s->free_cluster_index = cluster_index;
> > -    }
> > -    s->refcount_block_cache[block_index] = cpu_to_be16(refcount);
> > -    if (bdrv_pwrite(s->hd,
> > -                    refcount_block_offset + (block_index << REFCOUNT_SHIFT),
> > -                    &s->refcount_block_cache[block_index], 2) != 2)
> > -        return -EIO;
> >      return refcount;
> >  }
> >  
> > @@ -2437,7 +2469,7 @@ static void update_refcount(BlockDriverS
> >                              int addend)
> >  {
> >      BDRVQcowState *s = bs->opaque;
> > -    int64_t start, last, cluster_offset;
> > +    int64_t start, last;
> >  
> >  #ifdef DEBUG_ALLOC2
> >      printf("update_refcount: offset=%lld size=%lld addend=%d\n",
> > @@ -2445,12 +2477,9 @@ static void update_refcount(BlockDriverS
> >  #endif
> >      if (length <= 0)
> >          return;
> > -    start = offset & ~(s->cluster_size - 1);
> > -    last = (offset + length - 1) & ~(s->cluster_size - 1);
> > -    for(cluster_offset = start; cluster_offset <= last;
> > -        cluster_offset += s->cluster_size) {
> > -        update_cluster_refcount(bs, cluster_offset >> s->cluster_bits, addend);
> > -    }
> > +    start = offset >> s->cluster_bits;
> > +    last = (offset + length) >> s->cluster_bits;
> 
> Off by one for length % cluster_size == 0?

Explain, please (but notice the "<=" in the "for (...)").

> > +    update_cluster_refcount(bs, start, last - start + 1, addend);
> >  }
> >  
> >  #ifdef DEBUG_ALLOC
> > 
> 
> Kevin
> 
-- 
------------------ Laurent.Vivier@bull.net  ------------------
"Tout ce qui est impossible reste à accomplir"    Jules Verne
"Things are only impossible until they're not" Jean-Luc Picard