On 22.02.2016 17:43, Max Reitz wrote: > On 22.02.2016 13:50, Kevin Wolf wrote: >> Am 20.02.2016 um 18:39 hat Max Reitz geschrieben: >>> When passing -S 0 to qemu-img convert, the target image is supposed to >>> be fully allocated. Right now, this is not the case if the source image >>> contains areas which bdrv_get_block_status() reports as being zero. >>> >>> This patch introduces a new ImgConvertBlockStatus named BLK_ZERO_DATA >>> which is set by convert_iteration_sectors() if an area is detected to be >>> zero and min_sparse is 0. Simply using BLK_DATA is not optimal, because >>> knowing an area to be zero allows us to memset() the buffer to be >>> written directly rather than having to use blk_read(). >>> >>> Other than that, BLK_ZERO_DATA is handled exactly like BLK_DATA. >>> >>> This patch changes the reference output for iotest 122; contrary to what >>> it assumed, -S 0 really should allocate everything in the output, not >>> just areas that are filled with zeros (as opposed to being zeroed). >>> >>> Signed-off-by: Max Reitz <mre...@redhat.com> >> >> I don't like how you touch the conversion code all over the place. >> Specifically, convert_iteration_sectors() and convert_read() (and >> consequently s->status) are supposed to be only about the source file. >> -S 0 doesn't make a difference for what the source file looks like, so >> we shouldn't need to change anything there. >> >> The following change should do the same thing (it passes your test case >> anyway) and is more contained to the actual writeout of image data. > > Well, I briefly considered making @buf non-const in convert_write(), but > I discarded that idea, and I'm still not comfortable with that. If you > argue that convert_read() should only deal with the source image, I'll > argue that convert_write() should only deal with the target image. I > know that you're making @buf non-const because we need some scratch > buffer and, well, @buf is available, so why not use that? But it still > doesn't sit right with me. > > So I'd like to pull that memset() out of convert_write() and just tell > convert_write() to write that buffer as data. In fact, that is basically > what my patch does. But why does it then not just use BLK_DATA but this > new status? > > Because of two reasons: First, another issue with your patch is that > zeroed areas are not counted in the progress update. If we are writing > them as zeros, we should count them, however. Therefore, we need some > special-casing in convert_do_copy(). Effectively, we end up with stuff like: > >> if (s->status == BLK_DATA || >> (!s->min_sparse && s->status == BLK_ZERO)) > > I found that combination of (min_sparse && BLK_ZERO) to be ugly, and > liked it much better if we could do that test a single time in > convert_read and be done with it. This is why I added the new status.* > > And if you pull the memset() out of convert_write() and add a new > status, what you end up with is basically my patch. > > *Note that I initially thought we'd have this test in many more places > than we actually would, as it turned out. I'll take a look at how much > simpler this patch becomes if I drop the BLK_ZERO_DATA status.
Said patch would look like this:
diff --git a/qemu-img.c b/qemu-img.c
index 2edb139..b696ba4 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1509,10 +1509,6 @@ static int convert_read(ImgConvertState *s,
int64_t sector_num, int nb_sectors,
int n;
int ret;
- if (s->status == BLK_ZERO || s->status == BLK_BACKING_FILE) {
- return 0;
- }
-
assert(nb_sectors <= s->buf_sectors);
while (nb_sectors > 0) {
BlockBackend *blk;
@@ -1650,7 +1646,8 @@ static int convert_do_copy(ImgConvertState *s)
ret = n;
goto fail;
}
- if (s->status == BLK_DATA) {
+ if (s->status == BLK_DATA || (!s->min_sparse && s->status ==
BLK_ZERO))
+ {
s->allocated_sectors += n;
}
sector_num += n;
@@ -1670,17 +1667,24 @@ static int convert_do_copy(ImgConvertState *s)
ret = n;
goto fail;
}
- if (s->status == BLK_DATA) {
+ if (s->status == BLK_DATA || (!s->min_sparse && s->status ==
BLK_ZERO))
+ {
allocated_done += n;
qemu_progress_print(100.0 * allocated_done /
s->allocated_sectors,
0);
}
- ret = convert_read(s, sector_num, n, buf);
- if (ret < 0) {
- error_report("error while reading sector %" PRId64
- ": %s", sector_num, strerror(-ret));
- goto fail;
+ if (s->status == BLK_DATA) {
+ ret = convert_read(s, sector_num, n, buf);
+ if (ret < 0) {
+ error_report("error while reading sector %" PRId64
+ ": %s", sector_num, strerror(-ret));
+ goto fail;
+ }
+ } else if (!s->min_sparse && s->status == BLK_ZERO) {
+ n = MIN(n, s->buf_sectors);
+ memset(buf, 0, n * BDRV_SECTOR_SIZE);
+ s->status = BLK_DATA;
}
ret = convert_write(s, sector_num, n, buf);
I'd get the diffcount even smaller by keeping convert_read() unchanged
and continuing to call it unconditionally, but I like that change in
itself because I find it makes the logic clearer. I can be persuaded to
split this patch into two, however (one pulling the condition out of
convert_read(), and another one doing the rest of this patch).
Max
signature.asc
Description: OpenPGP digital signature
