Re: [PATCH] nbd: Grab aio context lock in more places

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2981 bytes --]

On 9/23/19 9:10 AM, Sergio Lopez wrote:
> 
> Eric Blake <eblake@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 <kwolf@redhat.com>
>> Signed-off-by: Eric Blake <eblake@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 <slp@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


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