Provide API for the COR-filter insertion/removal. Also, drop the filter child permissions for an inactive state when the filter node is being removed. Signed-off-by: Andrey Shinkevich <andrey.shinkevich(a)virtuozzo.com&gt; Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov(a)virtuozzo.com&gt; --- block/copy-on-read.c | 88 ++++++++++++++++++++++++++++++++++++++++++++++++++++ block/copy-on-read.h | 35 +++++++++++++++++++++ 2 files changed, 123 insertions(+) create mode 100644 block/copy-on-read.h diff --git a/block/copy-on-read.c b/block/copy-on-read.c index cb03e0f..bcccf0f 100644 --- a/block/copy-on-read.c +++ b/block/copy-on-read.c @@ -23,11 +23,21 @@ #include "qemu/osdep.h" #include "block/block_int.h" #include "qemu/module.h" +#include "qapi/error.h" +#include "qapi/qmp/qdict.h" +#include "block/copy-on-read.h" + + +typedef struct BDRVStateCOR { + bool active; +} BDRVStateCOR; static int cor_open(BlockDriverState *bs, QDict *options, int flags, Error **errp) { + BDRVStateCOR *state = bs->opaque; + bs->file = bdrv_open_child(NULL, options, "file", bs, &child_of_bds, BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY, false, errp); @@ -42,6 +52,13 @@ static int cor_open(BlockDriverState *bs, QDict *options, int flags, ((BDRV_REQ_FUA | BDRV_REQ_MAY_UNMAP | BDRV_REQ_NO_FALLBACK) & bs->file->bs->supported_zero_flags); + state->active = true; + + /* + * We don't need to call bdrv_child_refresh_perms() now as the permissions + * will be updated later when the filter node gets its parent. + */ + return 0; } @@ -57,6 +74,17 @@ static void cor_child_perm(BlockDriverState *bs, BdrvChild *c, uint64_t perm, uint64_t shared, uint64_t *nperm, uint64_t *nshared) { + BDRVStateCOR *s = bs->opaque; + + if (!s->active) { + /* + * While the filter is being removed + */ + *nperm = 0; + *nshared = BLK_PERM_ALL; + return; + } + *nperm = perm & PERM_PASSTHROUGH; *nshared = (shared & PERM_PASSTHROUGH) | PERM_UNCHANGED; @@ -135,6 +163,7 @@ static void cor_lock_medium(BlockDriverState *bs, bool locked) static BlockDriver bdrv_copy_on_read = { .format_name = "copy-on-read", + .instance_size = sizeof(BDRVStateCOR), .bdrv_open = cor_open, .bdrv_child_perm = cor_child_perm, @@ -159,4 +188,63 @@ static void bdrv_copy_on_read_init(void) bdrv_register(&bdrv_copy_on_read); } + +BlockDriverState *bdrv_cor_filter_append(BlockDriverState *bs, + QDict *node_options, + int flags, Error **errp) +{ + BlockDriverState *cor_filter_bs; + Error *local_err = NULL; + + cor_filter_bs = bdrv_open(NULL, NULL, node_options, flags, errp); + if (cor_filter_bs == NULL) { + error_prepend(errp, "Could not create COR-filter node: "); + return NULL; + } + + if (!qdict_get_try_str(node_options, "node-name")) { + cor_filter_bs->implicit = true; + } + + bdrv_drained_begin(bs); + bdrv_replace_node(bs, cor_filter_bs, &local_err); + bdrv_drained_end(bs); + + if (local_err) { + bdrv_unref(cor_filter_bs); + error_propagate(errp, local_err); + return NULL; + } + + return cor_filter_bs; +} + + +void bdrv_cor_filter_drop(BlockDriverState *cor_filter_bs) +{ + BdrvChild *child; + BlockDriverState *bs; + BDRVStateCOR *s = cor_filter_bs->opaque; + + child = bdrv_filter_child(cor_filter_bs); + if (!child) { + return; + } + bs = child->bs; + + /* Retain the BDS until we complete the graph change. */ + bdrv_ref(bs); + /* Hold a guest back from writing while permissions are being reset. */ + bdrv_drained_begin(bs); + /* Drop permissions before the graph change. */ + s->active = false; + bdrv_child_refresh_perms(cor_filter_bs, child, &error_abort); + bdrv_replace_node(cor_filter_bs, bs, &error_abort); + + bdrv_drained_end(bs); + bdrv_unref(bs); + bdrv_unref(cor_filter_bs); +} + + block_init(bdrv_copy_on_read_init); diff --git a/block/copy-on-read.h b/block/copy-on-read.h new file mode 100644 index 0000000..d6f2422 --- /dev/null +++ b/block/copy-on-read.h @@ -0,0 +1,35 @@ +/* + * Copy-on-read filter block driver + * + * The filter driver performs Copy-On-Read (COR) operations + * + * Copyright (c) 2018-2020 Virtuozzo International GmbH. + * + * Author: + * Andrey Shinkevich <andrey.shinkevich(a)virtuozzo.com&gt; + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see <http://www.gnu.org/licenses/>;. + */ + +#ifndef BLOCK_COPY_ON_READ +#define BLOCK_COPY_ON_READ + +#include "block/block_int.h" + +BlockDriverState *bdrv_cor_filter_append(BlockDriverState *bs, + QDict *node_options, + int flags, Error **errp); +void bdrv_cor_filter_drop(BlockDriverState *cor_filter_bs); + +#endif /* BLOCK_COPY_ON_READ */ -- 1.8.3.1

