[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v5 33/42] blockdev: Fix active commit choice
From: |
Vladimir Sementsov-Ogievskiy |
Subject: |
Re: [Qemu-devel] [PATCH v5 33/42] blockdev: Fix active commit choice |
Date: |
Wed, 19 Jun 2019 09:31:10 +0000 |
13.06.2019 1:09, Max Reitz wrote:
> We have to perform an active commit whenever the top node has a parent
> that has taken the WRITE permission on it.
>
> Signed-off-by: Max Reitz <address@hidden>
> ---
> blockdev.c | 24 +++++++++++++++++++++---
> 1 file changed, 21 insertions(+), 3 deletions(-)
>
> diff --git a/blockdev.c b/blockdev.c
> index a464cabf9e..5370f3b738 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -3294,6 +3294,7 @@ void qmp_block_commit(bool has_job_id, const char
> *job_id, const char *device,
> */
> BlockdevOnError on_error = BLOCKDEV_ON_ERROR_REPORT;
> int job_flags = JOB_DEFAULT;
> + uint64_t top_perm, top_shared;
>
> if (!has_speed) {
> speed = 0;
> @@ -3406,14 +3407,31 @@ void qmp_block_commit(bool has_job_id, const char
> *job_id, const char *device,
> goto out;
> }
>
> - if (top_bs == bs) {
> + /*
> + * Active commit is required if and only if someone has taken a
> + * WRITE permission on the top node. Historically, we have always
> + * used active commit for top nodes, so continue that practice.
> + * (Active commit is never really wrong.)
Hmm, if we start active commit when nobody has write access, than
we leave a possibility to someone to get this access during commit. And during
passive commit write access is blocked. So, may be right way is do active commit
always? Benefits:
1. One code path. and it shouldn't be worse when no writers, without guest
writes
mirror code shouldn't work worse than passive commit, if it is, it should be
fixed.
2. Possibility of write access if user needs it during commit
3. I'm sure that active commit (mirror code) actually works faster, as it uses
async requests and smarter handling of block status.
> + */
> + bdrv_get_cumulative_perm(top_bs, &top_perm, &top_shared);
> + if (top_perm & BLK_PERM_WRITE ||
> + bdrv_skip_rw_filters(top_bs) == bdrv_skip_rw_filters(bs))
> + {
> if (has_backing_file) {
> error_setg(errp, "'backing-file' specified,"
> " but 'top' is the active layer");
> goto out;
> }
> - commit_active_start(has_job_id ? job_id : NULL, bs, base_bs,
> - job_flags, speed, on_error,
> + if (!has_job_id) {
> + /*
> + * Emulate here what block_job_create() does, because it
> + * is possible that @bs != @top_bs (the block job should
> + * be named after @bs, even if @top_bs is the actual
> + * source)
> + */
> + job_id = bdrv_get_device_name(bs);
> + }
> + commit_active_start(job_id, top_bs, base_bs, job_flags, speed,
> on_error,
> filter_node_name, NULL, NULL, false,
> &local_err);
> } else {
> BlockDriverState *overlay_bs = bdrv_find_overlay(bs, top_bs);
>
--
Best regards,
Vladimir
[Qemu-devel] [PATCH v5 34/42] block: Inline bdrv_co_block_status_from_*(), Max Reitz, 2019/06/12
[Qemu-devel] [PATCH v5 36/42] iotests: Add tests for mirror @replaces loops, Max Reitz, 2019/06/12
[Qemu-devel] [PATCH v5 35/42] block: Fix check_to_replace_node(), Max Reitz, 2019/06/12
[Qemu-devel] [PATCH v5 38/42] iotests: Let complete_and_wait() work with commit, Max Reitz, 2019/06/12