Re: [Qemu-block] [Qemu-devel] [PATCH v7 00/11] Support streaming to an intermediate layer
On Wed, May 13, 2015 at 04:27:30PM +0300, Alberto Garcia wrote: > v7: > - Rebased against the current master > - Updated bdrv_drain_all() to use the new block_job_next() API. Please rebase onto https://github.com/stefanha/qemu 'block' branch and check that qemu-iotests 015 030 032 038 046 pass: make cd tests/qemu-iotests ./check -qcow2 015 030 032 038 046 pgpznnLjiecaU.pgp Description: PGP signature
Re: [Qemu-block] [Qemu-devel] [PATCH v7 00/11] Support streaming to an intermediate layer
On Thu 18 Jun 2015 02:36:13 PM CEST, Eric Blake wrote: [Detecting support for intermediate block streaming] >> One possibility is to try to stream to an intermediate node and see >> if it fails. >> >> Example: in a chain like [A] <- [B] <- [C], streaming to [B] using >> [A] as the 'base' parameter is a no-op (there's even a test for that >> in iotest 030). >> >> If QEMU does support streaming to [B], the operation will succeed but >> do nothing. Otherwise the operation will fail with a DeviceNotFound >> error. > In general, if a feature addition doesn't change API, but merely > converts what was previously an error into something that works, then > libvirt is probably okay with just trying the feature, and reporting > the error message if it fails (assuming the qemu error message is > sane). With the example I gave above you should be able to detect easily if the feature is present, but from what you say I understand that strictly speaking you wouldn't even need to do that. If you try to stream to a node and QEMU does not support that you will get a 'device not found' error, which is I guess the kind of error that you would expect. Since the only thing that changes with this feature is the 'device' parameter I don't think there's any room for ambiguity. Berto
Re: [Qemu-block] [Qemu-devel] [PATCH v7 00/11] Support streaming to an intermediate layer
[adding libvirt, to make sure I capture a design idea] On 06/18/2015 06:36 AM, Eric Blake wrote: > On 06/18/2015 06:07 AM, Alberto Garcia wrote: >> On Thu 18 Jun 2015 01:47:20 PM CEST, Kevin Wolf wrote: >> > I believe our conclusion from an earlier version of the series was > that we need QAPI introspection so that libvirt can detect the > presence of the feature. > > Detecting the presence of a feature allows libvirt the luxury of giving > its own error message, rather than relying on the qemu message. But > that's not to say libvirt HAS to use its own error message, and > therefore being unable to detect the feature may not be the end of the > world. > >> That said, I would prefer a way to detect the feature that does not >> involve testing commands for their error codes, but is there any? What >> does libvirt generally do in order to detect new features that don't >> depend on API changes? > > But libvirt has not yet set up node name management (I'm about to revive > Jeff's patch for auto-node-naming simultaneously with a libvirt patch > series that proves that it helps libvirt), and libvirt will need a new > API to allow users a way to request streams to an intermediate image. > So anything libvirt does to interact with the new stream-to-intermediate > will have to be new code, and I can worry about whether the qemu error > message is good enough, or whether I have to contrive some probing test > to see if it even works; but my initial thought is that merely probing > to see if auto-node-naming is in place is a good approximation filter > (if libvirt isn't managing its own node names, then the only way to use > stream-to-intermediate is via a node name automatically supplied by > qemu, especially nice if both features land in 2.4). Actually, in thinking more about it, libvirt won't need a new API; the existing virDomainBlockPull() and virDomainBlockRebase() are sufficient, if I allow libvirt to treat "vda[1]" as a destination (which is the first backing image of disk vda; pretty much similar to how qemu is adding node names rather than device names as a way to make the existing block-stream now stream to intermediate). And that is consistent with the way we have been retrofitting other existing libvirt API to refer to specific backing images. On libvirt's front, I may want to add a new flag (where the flag must be present to make it clear that stream-to-intermediate is desired, so that upper level applications can use the absence of the libvirt flag as their feature probe), but that has no bearing on what qemu has to do to turn on the feature. -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-block] [Qemu-devel] [PATCH v7 00/11] Support streaming to an intermediate layer
On 06/18/2015 06:07 AM, Alberto Garcia wrote: > On Thu 18 Jun 2015 01:47:20 PM CEST, Kevin Wolf wrote: > I believe our conclusion from an earlier version of the series was that we need QAPI introspection so that libvirt can detect the presence of the feature. Detecting the presence of a feature allows libvirt the luxury of giving its own error message, rather than relying on the qemu message. But that's not to say libvirt HAS to use its own error message, and therefore being unable to detect the feature may not be the end of the world. >>> >>> The initial version of this series had an extra 'top' parameter to >>> decide what image to stream data into. [...] That was later removed >>> when we agreed that we could just reuse the 'device' parameter to >>> refer to either a device or a node name. >>> >>> I don't think that introspection support would make a difference in >>> this case, or am I missing anything? >> >> Hm, yes, probably not. But how would libvirt detect the feature then? > > One possibility is to try to stream to an intermediate node and see if > it fails. > > Example: in a chain like [A] <- [B] <- [C], streaming to [B] using [A] > as the 'base' parameter is a no-op (there's even a test for that in > iotest 030). > > If QEMU does support streaming to [B], the operation will succeed but do > nothing. Otherwise the operation will fail with a DeviceNotFound error. > > That said, I would prefer a way to detect the feature that does not > involve testing commands for their error codes, but is there any? What > does libvirt generally do in order to detect new features that don't > depend on API changes? But libvirt has not yet set up node name management (I'm about to revive Jeff's patch for auto-node-naming simultaneously with a libvirt patch series that proves that it helps libvirt), and libvirt will need a new API to allow users a way to request streams to an intermediate image. So anything libvirt does to interact with the new stream-to-intermediate will have to be new code, and I can worry about whether the qemu error message is good enough, or whether I have to contrive some probing test to see if it even works; but my initial thought is that merely probing to see if auto-node-naming is in place is a good approximation filter (if libvirt isn't managing its own node names, then the only way to use stream-to-intermediate is via a node name automatically supplied by qemu, especially nice if both features land in 2.4). In general, if a feature addition doesn't change API, but merely converts what was previously an error into something that works, then libvirt is probably okay with just trying the feature, and reporting the error message if it fails (assuming the qemu error message is sane). -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-block] [Qemu-devel] [PATCH v7 00/11] Support streaming to an intermediate layer
On Thu 18 Jun 2015 01:47:20 PM CEST, Kevin Wolf wrote: >> > I believe our conclusion from an earlier version of the series was >> > that we need QAPI introspection so that libvirt can detect the >> > presence of the feature. >> >> The initial version of this series had an extra 'top' parameter to >> decide what image to stream data into. [...] That was later removed >> when we agreed that we could just reuse the 'device' parameter to >> refer to either a device or a node name. >> >> I don't think that introspection support would make a difference in >> this case, or am I missing anything? > > Hm, yes, probably not. But how would libvirt detect the feature then? One possibility is to try to stream to an intermediate node and see if it fails. Example: in a chain like [A] <- [B] <- [C], streaming to [B] using [A] as the 'base' parameter is a no-op (there's even a test for that in iotest 030). If QEMU does support streaming to [B], the operation will succeed but do nothing. Otherwise the operation will fail with a DeviceNotFound error. That said, I would prefer a way to detect the feature that does not involve testing commands for their error codes, but is there any? What does libvirt generally do in order to detect new features that don't depend on API changes? Berto
Re: [Qemu-block] [Qemu-devel] [PATCH v7 00/11] Support streaming to an intermediate layer
Am 18.06.2015 um 13:41 hat Alberto Garcia geschrieben: > On Thu 18 Jun 2015 12:45:35 PM CEST, Kevin Wolf wrote: > > > I believe our conclusion from an earlier version of the series was > > that we need QAPI introspection so that libvirt can detect the > > presence of the feature. > > The initial version of this series had an extra 'top' parameter to > decide what image to stream data into. That's what prompted the > discussion: > > https://lists.gnu.org/archive/html/qemu-devel/2015-02/msg04182.html > > That was later removed when we agreed that we could just reuse the > 'device' parameter to refer to either a device or a node name. > > I don't think that introspection support would make a difference in this > case, or am I missing anything? Hm, yes, probably not. But how would libvirt detect the feature then? Kevin
Re: [Qemu-block] [Qemu-devel] [PATCH v7 00/11] Support streaming to an intermediate layer
On Thu 18 Jun 2015 12:45:35 PM CEST, Kevin Wolf wrote: > I believe our conclusion from an earlier version of the series was > that we need QAPI introspection so that libvirt can detect the > presence of the feature. The initial version of this series had an extra 'top' parameter to decide what image to stream data into. That's what prompted the discussion: https://lists.gnu.org/archive/html/qemu-devel/2015-02/msg04182.html That was later removed when we agreed that we could just reuse the 'device' parameter to refer to either a device or a node name. I don't think that introspection support would make a difference in this case, or am I missing anything? Berto
Re: [Qemu-block] [Qemu-devel] [PATCH v7 00/11] Support streaming to an intermediate layer
Am 16.06.2015 um 10:51 hat Alberto Garcia geschrieben: > Ping... > > https://lists.gnu.org/archive/html/qemu-devel/2015-05/msg02580.html I believe our conclusion from an earlier version of the series was that we need QAPI introspection so that libvirt can detect the presence of the feature. Markus is planning to have the introspection code mergable when the 2.5 development phase starts. That's what I would wait for before I merge your series. Markus also offered to write a preliminary version of the introspection code for 2.4. However, I will be offline from tomorrow until the day before the hard freeze, so I won't be able to manage this if Markus and you still wanted to try it for 2.4. If you guys can come up with patches in time and convince Stefan and/or Max to send a pull request for it, that's fine with me. Kevin > On Wed, May 13, 2015 at 04:27:30PM +0300, Alberto Garcia wrote: > > v7: > > - Rebased against the current master > > - Updated bdrv_drain_all() to use the new block_job_next() API. > > > > v6: https://lists.gnu.org/archive/html/qemu-devel/2015-04/msg03046.html > > - fix the no-op test following Max's suggestions > > > > v5: https://lists.gnu.org/archive/html/qemu-devel/2015-04/msg03006.html > > - Fix a few typos > > - Minor documentation updates > > - Update test_stream_partial() to test no-ops > > - New test case: test_stream_parallel() > > - New test case: test_stream_overlapping() > > > > v4: https://lists.gnu.org/archive/html/qemu-devel/2015-04/msg01878.html > > - Refactor find_block_job to use the error from bdrv_lookup_bs() > > - Don't use QERR_DEVICE_IN_USE in block_job_create() since we can be > > dealing with nodes now. > > - Fix @device comment in the BlockJobInfo documentation > > - stream_start(): simplify the bdrv_reopen() call and use > > bdrv_get_device_or_node_name() for error messages. > > - Use a different variable name for BlockDriverState *i > > - Documentation fixes in docs/live-block-ops.txt > > - Update iotest 30 since now test_device_not_found() returns > > GenericError > > - Fix test case test_stream_partial() > > - Add new test case test_stream_intermediate() > > - Fix typos > > > > v3: https://lists.gnu.org/archive/html/qemu-devel/2015-04/msg00806.html > > - Keep a list of block jobs and make qmp_query_block_jobs() iterate > > over it. > > > > v2: https://lists.gnu.org/archive/html/qemu-devel/2015-03/msg04798.html > > - The 'block-stream' command does not have a 'node-name' parameter > > anymore and reuses 'device' for that purpose. > > - Block jobs can now be owned by any intermediate node, and not just > > by the ones at the root. query-block-jobs is updated to reflect that > > change. > > - The 'device' parameter of all 'block-job-*' commands can now take a > > node name. > > - The BlockJobInfo type and all BLOCK_JOB_* events report the node > > name in the 'device' field if the node does not have a device name. > > - All intermediate nodes are blocked (and checked for blockers) during > > the streaming operation. > > > > v1: https://lists.gnu.org/archive/html/qemu-devel/2015-02/msg04116.html > > > > Regards, > > > > Berto > > > > Alberto Garcia (11): > > block: keep a list of block jobs > > block: allow block jobs in any arbitrary node > > block: never cancel a streaming job without running stream_complete() > > block: Support streaming to an intermediate layer > > block: Add QMP support for streaming to an intermediate layer > > docs: Document how to stream to an intermediate layer > > qemu-iotests: fix test_stream_partial() > > qemu-iotests: add no-op streaming test > > qemu-iotests: test streaming to an intermediate layer > > qemu-iotests: test block-stream operations in parallel > > qemu-iotests: test overlapping block-stream operations > > > > block.c| 4 +- > > block/io.c | 21 +++ > > block/mirror.c | 5 +- > > block/stream.c | 44 -- > > blockdev.c | 55 - > > blockjob.c | 31 +++--- > > docs/live-block-ops.txt| 31 ++ > > docs/qmp/qmp-events.txt| 8 +-- > > include/block/blockjob.h | 14 + > > include/qapi/qmp/qerror.h | 3 - > > qapi/block-core.json | 30 + > > tests/qemu-iotests/030 | 148 > > - > > tests/qemu-iotests/030.out | 4 +- > > 13 files changed, 304 insertions(+), 94 deletions(-) > > > > -- > > 2.1.4 > > > > >
Re: [Qemu-block] [Qemu-devel] [PATCH v7 00/11] Support streaming to an intermediate layer
Ping... https://lists.gnu.org/archive/html/qemu-devel/2015-05/msg02580.html On Wed, May 13, 2015 at 04:27:30PM +0300, Alberto Garcia wrote: > v7: > - Rebased against the current master > - Updated bdrv_drain_all() to use the new block_job_next() API. > > v6: https://lists.gnu.org/archive/html/qemu-devel/2015-04/msg03046.html > - fix the no-op test following Max's suggestions > > v5: https://lists.gnu.org/archive/html/qemu-devel/2015-04/msg03006.html > - Fix a few typos > - Minor documentation updates > - Update test_stream_partial() to test no-ops > - New test case: test_stream_parallel() > - New test case: test_stream_overlapping() > > v4: https://lists.gnu.org/archive/html/qemu-devel/2015-04/msg01878.html > - Refactor find_block_job to use the error from bdrv_lookup_bs() > - Don't use QERR_DEVICE_IN_USE in block_job_create() since we can be > dealing with nodes now. > - Fix @device comment in the BlockJobInfo documentation > - stream_start(): simplify the bdrv_reopen() call and use > bdrv_get_device_or_node_name() for error messages. > - Use a different variable name for BlockDriverState *i > - Documentation fixes in docs/live-block-ops.txt > - Update iotest 30 since now test_device_not_found() returns > GenericError > - Fix test case test_stream_partial() > - Add new test case test_stream_intermediate() > - Fix typos > > v3: https://lists.gnu.org/archive/html/qemu-devel/2015-04/msg00806.html > - Keep a list of block jobs and make qmp_query_block_jobs() iterate > over it. > > v2: https://lists.gnu.org/archive/html/qemu-devel/2015-03/msg04798.html > - The 'block-stream' command does not have a 'node-name' parameter > anymore and reuses 'device' for that purpose. > - Block jobs can now be owned by any intermediate node, and not just > by the ones at the root. query-block-jobs is updated to reflect that > change. > - The 'device' parameter of all 'block-job-*' commands can now take a > node name. > - The BlockJobInfo type and all BLOCK_JOB_* events report the node > name in the 'device' field if the node does not have a device name. > - All intermediate nodes are blocked (and checked for blockers) during > the streaming operation. > > v1: https://lists.gnu.org/archive/html/qemu-devel/2015-02/msg04116.html > > Regards, > > Berto > > Alberto Garcia (11): > block: keep a list of block jobs > block: allow block jobs in any arbitrary node > block: never cancel a streaming job without running stream_complete() > block: Support streaming to an intermediate layer > block: Add QMP support for streaming to an intermediate layer > docs: Document how to stream to an intermediate layer > qemu-iotests: fix test_stream_partial() > qemu-iotests: add no-op streaming test > qemu-iotests: test streaming to an intermediate layer > qemu-iotests: test block-stream operations in parallel > qemu-iotests: test overlapping block-stream operations > > block.c| 4 +- > block/io.c | 21 +++ > block/mirror.c | 5 +- > block/stream.c | 44 -- > blockdev.c | 55 - > blockjob.c | 31 +++--- > docs/live-block-ops.txt| 31 ++ > docs/qmp/qmp-events.txt| 8 +-- > include/block/blockjob.h | 14 + > include/qapi/qmp/qerror.h | 3 - > qapi/block-core.json | 30 + > tests/qemu-iotests/030 | 148 > - > tests/qemu-iotests/030.out | 4 +- > 13 files changed, 304 insertions(+), 94 deletions(-) > > -- > 2.1.4 > >
