Re: [RFC PATCH 19/22] block/export: Move strong user reference to block_exports

From: Max Reitz <mreitz@redhat.com>
To: Kevin Wolf <kwolf@redhat.com>
Cc: qemu-devel@nongnu.org, qemu-block@nongnu.org
Subject: Re: [RFC PATCH 19/22] block/export: Move strong user reference to block_exports
Date: Wed, 19 Aug 2020 16:48:56 +0200	[thread overview]
Message-ID: <31aaea72-8352-145f-7f17-995b6cfe0e23@redhat.com> (raw)
In-Reply-To: <20200819142300.GD10272@linux.fritz.box>

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

On 19.08.20 16:23, Kevin Wolf wrote:
> Am 19.08.2020 um 13:56 hat Max Reitz geschrieben:
>> On 13.08.20 18:29, Kevin Wolf wrote:
>>> The reference owned by the user/monitor that is created when adding the
>>> export and dropped when removing it was tied to the 'exports' list in
>>> nbd/server.c. Every block export will have a user reference, so move it
>>> to the block export level and tie it to the 'block_exports' list in
>>> block/export/export.c instead. This is necessary for introducing a QMP
>>> command for removing exports.
>>>
>>> Note that exports are present in block_exports even after the user has
>>> requested shutdown. This is different from NBD's exports where exports
>>> are immediately removed on a shutdown request, even if they are still in
>>> the process of shutting down. In order to avoid that the user still
>>> interacts with an export that is shutting down (and possibly removes it
>>> a second time), we need to remember if the user actually still owns it.
>>>
>>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>>> ---
>>>  include/block/export.h | 8 ++++++++
>>>  block/export/export.c  | 4 ++++
>>>  blockdev-nbd.c         | 5 -----
>>>  nbd/server.c           | 2 --
>>>  4 files changed, 12 insertions(+), 7 deletions(-)
>>
>> With this patch, there’s an abort in iotest 281.  Perhaps because
>> blk_exp_unref() is now done by blk_exp_request_shutdown() outside of
>> where the AIO context is locked?
> 
> I have two fixes locally that were related to failing qemu-iotests. I
> guess the first one might be for what you're seeing?
> 
> Kevin
> 
> diff --git a/block/export/export.c b/block/export/export.c
> index 71d17bd440..d021b98b74 100644
> --- a/block/export/export.c
> +++ b/block/export/export.c
> @@ -105,9 +105,14 @@ void blk_exp_unref(BlockExport *exp)
>  {
>      assert(exp->refcount > 0);
>      if (--exp->refcount == 0) {

If this is done without locking the context, should this be an atomic
operation?

> +        AioContext *aio_context = exp->ctx;
> +
> +        aio_context_acquire(aio_context);
>          QLIST_REMOVE(exp, next);
>          exp->drv->delete(exp);
>          blk_unref(exp->blk);
> +        aio_context_release(aio_context);
> +

But for the crash I was seeing, this should be sufficient, yes.

Max

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