Re: [Qemu-devel] [PATCH v2] block: don't set the same context

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2911 bytes --]

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 <dplotnikov@virtuozzo.com>
>> ---
> 
> 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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]