On 9/23/19 9:10 AM, Sergio Lopez wrote: > > Eric Blake <ebl...@redhat.com> writes: > >> When iothreads are in use, the failure to grab the aio context results >> in an assertion failure when trying to unlock things during blk_unref, >> when trying to unlock a mutex that was not locked. In short, all >> calls to nbd_export_put need to done while within the correct aio >> context. But since nbd_export_put can recursively reach itself via >> nbd_export_close, and recursively grabbing the context would deadlock, >> we can't do the context grab directly in those functions, but must do >> so in their callers. >> >> Hoist the use of the correct aio_context from nbd_export_new() to its >> caller qmp_nbd_server_add(). Then tweak qmp_nbd_server_remove(), >> nbd_eject_notifier(), and nbd_esport_close_all() to grab the right >> context, so that all callers during qemu now own the context before >> nbd_export_put() can call blk_unref(). >> >> Remaining uses in qemu-nbd don't matter (since that use case does not >> support iothreads). >> >> Suggested-by: Kevin Wolf <kw...@redhat.com> >> Signed-off-by: Eric Blake <ebl...@redhat.com> >> --- >> >> With this in place, my emailed formula [1] for causing an iothread >> assertion failure no longer hits, and all the -nbd and -qcow2 iotests >> still pass. I would still like to update iotests to cover things (I >> could not quickly figure out how to make iotest 222 use iothreads - >> either we modify that one or add a new one), but wanted to get review >> started on this first. >> >> [1] https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg03383.html
I ended up patch 223 instead, as patch 2/1 in reply to the original.
>> void nbd_export_close(NBDExport *exp)
>> {
>> NBDClient *client, *next;
>>
>> nbd_export_get(exp);
>> +
>
> I'm not sure if this new line was added here on purpose.
>
Spurious leftovers from an alternative attempt. Will drop this.
>> /*
>> * TODO: Should we expand QMP NbdServerRemoveNode enum to allow a
>> * close mode that stops advertising the export to new clients but
>> @@ -1684,9 +1695,13 @@ BlockBackend *nbd_export_get_blockdev(NBDExport *exp)
>> void nbd_export_close_all(void)
>> {
>> NBDExport *exp, *next;
>> + AioContext *aio_context;
>>
>> QTAILQ_FOREACH_SAFE(exp, &exports, next, next) {
>> + aio_context = exp->ctx;
>> + aio_context_acquire(aio_context);
>> nbd_export_close(exp);
>> + aio_context_release(aio_context);
>> }
>> }
>
> Otherwise, LGTM.
>
> Reviewed-by: Sergio Lopez <s...@redhat.com>
>
Thanks; queuing this on my NBD tree, along with the iotest followup
(although there's still a bit of time for that to get a review before I
send the pull request later today).
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
