Message ID | c49daf8afe80eade79ac53d305b83eb24b264338.1496844254.git.berto@igalia.com |
---|---|
State | New |
Headers | show |
On 06/07/2017 09:08 AM, Alberto Garcia wrote: > Instead of calling perform_cow() twice with a different COW region > each time, call it just once and make perform_cow() handle both > regions. > > This patch simply moves code around. The next one will do the actual > reordering of the COW operations. > > Signed-off-by: Alberto Garcia <berto@igalia.com> > --- > block/qcow2-cluster.c | 38 +++++++++++++++++++++++--------------- > 1 file changed, 23 insertions(+), 15 deletions(-) > qemu_co_mutex_unlock(&s->lock); > - ret = do_perform_cow(bs, m->offset, m->alloc_offset, r->offset, r->nb_bytes); > + ret = do_perform_cow(bs, m->offset, m->alloc_offset, > + start->offset, start->nb_bytes); > + if (ret < 0) { > + goto fail; > + } > + > + ret = do_perform_cow(bs, m->offset, m->alloc_offset, > + end->offset, end->nb_bytes); > + > +fail: Since you reach this label even on success, it feels a bit awkward. I probably would have avoided the goto and used fewer lines by refactoring the logic as: ret = do_perform_cow(..., start->...); if (ret >= 0) { ret = do_perform_cow(..., end->...); } But that doesn't affect correctness, so whether or not you redo the logic in the shorter fashion: Reviewed-by: Eric Blake <eblake@redhat.com>
On Wed 07 Jun 2017 11:43:34 PM CEST, Eric Blake wrote: >> block/qcow2-cluster.c | 38 +++++++++++++++++++++++--------------- >> 1 file changed, 23 insertions(+), 15 deletions(-) > >> qemu_co_mutex_unlock(&s->lock); >> - ret = do_perform_cow(bs, m->offset, m->alloc_offset, r->offset, r->nb_bytes); >> + ret = do_perform_cow(bs, m->offset, m->alloc_offset, >> + start->offset, start->nb_bytes); >> + if (ret < 0) { >> + goto fail; >> + } >> + >> + ret = do_perform_cow(bs, m->offset, m->alloc_offset, >> + end->offset, end->nb_bytes); >> + >> +fail: > > Since you reach this label even on success, it feels a bit awkward. I > probably would have avoided the goto and used fewer lines by > refactoring the logic as: > > ret = do_perform_cow(..., start->...); > if (ret >= 0) { > ret = do_perform_cow(..., end->...); > } I actually wrote this code without any gotos the way you mention, and it works fine in this patch, but as I was making more changes I ended up with a quite large piece of code that needed to add "if (!ret)" to almost every if statement, so I didn't quite like the final result. I'm open to reconsider it, but take a look first at the code after the last patch and you'll see that it would not be as neat as it appears now. Berto
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c index a86c5a75a9..4c03639a72 100644 --- a/block/qcow2-cluster.c +++ b/block/qcow2-cluster.c @@ -414,6 +414,10 @@ static int coroutine_fn do_perform_cow(BlockDriverState *bs, struct iovec iov; int ret; + if (bytes == 0) { + return 0; + } + iov.iov_len = bytes; iov.iov_base = qemu_try_blockalign(bs, iov.iov_len); if (iov.iov_base == NULL) { @@ -751,31 +755,40 @@ uint64_t qcow2_alloc_compressed_cluster_offset(BlockDriverState *bs, return cluster_offset; } -static int perform_cow(BlockDriverState *bs, QCowL2Meta *m, Qcow2COWRegion *r) +static int perform_cow(BlockDriverState *bs, QCowL2Meta *m) { BDRVQcow2State *s = bs->opaque; + Qcow2COWRegion *start = &m->cow_start; + Qcow2COWRegion *end = &m->cow_end; int ret; - if (r->nb_bytes == 0) { + if (start->nb_bytes == 0 && end->nb_bytes == 0) { return 0; } qemu_co_mutex_unlock(&s->lock); - ret = do_perform_cow(bs, m->offset, m->alloc_offset, r->offset, r->nb_bytes); + ret = do_perform_cow(bs, m->offset, m->alloc_offset, + start->offset, start->nb_bytes); + if (ret < 0) { + goto fail; + } + + ret = do_perform_cow(bs, m->offset, m->alloc_offset, + end->offset, end->nb_bytes); + +fail: qemu_co_mutex_lock(&s->lock); - if (ret < 0) { - return ret; - } - /* * Before we update the L2 table to actually point to the new cluster, we * need to be sure that the refcounts have been increased and COW was * handled. */ - qcow2_cache_depends_on_flush(s->l2_table_cache); + if (ret == 0) { + qcow2_cache_depends_on_flush(s->l2_table_cache); + } - return 0; + return ret; } int qcow2_alloc_cluster_link_l2(BlockDriverState *bs, QCowL2Meta *m) @@ -795,12 +808,7 @@ int qcow2_alloc_cluster_link_l2(BlockDriverState *bs, QCowL2Meta *m) } /* copy content of unmodified sectors */ - ret = perform_cow(bs, m, &m->cow_start); - if (ret < 0) { - goto err; - } - - ret = perform_cow(bs, m, &m->cow_end); + ret = perform_cow(bs, m); if (ret < 0) { goto err; }
Instead of calling perform_cow() twice with a different COW region each time, call it just once and make perform_cow() handle both regions. This patch simply moves code around. The next one will do the actual reordering of the COW operations. Signed-off-by: Alberto Garcia <berto@igalia.com> --- block/qcow2-cluster.c | 38 +++++++++++++++++++++++--------------- 1 file changed, 23 insertions(+), 15 deletions(-)