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

[Max Reitz]

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

signature.asc
Description: OpenPGP digital signature