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 <dplotni...@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
signature.asc
Description: OpenPGP digital signature