On 14.10.2020 13:44, Max Reitz wrote: Actually, I did (and still can do) that for the 'append node' function only but not for the 'drop node' one so far... diff --git a/block.c b/block.c index 11ab55f..f41e876 100644 --- a/block.c +++ b/block.c @@ -4669,6 +4669,55 @@ static void bdrv_delete(BlockDriverState *bs) g_free(bs); } +BlockDriverState *bdrv_insert_node(BlockDriverState *bs, QDict *node_options, + int flags, Error **errp) +{ + BlockDriverState *new_node_bs; + Error *local_err = NULL; + + new_node_bs = bdrv_open(NULL, NULL, node_options, flags, errp); + if (new_node_bs == NULL) { + error_prepend(errp, "Could not create node: "); + return NULL; + } + + bdrv_drained_begin(bs); + bdrv_replace_node(bs, new_node_bs, &local_err); + bdrv_drained_end(bs); + + if (local_err) { + bdrv_unref(new_node_bs); + error_propagate(errp, local_err); + return NULL; + } + + return new_node_bs; +} + +void bdrv_remove_node(BlockDriverState *bs) +{ + BdrvChild *child; + BlockDriverState *inferior_bs; + + child = bdrv_filter_or_cow_child(bs); + if (!child) { + return; + } + inferior_bs = child->bs; + + /* Retain the BDS until we complete the graph change. */ + bdrv_ref(inferior_bs); + /* Hold a guest back from writing while permissions are being reset. */ + bdrv_drained_begin(inferior_bs); + /* Refresh permissions before the graph change. */ + bdrv_child_refresh_perms(bs, child, &error_abort); + bdrv_replace_node(bs, inferior_bs, &error_abort); + + bdrv_drained_end(inferior_bs); + bdrv_unref(inferior_bs); + bdrv_unref(bs); +} So, it is an intermediate solution in this patch of the series. I am going to make both functions generic once Vladimir overhauls the QEMU permission update system. Otherwise, the COR-filter node cannot be removed from the backing chain gracefully. Thank you for your r-b. If the next version comes, I can move the 'append node' function only to the generic layer. Andrey

