| Message ID | 20230915162016.141771-4-andrey.drobyshev@virtuozzo.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | qemu-img: rebase: add compression support | expand |
On Fri, Sep 15, 2023 at 07:20:11PM +0300, Andrey Drobyshev wrote: > Since commit bb1c05973cf ("qemu-img: Use qemu_blockalign"), buffers for > the data read from the old and new backing files are aligned using > BlockDriverState (or BlockBackend later on) referring to the target image. > However, this isn't quite right, because buf_new is only being used for > reading from the new backing, while buf_old is being used for both reading > from the old backing and writing to the target. Let's take that into account > and use more appropriate values as alignments. > > Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com> > --- > qemu-img.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/qemu-img.c b/qemu-img.c > index 50660ba920..d12e4a4753 100644 > --- a/qemu-img.c > +++ b/qemu-img.c > @@ -3750,8 +3750,13 @@ static int img_rebase(int argc, char **argv) > int64_t n; > float local_progress = 0; > > - buf_old = blk_blockalign(blk, IO_BUF_SIZE); > - buf_new = blk_blockalign(blk, IO_BUF_SIZE); > + if (blk_old_backing && bdrv_opt_mem_align(blk_bs(blk)) > > + bdrv_opt_mem_align(blk_bs(blk_old_backing))) { > + buf_old = blk_blockalign(blk, IO_BUF_SIZE); > + } else { > + buf_old = blk_blockalign(blk_old_backing, IO_BUF_SIZE); > + } Since bdrv_opt_mem_align(NULL) is safe, could we just simplify this to: buf_old = qemu_memalign(MAX(bdrv_opt_mem_align(blk_old_backing), bdrv_opt_mem_align(blk)), IO_BUF_SIZE); instead of going through an if statement? Or is the problem that bdrv_opt_mem_align(NULL) can return the host page size (perhaps 64k), which may be larger than technically needed in some scenarios? > + buf_new = blk_blockalign(blk_new_backing, IO_BUF_SIZE); > > size = blk_getlength(blk); > if (size < 0) { > -- > 2.39.3 At any rate, aligning the buffers by how they will be used makes sense (if the destination blk has looser requirements than the source blk_old_backing, then accesses into blk_old are suspect). Reviewed-by: Eric Blake <eblake@redhat.com.
On 9/15/23 21:39, Eric Blake wrote: > On Fri, Sep 15, 2023 at 07:20:11PM +0300, Andrey Drobyshev wrote: >> Since commit bb1c05973cf ("qemu-img: Use qemu_blockalign"), buffers for >> the data read from the old and new backing files are aligned using >> BlockDriverState (or BlockBackend later on) referring to the target image. >> However, this isn't quite right, because buf_new is only being used for >> reading from the new backing, while buf_old is being used for both reading >> from the old backing and writing to the target. Let's take that into account >> and use more appropriate values as alignments. >> >> Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com> >> --- >> qemu-img.c | 9 +++++++-- >> 1 file changed, 7 insertions(+), 2 deletions(-) >> >> diff --git a/qemu-img.c b/qemu-img.c >> index 50660ba920..d12e4a4753 100644 >> --- a/qemu-img.c >> +++ b/qemu-img.c >> @@ -3750,8 +3750,13 @@ static int img_rebase(int argc, char **argv) >> int64_t n; >> float local_progress = 0; >> >> - buf_old = blk_blockalign(blk, IO_BUF_SIZE); >> - buf_new = blk_blockalign(blk, IO_BUF_SIZE); >> + if (blk_old_backing && bdrv_opt_mem_align(blk_bs(blk)) > >> + bdrv_opt_mem_align(blk_bs(blk_old_backing))) { >> + buf_old = blk_blockalign(blk, IO_BUF_SIZE); >> + } else { >> + buf_old = blk_blockalign(blk_old_backing, IO_BUF_SIZE); >> + } > > Since bdrv_opt_mem_align(NULL) is safe, could we just simplify this to: > > buf_old = qemu_memalign(MAX(bdrv_opt_mem_align(blk_old_backing), > bdrv_opt_mem_align(blk)), IO_BUF_SIZE); > > instead of going through an if statement? Or is the problem that > bdrv_opt_mem_align(NULL) can return the host page size (perhaps 64k), > which may be larger than technically needed in some scenarios? > Although bdrv_opt_mem_align(NULL) is safe, blk_bs(NULL) is not. And bdrv_opt_mem_align() takes BlockDriverState* not BlockBackend*, so we would have to perform the same check and there would be no simplification. >> + buf_new = blk_blockalign(blk_new_backing, IO_BUF_SIZE); >> >> size = blk_getlength(blk); >> if (size < 0) { >> -- >> 2.39.3 > > At any rate, aligning the buffers by how they will be used makes sense > (if the destination blk has looser requirements than the source > blk_old_backing, then accesses into blk_old are suspect). > > Reviewed-by: Eric Blake <eblake@redhat.com. >
On 15.09.23 18:20, Andrey Drobyshev wrote: > Since commit bb1c05973cf ("qemu-img: Use qemu_blockalign"), buffers for > the data read from the old and new backing files are aligned using > BlockDriverState (or BlockBackend later on) referring to the target image. > However, this isn't quite right, because buf_new is only being used for > reading from the new backing, while buf_old is being used for both reading > from the old backing and writing to the target. Let's take that into account > and use more appropriate values as alignments. > > Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com> > --- > qemu-img.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/qemu-img.c b/qemu-img.c > index 50660ba920..d12e4a4753 100644 > --- a/qemu-img.c > +++ b/qemu-img.c > @@ -3750,8 +3750,13 @@ static int img_rebase(int argc, char **argv) > int64_t n; > float local_progress = 0; > > - buf_old = blk_blockalign(blk, IO_BUF_SIZE); > - buf_new = blk_blockalign(blk, IO_BUF_SIZE); > + if (blk_old_backing && bdrv_opt_mem_align(blk_bs(blk)) > > + bdrv_opt_mem_align(blk_bs(blk_old_backing))) { > + buf_old = blk_blockalign(blk, IO_BUF_SIZE); > + } else { > + buf_old = blk_blockalign(blk_old_backing, IO_BUF_SIZE); > + } As I read this, if blk_old_backing is NULL, we will go to the blk_blockalign(blk_old_backing, IO_BUF_SIZE) path. I think if it is NULL, we should align on blk instead. Hanna > + buf_new = blk_blockalign(blk_new_backing, IO_BUF_SIZE); > > size = blk_getlength(blk); > if (size < 0) {
On 9/19/23 11:18, Hanna Czenczek wrote: > On 15.09.23 18:20, Andrey Drobyshev wrote: >> Since commit bb1c05973cf ("qemu-img: Use qemu_blockalign"), buffers for >> the data read from the old and new backing files are aligned using >> BlockDriverState (or BlockBackend later on) referring to the target >> image. >> However, this isn't quite right, because buf_new is only being used for >> reading from the new backing, while buf_old is being used for both >> reading >> from the old backing and writing to the target. Let's take that into >> account >> and use more appropriate values as alignments. >> >> Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com> >> --- >> qemu-img.c | 9 +++++++-- >> 1 file changed, 7 insertions(+), 2 deletions(-) >> >> diff --git a/qemu-img.c b/qemu-img.c >> index 50660ba920..d12e4a4753 100644 >> --- a/qemu-img.c >> +++ b/qemu-img.c >> @@ -3750,8 +3750,13 @@ static int img_rebase(int argc, char **argv) >> int64_t n; >> float local_progress = 0; >> - buf_old = blk_blockalign(blk, IO_BUF_SIZE); >> - buf_new = blk_blockalign(blk, IO_BUF_SIZE); >> + if (blk_old_backing && bdrv_opt_mem_align(blk_bs(blk)) > >> + bdrv_opt_mem_align(blk_bs(blk_old_backing))) { >> + buf_old = blk_blockalign(blk, IO_BUF_SIZE); >> + } else { >> + buf_old = blk_blockalign(blk_old_backing, IO_BUF_SIZE); >> + } > > As I read this, if blk_old_backing is NULL, we will go to the > blk_blockalign(blk_old_backing, IO_BUF_SIZE) path. I think if it is > NULL, we should align on blk instead. > > Hanna You're right, thanks for noticing. Will fix that. > >> + buf_new = blk_blockalign(blk_new_backing, IO_BUF_SIZE); >> size = blk_getlength(blk); >> if (size < 0) { >
diff --git a/qemu-img.c b/qemu-img.c index 50660ba920..d12e4a4753 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -3750,8 +3750,13 @@ static int img_rebase(int argc, char **argv) int64_t n; float local_progress = 0; - buf_old = blk_blockalign(blk, IO_BUF_SIZE); - buf_new = blk_blockalign(blk, IO_BUF_SIZE); + if (blk_old_backing && bdrv_opt_mem_align(blk_bs(blk)) > + bdrv_opt_mem_align(blk_bs(blk_old_backing))) { + buf_old = blk_blockalign(blk, IO_BUF_SIZE); + } else { + buf_old = blk_blockalign(blk_old_backing, IO_BUF_SIZE); + } + buf_new = blk_blockalign(blk_new_backing, IO_BUF_SIZE); size = blk_getlength(blk); if (size < 0) {
Since commit bb1c05973cf ("qemu-img: Use qemu_blockalign"), buffers for the data read from the old and new backing files are aligned using BlockDriverState (or BlockBackend later on) referring to the target image. However, this isn't quite right, because buf_new is only being used for reading from the new backing, while buf_old is being used for both reading from the old backing and writing to the target. Let's take that into account and use more appropriate values as alignments. Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com> --- qemu-img.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)