Am 05.02.2015 um 16:58 hat Max Reitz geschrieben:
> qcow2_alloc_bytes() is a function with insufficient error handling and
> an unnecessary goto. This patch rewrites it.
>
> Signed-off-by: Max Reitz <mre...@redhat.com>
> ---
> v2:
> - s/free_cluster_index/free_byte_index/ [Eric]
> - added an assertion at the start of the function that
> s->free_byte_offset is either 0 or points to the tail of a cluster
> (but never to the start)
> - use ROUND_UP() instead of start_of_cluster() + cluster_size [Eric]
> - added an assertion that s->free_byte_offset is set before using it
> [Eric]
> ---
> block/qcow2-refcount.c | 77
> +++++++++++++++++++++++++++++---------------------
> 1 file changed, 45 insertions(+), 32 deletions(-)
>
> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
> index 9afdb40..eede60d 100644
> --- a/block/qcow2-refcount.c
> +++ b/block/qcow2-refcount.c
> @@ -759,46 +759,51 @@ int qcow2_alloc_clusters_at(BlockDriverState *bs,
> uint64_t offset,
> int64_t qcow2_alloc_bytes(BlockDriverState *bs, int size)
> {
> BDRVQcowState *s = bs->opaque;
> - int64_t offset, cluster_offset;
> - int free_in_cluster;
> + int64_t offset, new_cluster = 0, cluster_end;
> + size_t free_in_cluster;
>
> BLKDBG_EVENT(bs->file, BLKDBG_CLUSTER_ALLOC_BYTES);
> assert(size > 0 && size <= s->cluster_size);
> - if (s->free_byte_offset == 0) {
> - offset = qcow2_alloc_clusters(bs, s->cluster_size);
> - if (offset < 0) {
> - return offset;
> + assert(!s->free_byte_offset || offset_into_cluster(s,
> s->free_byte_offset));
> +
> + if (s->free_byte_offset) {
> + int refcount = qcow2_get_refcount(bs,
> + s->free_byte_offset >> s->cluster_bits);
> + if (refcount < 0) {
> + return refcount;
> + }
> +
> + if (refcount == 0xffff) {
> + s->free_byte_offset = 0;
> }
> - 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;
> - s->free_byte_offset += size;
> - free_in_cluster -= size;
> - if (free_in_cluster == 0)
> - s->free_byte_offset = 0;
> - if (offset_into_cluster(s, offset) != 0)
> - qcow2_update_cluster_refcount(bs, offset >> s->cluster_bits, 1,
> - QCOW2_DISCARD_NEVER);
> - } else {
> - offset = qcow2_alloc_clusters(bs, s->cluster_size);
> - if (offset < 0) {
> - return offset;
> +
> + if (!s->free_byte_offset || free_in_cluster < size) {
> + new_cluster = qcow2_alloc_clusters(bs, s->cluster_size);
The code could perhaps become a bit nicer if you used
alloc_clusters_noref() here...
> + if (new_cluster < 0) {
> + return new_cluster;
> + }
> +
> + cluster_end = ROUND_UP(s->free_byte_offset, s->cluster_size);
> + if (!s->free_byte_offset || cluster_end != new_cluster) {
> + s->free_byte_offset = new_cluster;
> }
> - cluster_offset = start_of_cluster(s, s->free_byte_offset);
> - if ((cluster_offset + s->cluster_size) == offset) {
> - /* we are lucky: contiguous data */
> - offset = s->free_byte_offset;
> - qcow2_update_cluster_refcount(bs, offset >> s->cluster_bits, 1,
> - QCOW2_DISCARD_NEVER);
> - s->free_byte_offset += size;
> - } else {
> - s->free_byte_offset = offset;
> - goto redo;
> + }
> +
> + assert(s->free_byte_offset);
> + if (offset_into_cluster(s, s->free_byte_offset)) {
...because this block could become unconditional then, ...
> + int ret = qcow2_update_cluster_refcount(bs,
> + s->free_byte_offset >> s->cluster_bits, 1,
> + QCOW2_DISCARD_NEVER);
...here you could use update_refcount() with the actual byte count
(which also avoids two unnecessary shifts)...
> + if (ret < 0) {
> + if (new_cluster > 0) {
> + qcow2_free_clusters(bs, new_cluster, s->cluster_size,
> + QCOW2_DISCARD_OTHER);
> + }
...and this part wouldn't be needed because update_refcount() already
tries to fail atomically.
> + return ret;
> }
> }
>
> @@ -807,6 +812,14 @@ int64_t qcow2_alloc_bytes(BlockDriverState *bs, int size)
> * be flushed before the caller's L2 table updates.
> */
It would also simplify the two lines of this comment that aren't in the
patch context any more. ;-)
> qcow2_cache_set_dependency(bs, s->l2_table_cache,
> s->refcount_block_cache);
> +
> + offset = s->free_byte_offset;
> +
> + s->free_byte_offset += size;
> + if (!offset_into_cluster(s, s->free_byte_offset)) {
> + s->free_byte_offset = 0;
> + }
> +
> return offset;
> }
The patch looks correct to me. Let me know if you'd like to address the
point I made above, or if I should apply it as it is.
Kevin
