| Message ID | 1360761733-25347-14-git-send-email-kwolf@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Feb 13, 2013 at 1:22 PM, Kevin Wolf <kwolf@redhat.com> wrote: > Look only for clusters that start at a given physical offset. > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > --- > block/qcow2-cluster.c | 26 ++++++++++++++++++-------- > 1 files changed, 18 insertions(+), 8 deletions(-) > > diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c > index 5ce2c88..90fe36c 100644 > --- a/block/qcow2-cluster.c > +++ b/block/qcow2-cluster.c > @@ -827,8 +827,6 @@ static int handle_dependencies(BlockDriverState *bs, uint64_t guest_offset, > * the length of the area that can be written to. > * > * -errno: in error cases > - * > - * TODO Make non-zero host_offset behave like describe above > */ > static int handle_copied(BlockDriverState *bs, uint64_t guest_offset, > uint64_t *host_offset, uint64_t *bytes, QCowL2Meta **m) > @@ -843,7 +841,6 @@ static int handle_copied(BlockDriverState *bs, uint64_t guest_offset, > > trace_qcow2_handle_copied(qemu_coroutine_self(), guest_offset, *host_offset, > *bytes); > - assert(*host_offset == 0); > > /* > * Calculate the number of clusters to look for. We stop at L2 table > @@ -867,6 +864,15 @@ static int handle_copied(BlockDriverState *bs, uint64_t guest_offset, > if (qcow2_get_cluster_type(cluster_offset) == QCOW2_CLUSTER_NORMAL > && (cluster_offset & QCOW_OFLAG_COPIED)) > { > + /* If a specific host_offset is required, check it */ > + if (*host_offset != 0 > + && (cluster_offset & L2E_OFFSET_MASK) != *host_offset) > + { Braces should cuddle with the previous line. > + *bytes = 0; > + ret = 0; > + goto out; > + } > + > /* We keep all QCOW_OFLAG_COPIED clusters */ > keep_clusters = > count_contiguous_clusters(nb_clusters, s->cluster_size, > @@ -880,19 +886,22 @@ static int handle_copied(BlockDriverState *bs, uint64_t guest_offset, > > ret = 1; > } else { > - cluster_offset = 0; > ret = 0; > } > > - cluster_offset &= L2E_OFFSET_MASK; > - *host_offset = cluster_offset; > - > /* Cleanup */ > +out: > pret = qcow2_cache_put(bs, s->l2_table_cache, (void**) &l2_table); > if (pret < 0) { > return pret; > } > > + /* Only return a host offset if we actually made progress. Otherwise we > + * would make requirements for handle_alloc() that it can't fulfill */ > + if (ret) { > + *host_offset = cluster_offset & L2E_OFFSET_MASK; > + } > + > return ret; > } > > @@ -1162,7 +1171,6 @@ again: > > /* > * 2. Count contiguous COPIED clusters. > - * TODO: Consider cluster_offset if set in step 1c. > */ > ret = handle_copied(bs, offset, &cluster_offset, &cur_bytes, m); > if (ret < 0) { > @@ -1175,6 +1183,8 @@ again: > if (!*host_offset) { > *host_offset = cluster_offset; > } > + } else if (cur_bytes == 0) { > + goto done; > } else { > keep_clusters = 0; > } > -- > 1.7.6.5 > >
Am 13.02.2013 22:17, schrieb Blue Swirl: > On Wed, Feb 13, 2013 at 1:22 PM, Kevin Wolf <kwolf@redhat.com> wrote: >> Look only for clusters that start at a given physical offset. >> >> Signed-off-by: Kevin Wolf <kwolf@redhat.com> >> --- >> block/qcow2-cluster.c | 26 ++++++++++++++++++-------- >> 1 files changed, 18 insertions(+), 8 deletions(-) >> >> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c >> index 5ce2c88..90fe36c 100644 >> --- a/block/qcow2-cluster.c >> +++ b/block/qcow2-cluster.c >> @@ -827,8 +827,6 @@ static int handle_dependencies(BlockDriverState *bs, uint64_t guest_offset, >> * the length of the area that can be written to. >> * >> * -errno: in error cases >> - * >> - * TODO Make non-zero host_offset behave like describe above >> */ >> static int handle_copied(BlockDriverState *bs, uint64_t guest_offset, >> uint64_t *host_offset, uint64_t *bytes, QCowL2Meta **m) >> @@ -843,7 +841,6 @@ static int handle_copied(BlockDriverState *bs, uint64_t guest_offset, >> >> trace_qcow2_handle_copied(qemu_coroutine_self(), guest_offset, *host_offset, >> *bytes); >> - assert(*host_offset == 0); >> >> /* >> * Calculate the number of clusters to look for. We stop at L2 table >> @@ -867,6 +864,15 @@ static int handle_copied(BlockDriverState *bs, uint64_t guest_offset, >> if (qcow2_get_cluster_type(cluster_offset) == QCOW2_CLUSTER_NORMAL >> && (cluster_offset & QCOW_OFLAG_COPIED)) >> { >> + /* If a specific host_offset is required, check it */ >> + if (*host_offset != 0 >> + && (cluster_offset & L2E_OFFSET_MASK) != *host_offset) >> + { > > Braces should cuddle with the previous line. Can we get rid of this rule for multiline ifs? Having the second/third/... line of the condition and the first line of the then branch with no clear separation severely hurts readability in my opinion. > >> + *bytes = 0; >> + ret = 0; >> + goto out; >> + } >> + >> /* We keep all QCOW_OFLAG_COPIED clusters */ >> keep_clusters = >> count_contiguous_clusters(nb_clusters, s->cluster_size, Kevin
On Thu, Feb 14, 2013 at 9:40 AM, Kevin Wolf <kwolf@redhat.com> wrote: > Am 13.02.2013 22:17, schrieb Blue Swirl: >> On Wed, Feb 13, 2013 at 1:22 PM, Kevin Wolf <kwolf@redhat.com> wrote: >>> Look only for clusters that start at a given physical offset. >>> >>> Signed-off-by: Kevin Wolf <kwolf@redhat.com> >>> --- >>> block/qcow2-cluster.c | 26 ++++++++++++++++++-------- >>> 1 files changed, 18 insertions(+), 8 deletions(-) >>> >>> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c >>> index 5ce2c88..90fe36c 100644 >>> --- a/block/qcow2-cluster.c >>> +++ b/block/qcow2-cluster.c >>> @@ -827,8 +827,6 @@ static int handle_dependencies(BlockDriverState *bs, uint64_t guest_offset, >>> * the length of the area that can be written to. >>> * >>> * -errno: in error cases >>> - * >>> - * TODO Make non-zero host_offset behave like describe above >>> */ >>> static int handle_copied(BlockDriverState *bs, uint64_t guest_offset, >>> uint64_t *host_offset, uint64_t *bytes, QCowL2Meta **m) >>> @@ -843,7 +841,6 @@ static int handle_copied(BlockDriverState *bs, uint64_t guest_offset, >>> >>> trace_qcow2_handle_copied(qemu_coroutine_self(), guest_offset, *host_offset, >>> *bytes); >>> - assert(*host_offset == 0); >>> >>> /* >>> * Calculate the number of clusters to look for. We stop at L2 table >>> @@ -867,6 +864,15 @@ static int handle_copied(BlockDriverState *bs, uint64_t guest_offset, >>> if (qcow2_get_cluster_type(cluster_offset) == QCOW2_CLUSTER_NORMAL >>> && (cluster_offset & QCOW_OFLAG_COPIED)) >>> { >>> + /* If a specific host_offset is required, check it */ >>> + if (*host_offset != 0 >>> + && (cluster_offset & L2E_OFFSET_MASK) != *host_offset) >>> + { >> >> Braces should cuddle with the previous line. > > Can we get rid of this rule for multiline ifs? Having the > second/third/... line of the condition and the first line of the then > branch with no clear separation severely hurts readability in my opinion. Perhaps the problem is that the condition is long, it could be rewritten in this style: bool has_host_offset = *host_offset != 0; bool offset_matches = (cluster_offset & L2E_OFFSET_MASK) != *host_offset; if (has_host_offset && offset_matches) { > >> >>> + *bytes = 0; >>> + ret = 0; >>> + goto out; >>> + } >>> + >>> /* We keep all QCOW_OFLAG_COPIED clusters */ >>> keep_clusters = >>> count_contiguous_clusters(nb_clusters, s->cluster_size, > > Kevin
On Thu, Feb 14, 2013 at 09:40:22PM +0000, Blue Swirl wrote: > On Thu, Feb 14, 2013 at 9:40 AM, Kevin Wolf <kwolf@redhat.com> wrote: > > Am 13.02.2013 22:17, schrieb Blue Swirl: > >> On Wed, Feb 13, 2013 at 1:22 PM, Kevin Wolf <kwolf@redhat.com> wrote: > >>> Look only for clusters that start at a given physical offset. > >>> > >>> Signed-off-by: Kevin Wolf <kwolf@redhat.com> > >>> --- > >>> block/qcow2-cluster.c | 26 ++++++++++++++++++-------- > >>> 1 files changed, 18 insertions(+), 8 deletions(-) > >>> > >>> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c > >>> index 5ce2c88..90fe36c 100644 > >>> --- a/block/qcow2-cluster.c > >>> +++ b/block/qcow2-cluster.c > >>> @@ -827,8 +827,6 @@ static int handle_dependencies(BlockDriverState *bs, uint64_t guest_offset, > >>> * the length of the area that can be written to. > >>> * > >>> * -errno: in error cases > >>> - * > >>> - * TODO Make non-zero host_offset behave like describe above > >>> */ > >>> static int handle_copied(BlockDriverState *bs, uint64_t guest_offset, > >>> uint64_t *host_offset, uint64_t *bytes, QCowL2Meta **m) > >>> @@ -843,7 +841,6 @@ static int handle_copied(BlockDriverState *bs, uint64_t guest_offset, > >>> > >>> trace_qcow2_handle_copied(qemu_coroutine_self(), guest_offset, *host_offset, > >>> *bytes); > >>> - assert(*host_offset == 0); > >>> > >>> /* > >>> * Calculate the number of clusters to look for. We stop at L2 table > >>> @@ -867,6 +864,15 @@ static int handle_copied(BlockDriverState *bs, uint64_t guest_offset, > >>> if (qcow2_get_cluster_type(cluster_offset) == QCOW2_CLUSTER_NORMAL > >>> && (cluster_offset & QCOW_OFLAG_COPIED)) > >>> { > >>> + /* If a specific host_offset is required, check it */ > >>> + if (*host_offset != 0 > >>> + && (cluster_offset & L2E_OFFSET_MASK) != *host_offset) > >>> + { > >> > >> Braces should cuddle with the previous line. > > > > Can we get rid of this rule for multiline ifs? Having the > > second/third/... line of the condition and the first line of the then > > branch with no clear separation severely hurts readability in my opinion. > > Perhaps the problem is that the condition is long, it could be > rewritten in this style: > bool has_host_offset = *host_offset != 0; > bool offset_matches = (cluster_offset & L2E_OFFSET_MASK) != *host_offset; > if (has_host_offset && offset_matches) { I consider the usefulness of this about the same as adding code in order to silence gcc warnings. Just that a complaining gcc breaks the build whereas a complaining Blue Swirl doesn't. But I'll do it here. Kevin
On Fri, Feb 15, 2013 at 9:35 AM, Kevin Wolf <kwolf@redhat.com> wrote: > On Thu, Feb 14, 2013 at 09:40:22PM +0000, Blue Swirl wrote: >> On Thu, Feb 14, 2013 at 9:40 AM, Kevin Wolf <kwolf@redhat.com> wrote: >> > Am 13.02.2013 22:17, schrieb Blue Swirl: >> >> On Wed, Feb 13, 2013 at 1:22 PM, Kevin Wolf <kwolf@redhat.com> wrote: >> >>> Look only for clusters that start at a given physical offset. >> >>> >> >>> Signed-off-by: Kevin Wolf <kwolf@redhat.com> >> >>> --- >> >>> block/qcow2-cluster.c | 26 ++++++++++++++++++-------- >> >>> 1 files changed, 18 insertions(+), 8 deletions(-) >> >>> >> >>> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c >> >>> index 5ce2c88..90fe36c 100644 >> >>> --- a/block/qcow2-cluster.c >> >>> +++ b/block/qcow2-cluster.c >> >>> @@ -827,8 +827,6 @@ static int handle_dependencies(BlockDriverState *bs, uint64_t guest_offset, >> >>> * the length of the area that can be written to. >> >>> * >> >>> * -errno: in error cases >> >>> - * >> >>> - * TODO Make non-zero host_offset behave like describe above >> >>> */ >> >>> static int handle_copied(BlockDriverState *bs, uint64_t guest_offset, >> >>> uint64_t *host_offset, uint64_t *bytes, QCowL2Meta **m) >> >>> @@ -843,7 +841,6 @@ static int handle_copied(BlockDriverState *bs, uint64_t guest_offset, >> >>> >> >>> trace_qcow2_handle_copied(qemu_coroutine_self(), guest_offset, *host_offset, >> >>> *bytes); >> >>> - assert(*host_offset == 0); >> >>> >> >>> /* >> >>> * Calculate the number of clusters to look for. We stop at L2 table >> >>> @@ -867,6 +864,15 @@ static int handle_copied(BlockDriverState *bs, uint64_t guest_offset, >> >>> if (qcow2_get_cluster_type(cluster_offset) == QCOW2_CLUSTER_NORMAL >> >>> && (cluster_offset & QCOW_OFLAG_COPIED)) >> >>> { >> >>> + /* If a specific host_offset is required, check it */ >> >>> + if (*host_offset != 0 >> >>> + && (cluster_offset & L2E_OFFSET_MASK) != *host_offset) >> >>> + { >> >> >> >> Braces should cuddle with the previous line. >> > >> > Can we get rid of this rule for multiline ifs? Having the >> > second/third/... line of the condition and the first line of the then >> > branch with no clear separation severely hurts readability in my opinion. >> >> Perhaps the problem is that the condition is long, it could be >> rewritten in this style: >> bool has_host_offset = *host_offset != 0; >> bool offset_matches = (cluster_offset & L2E_OFFSET_MASK) != *host_offset; >> if (has_host_offset && offset_matches) { > > I consider the usefulness of this about the same as adding code in order to > silence gcc warnings. Just that a complaining gcc breaks the build whereas a > complaining Blue Swirl doesn't. But I'll do it here. Maybe I complain too much. I think the booleans could actually make the logic clearer to someone not familiar with the code. > > Kevin
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c index 5ce2c88..90fe36c 100644 --- a/block/qcow2-cluster.c +++ b/block/qcow2-cluster.c @@ -827,8 +827,6 @@ static int handle_dependencies(BlockDriverState *bs, uint64_t guest_offset, * the length of the area that can be written to. * * -errno: in error cases - * - * TODO Make non-zero host_offset behave like describe above */ static int handle_copied(BlockDriverState *bs, uint64_t guest_offset, uint64_t *host_offset, uint64_t *bytes, QCowL2Meta **m) @@ -843,7 +841,6 @@ static int handle_copied(BlockDriverState *bs, uint64_t guest_offset, trace_qcow2_handle_copied(qemu_coroutine_self(), guest_offset, *host_offset, *bytes); - assert(*host_offset == 0); /* * Calculate the number of clusters to look for. We stop at L2 table @@ -867,6 +864,15 @@ static int handle_copied(BlockDriverState *bs, uint64_t guest_offset, if (qcow2_get_cluster_type(cluster_offset) == QCOW2_CLUSTER_NORMAL && (cluster_offset & QCOW_OFLAG_COPIED)) { + /* If a specific host_offset is required, check it */ + if (*host_offset != 0 + && (cluster_offset & L2E_OFFSET_MASK) != *host_offset) + { + *bytes = 0; + ret = 0; + goto out; + } + /* We keep all QCOW_OFLAG_COPIED clusters */ keep_clusters = count_contiguous_clusters(nb_clusters, s->cluster_size, @@ -880,19 +886,22 @@ static int handle_copied(BlockDriverState *bs, uint64_t guest_offset, ret = 1; } else { - cluster_offset = 0; ret = 0; } - cluster_offset &= L2E_OFFSET_MASK; - *host_offset = cluster_offset; - /* Cleanup */ +out: pret = qcow2_cache_put(bs, s->l2_table_cache, (void**) &l2_table); if (pret < 0) { return pret; } + /* Only return a host offset if we actually made progress. Otherwise we + * would make requirements for handle_alloc() that it can't fulfill */ + if (ret) { + *host_offset = cluster_offset & L2E_OFFSET_MASK; + } + return ret; } @@ -1162,7 +1171,6 @@ again: /* * 2. Count contiguous COPIED clusters. - * TODO: Consider cluster_offset if set in step 1c. */ ret = handle_copied(bs, offset, &cluster_offset, &cur_bytes, m); if (ret < 0) { @@ -1175,6 +1183,8 @@ again: if (!*host_offset) { *host_offset = cluster_offset; } + } else if (cur_bytes == 0) { + goto done; } else { keep_clusters = 0; }
Look only for clusters that start at a given physical offset. Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block/qcow2-cluster.c | 26 ++++++++++++++++++-------- 1 files changed, 18 insertions(+), 8 deletions(-)