| Message ID | 1423151912-24863-1-git-send-email-mreitz@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 02/05/2015 08:58 AM, Max Reitz wrote: > qcow2_alloc_bytes() is a function with insufficient error handling and > an unnecessary goto. This patch rewrites it. > > Signed-off-by: Max Reitz <mreitz@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] Looks nicer. > + cluster_end = ROUND_UP(s->free_byte_offset, s->cluster_size); > + if (!s->free_byte_offset || cluster_end != new_cluster) { Here, I can prove that the left side of the || makes no difference to the outcome (using just the right half is sufficient, since !s->free_byte_offset implies cluster_end == 0, but new_cluster will be non-zero because we never allocate the header cluster). I'd be fine if you wanted to respin that and add the comment for the micro-optimized shorter code; but for maintenance purposes, it's easier to rely on the explicit condition (even if redundant) than to think about whether a proof listed in a comment is correct, so I'm also fine leaving things as is. > + s->free_byte_offset = new_cluster; At this point, s->free_byte_offset violates the precondition if we just allocated a cluster, so we have to make sure that we restore the precondition before exiting... > + if (offset_into_cluster(s, s->free_byte_offset)) { > + int ret = qcow2_update_cluster_refcount(bs, > + s->free_byte_offset >> s->cluster_bits, 1, > + QCOW2_DISCARD_NEVER); > + if (ret < 0) { > + if (new_cluster > 0) { > + qcow2_free_clusters(bs, new_cluster, s->cluster_size, > + QCOW2_DISCARD_OTHER); > + } > + return ret; ...this early exit only happens if s->free_byte_offset was pointing to a tail, and there are no other early exits, with the final exit properly restoring the precondition. Reviewed-by: Eric Blake <eblake@redhat.com>
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 <mreitz@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
On 2015-02-06 at 09:08, Kevin Wolf wrote: > 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 <mreitz@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(-) >> [snip] > 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. Good question. On one hand, I like your suggestion because it would indeed make the code shorter. On the other hand, I somehow feel better using functions that are prefixed qcow2_* because I feel like it might make the code harder to read if sometimes we use qcow2_alloc_clusters() and alloc_clusters_noref() at other times; and sometimes we use qcow2_update_cluster_refcount() and update_refcount() at other times. But then again, it might make sense to have this function mirror qcow2_alloc_clusters(), which does use alloc_clusters_noref() and update_refcount() (of course). So I think I'll go with your suggestion, thanks! Max
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); + 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)) { + int ret = qcow2_update_cluster_refcount(bs, + s->free_byte_offset >> s->cluster_bits, 1, + QCOW2_DISCARD_NEVER); + if (ret < 0) { + if (new_cluster > 0) { + qcow2_free_clusters(bs, new_cluster, s->cluster_size, + QCOW2_DISCARD_OTHER); + } + return ret; } } @@ -807,6 +812,14 @@ int64_t qcow2_alloc_bytes(BlockDriverState *bs, int size) * be flushed before the caller's L2 table updates. */ 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; }
qcow2_alloc_bytes() is a function with insufficient error handling and an unnecessary goto. This patch rewrites it. Signed-off-by: Max Reitz <mreitz@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(-)