Re: [Qemu-devel] [PATCH v5 08/26] qcow2: Refcount overflow and qcow2_alloc_bytes()

[Max Reitz]

On 2015-02-04 at 06:55, Kevin Wolf wrote:
> Am 15.12.2014 um 13:50 hat Max Reitz geschrieben:
> > qcow2_alloc_bytes() may reuse a cluster multiple times, in which case
> > the refcount is increased accordingly. However, if this would lead to an
> > overflow the function should instead just not reuse this cluster and
> > allocate a new one.
> >
> > Signed-off-by: Max Reitz <address@hidden>
> > Reviewed-by: Eric Blake <address@hidden>
> > Reviewed-by: Stefan Hajnoczi <address@hidden>
> > ---
> >   block/qcow2-refcount.c | 31 ++++++++++++++++++++++++++++++-
> >   1 file changed, 30 insertions(+), 1 deletion(-)
> >
> > diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
> > index db81647..fd28a13 100644
> > --- a/block/qcow2-refcount.c
> > +++ b/block/qcow2-refcount.c
> > @@ -780,9 +780,11 @@ int64_t qcow2_alloc_bytes(BlockDriverState *bs, int size)
> >       BDRVQcowState *s = bs->opaque;
> >       int64_t offset, cluster_offset, new_cluster;
> >       int free_in_cluster, ret;
> > +    uint64_t refcount;
> >   
> >       BLKDBG_EVENT(bs->file, BLKDBG_CLUSTER_ALLOC_BYTES);
> >       assert(size > 0 && size <= s->cluster_size);
> > + redo:
> >       if (s->free_byte_offset == 0) {
> >           offset = qcow2_alloc_clusters(bs, s->cluster_size);
> >           if (offset < 0) {
> > @@ -790,12 +792,25 @@ int64_t qcow2_alloc_bytes(BlockDriverState *bs, int size)
> >           }
> >           s->free_byte_offset = offset;
> >       }
> > - redo:
> > +
> >       free_in_cluster = s->cluster_size -
> >           offset_into_cluster(s, s->free_byte_offset);
> >       if (size <= free_in_cluster) {
> >           /* enough space in current cluster */
> >           offset = s->free_byte_offset;
> > +
> > +        if (offset_into_cluster(s, offset) != 0) {
> > +            /* We will have to increase the refcount of this cluster; if the
> > +             * maximum has been reached already, this cluster cannot be used */
> > +            ret = qcow2_get_refcount(bs, offset >> s->cluster_bits, &refcount);
> > +            if (ret < 0) {
> > +                return ret;
> > +            } else if (refcount == s->refcount_max) {
> > +                s->free_byte_offset = 0;
> > +                goto redo;
> > +            }
> > +        }
> > +
> >           s->free_byte_offset += size;
> >           free_in_cluster -= size;
> >           if (free_in_cluster == 0)
> > @@ -816,6 +831,20 @@ int64_t qcow2_alloc_bytes(BlockDriverState *bs, int size)
> >           if ((cluster_offset + s->cluster_size) == new_cluster) {
> >               /* we are lucky: contiguous data */
> >               offset = s->free_byte_offset;
> > +
> > +            /* Same as above: In order to reuse the cluster, the refcount has 
> > to
> > +             * be increased; if that will not work, we are not so lucky after
> > +             * all */
> > +            ret = qcow2_get_refcount(bs, offset >> s->cluster_bits, &refcount);
> > +            if (ret < 0) {
> > +                qcow2_free_clusters(bs, new_cluster, s->cluster_size,
> > +                                    QCOW2_DISCARD_NEVER);
> > +                return ret;
> > +            } else if (refcount == s->refcount_max) {
> > +                s->free_byte_offset = offset;
> I think you mean 0. offset is already the old value.

Oh, right. Thanks for catching!

> > +                goto redo;
> > +            }
> > +
> >               ret = qcow2_update_cluster_refcount(bs, offset >> s->cluster_bits,
> >                                                   1, false, 
> > QCOW2_DISCARD_NEVER);
> >               if (ret < 0) {
> I wonder if the code duplication is necessary. I was already thinking
> that there was some duplication when I reviewed the previous patch, but
> now it seems to become even more obvious that the three parts of this
> function are:
>
> 1. Allocate a new cluster
> 2. Allocate space in the already allocated cluster
> 3. Allocate a new cluster and space inside it, which is just 1. + 2.

Well, I can try rewriting this function, but I guess the diffcount will 
be even higher.

I think I'll try to write an independent patch which rewrites this 
function and then drop these two patches from this series.

Max