| Message ID | 20180713111424.23670-3-mreitz@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 07/13/2018 06:14 AM, Max Reitz wrote: > Past the end of the source backing file, we memset() buf_old to zero, so > it is clearly easy to use blk_pwrite_zeroes() instead of blk_pwrite() > then. > > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > qemu-img.c | 11 +++++++++-- > 1 file changed, 9 insertions(+), 2 deletions(-) > > diff --git a/qemu-img.c b/qemu-img.c > index dd684d8bf0..2552e7dad6 100644 > --- a/qemu-img.c > +++ b/qemu-img.c > @@ -3403,6 +3403,8 @@ static int img_rebase(int argc, char **argv) > } > > for (offset = 0; offset < size; offset += n) { > + bool buf_old_is_zero = false; > + > /* How many bytes can we handle with the next read? */ > n = MIN(IO_BUF_SIZE, size - offset); > > @@ -3423,6 +3425,7 @@ static int img_rebase(int argc, char **argv) > */ > if (offset >= old_backing_size) { > memset(buf_old, 0, n); > + buf_old_is_zero = true; Do we still need to spend time on the memset(), or... > } else { > if (offset + n > old_backing_size) { > n = old_backing_size - offset; > @@ -3458,8 +3461,12 @@ static int img_rebase(int argc, char **argv) > if (compare_buffers(buf_old + written, buf_new + written, > n - written, &pnum)) > { > - ret = blk_pwrite(blk, offset + written, > - buf_old + written, pnum, 0); > + if (buf_old_is_zero) { > + ret = blk_pwrite_zeroes(blk, offset + written, pnum, 0); ...are we able to guarantee that old_buf will not be used when buf_old_is_zero? > + } else { > + ret = blk_pwrite(blk, offset + written, > + buf_old + written, pnum, 0); > + } > if (ret < 0) { > error_report("Error while writing to COW image: %s", > strerror(-ret)); > The series seems reasonable, but is post-3.0 material, so I haven't reviewed it any closer than this question.
On 2018-07-20 23:22, Eric Blake wrote: > On 07/13/2018 06:14 AM, Max Reitz wrote: >> Past the end of the source backing file, we memset() buf_old to zero, so >> it is clearly easy to use blk_pwrite_zeroes() instead of blk_pwrite() >> then. >> >> Signed-off-by: Max Reitz <mreitz@redhat.com> >> --- >> qemu-img.c | 11 +++++++++-- >> 1 file changed, 9 insertions(+), 2 deletions(-) >> >> diff --git a/qemu-img.c b/qemu-img.c >> index dd684d8bf0..2552e7dad6 100644 >> --- a/qemu-img.c >> +++ b/qemu-img.c >> @@ -3403,6 +3403,8 @@ static int img_rebase(int argc, char **argv) >> } >> for (offset = 0; offset < size; offset += n) { >> + bool buf_old_is_zero = false; >> + >> /* How many bytes can we handle with the next read? */ >> n = MIN(IO_BUF_SIZE, size - offset); >> @@ -3423,6 +3425,7 @@ static int img_rebase(int argc, char **argv) >> */ >> if (offset >= old_backing_size) { >> memset(buf_old, 0, n); >> + buf_old_is_zero = true; > > Do we still need to spend time on the memset(), or... > >> } else { >> if (offset + n > old_backing_size) { >> n = old_backing_size - offset; >> @@ -3458,8 +3461,12 @@ static int img_rebase(int argc, char **argv) >> if (compare_buffers(buf_old + written, buf_new + >> written, >> n - written, &pnum)) >> { >> - ret = blk_pwrite(blk, offset + written, >> - buf_old + written, pnum, 0); >> + if (buf_old_is_zero) { >> + ret = blk_pwrite_zeroes(blk, offset + >> written, pnum, 0); > > ...are we able to guarantee that old_buf will not be used when > buf_old_is_zero? It will certainly be used, as it is used in the compare_buffers() call. It could be optimized, but I fear that may lead down a small rabbit hole (we could then also get the block status of the target backing file, see whether it is zero, and so on). Considering that rebase is probably not something many people use all the time, I think it’s OK to be slower than possible for now. (Until someone complains.) Max >> + } else { >> + ret = blk_pwrite(blk, offset + written, >> + buf_old + written, pnum, 0); >> + } >> if (ret < 0) { >> error_report("Error while writing to COW >> image: %s", >> strerror(-ret)); >> > > The series seems reasonable, but is post-3.0 material, so I haven't > reviewed it any closer than this question. >
On 7/13/18 6:14 AM, Max Reitz wrote: > Past the end of the source backing file, we memset() buf_old to zero, so > it is clearly easy to use blk_pwrite_zeroes() instead of blk_pwrite() > then. > > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > qemu-img.c | 11 +++++++++-- > 1 file changed, 9 insertions(+), 2 deletions(-) > > @@ -3458,8 +3461,12 @@ static int img_rebase(int argc, char **argv) > if (compare_buffers(buf_old + written, buf_new + written, > n - written, &pnum)) > { > - ret = blk_pwrite(blk, offset + written, > - buf_old + written, pnum, 0); > + if (buf_old_is_zero) { > + ret = blk_pwrite_zeroes(blk, offset + written, pnum, 0); Should we allow BDRV_REQ_MAY_UNMAP here, either unconditionally, or based on a command line knob that told us whether the user is more interested in a sparse result (that still reads as zero) vs. a fully-allocated result? Reviewed-by: Eric Blake <eblake@redhat.com>
On 18.04.19 20:59, Eric Blake wrote: > On 7/13/18 6:14 AM, Max Reitz wrote: >> Past the end of the source backing file, we memset() buf_old to zero, so >> it is clearly easy to use blk_pwrite_zeroes() instead of blk_pwrite() >> then. >> >> Signed-off-by: Max Reitz <mreitz@redhat.com> >> --- >> qemu-img.c | 11 +++++++++-- >> 1 file changed, 9 insertions(+), 2 deletions(-) >> > >> @@ -3458,8 +3461,12 @@ static int img_rebase(int argc, char **argv) >> if (compare_buffers(buf_old + written, buf_new + written, >> n - written, &pnum)) >> { >> - ret = blk_pwrite(blk, offset + written, >> - buf_old + written, pnum, 0); >> + if (buf_old_is_zero) { >> + ret = blk_pwrite_zeroes(blk, offset + written, pnum, 0); > > Should we allow BDRV_REQ_MAY_UNMAP here, either unconditionally, or > based on a command line knob that told us whether the user is more > interested in a sparse result (that still reads as zero) vs. a > fully-allocated result? I wouldn’t trust that. We haven’t yet switched to the new backing file at this point, so I think a driver would be correct in handling such requests by punching a hole in the file -- which would lead to the new backing file poking through once we’ve switched to it. Max > Reviewed-by: Eric Blake <eblake@redhat.com>
diff --git a/qemu-img.c b/qemu-img.c index dd684d8bf0..2552e7dad6 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -3403,6 +3403,8 @@ static int img_rebase(int argc, char **argv) } for (offset = 0; offset < size; offset += n) { + bool buf_old_is_zero = false; + /* How many bytes can we handle with the next read? */ n = MIN(IO_BUF_SIZE, size - offset); @@ -3423,6 +3425,7 @@ static int img_rebase(int argc, char **argv) */ if (offset >= old_backing_size) { memset(buf_old, 0, n); + buf_old_is_zero = true; } else { if (offset + n > old_backing_size) { n = old_backing_size - offset; @@ -3458,8 +3461,12 @@ static int img_rebase(int argc, char **argv) if (compare_buffers(buf_old + written, buf_new + written, n - written, &pnum)) { - ret = blk_pwrite(blk, offset + written, - buf_old + written, pnum, 0); + if (buf_old_is_zero) { + ret = blk_pwrite_zeroes(blk, offset + written, pnum, 0); + } else { + ret = blk_pwrite(blk, offset + written, + buf_old + written, pnum, 0); + } if (ret < 0) { error_report("Error while writing to COW image: %s", strerror(-ret));
Past the end of the source backing file, we memset() buf_old to zero, so it is clearly easy to use blk_pwrite_zeroes() instead of blk_pwrite() then. Signed-off-by: Max Reitz <mreitz@redhat.com> --- qemu-img.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)