[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH v5 33/42] blockdev: Fix active commit choice
From: |
Vladimir Sementsov-Ogievskiy |
Subject: |
Re: [Qemu-block] [PATCH v5 33/42] blockdev: Fix active commit choice |
Date: |
Fri, 21 Jun 2019 13:26:13 +0000 |
19.06.2019 18:59, Max Reitz wrote:
> On 19.06.19 11:31, Vladimir Sementsov-Ogievskiy wrote:
>> 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.
>
> Isn’t that blocked by the commit filter? If it isn’t, it should be.
>
>> 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.
>
> Disadvantage: Something may break because the basic commit job does not
> emit BLOCK_JOB_READY and thus does not require block-job-complete.
>
> Technically everything should expect jobs to potentially emit
> BLOCK_JOB_READY, but who knows. I think we’d want at least a
> deprecation period.
>
> Max
OK, so this is for future.. Then:
Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>
>
>>> + */
>>> + 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-block] [PATCH v5 35/42] block: Fix check_to_replace_node(), Max Reitz, 2019/06/12
[Qemu-block] [PATCH v5 34/42] block: Inline bdrv_co_block_status_from_*(), Max Reitz, 2019/06/12