Provide the possibility to pass the 'filter-node-name' parameter to the block-stream job as it is done for the commit block job. Signed-off-by: Andrey Shinkevich <andrey.shinkevich(a)virtuozzo.com&gt; Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov(a)virtuozzo.com&gt; --- block/monitor/block-hmp-cmds.c | 4 ++-- block/stream.c | 4 +++- blockdev.c | 4 +++- include/block/block_int.h | 7 ++++++- qapi/block-core.json | 6 ++++++ 5 files changed, 20 insertions(+), 5 deletions(-) diff --git a/block/monitor/block-hmp-cmds.c b/block/monitor/block-hmp-cmds.c index 4d3db5e..4e66775 100644 --- a/block/monitor/block-hmp-cmds.c +++ b/block/monitor/block-hmp-cmds.c @@ -507,8 +507,8 @@ void hmp_block_stream(Monitor *mon, const QDict *qdict) qmp_block_stream(true, device, device, base != NULL, base, false, NULL, false, NULL, qdict_haskey(qdict, "speed"), speed, true, - BLOCKDEV_ON_ERROR_REPORT, false, false, false, false, - &error); + BLOCKDEV_ON_ERROR_REPORT, false, NULL, false, false, false, + false, &error); hmp_handle_error(mon, error); } diff --git a/block/stream.c b/block/stream.c index 8ce6729..e0540ee 100644 --- a/block/stream.c +++ b/block/stream.c @@ -221,7 +221,9 @@ static const BlockJobDriver stream_job_driver = { void stream_start(const char *job_id, BlockDriverState *bs, BlockDriverState *base, const char *backing_file_str, int creation_flags, int64_t speed, - BlockdevOnError on_error, Error **errp) + BlockdevOnError on_error, + const char *filter_node_name, + Error **errp) { StreamBlockJob *s; BlockDriverState *iter; diff --git a/blockdev.c b/blockdev.c index bebd3ba..d719c47 100644 --- a/blockdev.c +++ b/blockdev.c @@ -2489,6 +2489,7 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device, bool has_backing_file, const char *backing_file, bool has_speed, int64_t speed, bool has_on_error, BlockdevOnError on_error, + bool has_filter_node_name, const char *filter_node_name, bool has_auto_finalize, bool auto_finalize, bool has_auto_dismiss, bool auto_dismiss, Error **errp) @@ -2571,7 +2572,8 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device, } stream_start(has_job_id ? job_id : NULL, bs, base_bs, base_name, - job_flags, has_speed ? speed : 0, on_error, &local_err); + job_flags, has_speed ? speed : 0, on_error, + filter_node_name, &local_err); if (local_err) { error_propagate(errp, local_err); goto out; diff --git a/include/block/block_int.h b/include/block/block_int.h index 38cad9d..f782737 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -1134,6 +1134,9 @@ int is_windows_drive(const char *filename); * See @BlockJobCreateFlags * @speed: The maximum speed, in bytes per second, or 0 for unlimited. * @on_error: The action to take upon error. + * @filter_node_name: The node name that should be assigned to the filter + * driver that the commit job inserts into the graph above @bs. NULL means + * that a node name should be autogenerated. * @errp: Error object. * * Start a streaming operation on @bs. Clusters that are unallocated @@ -1146,7 +1149,9 @@ int is_windows_drive(const char *filename); void stream_start(const char *job_id, BlockDriverState *bs, BlockDriverState *base, const char *backing_file_str, int creation_flags, int64_t speed, - BlockdevOnError on_error, Error **errp); + BlockdevOnError on_error, + const char *filter_node_name, + Error **errp); /** * commit_start: diff --git a/qapi/block-core.json b/qapi/block-core.json index 3c16f1e..32fb097 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -2533,6 +2533,11 @@ # 'stop' and 'enospc' can only be used if the block device # supports io-status (see BlockInfo). Since 1.3. # +# @filter-node-name: the node name that should be assigned to the +# filter driver that the stream job inserts into the graph +# above @device. If this option is not given, a node name is +# autogenerated. (Since: 5.2) +# # @auto-finalize: When false, this job will wait in a PENDING state after it has # finished its work, waiting for @block-job-finalize before # making any block graph changes. @@ -2563,6 +2568,7 @@ 'data': { '*job-id': 'str', 'device': 'str', '*base': 'str', '*base-node': 'str', '*backing-file': 'str', '*speed': 'int', '*on-error': 'BlockdevOnError', + '*filter-node-name': 'str', '*auto-finalize': 'bool', '*auto-dismiss': 'bool' } } ## -- 1.8.3.1

On 12.10.20 19:43, Andrey Shinkevich wrote: Is there a reason why you didn’t add this option to QAPI (as part of a yet-to-be-created BlockdevOptionsCor)? Because I’d really like it there. Shouldn’t it be called base-overlay or above-base then? I think this is a use-after-free. The storage @base_overlay_node points to belongs to a QString, which is referenced only by @options; so deleting that element of @options should free that string. Max

14.10.2020 14:57, Max Reitz wrote: What about naming it just "bottom" or "bottom-node"? If we don't have base, it's strange to have something "relative to base". And we can document, that "bottom" must be a non-filter node in a backing chain of "top". -- Best regards, Vladimir

On 14.10.2020 14:09, Max Reitz wrote: I agree that passing a base overlay under the base option looks clumsy. We could pass the base node name and find its overlay ourselves here in cor_open(). In that case, we can use the existing QAPI. The reason I used the existing QAPI is to make it easier for a user to operate with the traditional options and to keep things simple. So, the user shouldn't think what overlay or above-base node to pass. If we introduce the specific BlockdevOptionsCor, what other options may come with? The base_overlay identifier is used below as the pointer to BS. The base_overlay_node stands for the name of the node. I used that identifier to differ between the types. And the above_base has another meaning per block/stream.c - it can be a temporary filter with a JSON-name. I will swap those two function calls (bdrv_lookup_bs(); qdict_del();). Thank you. Andrey

On 14.10.20 18:08, Andrey Shinkevich wrote: Sounds more complicated than to just split off BlockdevOptionsCor (from BlockdevOptionsGenericFormat, like e.g. BlockdevOptionsRaw does it). Well, they don’t have to pass anything if they don’t need a bottom node. So unless they want to use a bottom/base-overlay node, it won’t become more complicated. Nothing yet, I think. Yes, agreed; I’m not talking about the variable identifier though. I’m talking about the option name, which in this version is just “base”. (And whenever that option is used, once here, once later in block/stream.c, there is an explanatory comment why we don’t pass the base node through it, but the first overlay above the base. Which makes me believe perhaps it shouldn’t be called “base”.) Don’t forget that the error_setg() below also makes use of it: Max

On 12.10.20 19:43, Andrey Shinkevich wrote: In case of failure, a negative value is going to be returned, we won’t go into this conditional block, and local_flags isn’t going to contain BDRV_REQ_COPY_ON_READ. So the idea of CORing in case of failure sounds sound to me, but it doesn’t look like that’s done. I think this should either be bdrv_backing_chain_next() or we must rule out the possibility of bs->file->bs being a filter somewhere. I think I’d prefer the former. “ret == 1 || ret < 0” would be more explicit (and in line with the “!ret || ret < 0” probably needed above), but correct either way. Max

On 12.10.20 19:43, Andrey Shinkevich wrote: Just when I hit send I noticed that “end” would be a more fitting name for this variable. (because I got a bit confused looking at this) (Though dropping @size (or @end) and just checking when @bytes becomes 0 should work, too.) Furthermore, I just noticed – can the is_allocated functions not return 0 in @n, when @offset is a the EOF? Is that something to look out for? (I’m not sure.) Max

On 14.10.20 20:57, Andrey Shinkevich wrote: True. Is it that bad to lose that optimization, though? (And I would expect the case of a short backing file to be rather rare, too.) That sounds like a bad hack. I’d rather keep the double is_allocated(). But what would be the problem with losing the short backing file optimization? Just performance? Or would we end up writing actual zeroes into the overlay past the end of the backing file? Hm, probably not, if the COR filter would detect that case and handle it like stream does. So it seems only a question of performance to me, and I don’t think it would be that bad to in this rather rare case to have a bunch of useless is_allocated and is_allocated_above calls past the backing file’s EOF. (Maybe I’m wrong, though.) Max

15.10.2020 20:37, Andrey Shinkevich wrote: I'd keep is_allocated logic in block-stream as is for now. It's not good that we check block-status several times (in block-stream, than in cor filter, than in generic COR code), but it shouldn't be real problem, and we can postpone optimizations for the next step. Also, the resulting architecture is not final. I believe that in bright future block-stream will work through block-copy like backup does. And COR filter will call block_copy() by itself, and generic COR code will be dropped together with BDRV_REQ_COR flag. And stream will do just one background call of block_copy for the whole device, like backup in finish on my in-flight backup series. And all extra levels of block_status checking will leave. About EOF problem discussed here, let's look at more generic problem: we are going to skip _large_ area, but skipping chunk-by-chunk is inefficient. So, we just want to learn to skip large areas. The simplest way is just to call is_allocated/is_allocated_above from current offset to device end, if we decided to skip current chunk. Then we'll know how much to skip totally. But that kind of optimization is not directly related to these series and may be done in separate if needed. -- Best regards, Vladimir

On 12.10.20 19:43, Andrey Shinkevich wrote: There’s some word missing here, but I’m not sure what it is... At least an “is” before “applied”. Perhaps something like ”or when a COR filter is involved (in read operations)” would be better. And this “written to” confused me for a second, because we’re reading into qiov. Technically, that means writing into the buffer, but, you know. Could we rewrite the whole thing, perhaps? Something like “BDRV_REQ_PREFETCH makes sense only in the context of copy-on-read (i.e., together with the BDRV_REQ_COPY_ON_READ flag or when there is a COR filter), in which case it signals that the COR operation need not read the data into memory (qiov), but only ensure it is copied to the top layer (i.e., that COR is done).” I don’t know. Max

On 14.10.2020 15:22, Max Reitz wrote: I would modify a little: “BDRV_REQ_PREFETCH makes sense only in the context of copy-on-read (i.e., together with the BDRV_REQ_COPY_ON_READ flag or when a COR filter is involved), in which case it signals that the COR operation need not read the data into memory (qiov) but only ensure they are copied to the top layer (i.e., that COR operation is done).”

On 12.10.20 19:43, Andrey Shinkevich wrote: s/write/read/ Then we mustn’t let cor_co_preadv_part() pass the flag on to bdrv_co_preadv_part() unless BDRV_REQ_COPY_ON_READ is set, too. I suspect the following patch is going to do that, but in the meantime the code is wrong. Perhaps just swap both patches? And by the way, I’m also missing a patch that makes the block layer evaluate supported_read_flags and e.g. strip BDRV_REQ_PREFETCH if it isn’t supported, before it gets passed to such a block driver. Max

On 12.10.20 19:43, Andrey Shinkevich wrote: How about dropping the double negation and using a logical && instead of the binary &? Hm. I would have just written the negated form (!(flags & BDRV_REQ_PREFETCH) || (local_flags & BDRV_REQ_COPY_ON_READ)) and put the “skip” comment above that condition. (Since local_flags is initialized to flags, it can be written as a single comparison, but that’s a matter of taste and I’m not going to recommend either over the other: ((local_flags & (BDRV_REQ_PREFETCH | BDRV_REQ_COPY_ON_READ)) != BDRV_REQ_PREFETCH) ) Ah, OK. I see. I expected this to be a separate patch. I still wonder why it isn’t. Max

On 14.10.20 17:22, Vladimir Sementsov-Ogievskiy wrote: True. :/ Hm. But at least in the case of COR, PREFETCH is not something that can be optimized to be a no-op (unless the COR is a no-op); it still denotes a command that must be executed. So if we can’t pass it to the driver, I don’t think we should do nothing, but to return an error. Or maybe we could even assert that it isn’t set for drivers that don’t support it, because at least right now such a case would just be a bug. Sure. Max

On 14.10.20 18:39, Vladimir Sementsov-Ogievskiy wrote: Agreed. Max

On 21.10.2020 23:43, Andrey Shinkevich wrote: [...] I have found a workaround and am going to send all the related patches as a separate series. Andrey

On 14.10.2020 15:51, Max Reitz wrote: Yes, that's correct. I played with the flags to make the idea obvious for the eye of a beholder: "we neither read nor write". Comparing the BDRV_REQ_PREFETCH against the 'flags' means that the flag comes from outside of the function. And the empty section means we do nothing in that case. Eventually, I will pick up the brief expression below. Thanks, Andrey

On 12.10.20 19:43, Andrey Shinkevich wrote: I think you have to leave this querying s->backing_file_str, and instead change how qmp_block_stream() gets @base_name. block-stream has a backing-file parameter that can override the string that should be used here. (Or perhaps you can let qmp_block_stream() just set it to NULL if no backing-file parameter is passed and then fall back to base_unfiltered->filename only here. Probably better, actually, in case base_unfiltered is changed during the job run.) Max

Whereas the block-stream job starts using a backing file name of the base node overlay after the block-stream job completes, mark the QMP 'backing-file' argument as deprecated. Signed-off-by: Andrey Shinkevich <andrey.shinkevich(a)virtuozzo.com&gt; --- docs/system/deprecated.rst | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst index 8b3ab5b..7491fcf 100644 --- a/docs/system/deprecated.rst +++ b/docs/system/deprecated.rst @@ -285,6 +285,12 @@ details. The ``query-events`` command has been superseded by the more powerful and accurate ``query-qmp-schema`` command. +``block-stream`` argument ``backing-file`` (since 5.2) +''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''' + +The argument ``backing-file`` is deprecated. QEMU uses a backing file +name of the base node overlay after the block-stream job completes. + chardev client socket with ``wait`` option (since 4.0) '''''''''''''''''''''''''''''''''''''''''''''''''''''' -- 1.8.3.1

14.10.2020 18:03, Max Reitz wrote: My wrong idea, sorry. I believed that the argument is unused when I were reviewing v10. But it actually become unused during the series and it is wrong. -- Best regards, Vladimir

The 'backing-file' argument is not used by the block-stream job. It designates a backing file name to set in QCOW2 image header after the block-stream job finished. A backing file name of the node above base is used instead. Signed-off-by: Andrey Shinkevich <andrey.shinkevich(a)virtuozzo.com&gt; --- block/stream.c | 6 +----- blockdev.c | 21 ++++++--------------- include/block/block_int.h | 2 +- 3 files changed, 8 insertions(+), 21 deletions(-) diff --git a/block/stream.c b/block/stream.c index 51462bd..d3e1812 100644 --- a/block/stream.c +++ b/block/stream.c @@ -34,7 +34,6 @@ typedef struct StreamBlockJob { BlockDriverState *base_overlay; /* COW overlay (stream from this) */ BlockDriverState *above_base; /* Node directly above the base */ BlockdevOnError on_error; - char *backing_file_str; bool bs_read_only; bool chain_frozen; } StreamBlockJob; @@ -103,8 +102,6 @@ static void stream_clean(Job *job) blk_set_perm(bjob->blk, 0, BLK_PERM_ALL, &error_abort); bdrv_reopen_set_read_only(bs, true, NULL); } - - g_free(s->backing_file_str); } static int coroutine_fn stream_run(Job *job, Error **errp) @@ -220,7 +217,7 @@ static const BlockJobDriver stream_job_driver = { }; void stream_start(const char *job_id, BlockDriverState *bs, - BlockDriverState *base, const char *backing_file_str, + BlockDriverState *base, int creation_flags, int64_t speed, BlockdevOnError on_error, const char *filter_node_name, @@ -295,7 +292,6 @@ void stream_start(const char *job_id, BlockDriverState *bs, s->base_overlay = base_overlay; s->above_base = above_base; - s->backing_file_str = g_strdup(backing_file_str); s->bs_read_only = bs_read_only; s->chain_frozen = true; diff --git a/blockdev.c b/blockdev.c index d719c47..019b6e0 100644 --- a/blockdev.c +++ b/blockdev.c @@ -2498,7 +2498,6 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device, BlockDriverState *base_bs = NULL; AioContext *aio_context; Error *local_err = NULL; - const char *base_name = NULL; int job_flags = JOB_DEFAULT; if (!has_on_error) { @@ -2526,7 +2525,6 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device, goto out; } assert(bdrv_get_aio_context(base_bs) == aio_context); - base_name = base; } if (has_base_node) { @@ -2541,7 +2539,11 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device, } assert(bdrv_get_aio_context(base_bs) == aio_context); bdrv_refresh_filename(base_bs); - base_name = base_bs->filename; + } + + if (has_backing_file) { + warn_report("Use of \"backing-file\" argument is deprecated; " + "a backing file of the node above base is used instead"); } /* Check for op blockers in the whole chain between bs and base */ @@ -2553,17 +2555,6 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device, } } - /* if we are streaming the entire chain, the result will have no backing - * file, and specifying one is therefore an error */ - if (base_bs == NULL && has_backing_file) { - error_setg(errp, "backing file specified, but streaming the " - "entire chain"); - goto out; - } - - /* backing_file string overrides base bs filename */ - base_name = has_backing_file ? backing_file : base_name; - if (has_auto_finalize && !auto_finalize) { job_flags |= JOB_MANUAL_FINALIZE; } @@ -2571,7 +2562,7 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device, job_flags |= JOB_MANUAL_DISMISS; } - stream_start(has_job_id ? job_id : NULL, bs, base_bs, base_name, + stream_start(has_job_id ? job_id : NULL, bs, base_bs, job_flags, has_speed ? speed : 0, on_error, filter_node_name, &local_err); if (local_err) { diff --git a/include/block/block_int.h b/include/block/block_int.h index a142867..4f523c3 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -1151,7 +1151,7 @@ int is_windows_drive(const char *filename); * BlockDriverState. */ void stream_start(const char *job_id, BlockDriverState *bs, - BlockDriverState *base, const char *backing_file_str, + BlockDriverState *base, int creation_flags, int64_t speed, BlockdevOnError on_error, const char *filter_node_name, -- 1.8.3.1

This patch completes the series with the COR-filter insertion for block-stream operations. Adding the filter makes it possible for copied regions to be discarded in backing files during the block-stream job, what will reduce the disk overuse. The COR-filter insertion incurs changes in the iotests case 245:test_block_stream_4 that reopens the backing chain during a block-stream job. There are changes in the iotests #030 as well. The iotests case 030:test_stream_parallel was deleted due to multiple conflicts between the concurrent job operations over the same backing chain. The base backing node for one job is the top node for another job. It may change due to the filter node inserted into the backing chain while both jobs are running. Another issue is that the parts of the backing chain are being frozen by the running job and may not be changed by the concurrent job when needed. The concept of the parallel jobs with common nodes is considered vital no more. Signed-off-by: Andrey Shinkevich <andrey.shinkevich(a)virtuozzo.com&gt; --- block/stream.c | 93 +++++++++++++++++++++++++++++----------------- tests/qemu-iotests/030 | 51 +++---------------------- tests/qemu-iotests/030.out | 4 +- tests/qemu-iotests/141.out | 2 +- tests/qemu-iotests/245 | 19 +++++++--- 5 files changed, 81 insertions(+), 88 deletions(-) diff --git a/block/stream.c b/block/stream.c index d3e1812..93564db 100644 --- a/block/stream.c +++ b/block/stream.c @@ -17,8 +17,10 @@ #include "block/blockjob_int.h" #include "qapi/error.h" #include "qapi/qmp/qerror.h" +#include "qapi/qmp/qdict.h" #include "qemu/ratelimit.h" #include "sysemu/block-backend.h" +#include "block/copy-on-read.h" enum { /* @@ -33,6 +35,8 @@ typedef struct StreamBlockJob { BlockJob common; BlockDriverState *base_overlay; /* COW overlay (stream from this) */ BlockDriverState *above_base; /* Node directly above the base */ + BlockDriverState *cor_filter_bs; + BlockDriverState *target_bs; BlockdevOnError on_error; bool bs_read_only; bool chain_frozen; @@ -43,8 +47,7 @@ static int coroutine_fn stream_populate(BlockBackend *blk, { assert(bytes < SIZE_MAX); - return blk_co_preadv(blk, offset, bytes, NULL, - BDRV_REQ_COPY_ON_READ | BDRV_REQ_PREFETCH); + return blk_co_preadv(blk, offset, bytes, NULL, BDRV_REQ_PREFETCH); } static void stream_abort(Job *job) @@ -52,23 +55,20 @@ static void stream_abort(Job *job) StreamBlockJob *s = container_of(job, StreamBlockJob, common.job); if (s->chain_frozen) { - BlockJob *bjob = &s->common; - bdrv_unfreeze_backing_chain(blk_bs(bjob->blk), s->above_base); + bdrv_unfreeze_backing_chain(s->cor_filter_bs, s->above_base); } } static int stream_prepare(Job *job) { StreamBlockJob *s = container_of(job, StreamBlockJob, common.job); - BlockJob *bjob = &s->common; - BlockDriverState *bs = blk_bs(bjob->blk); - BlockDriverState *unfiltered_bs = bdrv_skip_filters(bs); + BlockDriverState *unfiltered_bs = bdrv_skip_filters(s->target_bs); BlockDriverState *base = bdrv_filter_or_cow_bs(s->above_base); BlockDriverState *base_unfiltered = bdrv_skip_filters(base); Error *local_err = NULL; int ret = 0; - bdrv_unfreeze_backing_chain(bs, s->above_base); + bdrv_unfreeze_backing_chain(s->cor_filter_bs, s->above_base); s->chain_frozen = false; if (bdrv_cow_child(unfiltered_bs)) { @@ -94,13 +94,14 @@ static void stream_clean(Job *job) { StreamBlockJob *s = container_of(job, StreamBlockJob, common.job); BlockJob *bjob = &s->common; - BlockDriverState *bs = blk_bs(bjob->blk); + + bdrv_cor_filter_drop(s->cor_filter_bs); /* Reopen the image back in read-only mode if necessary */ if (s->bs_read_only) { /* Give up write permissions before making it read-only */ blk_set_perm(bjob->blk, 0, BLK_PERM_ALL, &error_abort); - bdrv_reopen_set_read_only(bs, true, NULL); + bdrv_reopen_set_read_only(s->target_bs, true, NULL); } } @@ -108,9 +109,7 @@ static int coroutine_fn stream_run(Job *job, Error **errp) { StreamBlockJob *s = container_of(job, StreamBlockJob, common.job); BlockBackend *blk = s->common.blk; - BlockDriverState *bs = blk_bs(blk); - BlockDriverState *unfiltered_bs = bdrv_skip_filters(bs); - bool enable_cor = !bdrv_cow_child(s->base_overlay); + BlockDriverState *unfiltered_bs = bdrv_skip_filters(s->target_bs); int64_t len; int64_t offset = 0; uint64_t delay_ns = 0; @@ -122,21 +121,12 @@ static int coroutine_fn stream_run(Job *job, Error **errp) return 0; } - len = bdrv_getlength(bs); + len = bdrv_getlength(s->target_bs); if (len < 0) { return len; } job_progress_set_remaining(&s->common.job, len); - /* Turn on copy-on-read for the whole block device so that guest read - * requests help us make progress. Only do this when copying the entire - * backing chain since the copy-on-read operation does not take base into - * account. - */ - if (enable_cor) { - bdrv_enable_copy_on_read(bs); - } - for ( ; offset < len; offset += n) { bool copy; int ret; @@ -195,10 +185,6 @@ static int coroutine_fn stream_run(Job *job, Error **errp) } } - if (enable_cor) { - bdrv_disable_copy_on_read(bs); - } - /* Do not remove the backing file if an error was there but ignored. */ return error; } @@ -228,6 +214,7 @@ void stream_start(const char *job_id, BlockDriverState *bs, bool bs_read_only; int basic_flags = BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE_UNCHANGED; BlockDriverState *base_overlay = bdrv_find_overlay(bs, base); + BlockDriverState *cor_filter_bs = NULL; BlockDriverState *above_base; if (!base_overlay) { @@ -262,17 +249,48 @@ void stream_start(const char *job_id, BlockDriverState *bs, } } - /* Prevent concurrent jobs trying to modify the graph structure here, we - * already have our own plans. Also don't allow resize as the image size is - * queried only at the job start and then cached. */ - s = block_job_create(job_id, &stream_job_driver, NULL, bs, - basic_flags | BLK_PERM_GRAPH_MOD, - basic_flags | BLK_PERM_WRITE, + QDict *opts = qdict_new(); + + qdict_put_str(opts, "driver", "copy-on-read"); + qdict_put_str(opts, "file", bdrv_get_node_name(bs)); + if (base_overlay) { + /* Pass the base_overlay rather than base */ + qdict_put_str(opts, "base", base_overlay->node_name); + } + if (filter_node_name) { + qdict_put_str(opts, "node-name", filter_node_name); + } + + cor_filter_bs = bdrv_cor_filter_append(bs, opts, BDRV_O_RDWR, errp); + if (cor_filter_bs == NULL) { + goto fail; + } + + if (bdrv_freeze_backing_chain(cor_filter_bs, bs, errp) < 0) { + bdrv_cor_filter_drop(cor_filter_bs); + cor_filter_bs = NULL; + goto fail; + } + + s = block_job_create(job_id, &stream_job_driver, NULL, cor_filter_bs, + BLK_PERM_CONSISTENT_READ, + basic_flags | BLK_PERM_WRITE | BLK_PERM_GRAPH_MOD, speed, creation_flags, NULL, NULL, errp); if (!s) { goto fail; } + /* + * Prevent concurrent jobs trying to modify the graph structure here, we + * already have our own plans. Also don't allow resize as the image size is + * queried only at the job start and then cached. + */ + if (block_job_add_bdrv(&s->common, "active node", bs, + basic_flags | BLK_PERM_GRAPH_MOD, + basic_flags | BLK_PERM_WRITE, &error_abort)) { + goto fail; + } + /* Block all intermediate nodes between bs and base, because they will * disappear from the chain after this operation. The streaming job reads * every block only once, assuming that it doesn't change, so forbid writes @@ -292,6 +310,8 @@ void stream_start(const char *job_id, BlockDriverState *bs, s->base_overlay = base_overlay; s->above_base = above_base; + s->cor_filter_bs = cor_filter_bs; + s->target_bs = bs; s->bs_read_only = bs_read_only; s->chain_frozen = true; @@ -304,5 +324,10 @@ fail: if (bs_read_only) { bdrv_reopen_set_read_only(bs, true, NULL); } - bdrv_unfreeze_backing_chain(bs, above_base); + if (cor_filter_bs) { + bdrv_unfreeze_backing_chain(cor_filter_bs, above_base); + bdrv_cor_filter_drop(cor_filter_bs); + } else { + bdrv_unfreeze_backing_chain(bs, above_base); + } } diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030 index dcb4b5d..0064590 100755 --- a/tests/qemu-iotests/030 +++ b/tests/qemu-iotests/030 @@ -227,61 +227,20 @@ class TestParallelOps(iotests.QMPTestCase): for img in self.imgs: os.remove(img) - # Test that it's possible to run several block-stream operations - # in parallel in the same snapshot chain - @unittest.skipIf(os.environ.get('QEMU_CHECK_BLOCK_AUTO'), 'disabled in CI') - def test_stream_parallel(self): - self.assert_no_active_block_jobs() - - # Check that the maps don't match before the streaming operations - for i in range(2, self.num_imgs, 2): - self.assertNotEqual(qemu_io('-f', iotests.imgfmt, '-rU', '-c', 'map', self.imgs[i]), - qemu_io('-f', iotests.imgfmt, '-rU', '-c', 'map', self.imgs[i-1]), - 'image file map matches backing file before streaming') - - # Create all streaming jobs - pending_jobs = [] - for i in range(2, self.num_imgs, 2): - node_name = 'node%d' % i - job_id = 'stream-%s' % node_name - pending_jobs.append(job_id) - result = self.vm.qmp('block-stream', device=node_name, job_id=job_id, base=self.imgs[i-2], speed=1024) - self.assert_qmp(result, 'return', {}) - - for job in pending_jobs: - result = self.vm.qmp('block-job-set-speed', device=job, speed=0) - self.assert_qmp(result, 'return', {}) - - # Wait for all jobs to be finished. - while len(pending_jobs) > 0: - for event in self.vm.get_qmp_events(wait=True): - if event['event'] == 'BLOCK_JOB_COMPLETED': - job_id = self.dictpath(event, 'data/device') - self.assertTrue(job_id in pending_jobs) - self.assert_qmp_absent(event, 'data/error') - pending_jobs.remove(job_id) - - self.assert_no_active_block_jobs() - self.vm.shutdown() - - # Check that all maps match now - for i in range(2, self.num_imgs, 2): - self.assertEqual(qemu_io('-f', iotests.imgfmt, '-c', 'map', self.imgs[i]), - qemu_io('-f', iotests.imgfmt, '-c', 'map', self.imgs[i-1]), - 'image file map does not match backing file after streaming') - # Test that it's not possible to perform two block-stream # operations if there are nodes involved in both. def test_overlapping_1(self): self.assert_no_active_block_jobs() # Set a speed limit to make sure that this job blocks the rest - result = self.vm.qmp('block-stream', device='node4', job_id='stream-node4', base=self.imgs[1], speed=1024*1024) + result = self.vm.qmp('block-stream', device='node4', + job_id='stream-node4', base=self.imgs[1], + filter_node_name='stream-filter', speed=1024*1024) self.assert_qmp(result, 'return', {}) result = self.vm.qmp('block-stream', device='node5', job_id='stream-node5', base=self.imgs[2]) self.assert_qmp(result, 'error/desc', - "Node 'node4' is busy: block device is in use by block job: stream") + "Node 'stream-filter' is busy: block device is in use by block job: stream") result = self.vm.qmp('block-stream', device='node3', job_id='stream-node3', base=self.imgs[2]) self.assert_qmp(result, 'error/desc', @@ -294,7 +253,7 @@ class TestParallelOps(iotests.QMPTestCase): # block-commit should also fail if it touches nodes used by the stream job result = self.vm.qmp('block-commit', device='drive0', base=self.imgs[4], job_id='commit-node4') self.assert_qmp(result, 'error/desc', - "Node 'node4' is busy: block device is in use by block job: stream") + "Node 'stream-filter' is busy: block device is in use by block job: stream") result = self.vm.qmp('block-commit', device='drive0', base=self.imgs[1], top=self.imgs[3], job_id='commit-node1') self.assert_qmp(result, 'error/desc', diff --git a/tests/qemu-iotests/030.out b/tests/qemu-iotests/030.out index 6d9bee1..5eb508d 100644 --- a/tests/qemu-iotests/030.out +++ b/tests/qemu-iotests/030.out @@ -1,5 +1,5 @@ -........................... +.......................... ---------------------------------------------------------------------- -Ran 27 tests +Ran 26 tests OK diff --git a/tests/qemu-iotests/141.out b/tests/qemu-iotests/141.out index 08e0aec..028a16f 100644 --- a/tests/qemu-iotests/141.out +++ b/tests/qemu-iotests/141.out @@ -99,7 +99,7 @@ wrote 1048576/1048576 bytes at offset 0 {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "created", "id": "job0"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "running", "id": "job0"}} {'execute': 'blockdev-del', 'arguments': {'node-name': 'drv0'}} -{"error": {"class": "GenericError", "desc": "Node drv0 is in use"}} +{"error": {"class": "GenericError", "desc": "Node 'drv0' is busy: block device is in use by block job: stream"}} {'execute': 'block-job-cancel', 'arguments': {'device': 'job0'}} {"return": {}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "aborting", "id": "job0"}} diff --git a/tests/qemu-iotests/245 b/tests/qemu-iotests/245 index e60c832..940e85a 100755 --- a/tests/qemu-iotests/245 +++ b/tests/qemu-iotests/245 @@ -899,17 +899,26 @@ class TestBlockdevReopen(iotests.QMPTestCase): # make hd1 read-only and block-stream requires it to be read-write # (Which error message appears depends on whether the stream job is # already done with copying at this point.) - self.reopen(opts, {}, + # As the COR-filter node is inserted into the backing chain with the + # 'block-stream' operation, we move the options to their proper nodes. + opts = hd_opts(1) + opts['backing'] = hd_opts(2) + opts['backing']['backing'] = None + self.reopen(opts, {'read-only': True}, ["Can't set node 'hd1' to r/o with copy-on-read enabled", "Cannot make block node read-only, there is a writer on it"]) # We can't remove hd2 while the stream job is ongoing - opts['backing']['backing'] = None - self.reopen(opts, {'backing.read-only': False}, "Cannot change 'backing' link from 'hd1' to 'hd2'") + opts['backing'] = None + self.reopen(opts, {'read-only': False}, + "Cannot change 'backing' link from 'hd1' to 'hd2'") - # We can detach hd1 from hd0 because it doesn't affect the stream job + # We can't detach hd1 from hd0 because there is the COR-filter implicit + # node in between. + opts = hd_opts(0) opts['backing'] = None - self.reopen(opts) + self.reopen(opts, {}, + "Cannot change backing link if 'hd0' has an implicit backing file") self.vm.run_job('stream0', auto_finalize = False, auto_dismiss = True) -- 1.8.3.1

On 14.10.2020 19:24, Max Reitz wrote: [...] That is what I had kept in my mind when I tackled this patch. But there is an underwater reef to streamline. Namely, how the block-stream job gets known about a long unallocated tail to exit the loop earlier in the stream_run(). Shall we return the '-EOF' or another error code from the cor_co_preadv_part() to be handled by the stream_run()? Any other suggestions, if any, will be appreciated. The bdrv_freeze_backing_chain(bs, above_base, errp) is called before the bdrv_reopen_set_read_only() to keep the backing chain safe during the context switch. Then we will want to freeze the 'COR -> TOP BS' link as well. Freezing/unfreezing parts is simlier to manage than doing that with the whole chain. If we decide to invoke the bdrv_reopen_set_read_only() after freezing the backing chain together with the COR-filter, we will not be able to get the 'write' permission on the read-only node. When we invoke the block_job_add_bdrv(&s->common, "active node", bs,..) below (particularly, we need it to block the operations on the top node, bdrv_op_block_all()), we ask for the GRAPH_MOD permission for the top node. To allow that, the parent filter node should share that permission for the underlying node. Otherwise, we get assertion failed in the bdrv_check_update_perm() called from bdrv_replace_node() when we remove the filter. Yes, that's right. The only second message appears in the test case when I run it on my node. So, I will remove the first one as the bdrv_enable_copy_on_read() is not called for the top BS on the frozen backing chain anymore. Also, I will delet the part of the comment: "(Which error message appears depends on whether the stream job is already done with copying at this point.)" Yes, it is. Andrey