[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v10 09/16] block: Add QMP support for streaming
From: |
Alberto Garcia |
Subject: |
Re: [Qemu-devel] [PATCH v10 09/16] block: Add QMP support for streaming to an intermediate layer |
Date: |
Wed, 12 Oct 2016 16:48:45 +0200 |
User-agent: |
Notmuch/0.18.2 (http://notmuchmail.org) Emacs/24.4.1 (i586-pc-linux-gnu) |
On Wed 12 Oct 2016 04:30:27 PM CEST, Kevin Wolf <address@hidden> wrote:
>> if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_STREAM, errp)) {
>> goto out;
>> }
>
> Added a bit more context.
>
> This check is redundant now...
>
>> if (has_base) {
>> base_bs = bdrv_find_backing_image(bs, base);
>> if (base_bs == NULL) {
>> error_setg(errp, QERR_BASE_NOT_FOUND, base);
>> goto out;
>> }
>> assert(bdrv_get_aio_context(base_bs) == aio_context);
>> base_name = base;
>> }
>>
>> + /* Check for op blockers in the whole chain between bs and base */
>> + for (iter = bs; iter && iter != base_bs; iter = backing_bs(iter)) {
>> + if (bdrv_op_is_blocked(iter, BLOCK_OP_TYPE_STREAM, errp)) {
>> + goto out;
>> + }
>> + }
>
> ...because you start with iter = bs here.
You're right, I'll fix it.
> Another thought I had while looking at the previous few patches is
> whether all the op blocker checks wouldn't be better moved to the
> actual block job code (i.e. stream_start in this case).
I thought about that too. In some cases I don't know if it's a good idea
because the qmp_foo_bar() functions do a bit more than simply checking
blockers (e.g. blockdev-mirror creates the target image), so you would
want to have the checks before that.
However doing it in the actual block job code could allow us to do other
things. For example: at the moment when we call block-stream we check
whether a number of BDSs are blocked (in qmp_block_stream()), and if
they're not we proceed to block them (in stream_start()). We could make
block_job_add_bdrv() do both things. On the other hand, since the plan
is to move to a new block job API maybe it's better not to overdo things
now.
It's worth considering for the future anyway.
Berto
- Re: [Qemu-devel] [PATCH v10 09/16] block: Add QMP support for streaming to an intermediate layer, (continued)
- Re: [Qemu-devel] [PATCH v10 09/16] block: Add QMP support for streaming to an intermediate layer, Alberto Garcia, 2016/10/11
- Re: [Qemu-devel] [PATCH v10 09/16] block: Add QMP support for streaming to an intermediate layer, Kevin Wolf, 2016/10/11
- Re: [Qemu-devel] [PATCH v10 09/16] block: Add QMP support for streaming to an intermediate layer, Eric Blake, 2016/10/11
- Re: [Qemu-devel] [PATCH v10 09/16] block: Add QMP support for streaming to an intermediate layer, Markus Armbruster, 2016/10/11
- Re: [Qemu-devel] [PATCH v10 09/16] block: Add QMP support for streaming to an intermediate layer, Kevin Wolf, 2016/10/12
- Re: [Qemu-devel] [PATCH v10 09/16] block: Add QMP support for streaming to an intermediate layer, Alberto Garcia, 2016/10/12
- Re: [Qemu-devel] [PATCH v10 09/16] block: Add QMP support for streaming to an intermediate layer, Markus Armbruster, 2016/10/11
- Re: [Qemu-devel] [PATCH v10 09/16] block: Add QMP support for streaming to an intermediate layer, Alberto Garcia, 2016/10/12
- Re: [Qemu-devel] [PATCH v10 09/16] block: Add QMP support for streaming to an intermediate layer, Markus Armbruster, 2016/10/12
Re: [Qemu-devel] [PATCH v10 09/16] block: Add QMP support for streaming to an intermediate layer, Kevin Wolf, 2016/10/12
- Re: [Qemu-devel] [PATCH v10 09/16] block: Add QMP support for streaming to an intermediate layer,
Alberto Garcia <=
[Qemu-devel] [PATCH v10 06/16] block: Block all nodes involved in the block-commit operation, Alberto Garcia, 2016/10/06
[Qemu-devel] [PATCH v10 07/16] block: Block all intermediate nodes in commit_active_start(), Alberto Garcia, 2016/10/06
[Qemu-devel] [PATCH v10 02/16] block: Add block_job_add_bdrv(), Alberto Garcia, 2016/10/06
[Qemu-devel] [PATCH v10 08/16] block: Support streaming to an intermediate layer, Alberto Garcia, 2016/10/06