| Message ID | 20170420140514.GM24486@lemon.lan |
|---|---|
| State | New |
| Headers | show |
Am 20.04.2017 um 16:05 schrieb Fam Zheng: > On Tue, 02/28 14:35, Peter Lieven wrote: >> img_convert has been around before there was an ImgConvertState or >> a block backend, but it has never been modified to directly use >> these structs. Change this by parsing parameters directly into >> the ImgConvertState and directly use BlockBackend where possible. >> Futhermore variable initalization has been reworked and sorted. >> >> Signed-off-by: Peter Lieven <pl@kamp.de> > I see an iotest failure with this patch, in Kevin's block-next tree: > > 019 1s ... - output mismatch (see 019.out.bad) > --- /stor/work/qemu/tests/qemu-iotests/019.out 2017-04-17 16:19:56.523968474 +0800 > +++ 019.out.bad 2017-04-20 22:03:29.868216955 +0800 > @@ -1086,8 +1086,8 @@ > > Checking if backing clusters are allocated when they shouldn't > > -0/128 sectors allocated at offset 1 MiB > -0/128 sectors allocated at offset 4.001 GiB > +128/128 sectors allocated at offset 1 MiB > +128/128 sectors allocated at offset 4.001 GiB > Reading > > === IO: pattern 42 > Failures: 019 > Failed 1 of 1 tests Thanks for notifying, Fam. @Kevin: Can you drop the patch again? I will send a V2 addressing this issue and the other comments I have got. Peter
On 04/20/2017 09:05 AM, Fam Zheng wrote: > On Tue, 02/28 14:35, Peter Lieven wrote: >> img_convert has been around before there was an ImgConvertState or >> a block backend, but it has never been modified to directly use >> these structs. Change this by parsing parameters directly into >> the ImgConvertState and directly use BlockBackend where possible. >> Futhermore variable initalization has been reworked and sorted. >> >> Signed-off-by: Peter Lieven <pl@kamp.de> > > I see an iotest failure with this patch, in Kevin's block-next tree: > > 019 1s ... - output mismatch (see 019.out.bad) > --- /stor/work/qemu/tests/qemu-iotests/019.out 2017-04-17 16:19:56.523968474 +0800 > +++ 019.out.bad 2017-04-20 22:03:29.868216955 +0800 > @@ -1086,8 +1086,8 @@ > > Checking if backing clusters are allocated when they shouldn't > > -0/128 sectors allocated at offset 1 MiB > -0/128 sectors allocated at offset 4.001 GiB > +128/128 sectors allocated at offset 1 MiB > +128/128 sectors allocated at offset 4.001 GiB Hmm - I wonder if my patch to make 'zero with unmap' on an image with no backing file prefer pure unallocated clusters over a reads-as-zero would make a difference. https://lists.gnu.org/archive/html/qemu-devel/2017-04/msg01728.html Testing now...
On 04/20/2017 09:11 AM, Eric Blake wrote: > On 04/20/2017 09:05 AM, Fam Zheng wrote: >> On Tue, 02/28 14:35, Peter Lieven wrote: >>> img_convert has been around before there was an ImgConvertState or >>> a block backend, but it has never been modified to directly use >>> these structs. Change this by parsing parameters directly into >>> the ImgConvertState and directly use BlockBackend where possible. >>> Futhermore variable initalization has been reworked and sorted. >>> >>> Signed-off-by: Peter Lieven <pl@kamp.de> >> >> I see an iotest failure with this patch, in Kevin's block-next tree: >> >> 019 1s ... - output mismatch (see 019.out.bad) >> --- /stor/work/qemu/tests/qemu-iotests/019.out 2017-04-17 16:19:56.523968474 +0800 >> +++ 019.out.bad 2017-04-20 22:03:29.868216955 +0800 >> @@ -1086,8 +1086,8 @@ >> >> Checking if backing clusters are allocated when they shouldn't >> >> -0/128 sectors allocated at offset 1 MiB >> -0/128 sectors allocated at offset 4.001 GiB >> +128/128 sectors allocated at offset 1 MiB >> +128/128 sectors allocated at offset 4.001 GiB > > Hmm - I wonder if my patch to make 'zero with unmap' on an image with no > backing file prefer pure unallocated clusters over a reads-as-zero would > make a difference. > https://lists.gnu.org/archive/html/qemu-devel/2017-04/msg01728.html > > Testing now... Nope, did not make a difference, although it's certainly related; per my comment in that thread that: Note that technically, we _could_ write a cluster as unallocated rather than zero if a backing file exists but the backing file also reads as zero, but that's more expensive to determine, so this optimization is limited to qcow2 without a backing file.
Am 20.04.2017 um 16:18 schrieb Eric Blake: > On 04/20/2017 09:11 AM, Eric Blake wrote: >> On 04/20/2017 09:05 AM, Fam Zheng wrote: >>> On Tue, 02/28 14:35, Peter Lieven wrote: >>>> img_convert has been around before there was an ImgConvertState or >>>> a block backend, but it has never been modified to directly use >>>> these structs. Change this by parsing parameters directly into >>>> the ImgConvertState and directly use BlockBackend where possible. >>>> Futhermore variable initalization has been reworked and sorted. >>>> >>>> Signed-off-by: Peter Lieven <pl@kamp.de> >>> I see an iotest failure with this patch, in Kevin's block-next tree: >>> >>> 019 1s ... - output mismatch (see 019.out.bad) >>> --- /stor/work/qemu/tests/qemu-iotests/019.out 2017-04-17 16:19:56.523968474 +0800 >>> +++ 019.out.bad 2017-04-20 22:03:29.868216955 +0800 >>> @@ -1086,8 +1086,8 @@ >>> >>> Checking if backing clusters are allocated when they shouldn't >>> >>> -0/128 sectors allocated at offset 1 MiB >>> -0/128 sectors allocated at offset 4.001 GiB >>> +128/128 sectors allocated at offset 1 MiB >>> +128/128 sectors allocated at offset 4.001 GiB >> Hmm - I wonder if my patch to make 'zero with unmap' on an image with no >> backing file prefer pure unallocated clusters over a reads-as-zero would >> make a difference. >> https://lists.gnu.org/archive/html/qemu-devel/2017-04/msg01728.html >> >> Testing now... > Nope, did not make a difference, although it's certainly related; per my > comment in that thread that: > > Note that technically, we _could_ write a cluster as unallocated > rather than zero if a backing file exists but the backing file > also reads as zero, but that's more expensive to determine, so > this optimization is limited to qcow2 without a backing file. > > I will look into this tomorrow. Peter
Am 20.04.2017 um 16:08 hat Peter Lieven geschrieben: > Am 20.04.2017 um 16:05 schrieb Fam Zheng: > >On Tue, 02/28 14:35, Peter Lieven wrote: > >>img_convert has been around before there was an ImgConvertState or > >>a block backend, but it has never been modified to directly use > >>these structs. Change this by parsing parameters directly into > >>the ImgConvertState and directly use BlockBackend where possible. > >>Futhermore variable initalization has been reworked and sorted. > >> > >>Signed-off-by: Peter Lieven <pl@kamp.de> > >I see an iotest failure with this patch, in Kevin's block-next tree: > > > >019 1s ... - output mismatch (see 019.out.bad) > >--- /stor/work/qemu/tests/qemu-iotests/019.out 2017-04-17 16:19:56.523968474 +0800 > >+++ 019.out.bad 2017-04-20 22:03:29.868216955 +0800 > >@@ -1086,8 +1086,8 @@ > > Checking if backing clusters are allocated when they shouldn't > >-0/128 sectors allocated at offset 1 MiB > >-0/128 sectors allocated at offset 4.001 GiB > >+128/128 sectors allocated at offset 1 MiB > >+128/128 sectors allocated at offset 4.001 GiB > > Reading > > === IO: pattern 42 > >Failures: 019 > >Failed 1 of 1 tests > > Thanks for notifying, Fam. > > @Kevin: Can you drop the patch again? I will send a V2 addressing this > issue and the other comments I have got. I could, but if it doesn't take too long, I'd prefer to only do so when v2 is there. There are already a few conflicting patches and I asked their authors to rebase onto yours, so it wouldn't be nice if they wouldn't apply again because I removed your patch... Kevin
Am 20.04.2017 um 21:51 schrieb Kevin Wolf: > Am 20.04.2017 um 16:08 hat Peter Lieven geschrieben: >> Am 20.04.2017 um 16:05 schrieb Fam Zheng: >>> On Tue, 02/28 14:35, Peter Lieven wrote: >>>> img_convert has been around before there was an ImgConvertState or >>>> a block backend, but it has never been modified to directly use >>>> these structs. Change this by parsing parameters directly into >>>> the ImgConvertState and directly use BlockBackend where possible. >>>> Futhermore variable initalization has been reworked and sorted. >>>> >>>> Signed-off-by: Peter Lieven <pl@kamp.de> >>> I see an iotest failure with this patch, in Kevin's block-next tree: >>> >>> 019 1s ... - output mismatch (see 019.out.bad) >>> --- /stor/work/qemu/tests/qemu-iotests/019.out 2017-04-17 16:19:56.523968474 +0800 >>> +++ 019.out.bad 2017-04-20 22:03:29.868216955 +0800 >>> @@ -1086,8 +1086,8 @@ >>> Checking if backing clusters are allocated when they shouldn't >>> -0/128 sectors allocated at offset 1 MiB >>> -0/128 sectors allocated at offset 4.001 GiB >>> +128/128 sectors allocated at offset 1 MiB >>> +128/128 sectors allocated at offset 4.001 GiB >>> Reading >>> === IO: pattern 42 >>> Failures: 019 >>> Failed 1 of 1 tests >> Thanks for notifying, Fam. >> >> @Kevin: Can you drop the patch again? I will send a V2 addressing this >> issue and the other comments I have got. > I could, but if it doesn't take too long, I'd prefer to only do so when > v2 is there. There are already a few conflicting patches and I asked > their authors to rebase onto yours, so it wouldn't be nice if they > wouldn't apply again because I removed your patch... error found, if backing file is passed via -o backing_file, s.target_has_backing was not set to true. I will send a V2 shortly including the other comments I got. Peter > > Kevin
--- /stor/work/qemu/tests/qemu-iotests/019.out 2017-04-17 16:19:56.523968474 +0800 +++ 019.out.bad 2017-04-20 22:03:29.868216955 +0800 @@ -1086,8 +1086,8 @@ Checking if backing clusters are allocated when they shouldn't -0/128 sectors allocated at offset 1 MiB -0/128 sectors allocated at offset 4.001 GiB +128/128 sectors allocated at offset 1 MiB +128/128 sectors allocated at offset 4.001 GiB Reading === IO: pattern 42