Re: [Qemu-block] [PATCH] cutils: Move size_to_str() from "qemu-common.h" to "qemu/cutils.h"
On Tue, Sep 03, 2019 at 02:57:31PM -0400, John Snow wrote: [...] > Seems proper. It must be an oversight to begin with that we declared it > in qemu-common but defined it in cutils. Porbably true.. > > Reviewed-by: John Snow Reviewed-by: Peter Xu Thanks, -- Peter Xu
Re: [Qemu-block] [Libguestfs] [PATCH 0/1] NBD protocol change to add fast zero support
On 8/23/19 9:34 AM, Eric Blake wrote: > See the cross-post cover letter for details: > https://www.redhat.com/archives/libguestfs/2019-August/msg00322.html > > Eric Blake (1): > protocol: Add NBD_CMD_FLAG_FAST_ZERO > > doc/proto.md | 50 +- > 1 file changed, 49 insertions(+), 1 deletion(-) I think we've had enough review time with no major objections to this. Therefore, I've gone ahead and incorporated the wording changes that were mentioned in discussion on this patch, as well as Rich's URI specification, into the NBD project. We can still amend the specifications if any problems turn up. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-block] [PATCH] block/nfs: add support for nfs_umount
> Am 03.09.2019 um 16:56 schrieb Kevin Wolf :
>
> Am 03.09.2019 um 15:44 hat Peter Lieven geschrieben:
>> libnfs recently added support for unmounting. Add support
>> in Qemu too.
>>
>> Signed-off-by: Peter Lieven
>
> Looks trivial enough to review even for me. :-)
>
> Thanks, applied to the block branch.
>
> Kevin
I am not sure what the reason is, but with this patch I sometimes run into
nfs_process_read being called for a cdrom mounted from nfs after I ejected it
(and the whole nfs client context is already destroyed).
It seems that the following fixes the issue. It might be that the nfs_umount
call just reveals this bug. nfs_close is also doing sync I/O with the libnfs
library. If we mount a nfs share we are doing everything with sync calls and
then set up the aio stuff with nfs_set_events. I think that we need to stop the
aio before we are executing the sync calls to nfs_close and nfs_umount.
Also not sure if holding the mutex is necessary here.
diff --git a/block/nfs.c b/block/nfs.c
index ffa6484c1a..cb2e0d375a 100644
--- a/block/nfs.c
+++ b/block/nfs.c
@@ -389,6 +389,10 @@ static void nfs_attach_aio_context(BlockDriverState *bs,
static void nfs_client_close(NFSClient *client)
{
if (client->context) {
+qemu_mutex_lock(>mutex);
+aio_set_fd_handler(client->aio_context, nfs_get_fd(client->context),
+ false, NULL, NULL, NULL, NULL);
+qemu_mutex_unlock(>mutex);
if (client->fh) {
nfs_close(client->context, client->fh);
client->fh = NULL;
@@ -396,8 +400,6 @@ static void nfs_client_close(NFSClient *client)
#ifdef LIBNFS_FEATURE_UMOUNT
nfs_umount(client->context);
#endif
-aio_set_fd_handler(client->aio_context, nfs_get_fd(client->context),
- false, NULL, NULL, NULL, NULL);
nfs_destroy_context(client->context);
client->context = NULL;
}
Peter
Re: [Qemu-block] [PATCH] docs: Update preferred NBD device syntax
On 9/3/19 3:02 PM, Eric Blake wrote: > [adding libvirt list] > > On 9/3/19 1:50 PM, John Snow wrote: >> >> >> On 9/3/19 10:56 AM, Eric Blake wrote: >>> Mention the preferred URI form, especially since NBD is trying to >>> standardize that form: https://lists.debian.org/nbd/2019/06/msg00012.html >>> >>> Signed-off-by: Eric Blake >>> --- >>> qemu-doc.texi | 16 +++- >>> 1 file changed, 11 insertions(+), 5 deletions(-) >>> >>> diff --git a/qemu-doc.texi b/qemu-doc.texi >>> index 577d1e837640..c83fb347d77e 100644 >>> --- a/qemu-doc.texi >>> +++ b/qemu-doc.texi >>> @@ -297,7 +297,14 @@ qemu-system-i386 -drive >>> file=iscsi://192.0.2.1/iqn.2001-04.com.example/1 >>> >>> @item NBD >>> QEMU supports NBD (Network Block Devices) both using TCP protocol as well >>> -as Unix Domain Sockets. >>> +as Unix Domain Sockets. With TCP, the default port is 10809. >>> >>> -Syntax for specifying a NBD device using TCP >>> +Syntax for specifying a NBD device using TCP, in preferred URI form: >>> +``nbd://[:]/[]'' >>> + >>> +Syntax for specifying a NBD device using Unix Domain Sockets; remember >>> +that '?' is a shell glob character and may need quoting: >>> +``nbd+unix:///[]?socket='' >>> + >>> +Older syntax that is also recognized: >> >> Deprecated officially, or no? >> >>> ``nbd::[:exportname=]'' >>> >>> -Syntax for specifying a NBD device using Unix Domain Sockets >>> ``nbd:unix:[:exportname=]'' > > I didn't feel like starting a deprecation clock, in part because libvirt > is still using nbd:host:port:exportname during migration, similarly code > in virstoragefile.c is using only the old form. Do we want to start a > deprecation (as a separate patch), to prod faster changes in libvirt in > switching to the newer form where sensible? > Yeah, understood -- I was merely curious for wording purposes. Some people might wonder what "Older syntax" means and perhaps why they shouldn't use it. It sounds like we do want to wander away from it eventually but aren't prepared to do that yet. I think largely such a deprecation clock is up to the workload of whoever would have to update the libvirt workflow (You, Peter?) and how much benefit we'd gain by dropping it in QEMU (little?) If you don't have motivation for doing it unprompted I have little reason to coerce you into it. >>> >>> Example for TCP >>> @example >>> -qemu-system-i386 --drive file=nbd:192.0.2.1:3 >>> +qemu-system-i386 --drive file=nbd://192.0.2.1:3 >>> @end example >>> >>> Example for Unix Domain Sockets >>> @example >>> -qemu-system-i386 --drive file=nbd:unix:/tmp/nbd-socket >>> +qemu-system-i386 --drive "file=nbd+unix:///?socket=/tmp/nbd-socket" >>> @end example >>> >>> @item SSH >>> >> >> Reviewed-by: John Snow > > Thanks; will queue through my NBD tree (regardless of whether we decide > I should add more patches to start a deprecation cycle). >
Re: [Qemu-block] [PATCH] docs: Update preferred NBD device syntax
[adding libvirt list] On 9/3/19 1:50 PM, John Snow wrote: > > > On 9/3/19 10:56 AM, Eric Blake wrote: >> Mention the preferred URI form, especially since NBD is trying to >> standardize that form: https://lists.debian.org/nbd/2019/06/msg00012.html >> >> Signed-off-by: Eric Blake >> --- >> qemu-doc.texi | 16 +++- >> 1 file changed, 11 insertions(+), 5 deletions(-) >> >> diff --git a/qemu-doc.texi b/qemu-doc.texi >> index 577d1e837640..c83fb347d77e 100644 >> --- a/qemu-doc.texi >> +++ b/qemu-doc.texi >> @@ -297,7 +297,14 @@ qemu-system-i386 -drive >> file=iscsi://192.0.2.1/iqn.2001-04.com.example/1 >> >> @item NBD >> QEMU supports NBD (Network Block Devices) both using TCP protocol as well >> -as Unix Domain Sockets. >> +as Unix Domain Sockets. With TCP, the default port is 10809. >> >> -Syntax for specifying a NBD device using TCP >> +Syntax for specifying a NBD device using TCP, in preferred URI form: >> +``nbd://[:]/[]'' >> + >> +Syntax for specifying a NBD device using Unix Domain Sockets; remember >> +that '?' is a shell glob character and may need quoting: >> +``nbd+unix:///[]?socket='' >> + >> +Older syntax that is also recognized: > > Deprecated officially, or no? > >> ``nbd::[:exportname=]'' >> >> -Syntax for specifying a NBD device using Unix Domain Sockets >> ``nbd:unix:[:exportname=]'' I didn't feel like starting a deprecation clock, in part because libvirt is still using nbd:host:port:exportname during migration, similarly code in virstoragefile.c is using only the old form. Do we want to start a deprecation (as a separate patch), to prod faster changes in libvirt in switching to the newer form where sensible? >> >> Example for TCP >> @example >> -qemu-system-i386 --drive file=nbd:192.0.2.1:3 >> +qemu-system-i386 --drive file=nbd://192.0.2.1:3 >> @end example >> >> Example for Unix Domain Sockets >> @example >> -qemu-system-i386 --drive file=nbd:unix:/tmp/nbd-socket >> +qemu-system-i386 --drive "file=nbd+unix:///?socket=/tmp/nbd-socket" >> @end example >> >> @item SSH >> > > Reviewed-by: John Snow Thanks; will queue through my NBD tree (regardless of whether we decide I should add more patches to start a deprecation cycle). -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-block] [PATCH] cutils: Move size_to_str() from "qemu-common.h" to "qemu/cutils.h"
On 9/3/19 8:05 AM, Philippe Mathieu-Daudé wrote: > "qemu/cutils.h" contains various qemu_strtosz_*() functions > useful to convert strings to size. It seems natural to have > the opposite usage (from size to string) there too. > > The function definition is already in util/cutils.c. > > Signed-off-by: Philippe Mathieu-Daudé > --- > There are only 5 users, is it worthwhile renaming it qemu_sztostrt()? (On the other hand, "size_to_str" is easy to read and "sztostrt" looks like someone sneezed with their hand on the keyboard.) > Signed-off-by: Philippe Mathieu-Daudé > --- > block/qapi.c | 2 +- > include/qemu-common.h| 1 - > include/qemu/cutils.h| 2 ++ > qapi/string-output-visitor.c | 2 +- > 4 files changed, 4 insertions(+), 3 deletions(-) > > diff --git a/block/qapi.c b/block/qapi.c > index 15f1030264..7ee2ee065d 100644 > --- a/block/qapi.c > +++ b/block/qapi.c > @@ -23,7 +23,7 @@ > */ > > #include "qemu/osdep.h" > -#include "qemu-common.h" > +#include "qemu/cutils.h" I guess that's a more targeted inclusion. (That was the last thing we needed in there?) Seems proper. It must be an oversight to begin with that we declared it in qemu-common but defined it in cutils. Reviewed-by: John Snow
Re: [Qemu-block] [PATCH] docs: Update preferred NBD device syntax
On 9/3/19 10:56 AM, Eric Blake wrote: > Mention the preferred URI form, especially since NBD is trying to > standardize that form: https://lists.debian.org/nbd/2019/06/msg00012.html > > Signed-off-by: Eric Blake > --- > qemu-doc.texi | 16 +++- > 1 file changed, 11 insertions(+), 5 deletions(-) > > diff --git a/qemu-doc.texi b/qemu-doc.texi > index 577d1e837640..c83fb347d77e 100644 > --- a/qemu-doc.texi > +++ b/qemu-doc.texi > @@ -297,7 +297,14 @@ qemu-system-i386 -drive > file=iscsi://192.0.2.1/iqn.2001-04.com.example/1 > > @item NBD > QEMU supports NBD (Network Block Devices) both using TCP protocol as well > -as Unix Domain Sockets. > +as Unix Domain Sockets. With TCP, the default port is 10809. > > -Syntax for specifying a NBD device using TCP > +Syntax for specifying a NBD device using TCP, in preferred URI form: > +``nbd://[:]/[]'' > + > +Syntax for specifying a NBD device using Unix Domain Sockets; remember > +that '?' is a shell glob character and may need quoting: > +``nbd+unix:///[]?socket='' > + > +Older syntax that is also recognized: Deprecated officially, or no? > ``nbd::[:exportname=]'' > > -Syntax for specifying a NBD device using Unix Domain Sockets > ``nbd:unix:[:exportname=]'' > > Example for TCP > @example > -qemu-system-i386 --drive file=nbd:192.0.2.1:3 > +qemu-system-i386 --drive file=nbd://192.0.2.1:3 > @end example > > Example for Unix Domain Sockets > @example > -qemu-system-i386 --drive file=nbd:unix:/tmp/nbd-socket > +qemu-system-i386 --drive "file=nbd+unix:///?socket=/tmp/nbd-socket" > @end example > > @item SSH > Reviewed-by: John Snow
Re: [Qemu-block] [Qemu-devel] [PATCH 2/5] nbd: Prepare for NBD_CMD_FLAG_FAST_ZERO
On 8/30/19 1:07 PM, Vladimir Sementsov-Ogievskiy wrote: > 23.08.2019 17:37, Eric Blake wrote: >> Commit fe0480d6 and friends added BDRV_REQ_NO_FALLBACK as a way to >> avoid wasting time on a preliminary write-zero request that will later >> be rewritten by actual data, if it is known that the write-zero >> request will use a slow fallback; but in doing so, could not optimize >> for NBD. The NBD specification is now considering an extension that >> will allow passing on those semantics; this patch updates the new >> protocol bits and 'qemu-nbd --list' output to recognize the bit, as >> well as the new errno value possible when using the new flag; while >> upcoming patches will improve the client to use the feature when >> present, and the server to advertise support for it. >> >> +++ b/nbd/server.c >> @@ -55,6 +55,8 @@ static int system_errno_to_nbd_errno(int err) >> return NBD_ENOSPC; >> case EOVERFLOW: >> return NBD_EOVERFLOW; >> +case ENOTSUP: >> +return NBD_ENOTSUP; > I'm squashing this in: diff --git i/nbd/server.c w/nbd/server.c index b3bd08ef2953..4992148de1c4 100644 --- i/nbd/server.c +++ w/nbd/server.c @@ -56,6 +56,9 @@ static int system_errno_to_nbd_errno(int err) case EOVERFLOW: return NBD_EOVERFLOW; case ENOTSUP: +#if ENOTSUP != EOPNOTSUPP +case EOPNOTSUPP: +#endif return NBD_ENOTSUP; case ESHUTDOWN: return NBD_ESHUTDOWN; It makes no difference on Linux, but may matter on other platforms (since POSIX says the two may be equivalent, but may also be distinct). > -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-block] [PATCH v7 0/6] Allow Valgrind checking all QEMU processes
On 03/09/2019 17:28, Kevin Wolf wrote: > Am 03.09.2019 um 16:22 hat Andrey Shinkevich geschrieben: >> >> >> On 03/09/2019 13:02, Kevin Wolf wrote: >>> Am 01.09.2019 um 13:53 hat Andrey Shinkevich geschrieben: In the current implementation of the QEMU bash iotests, only qemu-io processes may be run under the Valgrind with the switch '-valgrind'. Let's allow the common.rc bash script running all other QEMU processes, such as qemu-kvm, qemu-img, qemu-ndb and qemu-vxhs, under the Valgrind. >>> >>> Thanks, applied to the block branch. >>> >>> Kevin >>> >> >> Kevin! >> Please postpone the pull request! >> The last optimization in the patch 1/6 broke the logic in the patch 2/3. >> So, the test 039 hangs under the Valgrind, as it was. >> The patch 2/6 must be optimized too. >> I am about to make a little change in the patch 2/6 and will send v8 >> today... > > Ok, I'll unstage v7. > > Kevin > The v8 is ready to be sent. The test 039 passes now being run under the Valgrind. The iotests pass with the v8 series applied being run without the Valgrind: ./check -qcow2 and ./check -nbd Now, I am waiting for all the iotests to pass with the switch '-valgrind'. It takes much more time actually. Few more hours are needed to complete running the iotests under the Valgrind. I could send the v8 now or better wait until tomorrow for assurance. Andrey -- With the best regards, Andrey Shinkevich
Re: [Qemu-block] [Qemu-devel] [PATCH 1/5] nbd: Improve per-export flag handling in server
On 8/30/19 6:32 PM, Eric Blake wrote:
@@ -458,10 +458,13 @@ static int
nbd_negotiate_handle_export_name(NBDClient *client,
return -EINVAL;
}
-trace_nbd_negotiate_new_style_size_flags(client->exp->size,
- client->exp->nbdflags |
myflags);
+myflags = client->exp->nbdflags;
+if (client->structured_reply) {
+myflags |= NBD_FLAG_SEND_DF;
+}
>>>
>>>
>>> why we cant do just
>>> client->exp->nbdflags |= NBD_FLAG_SEND_DF ?
>>
>> Because myflags is the runtime flags for _this_ client, while
>> client->exp->nbdflags are the base flags shared by _all_ clients. If
>> client A requests structured reply, but client B does not, then we don't
>> want to advertise DF to client B; but amending client->exp->nbdflags
>> would have that effect.
>
> I stand corrected - it looks like a fresh client->exp is created per
> client, as evidenced by:
I need to quit replying to myself, but my test was flawed. Modern
clients don't go through NBD_OPT_EXPORT_NAME, so my added line...
> +++ w/nbd/server.c
> @@ -457,6 +457,7 @@ static int
> nbd_negotiate_handle_export_name(NBDClient *client, bool no_zeroes,
> myflags = client->exp->nbdflags;
> if (client->structured_reply) {
> myflags |= NBD_FLAG_SEND_DF;
> +client->exp->nbdflags |= NBD_FLAG_SEND_DF;
> }
...was not getting reached. If I instead tweak NBD_OPT_GO:
diff --git i/nbd/server.c w/nbd/server.c
index 6f3a83704fb3..da1ef793f6df 100644
--- i/nbd/server.c
+++ w/nbd/server.c
@@ -640,6 +640,7 @@ static int nbd_negotiate_handle_info(NBDClient
*client, Error **errp)
myflags = exp->nbdflags;
if (client->structured_reply) {
myflags |= NBD_FLAG_SEND_DF;
+exp->nbdflags |= NBD_FLAG_SEND_DF;
}
trace_nbd_negotiate_new_style_size_flags(exp->size, myflags);
stq_be_p(buf, exp->size);
> $ ./qemu-nbd -r -f raw file -t &
>
> $ ~/qemu/qemu-io -r -f raw --trace=nbd_\*size_flags
> nbd://localhost:10809 -c quit
> 32145@1567207628.519883:nbd_receive_negotiate_size_flags Size is
> 1049088, export flags 0x48f
>
> $ MY_HACK=1 ~/qemu/qemu-io -r -f raw --trace=nbd_\*size_flags
> nbd://localhost:10809 -c quit
> 32156@1567207630.417815:nbd_receive_negotiate_size_flags Size is
> 1049088, export flags 0x40f
>
Then this reports 0x48f, proving that my initial reaction was correct:
client->exp is a shared resource across multiple connections, but
advertising DF must be a per-connection decision.
> $ ~/qemu/qemu-io -r -f raw --trace=nbd_\*size_flags
> nbd://localhost:10809 -c quit
> 32167@1567207635.202940:nbd_receive_negotiate_size_flags Size is
> 1049088, export flags 0x48f
>
> The export flags change per client, so I _can_ store into
> client->exp->nbdflags. Will do that for v2.
I see nothing to change for v2, so I'm inclined to take this patch as is.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-block] [Qemu-devel] [PATCH v2] iotests: skip 232 when run tests as root
On 9/3/19 8:50 AM, Vladimir Sementsov-Ogievskiy wrote: > chmod a-w don't help under root, so skip the test in such case. > > Signed-off-by: Vladimir Sementsov-Ogievskiy > --- > > v2: move new check under TEST_IMG=TEST_IMG_FILE workaround [Kevin] > > tests/qemu-iotests/232 | 6 ++ > 1 file changed, 6 insertions(+) > > diff --git a/tests/qemu-iotests/232 b/tests/qemu-iotests/232 > index 2063f78876..65b0e42063 100755 > --- a/tests/qemu-iotests/232 > +++ b/tests/qemu-iotests/232 > @@ -74,6 +74,12 @@ if [ -n "$TEST_IMG_FILE" ]; then > TEST_IMG=$TEST_IMG_FILE > fi > > +chmod a-w $TEST_IMG > +(echo test > $TEST_IMG) 2>/dev/null && \ > +_notrun "Readonly attribute is ignored, probably you run this test as" \ s/run/ran/ > +"root, which is unsupported." > +chmod a+w $TEST_IMG > + > echo > echo "=== -drive with read-write image: read-only/auto-read-only > combinations ===" > echo > -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org signature.asc Description: OpenPGP digital signature
[Qemu-block] [PATCH] docs: Update preferred NBD device syntax
Mention the preferred URI form, especially since NBD is trying to standardize that form: https://lists.debian.org/nbd/2019/06/msg00012.html Signed-off-by: Eric Blake --- qemu-doc.texi | 16 +++- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/qemu-doc.texi b/qemu-doc.texi index 577d1e837640..c83fb347d77e 100644 --- a/qemu-doc.texi +++ b/qemu-doc.texi @@ -297,7 +297,14 @@ qemu-system-i386 -drive file=iscsi://192.0.2.1/iqn.2001-04.com.example/1 @item NBD QEMU supports NBD (Network Block Devices) both using TCP protocol as well -as Unix Domain Sockets. +as Unix Domain Sockets. With TCP, the default port is 10809. -Syntax for specifying a NBD device using TCP +Syntax for specifying a NBD device using TCP, in preferred URI form: +``nbd://[:]/[]'' + +Syntax for specifying a NBD device using Unix Domain Sockets; remember +that '?' is a shell glob character and may need quoting: +``nbd+unix:///[]?socket='' + +Older syntax that is also recognized: ``nbd::[:exportname=]'' -Syntax for specifying a NBD device using Unix Domain Sockets ``nbd:unix:[:exportname=]'' Example for TCP @example -qemu-system-i386 --drive file=nbd:192.0.2.1:3 +qemu-system-i386 --drive file=nbd://192.0.2.1:3 @end example Example for Unix Domain Sockets @example -qemu-system-i386 --drive file=nbd:unix:/tmp/nbd-socket +qemu-system-i386 --drive "file=nbd+unix:///?socket=/tmp/nbd-socket" @end example @item SSH -- 2.21.0
Re: [Qemu-block] [PATCH] block/nfs: add support for nfs_umount
Am 03.09.2019 um 15:44 hat Peter Lieven geschrieben: > libnfs recently added support for unmounting. Add support > in Qemu too. > > Signed-off-by: Peter Lieven Looks trivial enough to review even for me. :-) Thanks, applied to the block branch. Kevin
Re: [Qemu-block] [PATCH v7 0/6] Allow Valgrind checking all QEMU processes
Am 03.09.2019 um 16:22 hat Andrey Shinkevich geschrieben: > > > On 03/09/2019 13:02, Kevin Wolf wrote: > > Am 01.09.2019 um 13:53 hat Andrey Shinkevich geschrieben: > >> In the current implementation of the QEMU bash iotests, only qemu-io > >> processes may be run under the Valgrind with the switch '-valgrind'. > >> Let's allow the common.rc bash script running all other QEMU processes, > >> such as qemu-kvm, qemu-img, qemu-ndb and qemu-vxhs, under the Valgrind. > > > > Thanks, applied to the block branch. > > > > Kevin > > > > Kevin! > Please postpone the pull request! > The last optimization in the patch 1/6 broke the logic in the patch 2/3. > So, the test 039 hangs under the Valgrind, as it was. > The patch 2/6 must be optimized too. > I am about to make a little change in the patch 2/6 and will send v8 > today... Ok, I'll unstage v7. Kevin
Re: [Qemu-block] [PATCH v7 0/6] Allow Valgrind checking all QEMU processes
On 03/09/2019 13:02, Kevin Wolf wrote: > Am 01.09.2019 um 13:53 hat Andrey Shinkevich geschrieben: >> In the current implementation of the QEMU bash iotests, only qemu-io >> processes may be run under the Valgrind with the switch '-valgrind'. >> Let's allow the common.rc bash script running all other QEMU processes, >> such as qemu-kvm, qemu-img, qemu-ndb and qemu-vxhs, under the Valgrind. > > Thanks, applied to the block branch. > > Kevin > Kevin! Please postpone the pull request! The last optimization in the patch 1/6 broke the logic in the patch 2/3. So, the test 039 hangs under the Valgrind, as it was. The patch 2/6 must be optimized too. I am about to make a little change in the patch 2/6 and will send v8 today... Andrey -- With the best regards, Andrey Shinkevich
Re: [Qemu-block] [PATCH v2] iotests: skip 232 when run tests as root
Am 03.09.2019 um 15:50 hat Vladimir Sementsov-Ogievskiy geschrieben: > chmod a-w don't help under root, so skip the test in such case. > > Signed-off-by: Vladimir Sementsov-Ogievskiy Thanks, applied to the block branch. Kevin
Re: [Qemu-block] [PATCH v4 3/3] qcow2: add zstd cluster compression
Am 28.08.2019 um 14:56 hat Denis Plotnikov geschrieben:
> zstd significantly reduces cluster compression time.
> It provides better compression performance maintaining
> the same level of compression ratio in comparison with
> zlib, which, at the moment, has been the only compression
> method available.
>
> The performance test results:
> Test compresses and decompresses qemu qcow2 image with just
> installed rhel-7.6 guest.
> Image cluster size: 64K. Image on disk size: 2.2G
>
> The test was conducted with brd disk to reduce the influence
> of disk subsystem to the test results.
> The results is given in seconds.
>
> compress cmd:
> time ./qemu-img convert -O qcow2 -c -o compression_type=[zlib|zstd]
> src.img [zlib|zstd]_compressed.img
> decompress cmd
> time ./qemu-img convert -O qcow2
> [zlib|zstd]_compressed.img uncompressed.img
>
>compression decompression
> zlib zstd zlib zstd
>
> real 65.5 16.3 (-75 %)1.9 1.6 (-16 %)
> user 65.0 15.85.3 2.5
> sys 3.30.22.0 2.0
>
> Both ZLIB and ZSTD gave the same compression ratio: 1.57
> compressed image size in both cases: 1.4G
>
> Signed-off-by: Denis Plotnikov
> ---
> block/qcow2-threads.c | 107 +
> block/qcow2.c | 7 +++
> configure | 34 +
> docs/interop/qcow2.txt | 20
> qapi/block-core.json | 3 +-
> 5 files changed, 170 insertions(+), 1 deletion(-)
>
> diff --git a/block/qcow2-threads.c b/block/qcow2-threads.c
> index 14b5bd76fb..f6643976f4 100644
> --- a/block/qcow2-threads.c
> +++ b/block/qcow2-threads.c
> @@ -28,6 +28,11 @@
> #define ZLIB_CONST
> #include
>
> +#ifdef CONFIG_ZSTD
> +#include
> +#include
> +#endif
> +
> #include "qcow2.h"
> #include "block/thread-pool.h"
> #include "crypto.h"
> @@ -165,6 +170,98 @@ static ssize_t qcow2_zlib_decompress(void *dest, size_t
> dest_size,
> return ret;
> }
>
> +#ifdef CONFIG_ZSTD
> +/*
> + * qcow2_zstd_compress()
> + *
> + * Compress @src_size bytes of data using zstd compression method
> + *
> + * @dest - destination buffer, @dest_size bytes
> + * @src - source buffer, @src_size bytes
> + *
> + * Returns: compressed size on success
> + * -ENOMEM destination buffer is not enough to store compressed data
> + * -EIOon any other error
> + */
> +
> +static ssize_t qcow2_zstd_compress(void *dest, size_t dest_size,
> + const void *src, size_t src_size)
> +{
> +ssize_t ret;
> +uint32_t *c_size = dest;
> +/* steal some bytes to store compressed chunk size */
> +char *d_buf = ((char *) dest) + sizeof(*c_size);
> +
> +/* snaity check that we can store the compressed data length */
sanity
> +if (dest_size < sizeof(*c_size)) {
> +return -ENOMEM;
> +}
> +
> +dest_size -= sizeof(*c_size);
> +
> +ret = ZSTD_compress(d_buf, dest_size, src, src_size, 5);
> +
> +if (ZSTD_isError(ret)) {
> +if (ZSTD_getErrorCode(ret) == ZSTD_error_dstSize_tooSmall) {
> +return -ENOMEM;
> +} else {
> +return -EIO;
> +}
> +}
> +
> +/* store the compressed chunk size in the very beginning of the buffer */
> +*c_size = cpu_to_be32(ret);
> +
> +return ret + sizeof(*c_size);
> +}
> +
> +/*
> + * qcow2_zstd_decompress()
> + *
> + * Decompress some data (not more than @src_size bytes) to produce exactly
> + * @dest_size bytes using zstd compression method
> + *
> + * @dest - destination buffer, @dest_size bytes
> + * @src - source buffer, @src_size bytes
> + *
> + * Returns: 0 on success
> + * -EIO on any error
> + */
> +
> +static ssize_t qcow2_zstd_decompress(void *dest, size_t dest_size,
> + const void *src, size_t src_size)
> +{
> +ssize_t ret;
> +/*
> + * zstd decompress wants to know the exact length of the data
> + * for that purpose, on the compression the length is stored in
> + * the very beginning of the compressed buffer
> + */
> +uint32_t s_size;
> +const char *s_buf = ((const char *) src) + sizeof(s_size);
> +
> +/* sanity check that we can read the content length */
> +if (src_size < sizeof(s_size)) {
> +return -EIO;
> +}
> +
> +s_size = be32_to_cpu( *(const uint32_t *) src);
> +
> +/* sanity check that the buffer is big enough to read the content */
> +if (src_size - sizeof(s_size) < s_size) {
> +return -EIO;
> +}
> +
> +ret = ZSTD_decompress(dest, dest_size, s_buf, s_size);
> +
> +if (ZSTD_isError(ret)) {
> +return -EIO;
> +}
> +
> +return 0;
> +}
> +#endif
> +
> static int qcow2_compress_pool_func(void *opaque)
> {
> Qcow2CompressData *data = opaque;
> @@
Re: [Qemu-block] [PATCH] iotests: skip 232 when run tests as root
Am 03.09.2019 um 15:21 hat Vladimir Sementsov-Ogievskiy geschrieben: > chmod a-w don't help under root, so skip the test in such case. > > Signed-off-by: Vladimir Sementsov-Ogievskiy > --- > > tests/qemu-iotests/232 | 6 ++ > 1 file changed, 6 insertions(+) > > diff --git a/tests/qemu-iotests/232 b/tests/qemu-iotests/232 > index 2063f78876..da35a63d85 100755 > --- a/tests/qemu-iotests/232 > +++ b/tests/qemu-iotests/232 > @@ -70,6 +70,12 @@ size=128M > > _make_test_img $size > > +chmod a-w $TEST_IMG > +(echo test > $TEST_IMG) 2>/dev/null && \ > +_notrun "Readonly attribute is ignored, probably you run this test as" \ > +"root, which is unsupported." > +chmod a+w $TEST_IMG > + > if [ -n "$TEST_IMG_FILE" ]; then > TEST_IMG=$TEST_IMG_FILE > fi I think you need to move the new check below this so that $TEST_IMG_FILE is considered because otherwise the test will fail for luks: +chmod: cannot access 'driver=luks,key-secret=keysec0,file.filename=/home/kwolf/source/qemu/tests/qemu-iotests/scratch/t.luks': No such file or directory +chmod: cannot access 'driver=luks,key-secret=keysec0,file.filename=/home/kwolf/source/qemu/tests/qemu-iotests/scratch/t.luks': No such file or directory Kevin
[Qemu-block] [PULL v2 15/16] tests/check-block: Skip iotests when sanitizers are enabled
From: Thomas Huth The sanitizers (especially the address sanitizer from Clang) are sometimes printing out warnings or false positives - this spoils the output of the iotests, causing some of the tests to fail. Thus let's skip the automatic iotests during "make check" when the user configured QEMU with --enable-sanitizers. Signed-off-by: Thomas Huth Message-id: 20190823084203.29734-1-th...@redhat.com Signed-off-by: Max Reitz --- tests/check-block.sh | 5 + 1 file changed, 5 insertions(+) diff --git a/tests/check-block.sh b/tests/check-block.sh index c8b6cec3f6..679aedec50 100755 --- a/tests/check-block.sh +++ b/tests/check-block.sh @@ -21,6 +21,11 @@ if grep -q "TARGET_GPROF=y" *-softmmu/config-target.mak 2>/dev/null ; then exit 0 fi +if grep -q "CFLAGS.*-fsanitize" config-host.mak 2>/dev/null ; then +echo "Sanitizers are enabled ==> Not running the qemu-iotests." +exit 0 +fi + if [ -z "$(find . -name 'qemu-system-*' -print)" ]; then echo "No qemu-system binary available ==> Not running the qemu-iotests." exit 0 -- 2.21.0
[Qemu-block] [PULL v2 13/16] iotests: Add -display none to the qemu options
Without this argument, qemu will print an angry message about not being able to connect to a display server if $DISPLAY is not set. For me, that breaks iotests.supported_formats() because it thus only sees ["Could", "not", "connect"] as the supported formats. Signed-off-by: Max Reitz Reviewed-by: Philippe Mathieu-Daudé Message-id: 20190819201851.24418-2-mre...@redhat.com Reviewed-by: Thomas Huth Signed-off-by: Max Reitz --- tests/qemu-iotests/check | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check index c24874ff4a..a58232eefb 100755 --- a/tests/qemu-iotests/check +++ b/tests/qemu-iotests/check @@ -581,13 +581,13 @@ export QEMU_PROG="$(type -p "$QEMU_PROG")" case "$QEMU_PROG" in *qemu-system-arm|*qemu-system-aarch64) -export QEMU_OPTIONS="-nodefaults -machine virt,accel=qtest" +export QEMU_OPTIONS="-nodefaults -display none -machine virt,accel=qtest" ;; *qemu-system-tricore) -export QEMU_OPTIONS="-nodefaults -machine tricore_testboard,accel=qtest" +export QEMU_OPTIONS="-nodefaults -display none -machine tricore_testboard,accel=qtest" ;; *) -export QEMU_OPTIONS="-nodefaults -machine accel=qtest" +export QEMU_OPTIONS="-nodefaults -display none -machine accel=qtest" ;; esac -- 2.21.0
[Qemu-block] [PULL v2 10/16] iotests: Disable 110 for vmdk.twoGbMaxExtentSparse
The error message for the test case where we have a quorum node for which no directory name can be generated is different: For twoGbMaxExtentSparse, it complains that it cannot open the extent file. For other (sub)formats, it just notes that it cannot determine the backing file path. Both are fine, but just disable twoGbMaxExtentSparse for simplicity's sake. Signed-off-by: Max Reitz Reviewed-by: John Snow Message-id: 20190815153638.4600-7-mre...@redhat.com Reviewed-by: John Snow Signed-off-by: Max Reitz --- tests/qemu-iotests/110 | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/qemu-iotests/110 b/tests/qemu-iotests/110 index 2cdc7c8a72..2ef516baf1 100755 --- a/tests/qemu-iotests/110 +++ b/tests/qemu-iotests/110 @@ -40,7 +40,8 @@ trap "_cleanup; exit \$status" 0 1 2 3 15 # Any format supporting backing files _supported_fmt qed qcow qcow2 vmdk _supported_proto file -_unsupported_imgopts "subformat=monolithicFlat" "subformat=twoGbMaxExtentFlat" +_unsupported_imgopts "subformat=monolithicFlat" "subformat=twoGbMaxExtentFlat" \ + "subformat=twoGbMaxExtentSparse" TEST_IMG_REL=$(basename "$TEST_IMG") -- 2.21.0
Re: [Qemu-block] [PATCH v4 2/3] qcow2: rework the cluster compression routine
Am 28.08.2019 um 14:56 hat Denis Plotnikov geschrieben:
> The patch allow to process image compression type defined
> in the image header and choose an appropriate method for
> image clusters (de)compression.
>
> Signed-off-by: Denis Plotnikov
> ---
> block/qcow2-threads.c | 78 +++
> 1 file changed, 64 insertions(+), 14 deletions(-)
>
> diff --git a/block/qcow2-threads.c b/block/qcow2-threads.c
> index 3b1e63fe41..14b5bd76fb 100644
> --- a/block/qcow2-threads.c
> +++ b/block/qcow2-threads.c
> @@ -73,8 +73,11 @@ typedef struct Qcow2CompressData {
> Qcow2CompressFunc func;
> } Qcow2CompressData;
>
> +
Accidentally added newline?
> /*
> - * qcow2_compress()
> + * qcow2_zlib_compress()
> + *
> + * Compress @src_size bytes of data using zlib compression method
> *
> * @dest - destination buffer, @dest_size bytes
> * @src - source buffer, @src_size bytes
> @@ -83,8 +86,8 @@ typedef struct Qcow2CompressData {
> * -ENOMEM destination buffer is not enough to store compressed data
> * -EIOon any other error
> */
> -static ssize_t qcow2_compress(void *dest, size_t dest_size,
> - const void *src, size_t src_size)
> +static ssize_t qcow2_zlib_compress(void *dest, size_t dest_size,
> + const void *src, size_t src_size)
> {
> ssize_t ret;
> z_stream strm;
> @@ -119,19 +122,19 @@ static ssize_t qcow2_compress(void *dest, size_t
> dest_size,
> }
>
> /*
> - * qcow2_decompress()
> + * qcow2_zlib_decompress()
> *
> * Decompress some data (not more than @src_size bytes) to produce exactly
> - * @dest_size bytes.
> + * @dest_size bytes using zlib compression method
> *
> * @dest - destination buffer, @dest_size bytes
> * @src - source buffer, @src_size bytes
> *
> * Returns: 0 on success
> - * -1 on fail
> + * -EIO on fail
> */
> -static ssize_t qcow2_decompress(void *dest, size_t dest_size,
> -const void *src, size_t src_size)
> +static ssize_t qcow2_zlib_decompress(void *dest, size_t dest_size,
> + const void *src, size_t src_size)
> {
> int ret = 0;
> z_stream strm;
> @@ -144,7 +147,7 @@ static ssize_t qcow2_decompress(void *dest, size_t
> dest_size,
>
> ret = inflateInit2(, -12);
> if (ret != Z_OK) {
> -return -1;
> +return -EIO;
> }
>
> ret = inflate(, Z_FINISH);
> @@ -154,7 +157,7 @@ static ssize_t qcow2_decompress(void *dest, size_t
> dest_size,
> * @src buffer may be processed partly (because in qcow2 we know
> size of
> * compressed data with precision of one sector)
> */
> -ret = -1;
> +ret = -EIO;
> }
>
> inflateEnd();
> @@ -189,20 +192,67 @@ qcow2_co_do_compress(BlockDriverState *bs, void *dest,
> size_t dest_size,
> return arg.ret;
> }
>
> +/*
> + * qcow2_co_compress()
> + *
> + * Compress @src_size bytes of data using the compression
> + * method defined by the image compression type
> + *
> + * @dest - destination buffer, @dest_size bytes
> + * @src - source buffer, @src_size bytes
> + *
> + * Returns: 0 on success
> + * a negative error code on fail
on failure
> + */
> ssize_t coroutine_fn
> qcow2_co_compress(BlockDriverState *bs, void *dest, size_t dest_size,
>const void *src, size_t src_size)
> {
> -return qcow2_co_do_compress(bs, dest, dest_size, src, src_size,
> -qcow2_compress);
> +BDRVQcow2State *s = bs->opaque;
> +Qcow2CompressFunc fn;
> +
> +switch (s->compression_type) {
> +case QCOW2_COMPRESSION_TYPE_ZLIB:
> +fn = qcow2_zlib_compress;
> +break;
> +
> +default:
> +return -ENOTSUP;
> +}
> +
> +return qcow2_co_do_compress(bs, dest, dest_size, src, src_size, fn);
> }
>
> +/*
> + * qcow2_co_decompress()
> + *
> + * Decompress some data (not more than @src_size bytes) to produce exactly
> + * @dest_size bytes using the compression method defined by the image
> + * compression type
> + *
> + * @dest - destination buffer, @dest_size bytes
> + * @src - source buffer, @src_size bytes
> + *
> + * Returns: 0 on success
> + * a negative error code on fail
on failure
> + */
> ssize_t coroutine_fn
> qcow2_co_decompress(BlockDriverState *bs, void *dest, size_t dest_size,
> const void *src, size_t src_size)
> {
> -return qcow2_co_do_compress(bs, dest, dest_size, src, src_size,
> -qcow2_decompress);
> +BDRVQcow2State *s = bs->opaque;
> +Qcow2CompressFunc fn;
> +
> +switch (s->compression_type) {
> +case QCOW2_COMPRESSION_TYPE_ZLIB:
> +fn = qcow2_zlib_decompress;
> +break;
> +
> +default:
> +return -ENOTSUP;
> +}
> +
> +return qcow2_co_do_compress(bs, dest, dest_size, src, src_size, fn);
> }
Re: [Qemu-block] [PATCH] iotests: skip 232 when run tests as root
03.09.2019 16:38, Kevin Wolf wrote: > Am 03.09.2019 um 15:21 hat Vladimir Sementsov-Ogievskiy geschrieben: >> chmod a-w don't help under root, so skip the test in such case. >> >> Signed-off-by: Vladimir Sementsov-Ogievskiy >> --- >> >> tests/qemu-iotests/232 | 6 ++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/tests/qemu-iotests/232 b/tests/qemu-iotests/232 >> index 2063f78876..da35a63d85 100755 >> --- a/tests/qemu-iotests/232 >> +++ b/tests/qemu-iotests/232 >> @@ -70,6 +70,12 @@ size=128M >> >> _make_test_img $size >> >> +chmod a-w $TEST_IMG >> +(echo test > $TEST_IMG) 2>/dev/null && \ >> +_notrun "Readonly attribute is ignored, probably you run this test as" \ >> +"root, which is unsupported." >> +chmod a+w $TEST_IMG >> + >> if [ -n "$TEST_IMG_FILE" ]; then >> TEST_IMG=$TEST_IMG_FILE >> fi > > I think you need to move the new check below this so that $TEST_IMG_FILE > is considered because otherwise the test will fail for luks: > > +chmod: cannot access > 'driver=luks,key-secret=keysec0,file.filename=/home/kwolf/source/qemu/tests/qemu-iotests/scratch/t.luks': > No such file or directory > +chmod: cannot access > 'driver=luks,key-secret=keysec0,file.filename=/home/kwolf/source/qemu/tests/qemu-iotests/scratch/t.luks': > No such file or directory > Thanks, will resend -- Best regards, Vladimir
[Qemu-block] [PATCH v2] iotests: skip 232 when run tests as root
chmod a-w don't help under root, so skip the test in such case. Signed-off-by: Vladimir Sementsov-Ogievskiy --- v2: move new check under TEST_IMG=TEST_IMG_FILE workaround [Kevin] tests/qemu-iotests/232 | 6 ++ 1 file changed, 6 insertions(+) diff --git a/tests/qemu-iotests/232 b/tests/qemu-iotests/232 index 2063f78876..65b0e42063 100755 --- a/tests/qemu-iotests/232 +++ b/tests/qemu-iotests/232 @@ -74,6 +74,12 @@ if [ -n "$TEST_IMG_FILE" ]; then TEST_IMG=$TEST_IMG_FILE fi +chmod a-w $TEST_IMG +(echo test > $TEST_IMG) 2>/dev/null && \ +_notrun "Readonly attribute is ignored, probably you run this test as" \ +"root, which is unsupported." +chmod a+w $TEST_IMG + echo echo "=== -drive with read-write image: read-only/auto-read-only combinations ===" echo -- 2.18.0
[Qemu-block] [PULL v2 08/16] vmdk: Reject invalid compressed writes
Compressed writes generally have to write full clusters, not just in
theory but also in practice when it comes to vmdk's streamOptimized
subformat. It currently is just silently broken for writes with
non-zero in-cluster offsets:
$ qemu-img create -f vmdk -o subformat=streamOptimized foo.vmdk 1M
$ qemu-io -c 'write 4k 4k' -c 'read 4k 4k' foo.vmdk
wrote 4096/4096 bytes at offset 4096
4 KiB, 1 ops; 00.01 sec (443.724 KiB/sec and 110.9309 ops/sec)
read failed: Invalid argument
(The technical reason is that vmdk_write_extent() just writes the
incomplete compressed data actually to offset 4k. When reading the
data, vmdk_read_extent() looks at offset 0 and finds the compressed data
size to be 0, because that is what it reads from there. This yields an
error.)
For incomplete writes with zero in-cluster offsets, the error path when
reading the rest of the cluster is a bit different, but the result is
the same:
$ qemu-img create -f vmdk -o subformat=streamOptimized foo.vmdk 1M
$ qemu-io -c 'write 0k 4k' -c 'read 4k 4k' foo.vmdk
wrote 4096/4096 bytes at offset 0
4 KiB, 1 ops; 00.01 sec (362.641 KiB/sec and 90.6603 ops/sec)
read failed: Invalid argument
(Here, vmdk_read_extent() finds the data and then sees that the
uncompressed data is short.)
It is better to reject invalid writes than to make the user believe they
might have succeeded and then fail when trying to read it back.
Signed-off-by: Max Reitz
Reviewed-by: John Snow
Message-id: 20190815153638.4600-5-mre...@redhat.com
Reviewed-by: John Snow
Signed-off-by: Max Reitz
---
block/vmdk.c | 10 ++
1 file changed, 10 insertions(+)
diff --git a/block/vmdk.c b/block/vmdk.c
index a7f82e665e..fed3b50c8a 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -1734,6 +1734,16 @@ static int vmdk_write_extent(VmdkExtent *extent, int64_t
cluster_offset,
if (extent->compressed) {
void *compressed_data;
+/* Only whole clusters */
+if (offset_in_cluster ||
+n_bytes > (extent->cluster_sectors * SECTOR_SIZE) ||
+(n_bytes < (extent->cluster_sectors * SECTOR_SIZE) &&
+ offset + n_bytes != extent->end_sector * SECTOR_SIZE))
+{
+ret = -EINVAL;
+goto out;
+}
+
if (!extent->has_marker) {
ret = -EINVAL;
goto out;
--
2.21.0
[Qemu-block] [PULL v2 14/16] iotests: Check for enabled drivers before testing them
From: Thomas Huth
It is possible to enable only a subset of the block drivers with the
"--block-drv-rw-whitelist" option of the "configure" script. All other
drivers are marked as unusable (or only included as read-only with the
"--block-drv-ro-whitelist" option). If an iotest is now using such a
disabled block driver, it is failing - which is bad, since at least the
tests in the "auto" group should be able to deal with this situation.
Thus let's introduce a "_require_drivers" function that can be used by
the shell tests to check for the availability of certain drivers first,
and marks the test as "not run" if one of the drivers is missing.
This patch mainly targets the test in the "auto" group which should
never fail in such a case, but also improves some of the other tests
along the way. Note that we also assume that the "qcow2" and "file"
drivers are always available - otherwise it does not make sense to
run "make check-block" at all (which only tests with qcow2 by default).
Signed-off-by: Thomas Huth
Message-id: 20190823133552.11680-1-th...@redhat.com
Signed-off-by: Max Reitz
---
tests/qemu-iotests/071 | 1 +
tests/qemu-iotests/081 | 4 +---
tests/qemu-iotests/099 | 1 +
tests/qemu-iotests/120 | 1 +
tests/qemu-iotests/162 | 4 +---
tests/qemu-iotests/184 | 1 +
tests/qemu-iotests/186 | 1 +
tests/qemu-iotests/common.rc | 14 ++
8 files changed, 21 insertions(+), 6 deletions(-)
diff --git a/tests/qemu-iotests/071 b/tests/qemu-iotests/071
index 1cca9233d0..fab52b 100755
--- a/tests/qemu-iotests/071
+++ b/tests/qemu-iotests/071
@@ -38,6 +38,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
_supported_fmt qcow2
_supported_proto file
+_require_drivers blkdebug blkverify
do_run_qemu()
{
diff --git a/tests/qemu-iotests/081 b/tests/qemu-iotests/081
index c418bab093..85acdf76d4 100755
--- a/tests/qemu-iotests/081
+++ b/tests/qemu-iotests/081
@@ -41,6 +41,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
_supported_fmt raw
_supported_proto file
_supported_os Linux
+_require_drivers quorum
do_run_qemu()
{
@@ -55,9 +56,6 @@ run_qemu()
| _filter_qemu_io | _filter_generated_node_ids
}
-test_quorum=$($QEMU_IMG --help|grep quorum)
-[ "$test_quorum" = "" ] && _supported_fmt quorum
-
quorum="driver=raw,file.driver=quorum,file.vote-threshold=2"
quorum="$quorum,file.children.0.file.filename=$TEST_DIR/1.raw"
quorum="$quorum,file.children.1.file.filename=$TEST_DIR/2.raw"
diff --git a/tests/qemu-iotests/099 b/tests/qemu-iotests/099
index ae02f27afe..c3cf66798a 100755
--- a/tests/qemu-iotests/099
+++ b/tests/qemu-iotests/099
@@ -42,6 +42,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
_supported_fmt qcow qcow2 qed vdi vhdx vmdk vpc
_supported_proto file
_supported_os Linux
+_require_drivers blkdebug blkverify
_unsupported_imgopts "subformat=monolithicFlat" "subformat=twoGbMaxExtentFlat"
\
"subformat=twoGbMaxExtentSparse"
diff --git a/tests/qemu-iotests/120 b/tests/qemu-iotests/120
index e9b4fbb009..2931a7550f 100755
--- a/tests/qemu-iotests/120
+++ b/tests/qemu-iotests/120
@@ -40,6 +40,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
_supported_fmt generic
_supported_proto file
_unsupported_fmt luks
+_require_drivers raw
_make_test_img 64M
diff --git a/tests/qemu-iotests/162 b/tests/qemu-iotests/162
index 4e5ed74fd5..2d719afbed 100755
--- a/tests/qemu-iotests/162
+++ b/tests/qemu-iotests/162
@@ -39,9 +39,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
. ./common.filter
_supported_fmt generic
-
-test_ssh=$($QEMU_IMG --help | grep '^Supported formats:.* ssh\( \|$\)')
-[ "$test_ssh" = "" ] && _notrun "ssh support required"
+_require_drivers ssh
echo
echo '=== NBD ==='
diff --git a/tests/qemu-iotests/184 b/tests/qemu-iotests/184
index cb0c181228..33dd8d2a4f 100755
--- a/tests/qemu-iotests/184
+++ b/tests/qemu-iotests/184
@@ -33,6 +33,7 @@ trap "exit \$status" 0 1 2 3 15
. ./common.filter
_supported_os Linux
+_require_drivers throttle
do_run_qemu()
{
diff --git a/tests/qemu-iotests/186 b/tests/qemu-iotests/186
index 5f6b18c150..3ea0442d44 100755
--- a/tests/qemu-iotests/186
+++ b/tests/qemu-iotests/186
@@ -38,6 +38,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
_supported_fmt qcow2
_supported_proto file
+_require_drivers null-co
if [ "$QEMU_DEFAULT_MACHINE" != "pc" ]; then
_notrun "Requires a PC machine"
diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
index 5502c3da2f..ee20be8920 100644
--- a/tests/qemu-iotests/common.rc
+++ b/tests/qemu-iotests/common.rc
@@ -520,5 +520,19 @@ _require_command()
[ -x "$c" ] || _notrun "$1 utility required, skipped this test"
}
+# Check that a set of drivers has been whitelisted in the QEMU binary
+#
+_require_drivers()
+{
+available=$($QEMU -drive format=help | \
+sed -e '/Supported formats:/!d' -e 's/Supported formats://')
+for driver
+do
+if ! echo "$available" | grep -q
[Qemu-block] [PULL v2 11/16] iotests: Disable 126 for flat vmdk subformats
iotest 126 requires backing file support, which flat vmdks cannot offer. Skip this test for such subformats. Signed-off-by: Max Reitz Message-id: 20190815153638.4600-8-mre...@redhat.com Reviewed-by: John Snow Signed-off-by: Max Reitz --- tests/qemu-iotests/126 | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/qemu-iotests/126 b/tests/qemu-iotests/126 index 9b0dcf9255..b7fce1e59d 100755 --- a/tests/qemu-iotests/126 +++ b/tests/qemu-iotests/126 @@ -33,6 +33,8 @@ status=1 # failure is the default! # Needs backing file support _supported_fmt qcow qcow2 qed vmdk +_unsupported_imgopts "subformat=monolithicFlat" \ + "subformat=twoGbMaxExtentFlat" # This is the default protocol (and we want to test the difference between # colons which separate a protocol prefix from the rest and colons which are # just part of the filename, so we cannot test protocols which require a prefix) -- 2.21.0
[Qemu-block] [PULL v2 16/16] iotests: Unify cache mode quoting
From: Nir Soffer Quoting cache mode is not needed, and most tests use unquoted values. Unify all test to use the same style. Message-id: 20190827173432.7656-1-nsof...@redhat.com Signed-off-by: Nir Soffer Signed-off-by: Max Reitz --- tests/qemu-iotests/026 | 4 ++-- tests/qemu-iotests/039 | 4 ++-- tests/qemu-iotests/052 | 2 +- tests/qemu-iotests/091 | 4 ++-- 4 files changed, 7 insertions(+), 7 deletions(-) diff --git a/tests/qemu-iotests/026 b/tests/qemu-iotests/026 index e30243608b..ffb18ab6b5 100755 --- a/tests/qemu-iotests/026 +++ b/tests/qemu-iotests/026 @@ -41,8 +41,8 @@ trap "_cleanup; exit \$status" 0 1 2 3 15 # Currently only qcow2 supports rebasing _supported_fmt qcow2 _supported_proto file -_default_cache_mode "writethrough" -_supported_cache_modes "writethrough" "none" +_default_cache_mode writethrough +_supported_cache_modes writethrough none # The refcount table tests expect a certain minimum width for refcount entries # (so that the refcount table actually needs to grow); that minimum is 16 bits, # being the default refcount entry width. diff --git a/tests/qemu-iotests/039 b/tests/qemu-iotests/039 index 0d4e963bd4..7c730d94a7 100755 --- a/tests/qemu-iotests/039 +++ b/tests/qemu-iotests/039 @@ -42,8 +42,8 @@ trap "_cleanup; exit \$status" 0 1 2 3 15 _supported_fmt qcow2 _supported_proto file _supported_os Linux -_default_cache_mode "writethrough" -_supported_cache_modes "writethrough" +_default_cache_mode writethrough +_supported_cache_modes writethrough size=128M diff --git a/tests/qemu-iotests/052 b/tests/qemu-iotests/052 index 6e2ecbfe21..45a140910d 100755 --- a/tests/qemu-iotests/052 +++ b/tests/qemu-iotests/052 @@ -40,7 +40,7 @@ _supported_fmt generic _supported_proto file # Don't do O_DIRECT on tmpfs -_supported_cache_modes "writeback" "writethrough" "unsafe" +_supported_cache_modes writeback writethrough unsafe size=128M _make_test_img $size diff --git a/tests/qemu-iotests/091 b/tests/qemu-iotests/091 index d62ef18a02..f4b44659ae 100755 --- a/tests/qemu-iotests/091 +++ b/tests/qemu-iotests/091 @@ -46,8 +46,8 @@ trap "_cleanup; exit \$status" 0 1 2 3 15 _supported_fmt qcow2 _supported_proto file _supported_os Linux -_default_cache_mode "none" -_supported_cache_modes "writethrough" "none" "writeback" +_default_cache_mode none +_supported_cache_modes writethrough none writeback size=1G -- 2.21.0
[Qemu-block] [PULL v2 01/16] qemu-io: add pattern file for write command
From: Denis Plotnikov
The patch allows to provide a pattern file for write
command. There was no similar ability before.
Signed-off-by: Denis Plotnikov
Message-id: 20190820164616.4072-1-dplotni...@virtuozzo.com
Reviewed-by: Eric Blake
[mreitz: Keep optstring in alphabetical order]
Signed-off-by: Max Reitz
---
qemu-io-cmds.c | 99 +++---
1 file changed, 93 insertions(+), 6 deletions(-)
diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
index 8904733961..d46fa166d3 100644
--- a/qemu-io-cmds.c
+++ b/qemu-io-cmds.c
@@ -350,6 +350,79 @@ static void qemu_io_free(void *p)
qemu_vfree(p);
}
+/*
+ * qemu_io_alloc_from_file()
+ *
+ * Allocates the buffer and populates it with the content of the given file
+ * up to @len bytes. If the file length is less than @len, then the buffer
+ * is populated with the file content cyclically.
+ *
+ * @blk - the block backend where the buffer content is going to be written to
+ * @len - the buffer length
+ * @file_name - the file to read the content from
+ *
+ * Returns: the buffer pointer on success
+ * NULL on error
+ */
+static void *qemu_io_alloc_from_file(BlockBackend *blk, size_t len,
+ const char *file_name)
+{
+char *buf, *buf_origin;
+FILE *f = fopen(file_name, "r");
+int pattern_len;
+
+if (!f) {
+perror(file_name);
+return NULL;
+}
+
+if (qemuio_misalign) {
+len += MISALIGN_OFFSET;
+}
+
+buf_origin = buf = blk_blockalign(blk, len);
+
+if (qemuio_misalign) {
+buf_origin += MISALIGN_OFFSET;
+buf += MISALIGN_OFFSET;
+len -= MISALIGN_OFFSET;
+}
+
+pattern_len = fread(buf_origin, 1, len, f);
+
+if (ferror(f)) {
+perror(file_name);
+goto error;
+}
+
+if (pattern_len == 0) {
+fprintf(stderr, "%s: file is empty\n", file_name);
+goto error;
+}
+
+fclose(f);
+
+if (len > pattern_len) {
+len -= pattern_len;
+buf += pattern_len;
+
+while (len > 0) {
+size_t len_to_copy = MIN(pattern_len, len);
+
+memcpy(buf, buf_origin, len_to_copy);
+
+len -= len_to_copy;
+buf += len_to_copy;
+}
+}
+
+return buf_origin;
+
+error:
+qemu_io_free(buf_origin);
+return NULL;
+}
+
static void dump_buffer(const void *buffer, int64_t offset, int64_t len)
{
uint64_t i;
@@ -948,6 +1021,7 @@ static void write_help(void)
" -n, -- with -z, don't allow slow fallback\n"
" -p, -- ignored for backwards compatibility\n"
" -P, -- use different pattern to fill file\n"
+" -s, -- use a pattern file to fill the write buffer\n"
" -C, -- report statistics in a machine parsable format\n"
" -q, -- quiet mode, do not show I/O statistics\n"
" -u, -- with -z, allow unmapping\n"
@@ -964,7 +1038,7 @@ static const cmdinfo_t write_cmd = {
.perm = BLK_PERM_WRITE,
.argmin = 2,
.argmax = -1,
-.args = "[-bcCfnquz] [-P pattern] off len",
+.args = "[-bcCfnquz] [-P pattern | -s source_file] off len",
.oneline= "writes a number of bytes at a specified offset",
.help = write_help,
};
@@ -973,7 +1047,7 @@ static int write_f(BlockBackend *blk, int argc, char
**argv)
{
struct timespec t1, t2;
bool Cflag = false, qflag = false, bflag = false;
-bool Pflag = false, zflag = false, cflag = false;
+bool Pflag = false, zflag = false, cflag = false, sflag = false;
int flags = 0;
int c, cnt, ret;
char *buf = NULL;
@@ -982,8 +1056,9 @@ static int write_f(BlockBackend *blk, int argc, char
**argv)
/* Some compilers get confused and warn if this is not initialized. */
int64_t total = 0;
int pattern = 0xcd;
+const char *file_name = NULL;
-while ((c = getopt(argc, argv, "bcCfnpP:quz")) != -1) {
+while ((c = getopt(argc, argv, "bcCfnpP:qs:uz")) != -1) {
switch (c) {
case 'b':
bflag = true;
@@ -1013,6 +1088,10 @@ static int write_f(BlockBackend *blk, int argc, char
**argv)
case 'q':
qflag = true;
break;
+case 's':
+sflag = true;
+file_name = optarg;
+break;
case 'u':
flags |= BDRV_REQ_MAY_UNMAP;
break;
@@ -1050,8 +1129,9 @@ static int write_f(BlockBackend *blk, int argc, char
**argv)
return -EINVAL;
}
-if (zflag && Pflag) {
-printf("-z and -P cannot be specified at the same time\n");
+if (zflag + Pflag + sflag > 1) {
+printf("Only one of -z, -P, and -s "
+ "can be specified at the same time\n");
return -EINVAL;
}
@@ -1087,7 +1167,14 @@ static int write_f(BlockBackend *blk, int argc, char
**argv)
}
if (!zflag) {
-buf = qemu_io_alloc(blk, count, pattern);
+if (sflag) {
+buf = qemu_io_alloc_from_file(blk, count, file_name);
+
Re: [Qemu-block] [PATCH v4 1/3] qcow2: introduce compression type feature
Am 28.08.2019 um 14:56 hat Denis Plotnikov geschrieben:
> The patch adds some preparation parts for incompatible compression type
> feature to QCOW2 header that indicates that *all* compressed clusters
> must be (de)compressed using a certain compression type.
>
> It is implied that the compression type is set on the image creation and
> can be changed only later by image conversion, thus compression type
> defines the only compression algorithm used for the image.
>
> The goal of the feature is to add support of other compression algorithms
> to qcow2. For example, ZSTD which is more effective on compression than ZLIB.
> It works roughly 2x faster than ZLIB providing a comparable compression ratio
> and therefore provides a performance advantage in backup scenarios.
>
> The default compression is ZLIB. Images created with ZLIB compression type
> are backward compatible with older qemu versions.
>
> Signed-off-by: Denis Plotnikov
> @@ -359,6 +364,13 @@ typedef struct BDRVQcow2State {
>
> bool metadata_preallocation_checked;
> bool metadata_preallocation;
> +/*
> + * Compression type used for the image. Default: 0 - ZLIB
> + * The image compression type is set on image creation.
> + * The only way to change the compression type is to convert the image
> + * with the desired compression type set
> + */
> +uint32_t compression_type;
Should this use the enum Qcow2CompressionType instead of a plain int?
If you don't need to respin, I can make this change while applying.
Kevin
[Qemu-block] [PATCH] block/nfs: add support for nfs_umount
libnfs recently added support for unmounting. Add support in Qemu too. Signed-off-by: Peter Lieven --- block/nfs.c | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/block/nfs.c b/block/nfs.c index 0ec50953e4..9d30963fd8 100644 --- a/block/nfs.c +++ b/block/nfs.c @@ -1,7 +1,7 @@ /* * QEMU Block driver for native access to files on NFS shares * - * Copyright (c) 2014-2017 Peter Lieven + * Copyright (c) 2014-2019 Peter Lieven * * Permission is hereby granted, free of charge, to any person obtaining a copy * of this software and associated documentation files (the "Software"), to deal @@ -394,6 +394,9 @@ static void nfs_client_close(NFSClient *client) nfs_close(client->context, client->fh); client->fh = NULL; } +#ifdef LIBNFS_FEATURE_UMOUNT +nfs_umount(client->context); +#endif aio_set_fd_handler(client->aio_context, nfs_get_fd(client->context), false, NULL, NULL, NULL, NULL); nfs_destroy_context(client->context); -- 2.17.1
[Qemu-block] [PULL v2 12/16] file-posix: fix request_alignment typo
From: Stefan Hajnoczi
Fixes: a6b257a08e3d72219f03e461a52152672fec0612
("file-posix: Handle undetectable alignment")
Signed-off-by: Stefan Hajnoczi
Message-id: 20190827101328.4062-1-stefa...@redhat.com
Reviewed-by: Thomas Huth
Signed-off-by: Max Reitz
---
block/file-posix.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/block/file-posix.c b/block/file-posix.c
index 447f937aa1..71f168ee2f 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -380,7 +380,7 @@ static void raw_probe_alignment(BlockDriverState *bs, int
fd, Error **errp)
for (i = 0; i < ARRAY_SIZE(alignments); i++) {
align = alignments[i];
if (raw_is_io_aligned(fd, buf + align, max_align)) {
-/* Fallback to request_aligment. */
+/* Fallback to request_alignment. */
s->buf_align = (align != 1) ? align : bs->bl.request_alignment;
break;
}
--
2.21.0
[Qemu-block] [PULL v2 09/16] iotests: Disable broken streamOptimized tests
streamOptimized does not support writes that do not span exactly one cluster. Furthermore, it cannot rewrite already allocated clusters. As such, many iotests do not work with it. Disable them. Signed-off-by: Max Reitz Message-id: 20190815153638.4600-6-mre...@redhat.com Reviewed-by: John Snow Signed-off-by: Max Reitz --- tests/qemu-iotests/002 | 1 + tests/qemu-iotests/003 | 1 + tests/qemu-iotests/005 | 3 ++- tests/qemu-iotests/009 | 1 + tests/qemu-iotests/010 | 1 + tests/qemu-iotests/011 | 1 + tests/qemu-iotests/017 | 3 ++- tests/qemu-iotests/018 | 3 ++- tests/qemu-iotests/019 | 3 ++- tests/qemu-iotests/020 | 3 ++- tests/qemu-iotests/027 | 1 + tests/qemu-iotests/032 | 1 + tests/qemu-iotests/033 | 1 + tests/qemu-iotests/034 | 3 ++- tests/qemu-iotests/037 | 3 ++- tests/qemu-iotests/063 | 3 ++- tests/qemu-iotests/072 | 1 + tests/qemu-iotests/105 | 3 ++- tests/qemu-iotests/197 | 1 + tests/qemu-iotests/215 | 1 + tests/qemu-iotests/251 | 1 + 21 files changed, 30 insertions(+), 9 deletions(-) diff --git a/tests/qemu-iotests/002 b/tests/qemu-iotests/002 index fd413bce48..1a0d411df5 100755 --- a/tests/qemu-iotests/002 +++ b/tests/qemu-iotests/002 @@ -38,6 +38,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15 _supported_fmt generic _supported_proto generic +_unsupported_imgopts "subformat=streamOptimized" size=128M diff --git a/tests/qemu-iotests/003 b/tests/qemu-iotests/003 index ccd3a39dfb..33eeade0de 100755 --- a/tests/qemu-iotests/003 +++ b/tests/qemu-iotests/003 @@ -38,6 +38,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15 _supported_fmt generic _supported_proto generic +_unsupported_imgopts "subformat=streamOptimized" size=128M offset=67M diff --git a/tests/qemu-iotests/005 b/tests/qemu-iotests/005 index 9c7681c19b..58442762fe 100755 --- a/tests/qemu-iotests/005 +++ b/tests/qemu-iotests/005 @@ -43,7 +43,8 @@ _supported_fmt generic _supported_proto generic _supported_os Linux _unsupported_imgopts "subformat=twoGbMaxExtentFlat" \ - "subformat=twoGbMaxExtentSparse" + "subformat=twoGbMaxExtentSparse" \ + "subformat=streamOptimized" # vpc is limited to 127GB, so we can't test it here if [ "$IMGFMT" = "vpc" ]; then diff --git a/tests/qemu-iotests/009 b/tests/qemu-iotests/009 index 51b200db1d..4dc7d210f9 100755 --- a/tests/qemu-iotests/009 +++ b/tests/qemu-iotests/009 @@ -38,6 +38,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15 _supported_fmt generic _supported_proto generic +_unsupported_imgopts "subformat=streamOptimized" size=6G diff --git a/tests/qemu-iotests/010 b/tests/qemu-iotests/010 index 48c533f632..df809b3088 100755 --- a/tests/qemu-iotests/010 +++ b/tests/qemu-iotests/010 @@ -38,6 +38,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15 _supported_fmt generic _supported_proto generic +_unsupported_imgopts "subformat=streamOptimized" size=6G diff --git a/tests/qemu-iotests/011 b/tests/qemu-iotests/011 index 56f704b5b9..57b99ae4a9 100755 --- a/tests/qemu-iotests/011 +++ b/tests/qemu-iotests/011 @@ -38,6 +38,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15 _supported_fmt generic _supported_proto generic +_unsupported_imgopts "subformat=streamOptimized" size=6G diff --git a/tests/qemu-iotests/017 b/tests/qemu-iotests/017 index 79875de454..0a4b854e65 100755 --- a/tests/qemu-iotests/017 +++ b/tests/qemu-iotests/017 @@ -41,7 +41,8 @@ trap "_cleanup; exit \$status" 0 1 2 3 15 _supported_fmt qcow qcow2 vmdk qed _supported_proto generic _unsupported_proto vxhs -_unsupported_imgopts "subformat=monolithicFlat" "subformat=twoGbMaxExtentFlat" +_unsupported_imgopts "subformat=monolithicFlat" "subformat=twoGbMaxExtentFlat" \ + "subformat=streamOptimized" TEST_OFFSETS="0 4294967296" diff --git a/tests/qemu-iotests/018 b/tests/qemu-iotests/018 index 78169838ba..c69ce09209 100755 --- a/tests/qemu-iotests/018 +++ b/tests/qemu-iotests/018 @@ -41,7 +41,8 @@ trap "_cleanup; exit \$status" 0 1 2 3 15 _supported_fmt qcow qcow2 vmdk qed _supported_proto file _supported_os Linux -_unsupported_imgopts "subformat=monolithicFlat" "subformat=twoGbMaxExtentFlat" +_unsupported_imgopts "subformat=monolithicFlat" "subformat=twoGbMaxExtentFlat" \ + "streamOptimized" TEST_OFFSETS="0 4294967296" diff --git a/tests/qemu-iotests/019 b/tests/qemu-iotests/019 index a56dd30bed..b4f5234609 100755 --- a/tests/qemu-iotests/019 +++ b/tests/qemu-iotests/019 @@ -47,7 +47,8 @@ _supported_proto file _supported_os Linux _unsupported_imgopts "subformat=monolithicFlat" \ "subformat=twoGbMaxExtentFlat" \ - "subformat=twoGbMaxExtentSparse" + "subformat=twoGbMaxExtentSparse" \ + "subformat=streamOptimized" TEST_OFFSETS="0 4294967296" CLUSTER_SIZE=65536 diff --git a/tests/qemu-iotests/020 b/tests/qemu-iotests/020 index 6b0ebb37d2..f41b92f35f 100755 --- a/tests/qemu-iotests/020 +++
[Qemu-block] [PULL v2 03/16] block: posix: Always allocate the first block
From: Nir Soffer
When creating an image with preallocation "off" or "falloc", the first
block of the image is typically not allocated. When using Gluster
storage backed by XFS filesystem, reading this block using direct I/O
succeeds regardless of request length, fooling alignment detection.
In this case we fallback to a safe value (4096) instead of the optimal
value (512), which may lead to unneeded data copying when aligning
requests. Allocating the first block avoids the fallback.
Since we allocate the first block even with preallocation=off, we no
longer create images with zero disk size:
$ ./qemu-img create -f raw test.raw 1g
Formatting 'test.raw', fmt=raw size=1073741824
$ ls -lhs test.raw
4.0K -rw-r--r--. 1 nsoffer nsoffer 1.0G Aug 16 23:48 test.raw
And converting the image requires additional cluster:
$ ./qemu-img measure -f raw -O qcow2 test.raw
required size: 458752
fully allocated size: 1074135040
When using format like vmdk with multiple files per image, we allocate
one block per file:
$ ./qemu-img create -f vmdk -o subformat=twoGbMaxExtentFlat test.vmdk 4g
Formatting 'test.vmdk', fmt=vmdk size=4294967296 compat6=off
hwversion=undefined subformat=twoGbMaxExtentFlat
$ ls -lhs test*.vmdk
4.0K -rw-r--r--. 1 nsoffer nsoffer 2.0G Aug 27 03:23 test-f001.vmdk
4.0K -rw-r--r--. 1 nsoffer nsoffer 2.0G Aug 27 03:23 test-f002.vmdk
4.0K -rw-r--r--. 1 nsoffer nsoffer 353 Aug 27 03:23 test.vmdk
I did quick performance test for copying disks with qemu-img convert to
new raw target image to Gluster storage with sector size of 512 bytes:
for i in $(seq 10); do
rm -f dst.raw
sleep 10
time ./qemu-img convert -f raw -O raw -t none -T none src.raw dst.raw
done
Here is a table comparing the total time spent:
TypeBefore(s) After(s)Diff(%)
---
real 530.028469.123 -11.4
user 17.204 10.768 -37.4
sys17.881 7.011 -60.7
We can see very clear improvement in CPU usage.
Signed-off-by: Nir Soffer
Message-id: 20190827010528.8818-2-nsof...@redhat.com
Reviewed-by: Max Reitz
Signed-off-by: Max Reitz
---
block/file-posix.c| 51 +++
tests/qemu-iotests/059.out| 2 +-
tests/qemu-iotests/{150.out => 150.out.qcow2} | 0
tests/qemu-iotests/150.out.raw| 12 +
tests/qemu-iotests/175| 19 ---
tests/qemu-iotests/175.out| 8 +--
tests/qemu-iotests/178.out.qcow2 | 4 +-
tests/qemu-iotests/221.out| 12 +++--
tests/qemu-iotests/253.out| 12 +++--
9 files changed, 99 insertions(+), 21 deletions(-)
rename tests/qemu-iotests/{150.out => 150.out.qcow2} (100%)
create mode 100644 tests/qemu-iotests/150.out.raw
diff --git a/block/file-posix.c b/block/file-posix.c
index fbeb0068db..447f937aa1 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -1749,6 +1749,43 @@ static int handle_aiocb_discard(void *opaque)
return ret;
}
+/*
+ * Help alignment probing by allocating the first block.
+ *
+ * When reading with direct I/O from unallocated area on Gluster backed by XFS,
+ * reading succeeds regardless of request length. In this case we fallback to
+ * safe alignment which is not optimal. Allocating the first block avoids this
+ * fallback.
+ *
+ * fd may be opened with O_DIRECT, but we don't know the buffer alignment or
+ * request alignment, so we use safe values.
+ *
+ * Returns: 0 on success, -errno on failure. Since this is an optimization,
+ * caller may ignore failures.
+ */
+static int allocate_first_block(int fd, size_t max_size)
+{
+size_t write_size = (max_size < MAX_BLOCKSIZE)
+? BDRV_SECTOR_SIZE
+: MAX_BLOCKSIZE;
+size_t max_align = MAX(MAX_BLOCKSIZE, getpagesize());
+void *buf;
+ssize_t n;
+int ret;
+
+buf = qemu_memalign(max_align, write_size);
+memset(buf, 0, write_size);
+
+do {
+n = pwrite(fd, buf, write_size, 0);
+} while (n == -1 && errno == EINTR);
+
+ret = (n == -1) ? -errno : 0;
+
+qemu_vfree(buf);
+return ret;
+}
+
static int handle_aiocb_truncate(void *opaque)
{
RawPosixAIOData *aiocb = opaque;
@@ -1788,6 +1825,17 @@ static int handle_aiocb_truncate(void *opaque)
/* posix_fallocate() doesn't set errno. */
error_setg_errno(errp, -result,
"Could not preallocate new data");
+} else if (current_length == 0) {
+/*
+ * posix_fallocate() uses fallocate() if the filesystem
+ * supports it, or fallback to manually writing zeroes. If
+ * fallocate() was used, unaligned reads from the fallocated
+ * area in raw_probe_alignment() will succeed, hence we need to
+ * allocate the first block.
+
[Qemu-block] [PULL v2 06/16] vmdk: Use bdrv_dirname() for relative extent paths
This makes iotest 033 pass with e.g. subformat=monolithicFlat. It also
turns a former error in 059 into success.
Signed-off-by: Max Reitz
Message-id: 20190815153638.4600-3-mre...@redhat.com
Reviewed-by: John Snow
Signed-off-by: Max Reitz
---
block/vmdk.c | 54 --
tests/qemu-iotests/059 | 7 +++--
tests/qemu-iotests/059.out | 4 ++-
3 files changed, 42 insertions(+), 23 deletions(-)
diff --git a/block/vmdk.c b/block/vmdk.c
index fd78fd0ccf..a7f82e665e 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -1076,8 +1076,7 @@ static const char *next_line(const char *s)
}
static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
- const char *desc_file_path, QDict *options,
- Error **errp)
+ QDict *options, Error **errp)
{
int ret;
int matches;
@@ -1087,6 +1086,7 @@ static int vmdk_parse_extents(const char *desc,
BlockDriverState *bs,
const char *p, *np;
int64_t sectors = 0;
int64_t flat_offset;
+char *desc_file_dir = NULL;
char *extent_path;
BdrvChild *extent_file;
BDRVVmdkState *s = bs->opaque;
@@ -1130,16 +1130,23 @@ static int vmdk_parse_extents(const char *desc,
BlockDriverState *bs,
continue;
}
-if (!path_is_absolute(fname) && !path_has_protocol(fname) &&
-!desc_file_path[0])
-{
-bdrv_refresh_filename(bs->file->bs);
-error_setg(errp, "Cannot use relative extent paths with VMDK "
- "descriptor file '%s'", bs->file->bs->filename);
-return -EINVAL;
-}
+if (path_is_absolute(fname)) {
+extent_path = g_strdup(fname);
+} else {
+if (!desc_file_dir) {
+desc_file_dir = bdrv_dirname(bs->file->bs, errp);
+if (!desc_file_dir) {
+bdrv_refresh_filename(bs->file->bs);
+error_prepend(errp, "Cannot use relative paths with VMDK "
+ "descriptor file '%s': ",
+ bs->file->bs->filename);
+ret = -EINVAL;
+goto out;
+}
+}
-extent_path = path_combine(desc_file_path, fname);
+extent_path = g_strconcat(desc_file_dir, fname, NULL);
+}
ret = snprintf(extent_opt_prefix, 32, "extents.%d", s->num_extents);
assert(ret < 32);
@@ -1149,7 +1156,8 @@ static int vmdk_parse_extents(const char *desc,
BlockDriverState *bs,
g_free(extent_path);
if (local_err) {
error_propagate(errp, local_err);
-return -EINVAL;
+ret = -EINVAL;
+goto out;
}
/* save to extents array */
@@ -1160,7 +1168,7 @@ static int vmdk_parse_extents(const char *desc,
BlockDriverState *bs,
0, 0, 0, 0, 0, , errp);
if (ret < 0) {
bdrv_unref_child(bs, extent_file);
-return ret;
+goto out;
}
extent->flat_start_offset = flat_offset << 9;
} else if (!strcmp(type, "SPARSE") || !strcmp(type, "VMFSSPARSE")) {
@@ -1175,24 +1183,27 @@ static int vmdk_parse_extents(const char *desc,
BlockDriverState *bs,
g_free(buf);
if (ret) {
bdrv_unref_child(bs, extent_file);
-return ret;
+goto out;
}
extent = >extents[s->num_extents - 1];
} else if (!strcmp(type, "SESPARSE")) {
ret = vmdk_open_se_sparse(bs, extent_file, bs->open_flags, errp);
if (ret) {
bdrv_unref_child(bs, extent_file);
-return ret;
+goto out;
}
extent = >extents[s->num_extents - 1];
} else {
error_setg(errp, "Unsupported extent type '%s'", type);
bdrv_unref_child(bs, extent_file);
-return -ENOTSUP;
+ret = -ENOTSUP;
+goto out;
}
extent->type = g_strdup(type);
}
-return 0;
+
+ret = 0;
+goto out;
invalid:
np = next_line(p);
@@ -1201,7 +1212,11 @@ invalid:
np--;
}
error_setg(errp, "Invalid extent line: %.*s", (int)(np - p), p);
-return -EINVAL;
+ret = -EINVAL;
+
+out:
+g_free(desc_file_dir);
+return ret;
}
static int vmdk_open_desc_file(BlockDriverState *bs, int flags, char *buf,
@@ -1228,8 +1243,7 @@ static int vmdk_open_desc_file(BlockDriverState *bs, int
flags, char *buf,
}
s->create_type = g_strdup(ct);
s->desc_offset = 0;
-ret = vmdk_parse_extents(buf, bs, bs->file->bs->exact_filename, options,
- errp);
+ret = vmdk_parse_extents(buf, bs, options, errp);
exit:
return ret;
}
diff --git
[Qemu-block] [PULL v2 07/16] iotests: Keep testing broken relative extent paths
We had a test for a case where relative extent paths did not work, but
unfortunately we just fixed the underlying problem, so it works now.
This patch adds a new test case that still fails.
Signed-off-by: Max Reitz
Reviewed-by: John Snow
Message-id: 20190815153638.4600-4-mre...@redhat.com
Reviewed-by: John Snow
Signed-off-by: Max Reitz
---
tests/qemu-iotests/059 | 27 +++
tests/qemu-iotests/059.out | 4
2 files changed, 31 insertions(+)
diff --git a/tests/qemu-iotests/059 b/tests/qemu-iotests/059
index fbed5f9483..10bfbaecec 100755
--- a/tests/qemu-iotests/059
+++ b/tests/qemu-iotests/059
@@ -114,6 +114,8 @@ $QEMU_IMG convert -f qcow2 -O vmdk -o
subformat=streamOptimized "$TEST_IMG.qcow2
echo
echo "=== Testing monolithicFlat with internally generated JSON file name ==="
+
+echo '--- blkdebug ---'
# Should work, because bdrv_dirname() works fine with blkdebug
IMGOPTS="subformat=monolithicFlat" _make_test_img 64M
$QEMU_IO -c "open -o
driver=$IMGFMT,file.driver=blkdebug,file.image.filename=$TEST_IMG,file.inject-error.0.event=read_aio"
\
@@ -122,6 +124,31 @@ $QEMU_IO -c "open -o
driver=$IMGFMT,file.driver=blkdebug,file.image.filename=$TE
| _filter_testdir | _filter_imgfmt | _filter_img_info
_cleanup_test_img
+echo '--- quorum ---'
+# Should not work, because bdrv_dirname() does not work with quorum
+IMGOPTS="subformat=monolithicFlat" _make_test_img 64M
+cp "$TEST_IMG" "$TEST_IMG.orig"
+
+filename="json:{
+\"driver\": \"$IMGFMT\",
+\"file\": {
+\"driver\": \"quorum\",
+\"children\": [ {
+\"driver\": \"file\",
+\"filename\": \"$TEST_IMG\"
+}, {
+\"driver\": \"file\",
+\"filename\": \"$TEST_IMG.orig\"
+} ],
+\"vote-threshold\": 1
+} }"
+
+filename=$(echo "$filename" | tr '\n' ' ' | sed -e 's/\s\+/ /g')
+$QEMU_IMG info "$filename" 2>&1 \
+| sed -e "s/'json:[^']*'/\$QUORUM_FILE/g" \
+| _filter_testdir | _filter_imgfmt | _filter_img_info
+
+
echo
echo "=== Testing version 3 ==="
_use_sample_img iotest-version3.vmdk.bz2
diff --git a/tests/qemu-iotests/059.out b/tests/qemu-iotests/059.out
index a51b571d27..39bf7e211d 100644
--- a/tests/qemu-iotests/059.out
+++ b/tests/qemu-iotests/059.out
@@ -2049,10 +2049,14 @@ wrote 512/512 bytes at offset 10240
512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
=== Testing monolithicFlat with internally generated JSON file name ===
+--- blkdebug ---
Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
format name: IMGFMT
cluster size: 0 bytes
vm state offset: 0 bytes
+--- quorum ---
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+qemu-img: Could not open $QUORUM_FILE: Cannot use relative paths with VMDK
descriptor file $QUORUM_FILE: Cannot generate a base directory for quorum nodes
=== Testing version 3 ===
image: TEST_DIR/iotest-version3.IMGFMT
--
2.21.0
[Qemu-block] [PULL v2 00/16] Block patches
The following changes since commit 54b89db5309d5fa8b5d3fe5fe56f81704e2f9706:
Merge remote-tracking branch 'remotes/stefanha/tags/block-pull-request' into
staging (2019-09-03 09:43:26 +0100)
are available in the Git repository at:
https://github.com/XanClic/qemu.git tags/pull-block-2019-09-03
for you to fetch changes up to 755c5fe79d88717600356d3edf04835bba43dcb6:
iotests: Unify cache mode quoting (2019-09-03 14:56:06 +0200)
Block patches:
- qemu-io now accepts a file to read a write pattern from
- Ensure that raw files have their first block allocated so we can probe
the O_DIRECT alignment if necessary
- Various fixes
v2:
- Added a patch we already had on the list to keep the iotests passing
when $DISPLAY is not set
Denis Plotnikov (1):
qemu-io: add pattern file for write command
Max Reitz (8):
iotests: Fix _filter_img_create()
vmdk: Use bdrv_dirname() for relative extent paths
iotests: Keep testing broken relative extent paths
vmdk: Reject invalid compressed writes
iotests: Disable broken streamOptimized tests
iotests: Disable 110 for vmdk.twoGbMaxExtentSparse
iotests: Disable 126 for flat vmdk subformats
iotests: Add -display none to the qemu options
Nir Soffer (3):
block: posix: Always allocate the first block
iotests: Test allocate_first_block() with O_DIRECT
iotests: Unify cache mode quoting
Stefan Hajnoczi (1):
file-posix: fix request_alignment typo
Thomas Huth (2):
iotests: Check for enabled drivers before testing them
tests/check-block: Skip iotests when sanitizers are enabled
Vladimir Sementsov-Ogievskiy (1):
block: fix permission update in bdrv_replace_node
block.c | 5 +-
block/file-posix.c| 53 +-
block/vmdk.c | 64
qemu-io-cmds.c| 99 +--
tests/check-block.sh | 5 +
tests/qemu-iotests/002| 1 +
tests/qemu-iotests/003| 1 +
tests/qemu-iotests/005| 3 +-
tests/qemu-iotests/009| 1 +
tests/qemu-iotests/010| 1 +
tests/qemu-iotests/011| 1 +
tests/qemu-iotests/017| 3 +-
tests/qemu-iotests/018| 3 +-
tests/qemu-iotests/019| 3 +-
tests/qemu-iotests/020| 3 +-
tests/qemu-iotests/026| 4 +-
tests/qemu-iotests/027| 1 +
tests/qemu-iotests/032| 1 +
tests/qemu-iotests/033| 1 +
tests/qemu-iotests/034| 3 +-
tests/qemu-iotests/037| 3 +-
tests/qemu-iotests/039| 4 +-
tests/qemu-iotests/052| 2 +-
tests/qemu-iotests/059| 34 ++-
tests/qemu-iotests/059.out| 26 +++--
tests/qemu-iotests/063| 3 +-
tests/qemu-iotests/071| 1 +
tests/qemu-iotests/072| 1 +
tests/qemu-iotests/081| 4 +-
tests/qemu-iotests/091| 4 +-
tests/qemu-iotests/099| 1 +
tests/qemu-iotests/105| 3 +-
tests/qemu-iotests/110| 3 +-
tests/qemu-iotests/120| 1 +
tests/qemu-iotests/126| 2 +
tests/qemu-iotests/{150.out => 150.out.qcow2} | 0
tests/qemu-iotests/150.out.raw| 12 +++
tests/qemu-iotests/162| 4 +-
tests/qemu-iotests/175| 47 +++--
tests/qemu-iotests/175.out| 16 ++-
tests/qemu-iotests/178.out.qcow2 | 4 +-
tests/qemu-iotests/184| 1 +
tests/qemu-iotests/186| 1 +
tests/qemu-iotests/197| 1 +
tests/qemu-iotests/215| 1 +
tests/qemu-iotests/221.out| 12 ++-
tests/qemu-iotests/251| 1 +
tests/qemu-iotests/253.out| 12 ++-
tests/qemu-iotests/check | 6 +-
tests/qemu-iotests/common.filter | 4 +-
tests/qemu-iotests/common.rc | 14 +++
51 files changed, 394 insertions(+), 90 deletions(-)
rename tests/qemu-iotests/{150.out => 150.out.qcow2} (100%)
create mode 100644 tests/qemu-iotests/150.out.raw
--
2.21.0
[Qemu-block] [PULL v2 05/16] iotests: Fix _filter_img_create()
fe646693acc changed qemu-img create's output so that it no longer prints
single quotes around parameter values. The subformat and adapter_type
filters in _filter_img_create() have never been adapted to that change.
Fixes: fe646693acc13ac48b98435d14149ab04dc597bc
Signed-off-by: Max Reitz
Reviewed-by: John Snow
Message-id: 20190815153638.4600-2-mre...@redhat.com
Reviewed-by: John Snow
Signed-off-by: Max Reitz
---
tests/qemu-iotests/059.out | 16
tests/qemu-iotests/common.filter | 4 ++--
2 files changed, 10 insertions(+), 10 deletions(-)
diff --git a/tests/qemu-iotests/059.out b/tests/qemu-iotests/059.out
index fe3f861f3c..b2e718d29f 100644
--- a/tests/qemu-iotests/059.out
+++ b/tests/qemu-iotests/059.out
@@ -13,17 +13,17 @@ Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
qemu-io: can't open device TEST_DIR/t.vmdk: L1 size too big
=== Testing monolithicFlat creation and opening ===
-Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=2147483648
subformat=monolithicFlat
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=2147483648
image: TEST_DIR/t.IMGFMT
file format: IMGFMT
virtual size: 2 GiB (2147483648 bytes)
=== Testing monolithicFlat with zeroed_grain ===
qemu-img: TEST_DIR/t.IMGFMT: Flat image can't enable zeroed grain
-Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=2147483648
subformat=monolithicFlat
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=2147483648
=== Testing big twoGbMaxExtentFlat ===
-Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824000
subformat=twoGbMaxExtentFlat
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824000
image: TEST_DIR/t.vmdk
file format: vmdk
virtual size: 0.977 TiB (1073741824000 bytes)
@@ -2038,7 +2038,7 @@ Format specific information:
qemu-img: Could not open 'TEST_DIR/t.IMGFMT': Invalid extent line: RW 12582912
VMFS "dummy.IMGFMT" 1
=== Testing truncated sparse ===
-Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=107374182400
subformat=monolithicSparse
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=107374182400
qemu-img: Could not open 'TEST_DIR/t.IMGFMT': File truncated, expecting at
least 13172736 bytes
=== Converting to streamOptimized from image with small cluster size===
@@ -2049,7 +2049,7 @@ wrote 512/512 bytes at offset 10240
512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
=== Testing monolithicFlat with internally generated JSON file name ===
-Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
subformat=monolithicFlat
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
qemu-io: can't open: Cannot use relative extent paths with VMDK descriptor
file 'json:{"image": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT"},
"driver": "blkdebug", "inject-error.0.event": "read_aio"}'
=== Testing version 3 ===
@@ -2259,7 +2259,7 @@ read 512/512 bytes at offset 64931328
512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
=== Testing 4TB monolithicFlat creation and IO ===
-Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=4398046511104
subformat=monolithicFlat
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=4398046511104
image: TEST_DIR/t.IMGFMT
file format: IMGFMT
virtual size: 4 TiB (4398046511104 bytes)
@@ -2333,7 +2333,7 @@ read 1024/1024 bytes at offset 966367641600
1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
=== Testing qemu-img map on extents ===
-Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=33285996544
subformat=monolithicSparse
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=33285996544
wrote 1024/1024 bytes at offset 65024
1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
wrote 1024/1024 bytes at offset 2147483136
@@ -2344,7 +2344,7 @@ Offset Length Mapped to File
0 0x2 0x3fTEST_DIR/t.vmdk
0x7fff 0x2 0x41TEST_DIR/t.vmdk
0x14000 0x1 0x43TEST_DIR/t.vmdk
-Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=33285996544
subformat=twoGbMaxExtentSparse
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=33285996544
wrote 1024/1024 bytes at offset 65024
1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
wrote 1024/1024 bytes at offset 2147483136
diff --git a/tests/qemu-iotests/common.filter b/tests/qemu-iotests/common.filter
index 8e9235d6fe..445a1c23e0 100644
--- a/tests/qemu-iotests/common.filter
+++ b/tests/qemu-iotests/common.filter
@@ -130,8 +130,8 @@ _filter_img_create()
-e "s# compat6=\\(on\\|off\\)##g" \
-e "s# static=\\(on\\|off\\)##g" \
-e "s# zeroed_grain=\\(on\\|off\\)##g" \
--e "s# subformat='[^']*'##g" \
--e "s# adapter_type='[^']*'##g" \
+-e "s# subformat=[^ ]*##g" \
+-e "s# adapter_type=[^ ]*##g" \
-e "s# hwversion=[^ ]*##g" \
-e "s# lazy_refcounts=\\(on\\|off\\)##g" \
-e "s# block_size=[0-9]\\+##g" \
--
2.21.0
Re: [Qemu-block] [PATCH V2] block/vhdx: add check for truncated image files
Am 03.09.2019 um 15:10 hat Peter Lieven geschrieben:
> Am 03.09.19 um 15:02 schrieb Kevin Wolf:
> > Am 02.09.2019 um 17:24 hat Peter Lieven geschrieben:
> > > qemu is currently not able to detect truncated vhdx image files.
> > > Add a basic check if all allocated blocks are reachable at open and
> > > report all errors during bdrv_co_check.
> > >
> > > Signed-off-by: Peter Lieven
> > > ---
> > > V2: - add error reporting [Kevin]
> > > - use bdrv_getlength instead of bdrv_get_allocated_file_size [Kevin]
> > > - factor out BAT entry check and add error reporting for region
> > >overlaps
> > > - already check on vhdx_open
> > >
> > > block/vhdx.c | 85 +---
> > > 1 file changed, 68 insertions(+), 17 deletions(-)
> > >
> > > diff --git a/block/vhdx.c b/block/vhdx.c
> > > index 6a09d0a55c..6afba5e8c2 100644
> > > --- a/block/vhdx.c
> > > +++ b/block/vhdx.c
> > > @@ -24,6 +24,7 @@
> > > #include "qemu/option.h"
> > > #include "qemu/crc32c.h"
> > > #include "qemu/bswap.h"
> > > +#include "qemu/error-report.h"
> > > #include "vhdx.h"
> > > #include "migration/blocker.h"
> > > #include "qemu/uuid.h"
> > > @@ -235,6 +236,9 @@ static int vhdx_region_check(BDRVVHDXState *s,
> > > uint64_t start, uint64_t length)
> > > end = start + length;
> > > QLIST_FOREACH(r, >regions, entries) {
> > > if (!((start >= r->end) || (end <= r->start))) {
> > > +error_report("VHDX region %" PRIu64 "-%" PRIu64 " overlaps
> > > with "
> > > + "region %" PRIu64 "-%." PRIu64, start, end,
> > > r->start,
> > > + r->end);
> > > ret = -EINVAL;
> > > goto exit;
> > > }
> > > @@ -877,6 +881,60 @@ static void vhdx_calc_bat_entries(BDRVVHDXState *s)
> > > }
> > > +static int vhdx_check_bat_entries(BlockDriverState *bs, int *errcnt)
> > > +{
> > > +BDRVVHDXState *s = bs->opaque;
> > > +int64_t image_file_size = bdrv_getlength(bs->file->bs);
> > > +uint64_t payblocks = s->chunk_ratio;
> > > +int i, ret = 0;
> > bdrv_getlength() can fail. It's probably better to error out immediately
> > instead of reporting that every BAT entry is > -1.
> >
> > > +for (i = 0; i < s->bat_entries; i++) {
> > s->bat_entries is uint32_t, so i should probably be the same.
> >
> > > +if ((s->bat[i] & VHDX_BAT_STATE_BIT_MASK) ==
> > > +PAYLOAD_BLOCK_FULLY_PRESENT) {
> > > +/*
> > > + * Check if fully allocated BAT entries do not reside after
> > > + * end of the image file.
> > > + */
> > > +if ((s->bat[i] & VHDX_BAT_FILE_OFF_MASK) + s->block_size >
> > > +image_file_size) {
> > Didn't we want to introduce an overflow check before making this check?
> > Something like if (bat_offset > UINT64_MAX - s->block_size)?
>
> Sorry, i missed that.
>
> The bat entries are UINT64_T so this check will always be false for the
> default block size of 1MB. In fact we should check for
>
> bat_offset > INT64_MAX - s->block_size
>
> right?
Hm, VHDX_BAT_FILE_OFF_MASK is 0xFFF0ULL, so 2^64 - 1 MB.
With a block size of 1 MB, this check would trigger because the offset
would be one byte higher than allowed (because offset + block_size
would be 0). For larger block sizes, it's more obvious that we can run
into this case.
As for INT64_MAX, I'm not sure if it's strictly necessary because the
code seems to use unsigned variables everywhere. But it feels safer and
shouldn't make any difference in practice, so I agree with using it.
Kevin
[Qemu-block] [PULL v2 04/16] iotests: Test allocate_first_block() with O_DIRECT
From: Nir Soffer
Using block_resize we can test allocate_first_block() with file
descriptor opened with O_DIRECT, ensuring that it works for any size
larger than 4096 bytes.
Testing smaller sizes is tricky as the result depends on the filesystem
used for testing. For example on NFS any size will work since O_DIRECT
does not require any alignment.
Signed-off-by: Nir Soffer
Reviewed-by: Max Reitz
Message-id: 20190827010528.8818-3-nsof...@redhat.com
Signed-off-by: Max Reitz
---
tests/qemu-iotests/175 | 28
tests/qemu-iotests/175.out | 8
2 files changed, 36 insertions(+)
diff --git a/tests/qemu-iotests/175 b/tests/qemu-iotests/175
index 7ba28b3c1b..55db2803ed 100755
--- a/tests/qemu-iotests/175
+++ b/tests/qemu-iotests/175
@@ -49,6 +49,23 @@ _filter_blocks()
-e "s/blocks=$((extra_blocks + img_size / 512))\\(\$\\|[^0-9]\\)/max
allocation/"
}
+# Resize image using block_resize.
+# Parameter 1: image path
+# Parameter 2: new size
+_block_resize()
+{
+local path=$1
+local size=$2
+
+$QEMU -qmp stdio -nographic -nodefaults \
+-blockdev file,node-name=file,filename=$path,cache.direct=on \
+<
[Qemu-block] [PULL v2 02/16] block: fix permission update in bdrv_replace_node
From: Vladimir Sementsov-Ogievskiy
It's wrong to OR shared permissions. It may lead to crash on further
permission updates.
Also, no needs to consider previously calculated permissions, as at
this point we already bind all new parents and bdrv_get_cumulative_perm
result is enough. So fix the bug by just set permissions by
bdrv_get_cumulative_perm result.
Bug was introduced in long ago 234ac1a9025, in 2.9.
Signed-off-by: Vladimir Sementsov-Ogievskiy
Message-id: 20190824100740.61635-1-vsement...@virtuozzo.com
Signed-off-by: Max Reitz
---
block.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/block.c b/block.c
index 874a29a983..5944124845 100644
--- a/block.c
+++ b/block.c
@@ -4165,7 +4165,6 @@ void bdrv_replace_node(BlockDriverState *from,
BlockDriverState *to,
{
BdrvChild *c, *next;
GSList *list = NULL, *p;
-uint64_t old_perm, old_shared;
uint64_t perm = 0, shared = BLK_PERM_ALL;
int ret;
@@ -4211,8 +4210,8 @@ void bdrv_replace_node(BlockDriverState *from,
BlockDriverState *to,
bdrv_unref(from);
}
-bdrv_get_cumulative_perm(to, _perm, _shared);
-bdrv_set_perm(to, old_perm | perm, old_shared | shared);
+bdrv_get_cumulative_perm(to, , );
+bdrv_set_perm(to, perm, shared);
out:
g_slist_free(list);
--
2.21.0
[Qemu-block] [PATCH V3] block/vhdx: add check for truncated image files
qemu is currently not able to detect truncated vhdx image files.
Add a basic check if all allocated blocks are reachable at open and
report all errors during bdrv_co_check.
Signed-off-by: Peter Lieven
---
V3: - check for bdrv_getlength failure [Kevin]
- use uint32_t for i [Kevin]
- check for BAT entry overflow [Kevin]
- break on !errcnt in second check
V2: - add error reporting [Kevin]
- use bdrv_getlength instead of bdrv_get_allocated_file_size [Kevin]
- factor out BAT entry check and add error reporting for region
overlaps
- already check on vhdx_open
block/vhdx.c | 102 ++-
1 file changed, 85 insertions(+), 17 deletions(-)
diff --git a/block/vhdx.c b/block/vhdx.c
index 6a09d0a55c..253e32d524 100644
--- a/block/vhdx.c
+++ b/block/vhdx.c
@@ -24,6 +24,7 @@
#include "qemu/option.h"
#include "qemu/crc32c.h"
#include "qemu/bswap.h"
+#include "qemu/error-report.h"
#include "vhdx.h"
#include "migration/blocker.h"
#include "qemu/uuid.h"
@@ -235,6 +236,9 @@ static int vhdx_region_check(BDRVVHDXState *s, uint64_t
start, uint64_t length)
end = start + length;
QLIST_FOREACH(r, >regions, entries) {
if (!((start >= r->end) || (end <= r->start))) {
+error_report("VHDX region %" PRIu64 "-%" PRIu64 " overlaps with "
+ "region %" PRIu64 "-%." PRIu64, start, end, r->start,
+ r->end);
ret = -EINVAL;
goto exit;
}
@@ -877,6 +881,77 @@ static void vhdx_calc_bat_entries(BDRVVHDXState *s)
}
+static int vhdx_check_bat_entries(BlockDriverState *bs, int *errcnt)
+{
+BDRVVHDXState *s = bs->opaque;
+int64_t image_file_size = bdrv_getlength(bs->file->bs);
+uint64_t payblocks = s->chunk_ratio;
+uint32_t i;
+int ret = 0;
+
+if (image_file_size < 0) {
+error_report("Could not determinate VHDX image file size.");
+return image_file_size;
+}
+
+for (i = 0; i < s->bat_entries; i++) {
+if ((s->bat[i] & VHDX_BAT_STATE_BIT_MASK) ==
+PAYLOAD_BLOCK_FULLY_PRESENT) {
+uint64_t offset = s->bat[i] & VHDX_BAT_FILE_OFF_MASK;
+/*
+ * Check for BAT entry overflow.
+ */
+if (offset > INT64_MAX - s->block_size) {
+error_report("VHDX BAT entry %" PRIu32 " offset overflow.", i);
+ret = -EINVAL;
+if (!errcnt) {
+break;
+}
+(*errcnt)++;
+}
+/*
+ * Check if fully allocated BAT entries do not reside after
+ * end of the image file.
+ */
+if (offset + s->block_size > image_file_size) {
+error_report("VHDX BAT entry %" PRIu32 " offset points after "
+ "end of file. Image has probably been truncated.",
+ i);
+ret = -EINVAL;
+if (!errcnt) {
+break;
+}
+(*errcnt)++;
+}
+
+/*
+ * verify populated BAT field file offsets against
+ * region table and log entries
+ */
+if (payblocks--) {
+/* payload bat entries */
+int ret2;
+ret2 = vhdx_region_check(s, offset, s->block_size);
+if (ret2 < 0) {
+ret = -EINVAL;
+if (!errcnt) {
+break;
+}
+(*errcnt)++;
+}
+} else {
+payblocks = s->chunk_ratio;
+/*
+ * Once differencing files are supported, verify sector bitmap
+ * blocks here
+ */
+}
+}
+}
+
+return ret;
+}
+
static void vhdx_close(BlockDriverState *bs)
{
BDRVVHDXState *s = bs->opaque;
@@ -981,25 +1056,15 @@ static int vhdx_open(BlockDriverState *bs, QDict
*options, int flags,
goto fail;
}
-uint64_t payblocks = s->chunk_ratio;
-/* endian convert, and verify populated BAT field file offsets against
- * region table and log entries */
+/* endian convert populated BAT field entires */
for (i = 0; i < s->bat_entries; i++) {
s->bat[i] = le64_to_cpu(s->bat[i]);
-if (payblocks--) {
-/* payload bat entries */
-if ((s->bat[i] & VHDX_BAT_STATE_BIT_MASK) ==
-PAYLOAD_BLOCK_FULLY_PRESENT) {
-ret = vhdx_region_check(s, s->bat[i] & VHDX_BAT_FILE_OFF_MASK,
-s->block_size);
-if (ret < 0) {
-goto fail;
-}
-}
-} else {
-payblocks = s->chunk_ratio;
-/* Once differencing files are supported, verify sector bitmap
-
[Qemu-block] [PATCH] iotests: skip 232 when run tests as root
chmod a-w don't help under root, so skip the test in such case. Signed-off-by: Vladimir Sementsov-Ogievskiy --- tests/qemu-iotests/232 | 6 ++ 1 file changed, 6 insertions(+) diff --git a/tests/qemu-iotests/232 b/tests/qemu-iotests/232 index 2063f78876..da35a63d85 100755 --- a/tests/qemu-iotests/232 +++ b/tests/qemu-iotests/232 @@ -70,6 +70,12 @@ size=128M _make_test_img $size +chmod a-w $TEST_IMG +(echo test > $TEST_IMG) 2>/dev/null && \ +_notrun "Readonly attribute is ignored, probably you run this test as" \ +"root, which is unsupported." +chmod a+w $TEST_IMG + if [ -n "$TEST_IMG_FILE" ]; then TEST_IMG=$TEST_IMG_FILE fi -- 2.18.0
Re: [Qemu-block] [PATCH v2] iotests: Check for enabled drivers before testing them
On 03/09/2019 14.55, Max Reitz wrote: > On 23.08.19 15:35, Thomas Huth wrote: >> It is possible to enable only a subset of the block drivers with the >> "--block-drv-rw-whitelist" option of the "configure" script. All other >> drivers are marked as unusable (or only included as read-only with the >> "--block-drv-ro-whitelist" option). If an iotest is now using such a >> disabled block driver, it is failing - which is bad, since at least the >> tests in the "auto" group should be able to deal with this situation. >> Thus let's introduce a "_require_drivers" function that can be used by >> the shell tests to check for the availability of certain drivers first, >> and marks the test as "not run" if one of the drivers is missing. >> >> This patch mainly targets the test in the "auto" group which should >> never fail in such a case, but also improves some of the other tests >> along the way. Note that we also assume that the "qcow2" and "file" >> drivers are always available - otherwise it does not make sense to >> run "make check-block" at all (which only tests with qcow2 by default). >> >> Signed-off-by: Thomas Huth >> --- >> v2: >> - Update the check in _require_drivers() according to Max' suggestion >> - Remove superfluous check in test 081 >> - Mark 120 to require "raw" >> - Replaced the check in 162 to use the new _require_drivers() function >> - Mark 186 to require "null-co" >> >> tests/qemu-iotests/071 | 1 + >> tests/qemu-iotests/081 | 4 +--- >> tests/qemu-iotests/099 | 1 + >> tests/qemu-iotests/120 | 1 + >> tests/qemu-iotests/162 | 4 +--- >> tests/qemu-iotests/184 | 1 + >> tests/qemu-iotests/186 | 1 + >> tests/qemu-iotests/common.rc | 14 ++ >> 8 files changed, 21 insertions(+), 6 deletions(-) > > This patch breaks these iotests when $DISPLAY is not set. It does work > with “iotests: Add -display none to the qemu options”. > > Hm. You reviewed that one, so I suppose I’ll just take it into v2 of my > pull request as well. Sounds like the righ way to go, thanks! > (I’m not going to say having added the iotests to make check gives me as > a maintainer more trouble than I had before, but, you know.) I expected some initial trouble, since the iotests now get exposed to much more different systems. But I hope it will pay off in the long run! (Remember the 4.1 development cycles? The iotests have been completely broken in the master branch by accident two or three times ... I hope at least those days will now be gone or at least not happen that often anymore) Thomas signature.asc Description: OpenPGP digital signature
Re: [Qemu-block] [PATCH V2] block/vhdx: add check for truncated image files
Am 03.09.19 um 15:02 schrieb Kevin Wolf:
Am 02.09.2019 um 17:24 hat Peter Lieven geschrieben:
qemu is currently not able to detect truncated vhdx image files.
Add a basic check if all allocated blocks are reachable at open and
report all errors during bdrv_co_check.
Signed-off-by: Peter Lieven
---
V2: - add error reporting [Kevin]
- use bdrv_getlength instead of bdrv_get_allocated_file_size [Kevin]
- factor out BAT entry check and add error reporting for region
overlaps
- already check on vhdx_open
block/vhdx.c | 85 +---
1 file changed, 68 insertions(+), 17 deletions(-)
diff --git a/block/vhdx.c b/block/vhdx.c
index 6a09d0a55c..6afba5e8c2 100644
--- a/block/vhdx.c
+++ b/block/vhdx.c
@@ -24,6 +24,7 @@
#include "qemu/option.h"
#include "qemu/crc32c.h"
#include "qemu/bswap.h"
+#include "qemu/error-report.h"
#include "vhdx.h"
#include "migration/blocker.h"
#include "qemu/uuid.h"
@@ -235,6 +236,9 @@ static int vhdx_region_check(BDRVVHDXState *s, uint64_t
start, uint64_t length)
end = start + length;
QLIST_FOREACH(r, >regions, entries) {
if (!((start >= r->end) || (end <= r->start))) {
+error_report("VHDX region %" PRIu64 "-%" PRIu64 " overlaps with "
+ "region %" PRIu64 "-%." PRIu64, start, end, r->start,
+ r->end);
ret = -EINVAL;
goto exit;
}
@@ -877,6 +881,60 @@ static void vhdx_calc_bat_entries(BDRVVHDXState *s)
}
+static int vhdx_check_bat_entries(BlockDriverState *bs, int *errcnt)
+{
+BDRVVHDXState *s = bs->opaque;
+int64_t image_file_size = bdrv_getlength(bs->file->bs);
+uint64_t payblocks = s->chunk_ratio;
+int i, ret = 0;
bdrv_getlength() can fail. It's probably better to error out immediately
instead of reporting that every BAT entry is > -1.
+for (i = 0; i < s->bat_entries; i++) {
s->bat_entries is uint32_t, so i should probably be the same.
+if ((s->bat[i] & VHDX_BAT_STATE_BIT_MASK) ==
+PAYLOAD_BLOCK_FULLY_PRESENT) {
+/*
+ * Check if fully allocated BAT entries do not reside after
+ * end of the image file.
+ */
+if ((s->bat[i] & VHDX_BAT_FILE_OFF_MASK) + s->block_size >
+image_file_size) {
Didn't we want to introduce an overflow check before making this check?
Something like if (bat_offset > UINT64_MAX - s->block_size)?
Sorry, i missed that.
The bat entries are UINT64_T so this check will always be false for the
default block size of 1MB. In fact we should check for
bat_offset > INT64_MAX - s->block_size
right?
Peter
Re: [Qemu-block] [PATCH V2] block/vhdx: add check for truncated image files
Am 02.09.2019 um 17:24 hat Peter Lieven geschrieben:
> qemu is currently not able to detect truncated vhdx image files.
> Add a basic check if all allocated blocks are reachable at open and
> report all errors during bdrv_co_check.
>
> Signed-off-by: Peter Lieven
> ---
> V2: - add error reporting [Kevin]
> - use bdrv_getlength instead of bdrv_get_allocated_file_size [Kevin]
> - factor out BAT entry check and add error reporting for region
> overlaps
> - already check on vhdx_open
>
> block/vhdx.c | 85 +---
> 1 file changed, 68 insertions(+), 17 deletions(-)
>
> diff --git a/block/vhdx.c b/block/vhdx.c
> index 6a09d0a55c..6afba5e8c2 100644
> --- a/block/vhdx.c
> +++ b/block/vhdx.c
> @@ -24,6 +24,7 @@
> #include "qemu/option.h"
> #include "qemu/crc32c.h"
> #include "qemu/bswap.h"
> +#include "qemu/error-report.h"
> #include "vhdx.h"
> #include "migration/blocker.h"
> #include "qemu/uuid.h"
> @@ -235,6 +236,9 @@ static int vhdx_region_check(BDRVVHDXState *s, uint64_t
> start, uint64_t length)
> end = start + length;
> QLIST_FOREACH(r, >regions, entries) {
> if (!((start >= r->end) || (end <= r->start))) {
> +error_report("VHDX region %" PRIu64 "-%" PRIu64 " overlaps with "
> + "region %" PRIu64 "-%." PRIu64, start, end,
> r->start,
> + r->end);
> ret = -EINVAL;
> goto exit;
> }
> @@ -877,6 +881,60 @@ static void vhdx_calc_bat_entries(BDRVVHDXState *s)
>
> }
>
> +static int vhdx_check_bat_entries(BlockDriverState *bs, int *errcnt)
> +{
> +BDRVVHDXState *s = bs->opaque;
> +int64_t image_file_size = bdrv_getlength(bs->file->bs);
> +uint64_t payblocks = s->chunk_ratio;
> +int i, ret = 0;
bdrv_getlength() can fail. It's probably better to error out immediately
instead of reporting that every BAT entry is > -1.
> +for (i = 0; i < s->bat_entries; i++) {
s->bat_entries is uint32_t, so i should probably be the same.
> +if ((s->bat[i] & VHDX_BAT_STATE_BIT_MASK) ==
> +PAYLOAD_BLOCK_FULLY_PRESENT) {
> +/*
> + * Check if fully allocated BAT entries do not reside after
> + * end of the image file.
> + */
> +if ((s->bat[i] & VHDX_BAT_FILE_OFF_MASK) + s->block_size >
> +image_file_size) {
Didn't we want to introduce an overflow check before making this check?
Something like if (bat_offset > UINT64_MAX - s->block_size)?
> +error_report("VHDX BAT entry %d offset points after end of "
> + "file. Image has probably been truncated.", i);
> +ret = -EINVAL;
> +if (!errcnt) {
> +break;
> +}
> +(*errcnt)++;
> +}
> +
> +/*
> + * verify populated BAT field file offsets against
> + * region table and log entries
> + */
> +if (payblocks--) {
> +/* payload bat entries */
> +int ret2;
> +ret2 = vhdx_region_check(s, s->bat[i] &
> VHDX_BAT_FILE_OFF_MASK,
> + s->block_size);
> +if (ret2 < 0) {
> +ret = -EINVAL;
> +if (errcnt) {
This one you already noticed yourself.
> +break;
> +}
> +(*errcnt)++;
> +}
> +} else {
> +payblocks = s->chunk_ratio;
> +/*
> + * Once differencing files are supported, verify sector
> bitmap
> + * blocks here
> + */
> +}
> +}
> +}
> +
> +return ret;
> +}
The rest looks good to me.
Kevin
Re: [Qemu-block] [PATCH v2] iotests: Check for enabled drivers before testing them
On 23.08.19 15:35, Thomas Huth wrote: > It is possible to enable only a subset of the block drivers with the > "--block-drv-rw-whitelist" option of the "configure" script. All other > drivers are marked as unusable (or only included as read-only with the > "--block-drv-ro-whitelist" option). If an iotest is now using such a > disabled block driver, it is failing - which is bad, since at least the > tests in the "auto" group should be able to deal with this situation. > Thus let's introduce a "_require_drivers" function that can be used by > the shell tests to check for the availability of certain drivers first, > and marks the test as "not run" if one of the drivers is missing. > > This patch mainly targets the test in the "auto" group which should > never fail in such a case, but also improves some of the other tests > along the way. Note that we also assume that the "qcow2" and "file" > drivers are always available - otherwise it does not make sense to > run "make check-block" at all (which only tests with qcow2 by default). > > Signed-off-by: Thomas Huth > --- > v2: > - Update the check in _require_drivers() according to Max' suggestion > - Remove superfluous check in test 081 > - Mark 120 to require "raw" > - Replaced the check in 162 to use the new _require_drivers() function > - Mark 186 to require "null-co" > > tests/qemu-iotests/071 | 1 + > tests/qemu-iotests/081 | 4 +--- > tests/qemu-iotests/099 | 1 + > tests/qemu-iotests/120 | 1 + > tests/qemu-iotests/162 | 4 +--- > tests/qemu-iotests/184 | 1 + > tests/qemu-iotests/186 | 1 + > tests/qemu-iotests/common.rc | 14 ++ > 8 files changed, 21 insertions(+), 6 deletions(-) This patch breaks these iotests when $DISPLAY is not set. It does work with “iotests: Add -display none to the qemu options”. Hm. You reviewed that one, so I suppose I’ll just take it into v2 of my pull request as well. (I’m not going to say having added the iotests to make check gives me as a maintainer more trouble than I had before, but, you know.) Max signature.asc Description: OpenPGP digital signature
Re: [Qemu-block] [PULL 00/15] Block patches
On 03.09.19 10:39, Peter Maydell wrote: > On Tue, 27 Aug 2019 at 19:23, Max Reitz wrote: >> >> The following changes since commit 23919ddfd56135cad3cb468a8f54d5a595f024f4: >> >> Merge remote-tracking branch 'remotes/aperard/tags/pull-xen-20190827' into >> staging (2019-08-27 15:52:36 +0100) >> >> are available in the Git repository at: >> >> https://github.com/XanClic/qemu.git tags/pull-block-2019-08-27 >> >> for you to fetch changes up to bb043c056cffcc2f3ce88bfdaf2e76e455c09e2c: >> >> iotests: Unify cache mode quoting (2019-08-27 19:48:44 +0200) >> >> >> Block patches: >> - qemu-io now accepts a file to read a write pattern from >> - Ensure that raw files have their first block allocated so we can probe >> the O_DIRECT alignment if necessary >> - Various fixes > > Fails make check running the iotests (on some platforms, > including x86-64 Linux): > > Not run: 220 > Failures: 071 099 120 184 186 > Failed 5 of 105 tests > /home/petmay01/linaro/qemu-for-merges/tests/Makefile.include:1100: > recipe for target 'check-tests/check-block.sh' failed > > The printed diff output for the failures generally looks like: > --- /home/petmay01/linaro/qemu-for-merges/tests/qemu-iotests/071.out > 2018-12-19 15:31:00.523062228 + > +++ > /home/petmay01/linaro/qemu-for-merges/build/all/tests/qemu-iotests/071.out.bad > 2019-09-03 09:01:43.665180692 +0100 > @@ -1,4 +1,5 @@ > QA output created by 071 > +Unable to init server: Could not connect: Connection refused OK, I think I know which patch is to blame. (The problem is probably that you don’t have a $DISPLAY on your test machine. Neither had I until a couple of weeks ago.,,) (Well, I personally blame adding the iotests to make check, but, well.) Max signature.asc Description: OpenPGP digital signature
Re: [Qemu-block] [PATCH v4 0/5] qcow2: async handling of fragmented io
Pinging, as Stefan's branch merged into master and now these series based on
master.
16.08.2019 18:30, Vladimir Sementsov-Ogievskiy wrote:
> Hi all!
>
> Here is an asynchronous scheme for handling fragmented qcow2
> reads and writes. Both qcow2 read and write functions loops through
> sequential portions of data. The series aim it to parallelize these
> loops iterations.
> It improves performance for fragmented qcow2 images, I've tested it
> as described below.
>
> v4 [perf results not updated]:
> 01: new patch. Unrelated, but need to fix 026 before the series to
> correctly fix it after :)
> 02: - use coroutine_fn where appropriate (i.e. in aio_task_pool_new too)
> - add Max's r-b
> 03,04: add Max's r-b
> 05: fix 026 output
>
> v3 (by Max's comments) [perf results not updated]:
>
> 01: - use coroutine_fn where appropriate !!!
> - add aio_task_pool_free
> - add some comments
> - move header to include/block
> - s/wait_done/waiting
> 02: - Rewrite note about decryption in guest buffers [thx to Eric]
> - separate g_assert_not_reached for QCOW2_CLUSTER_ZERO_*
> - drop return after g_assert_not_reached
> 03: - drop bytes_done and correctly use qiov_offset
> - fix comment
> 04: - move QCOW2_MAX_WORKERS to block/qcow2.h
> - initialize ret in qcow2_co_preadv_part
> Based-on: https://github.com/stefanha/qemu/commits/block
>
>
> v2: changed a lot, as
> 1. a lot of preparations around locks, hd_qiovs, threads for encryption
> are done
> 2. I decided to create separate file with async request handling API, to
> reuse it for backup, stream and copy-on-read to improve their performance
> too. Mirror and qemu-img convert has their own async request handling,
> may be we'll be able finally merge all these similar code into one
> feature.
> Note that not all API calls used in qcow2, some will be needed on
> following steps for parallelizing other io loops.
>
> About testing:
>
> I have four 4G qcow2 images (with default 64k block size) on my ssd disk:
> t-seq.qcow2 - sequentially written qcow2 image
> t-reverse.qcow2 - filled by writing 64k portions from end to the start
> t-rand.qcow2 - filled by writing 64k portions (aligned) in random order
> t-part-rand.qcow2 - filled by shuffling order of 64k writes in 1m clusters
> (see source code of image generation in the end for details)
>
> and I've done several runs like the following (sequential io by 1mb chunks):
>
> out=/tmp/block; echo > $out; cat /tmp/files | while read file; do for wr
> in {"","-w"}; do echo "$file" $wr; ./qemu-img bench -c 4096 -d 1 -f qcow2 -n
> -s 1m -t none $wr "$file" | grep 'Run completed in' | awk '{print $4}' >>
> $out; done; done
>
>
> short info about parameters:
>-w - do writes (otherwise do reads)
>-c - count of blocks
>-s - block size
>-t none - disable cache
>-n - native aio
>-d 1 - don't use parallel requests provided by qemu-img bench itself
>
> results:
>
> +---+-+-+
> | file | master | async |
> +---+-+-+
> | /ssd/t-part-rand.qcow2| 14.671 | 9.193 |
> +---+-+-+
> | /ssd/t-part-rand.qcow2 -w | 11.434 | 8.621 |
> +---+-+-+
> | /ssd/t-rand.qcow2 | 20.421 | 10.05 |
> +---+-+-+
> | /ssd/t-rand.qcow2 -w | 11.097 | 8.915 |
> +---+-+-+
> | /ssd/t-reverse.qcow2 | 17.515 | 9.407 |
> +---+-+-+
> | /ssd/t-reverse.qcow2 -w | 11.255 | 8.649 |
> +---+-+-+
> | /ssd/t-seq.qcow2 | 9.081 | 9.072 |
> +---+-+-+
> | /ssd/t-seq.qcow2 -w | 8.761 | 8.747 |
> +---+-+-+
> | /tmp/t-part-rand.qcow2| 41.179 | 41.37 |
> +---+-+-+
> | /tmp/t-part-rand.qcow2 -w | 54.097 | 55.323 |
> +---+-+-+
> | /tmp/t-rand.qcow2 | 711.899 | 514.339 |
> +---+-+-+
> | /tmp/t-rand.qcow2 -w | 546.259 | 642.114 |
> +---+-+-+
> | /tmp/t-reverse.qcow2 | 86.065 | 96.522 |
> +---+-+-+
> | /tmp/t-reverse.qcow2 -w | 46.557 | 48.499 |
> +---+-+-+
> | /tmp/t-seq.qcow2 | 33.804 | 33.862 |
> +---+-+-+
> | /tmp/t-seq.qcow2 -w | 34.299 | 34.233 |
>
[Qemu-block] [PATCH] cutils: Move size_to_str() from "qemu-common.h" to "qemu/cutils.h"
"qemu/cutils.h" contains various qemu_strtosz_*() functions useful to convert strings to size. It seems natural to have the opposite usage (from size to string) there too. The function definition is already in util/cutils.c. Signed-off-by: Philippe Mathieu-Daudé --- There are only 5 users, is it worthwhile renaming it qemu_sztostrt()? Signed-off-by: Philippe Mathieu-Daudé --- block/qapi.c | 2 +- include/qemu-common.h| 1 - include/qemu/cutils.h| 2 ++ qapi/string-output-visitor.c | 2 +- 4 files changed, 4 insertions(+), 3 deletions(-) diff --git a/block/qapi.c b/block/qapi.c index 15f1030264..7ee2ee065d 100644 --- a/block/qapi.c +++ b/block/qapi.c @@ -23,7 +23,7 @@ */ #include "qemu/osdep.h" -#include "qemu-common.h" +#include "qemu/cutils.h" #include "block/qapi.h" #include "block/block_int.h" #include "block/throttle-groups.h" diff --git a/include/qemu-common.h b/include/qemu-common.h index 0235cd3b91..8d84db90b0 100644 --- a/include/qemu-common.h +++ b/include/qemu-common.h @@ -123,7 +123,6 @@ void qemu_hexdump(const char *buf, FILE *fp, const char *prefix, size_t size); int parse_debug_env(const char *name, int max, int initial); const char *qemu_ether_ntoa(const MACAddr *mac); -char *size_to_str(uint64_t val); void page_size_init(void); /* returns non-zero if dump is in progress, otherwise zero is diff --git a/include/qemu/cutils.h b/include/qemu/cutils.h index 12301340a4..b54c847e0f 100644 --- a/include/qemu/cutils.h +++ b/include/qemu/cutils.h @@ -155,6 +155,8 @@ int qemu_strtosz(const char *nptr, const char **end, uint64_t *result); int qemu_strtosz_MiB(const char *nptr, const char **end, uint64_t *result); int qemu_strtosz_metric(const char *nptr, const char **end, uint64_t *result); +char *size_to_str(uint64_t val); + /* used to print char* safely */ #define STR_OR_NULL(str) ((str) ? (str) : "null") diff --git a/qapi/string-output-visitor.c b/qapi/string-output-visitor.c index 7ab64468d9..0d93605d77 100644 --- a/qapi/string-output-visitor.c +++ b/qapi/string-output-visitor.c @@ -11,7 +11,7 @@ */ #include "qemu/osdep.h" -#include "qemu-common.h" +#include "qemu/cutils.h" #include "qapi/string-output-visitor.h" #include "qapi/visitor-impl.h" #include "qemu/host-utils.h" -- 2.20.1
Re: [Qemu-block] [PATCH v7 0/6] Allow Valgrind checking all QEMU processes
On 03/09/2019 13:02, Kevin Wolf wrote: > Am 01.09.2019 um 13:53 hat Andrey Shinkevich geschrieben: >> In the current implementation of the QEMU bash iotests, only qemu-io >> processes may be run under the Valgrind with the switch '-valgrind'. >> Let's allow the common.rc bash script running all other QEMU processes, >> such as qemu-kvm, qemu-img, qemu-ndb and qemu-vxhs, under the Valgrind. > > Thanks, applied to the block branch. > > Kevin > Thanks a lot, Kevin! Andrey -- With the best regards, Andrey Shinkevich
Re: [Qemu-block] [PATCH] block: qcow2: free 'refcount_table' in error path
Am 31.08.2019 um 04:04 hat Li Qiang geschrieben:
> Currently, when doing './check -qcow2 098'. We can get following
> asan output:
>
> qemu-img: Could not empty blkdebug:TEST_DIR/blkdebug.conf:TEST_DIR/t.IMGFMT:
> Input/output error
> +
> +=
> +==60365==ERROR: LeakSanitizer: detected memory leaks
> +
> +Direct leak of 65536 byte(s) in 1 object(s) allocated from:
> +#0 0x7f3ed729fd38 in __interceptor_calloc
> (/usr/lib/x86_64-linux-gnu/libasan.so.4+0xded38)
> +#1 0x56274517fe66 in make_completely_empty block/IMGFMT.c:4219
> +#2 0x562745180e51 in IMGFMT_make_empty block/IMGFMT.c:4313
> +#3 0x56274509b14e in img_commit /home/test/qemu5/qemu/qemu-img.c:1053
> +#4 0x5627450b4b74 in main /home/test/qemu5/qemu/qemu-img.c:5097
> +#5 0x7f3ed4f2fb96 in __libc_start_main
> (/lib/x86_64-linux-gnu/libc.so.6+0x21b96)
>
> This is because the logic of clean resource in 'make_completely_empty' is
> wrong. The patch frees the 's->refcount_table' in error path.
>
> Signed-off-by: Li Qiang
This is wrong. You can never free s->refcount_table and leave it as a
dangling pointer. It is state that is only supposed to be freed in
qcow2_close() -> qcow2_refcount_close().
The only reason why it doesn't crash with your change is that you also
make the error fatal (bs->drv = NULL) so that any further I/O on the
image will fail anyway. But there is no good reason to make these errors
fatal.
Kevin
> block/qcow2.c | 9 +
> 1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 7c5a4859f7..23fe713d4c 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -4243,7 +4243,7 @@ static int make_completely_empty(BlockDriverState *bs)
> ret = bdrv_pwrite_sync(bs->file, s->cluster_size,
> _entry, sizeof(rt_entry));
> if (ret < 0) {
> -goto fail_broken_refcounts;
> +goto fail;
> }
> s->refcount_table[0] = 2 * s->cluster_size;
>
> @@ -4252,7 +4252,7 @@ static int make_completely_empty(BlockDriverState *bs)
> offset = qcow2_alloc_clusters(bs, 3 * s->cluster_size + l1_size2);
> if (offset < 0) {
> ret = offset;
> -goto fail_broken_refcounts;
> +goto fail;
> } else if (offset > 0) {
> error_report("First cluster in emptied image is in use");
> abort();
> @@ -4274,6 +4274,9 @@ static int make_completely_empty(BlockDriverState *bs)
>
> return 0;
>
> +fail:
> +g_free(s->refcount_table);
> +
> fail_broken_refcounts:
> /* The BDS is unusable at this point. If we wanted to make it usable, we
> * would have to call qcow2_refcount_close(), qcow2_refcount_init(),
> @@ -4283,8 +4286,6 @@ fail_broken_refcounts:
> * that that sequence will fail as well. Therefore, just eject the BDS.
> */
> bs->drv = NULL;
>
> -fail:
> -g_free(new_reftable);
> return ret;
> }
>
> --
> 2.17.1
>
>
Re: [Qemu-block] [PULL 00/12] Block patches
On Tue, 27 Aug 2019 at 21:16, Stefan Hajnoczi wrote: > > The following changes since commit dac03af5d5482ec7ee9c23db467bb7230b33c0d9: > > Merge remote-tracking branch 'remotes/rth/tags/pull-axp-20190825' into > staging (2019-08-27 10:00:51 +0100) > > are available in the Git repository at: > > https://github.com/stefanha/qemu.git tags/block-pull-request > > for you to fetch changes up to 5396234b96a2ac743f48644529771498e036e698: > > block/qcow2: implement .bdrv_co_pwritev(_compressed)_part (2019-08-27 > 14:58:42 +0100) > > > Pull request Applied, thanks. Please update the changelog at https://wiki.qemu.org/ChangeLog/4.2 for any user-visible changes. -- PMM
Re: [Qemu-block] [PATCH v7 0/6] Allow Valgrind checking all QEMU processes
Am 01.09.2019 um 13:53 hat Andrey Shinkevich geschrieben: > In the current implementation of the QEMU bash iotests, only qemu-io > processes may be run under the Valgrind with the switch '-valgrind'. > Let's allow the common.rc bash script running all other QEMU processes, > such as qemu-kvm, qemu-img, qemu-ndb and qemu-vxhs, under the Valgrind. Thanks, applied to the block branch. Kevin
Re: [Qemu-block] [PATCH v6 42/42] iotests: Test committing to overridden backing
09.08.2019 19:14, Max Reitz wrote: > Signed-off-by: Max Reitz Reviewed-by: Vladimir Sementsov-Ogievskiy -- Best regards, Vladimir
Re: [Qemu-block] [PATCH V2] block/vhdx: add check for truncated image files
Am 02.09.19 um 17:24 schrieb Peter Lieven:
qemu is currently not able to detect truncated vhdx image files.
Add a basic check if all allocated blocks are reachable at open and
report all errors during bdrv_co_check.
Signed-off-by: Peter Lieven
---
V2: - add error reporting [Kevin]
- use bdrv_getlength instead of bdrv_get_allocated_file_size [Kevin]
- factor out BAT entry check and add error reporting for region
overlaps
- already check on vhdx_open
block/vhdx.c | 85 +---
1 file changed, 68 insertions(+), 17 deletions(-)
diff --git a/block/vhdx.c b/block/vhdx.c
index 6a09d0a55c..6afba5e8c2 100644
--- a/block/vhdx.c
+++ b/block/vhdx.c
@@ -24,6 +24,7 @@
#include "qemu/option.h"
#include "qemu/crc32c.h"
#include "qemu/bswap.h"
+#include "qemu/error-report.h"
#include "vhdx.h"
#include "migration/blocker.h"
#include "qemu/uuid.h"
@@ -235,6 +236,9 @@ static int vhdx_region_check(BDRVVHDXState *s, uint64_t
start, uint64_t length)
end = start + length;
QLIST_FOREACH(r, >regions, entries) {
if (!((start >= r->end) || (end <= r->start))) {
+error_report("VHDX region %" PRIu64 "-%" PRIu64 " overlaps with "
+ "region %" PRIu64 "-%." PRIu64, start, end, r->start,
+ r->end);
ret = -EINVAL;
goto exit;
}
@@ -877,6 +881,60 @@ static void vhdx_calc_bat_entries(BDRVVHDXState *s)
}
+static int vhdx_check_bat_entries(BlockDriverState *bs, int *errcnt)
+{
+BDRVVHDXState *s = bs->opaque;
+int64_t image_file_size = bdrv_getlength(bs->file->bs);
+uint64_t payblocks = s->chunk_ratio;
+int i, ret = 0;
+
+for (i = 0; i < s->bat_entries; i++) {
+if ((s->bat[i] & VHDX_BAT_STATE_BIT_MASK) ==
+PAYLOAD_BLOCK_FULLY_PRESENT) {
+/*
+ * Check if fully allocated BAT entries do not reside after
+ * end of the image file.
+ */
+if ((s->bat[i] & VHDX_BAT_FILE_OFF_MASK) + s->block_size >
+image_file_size) {
+error_report("VHDX BAT entry %d offset points after end of "
+ "file. Image has probably been truncated.", i);
+ret = -EINVAL;
+if (!errcnt) {
+break;
+}
+(*errcnt)++;
+}
+
+/*
+ * verify populated BAT field file offsets against
+ * region table and log entries
+ */
+if (payblocks--) {
+/* payload bat entries */
+int ret2;
+ret2 = vhdx_region_check(s, s->bat[i] & VHDX_BAT_FILE_OFF_MASK,
+ s->block_size);
+if (ret2 < 0) {
+ret = -EINVAL;
+if (errcnt) {
+break;
+}
This should be if (!errcnt) ...
I will respin, but wait for feedback regarding the remainder of the patch.
Peter
Re: [Qemu-block] [PATCH v2 0/5] vpc: Return 0 from vpc_co_create() on success
Am 02.09.2019 um 21:33 hat Max Reitz geschrieben:
> (v2 for “block: Let blockdev-create return 0 on success”)
>
> Jobs are expected to return 0 on success, so this extends to
> .bdrv_co_create(). After some inspection, it turns out that vpc is the
> only block driver that may return a positive value instead (to indicate
> success). Fix that.
>
> Without this patch, blockdev-create is likely to fail for VPC images.
> Hence patch 5.
>
> John indicated his preference for me to use iotests.script_main(). I
> did that; but I still wanted to retain some form of verify_protocol().
> Patch 2 adds @supported_protocols to execute_test() (and thus to
> iotests.script_main() and iotests.main()). Then I noticed we should
> probably make all Python tests (that use either script_main() or main())
> pass something for that parameter, because it’s a bit silly to run all
> Python tests for raw when you just want to run the nbd tests (which are
> five or so). Enter patches 3 and 4.
>
> (There are two Python tests (093 and 136) which I didn’t change to pass
> supported_protocols, because they use null-{co,aio} as their protocol.
> As these are not actually testee protocols for the iotests, I decided to
> just keep running these tests for any protocol.)
Thanks, applied to the block branch.
Re: [Qemu-block] [PULL 00/15] Block patches
On Tue, 27 Aug 2019 at 19:23, Max Reitz wrote: > > The following changes since commit 23919ddfd56135cad3cb468a8f54d5a595f024f4: > > Merge remote-tracking branch 'remotes/aperard/tags/pull-xen-20190827' into > staging (2019-08-27 15:52:36 +0100) > > are available in the Git repository at: > > https://github.com/XanClic/qemu.git tags/pull-block-2019-08-27 > > for you to fetch changes up to bb043c056cffcc2f3ce88bfdaf2e76e455c09e2c: > > iotests: Unify cache mode quoting (2019-08-27 19:48:44 +0200) > > > Block patches: > - qemu-io now accepts a file to read a write pattern from > - Ensure that raw files have their first block allocated so we can probe > the O_DIRECT alignment if necessary > - Various fixes Fails make check running the iotests (on some platforms, including x86-64 Linux): Not run: 220 Failures: 071 099 120 184 186 Failed 5 of 105 tests /home/petmay01/linaro/qemu-for-merges/tests/Makefile.include:1100: recipe for target 'check-tests/check-block.sh' failed The printed diff output for the failures generally looks like: --- /home/petmay01/linaro/qemu-for-merges/tests/qemu-iotests/071.out 2018-12-19 15:31:00.523062228 + +++ /home/petmay01/linaro/qemu-for-merges/build/all/tests/qemu-iotests/071.out.bad 2019-09-03 09:01:43.665180692 +0100 @@ -1,4 +1,5 @@ QA output created by 071 +Unable to init server: Could not connect: Connection refused thanks -- PMM
Re: [Qemu-block] [PATCH v6 25/42] mirror: Deal with filters
02.09.2019 17:35, Max Reitz wrote:
> On 31.08.19 11:57, Vladimir Sementsov-Ogievskiy wrote:
>> 09.08.2019 19:13, Max Reitz wrote:
>>> This includes some permission limiting (for example, we only need to
>>> take the RESIZE permission for active commits where the base is smaller
>>> than the top).
>>>
>>> Signed-off-by: Max Reitz
>>> ---
>>>block/mirror.c | 117 ++---
>>>blockdev.c | 47 +---
>>>2 files changed, 131 insertions(+), 33 deletions(-)
>>>
>>> diff --git a/block/mirror.c b/block/mirror.c
>>> index 54bafdf176..6ddbfb9708 100644
>>> --- a/block/mirror.c
>>> +++ b/block/mirror.c
>>> @@ -42,6 +42,7 @@ typedef struct MirrorBlockJob {
>>>BlockBackend *target;
>>>BlockDriverState *mirror_top_bs;
>>>BlockDriverState *base;
>>> +BlockDriverState *base_overlay;
>>>
>>>/* The name of the graph node to replace */
>>>char *replaces;
>>> @@ -665,8 +666,10 @@ static int mirror_exit_common(Job *job)
>>> _abort);
>>>if (!abort && s->backing_mode == MIRROR_SOURCE_BACKING_CHAIN) {
>>>BlockDriverState *backing = s->is_none_mode ? src : s->base;
>>> -if (backing_bs(target_bs) != backing) {
>>> -bdrv_set_backing_hd(target_bs, backing, _err);
>>> +BlockDriverState *unfiltered_target =
>>> bdrv_skip_rw_filters(target_bs);
>>> +
>>> +if (bdrv_filtered_cow_bs(unfiltered_target) != backing) {
>>> +bdrv_set_backing_hd(unfiltered_target, backing, _err);
>>>if (local_err) {
>>>error_report_err(local_err);
>>>ret = -EPERM;
>>> @@ -715,7 +718,7 @@ static int mirror_exit_common(Job *job)
>>> * valid.
>>> */
>>>block_job_remove_all_bdrv(bjob);
>>> -bdrv_replace_node(mirror_top_bs, backing_bs(mirror_top_bs),
>>> _abort);
>>> +bdrv_replace_node(mirror_top_bs, mirror_top_bs->backing->bs,
>>> _abort);
>>>
>>>/* We just changed the BDS the job BB refers to (with either or both
>>> of the
>>> * bdrv_replace_node() calls), so switch the BB back so the cleanup
>>> does
>>> @@ -812,7 +815,8 @@ static int coroutine_fn
>>> mirror_dirty_init(MirrorBlockJob *s)
>>>return 0;
>>>}
>>>
>>> -ret = bdrv_is_allocated_above(bs, base, false, offset, bytes,
>>> );
>>> +ret = bdrv_is_allocated_above(bs, s->base_overlay, true, offset,
>>> bytes,
>>> + );
>>>if (ret < 0) {
>>>return ret;
>>>}
>>> @@ -908,7 +912,7 @@ static int coroutine_fn mirror_run(Job *job, Error
>>> **errp)
>>>} else {
>>>s->target_cluster_size = BDRV_SECTOR_SIZE;
>>>}
>>> -if (backing_filename[0] && !target_bs->backing &&
>>> +if (backing_filename[0] && !bdrv_backing_chain_next(target_bs) &&
>>>s->granularity < s->target_cluster_size) {
>>>s->buf_size = MAX(s->buf_size, s->target_cluster_size);
>>>s->cow_bitmap = bitmap_new(length);
>>> @@ -1088,8 +1092,9 @@ static void mirror_complete(Job *job, Error **errp)
>>>if (s->backing_mode == MIRROR_OPEN_BACKING_CHAIN) {
>>>int ret;
>>>
>>> -assert(!target->backing);
>>> -ret = bdrv_open_backing_file(target, NULL, "backing", errp);
>>> +assert(!bdrv_backing_chain_next(target));
>>
>> Preexisting, but seems we may crash here, I don't see where it is checked
>> before, to
>> return error if there is some backing. And even if we do so, we don't
>> prevent appearing
>> of target backing during mirror operation.
>
> The idea is that MIRROR_OPEN_BACKING_CHAIN is set only when using
> drive-mirror with mode=existing. In this case, we also set
> BDRV_O_NO_BACKING for the target.
>
> You’re right that a user could add a backing chain to the target during
> the operation. They really have to make an effort to shoot themselves
> in the foot for this because the target must have an auto-generated node
> name.
>
> I suppose the best would be not to open the backing chain if the target
> node already has a backing child?
Hmm, but we still should generate an error, as we can't do what was requested.
>
>>> +ret = bdrv_open_backing_file(bdrv_skip_rw_filters(target), NULL,
>>> + "backing", errp);
>>>if (ret < 0) {
>>>return;
>>>}
>>> @@ -1531,8 +1536,8 @@ static BlockJob *mirror_start_job(
>>>MirrorBlockJob *s;
>>>MirrorBDSOpaque *bs_opaque;
>>>BlockDriverState *mirror_top_bs;
>>> -bool target_graph_mod;
>>>bool target_is_backing;
>>> +uint64_t target_perms, target_shared_perms;
>>>Error *local_err = NULL;
>>>int ret;
>>>
>>> @@ -1551,7 +1556,7 @@ static BlockJob *mirror_start_job(
>>>buf_size = DEFAULT_MIRROR_BUF_SIZE;
Re: [Qemu-block] [PATCH v9 13/13] block/backup: use backup-top instead of write notifiers
02.09.2019 19:34, Max Reitz wrote: > On 29.08.19 16:55, Vladimir Sementsov-Ogievskiy wrote: >> 28.08.2019 22:50, Max Reitz wrote: >>> On 26.08.19 18:13, Vladimir Sementsov-Ogievskiy wrote: Drop write notifiers and use filter node instead. = Changes = 1. add filter-node-name argument for backup qmp api. We have to do it in this commit, as 257 needs to be fixed. >>> >>> I feel a bit bad about it not being an implicit node. But I know you >>> don’t like them, they’re probably just broken altogether and because >>> libvirt doesn’t use backup (yet), probably nobody cares. >>> 2. there no move write notifiers here, so is_write_notifier parameter >>> >>> s/there/there are/, I suppose? >>> is dropped from block-copy paths. 3. Intersecting requests handling changed, now synchronization between backup-top, backup and guest writes are all done in block/block-copy.c and works as follows: On copy operation, we work only with dirty areas. If bits are dirty it means that there are no requests intersecting with this area. We clear dirty bits and take bdrv range lock (bdrv_co_try_lock) on this area to prevent further operations from interaction with guest (only with guest, as neither backup nor backup-top will touch non-dirty area). If copy-operation failed we set dirty bits back together with releasing the lock. The actual difference with old scheme is that on guest writes we don't lock the whole region but only dirty-parts, and to be more precise: only dirty-part we are currently operate on. In old scheme guest write to non-dirty area (which may be safely ignored by backup) may wait for intersecting request, touching some other area which is dirty. 4. To sync with in-flight requests at job finish we now have drained removing of the filter, we don't need rw-lock. = Notes = Note the consequence of three objects appearing: backup-top, backup job and block-copy-state: 1. We want to insert backup-top before job creation, to behave similar with mirror and commit, where job is started upon filter. 2. We also have to create block-copy-state after filter injection, as we don't want it's source child be replaced by fitler. Instead we want >>> >>> s/it's/its/, s/filter/filter/ (although “fitler” does have an amusing >>> ring to it) >>> to keep BCS.source to be real source node, as we want to use bdrv_co_try_lock in CBW operations and it can't be used on filter, as on filter we already have in-flight (write) request from upper layer. >>> >>> Reasonable, even more so as I suppose BCS.source should eventually be a >>> BdrvChild of backup-top. >>> >>> What looks wrong is that the sync_bitmap is created on the source (“bs” >>> in backup_job_create()), but backup_cleanup_sync_bitmap() still assumes >>> it is on blk_bs(job->common.blk) (which is no longer true). >>> So, we firstly create inject backup-top, then create job and BCS. BCS is the latest just to not create extra variable for it. Finally we set bcs for backup-top filter. = Iotest changes = 56: op-blocker doesn't shot now, as we set it on source, but then check >>> >>> s/shot/show/? >>> on filter, when trying to start second backup, so error caught in test_dismiss_collision is changed. It's OK anyway, as this test-case seems to test that after some collision we can dismiss first job and successfully start the second one. So, the change is that collision is changed from op-blocker to file-posix locks. >>> >>> Well, but the op blocker belongs to the source, which should be covered >>> by internal permissions. The fact that it now shows up as a file-posix >>> error thus shows that the conflict also moves from the source to the >>> target. It’s OK because we actually don’t have a conflict on the source. >>> >>> But I wonder whether it would be better for test_dismiss_collision() to >>> do a blockdev-backup instead where we can see the collision on the target. >>> >>> Hm. On second thought, why do we even get a conflict on the target? >>> block-copy does share the WRITE permission for it... >> >> Not sure, but assume that this is because in file-posix.c in raw_co_create >> we do want RESIZE perm. >> >> I can instead move this test to use specified job-id, to move the collision >> to "job-id already exists" error. Is it better? > > It’s probably the only thing that actually makes sense. > >> I'm afraid that posix locks will not work if disable them in config. > > Yes (or if the environment doesn’t support them); which is why I > suggested using blockdev-backup. (But that would have the same problem > of “Where does the permission conflict even come from and is it > inevitable?”) > > [...] > diff --git a/block/block-copy.c b/block/block-copy.c index 6828c46ba0..f3102ec3ff 100644 ---
