[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2] block: don't set the same context
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH v2] block: don't set the same context |
Date: |
Fri, 15 Feb 2019 11:01:15 -0600 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.4.0 |
On 2/15/19 7:12 AM, Eric Blake wrote:
> On 2/15/19 7:03 AM, Denis Plotnikov wrote:
>> Adds a fast path on aio context setting preventing
>> unnecessary context setting routine.
>> Also, it prevents issues with cyclic walk of child
>> bds-es appeared because of registering aio walking
>> notifiers:
>
>> Signed-off-by: Denis Plotnikov <address@hidden>
>> ---
>
> Right here, after the ---, is a good place to list how v2 differs from
> v1. It looks to me like the only change was a typo fix in the commit
> message? But that still doesn't address the concern on whether this
> approach is right in the face of nesting (if attach/detach is always
> paired, then attach-attach-detach-detach will cause the inner detach to
> pair to the outer attach, any actions between the inner and outer detach
> will not be against an attached context, and the outer detach needs to
> be safe as a no-op). The patch may still be right, but I'm not enough
> of an expert in how aio attach/detach are supposed to work to know for
> sure. Or, we may need some form of reference counting, so that the
> inner detach is a no-op if the inner attach was short-circuited, and the
> outer detach then resumes being the proper pair against the outer
> attach. If you have checked why the patch works, then amending the
> commit message to address my question will also help future readers that
> might otherwise ask the same question. Posting a rapid-turnaround v2 for
> just a typo fix, while not addressing the larger question raised as to
> whether the patch is correct, is just churn.
Having now re-read the patch, I see that you are patching
bdrv_set_aio_context, even though your commit message focused my
attention on the nesting:
>
> 3 __GI___assert_fail
> 4 bdrv_detach_aio_context (bs=0x55f54d65c000) <<<
> 5 bdrv_detach_aio_context (bs=0x55f54fc8a800)
> 6 bdrv_set_aio_context (bs=0x55f54fc8a800, ...)
> 7 block_job_attached_aio_context
> 8 bdrv_attach_aio_context (bs=0x55f54d65c000, ...) <<<
> 9 bdrv_set_aio_context (bs=0x55f54d65c000)
That is, you marked lines 4 and 8 as forming nested attach/detach pairs,
rather than 6 and 9 as nested set calls. If I understand the point of
this patch, your goal is to realize that setting the context to what it
already has is pointless, and by short-circuiting the second set, you
can then completely bypass the nested attach/detach. So it looks like
the change is correct, and that it is merely that the commit message
could still be improved to do a better job of calling out that the
symptoms were a nested attach/detach, but the fix is to avoid the nested
calls by fixing the setting to be smarter on the realization that
setting can be reached more than once.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature