Am 04.02.2015 um 16:33 hat Max Reitz geschrieben:
> 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 <mre...@redhat.com>
> >>Reviewed-by: Eric Blake <ebl...@redhat.com>
> >>Reviewed-by: Stefan Hajnoczi <stefa...@redhat.com>
> >>---
> >> 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.
Anything that makes the series shorter is welcome. ;-)
In fact, I've been considering merging the first few patches even if
they aren't valuable in themselves, just to make v6 shorter.
Kevin
