| Message ID | b3659d971731e5ef832399400a0f14daa580fce4.1495536228.git.berto@igalia.com |
|---|---|
| State | New |
| Headers | show |
Am 23.05.2017 um 13:22 hat Alberto Garcia geschrieben: > 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(-) > > diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c > index 6757875ec9..59a0ffba1a 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; > } With this change, it can now happen that we call do_perform_cow() with bytes == 0. Maybe it would be better for do_perform_cow() to check bytes first so that it doesn't bdrv_co_preadv/pwritev() for 0 bytes? Kevin
On Fri 26 May 2017 10:11:29 AM CEST, Kevin Wolf <kwolf@redhat.com> wrote: >> --- 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; >> + } >> + [...] >> +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; >> } > > With this change, it can now happen that we call do_perform_cow() with > bytes == 0. Yes, but see the change I made to do_perform_cow() in the same patch (quoted above). Berto
Am 26.05.2017 um 11:10 hat Alberto Garcia geschrieben: > On Fri 26 May 2017 10:11:29 AM CEST, Kevin Wolf <kwolf@redhat.com> wrote: > >> --- 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; > >> + } > >> + > > [...] > > >> +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; > >> } > > > > With this change, it can now happen that we call do_perform_cow() with > > bytes == 0. > > Yes, but see the change I made to do_perform_cow() in the same patch > (quoted above). Wait... How did you manage to hack my email account and insert this retroactively? :-) Sorry for the noise then, I must have been looking at the source code of the wrong commit. Kevin
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c index 6757875ec9..59a0ffba1a 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(-)