Re: [RFC PATCH 14/22] block/export: Move AioContext from NBDExport to BlockExport

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 14/22] block/export: Move AioContext from NBDExport to BlockExport
Date: Mon, 17 Aug 2020 17:47:18 +0200	[thread overview]
Message-ID: <95f08b8c-d2d7-aaca-0807-9361d667847d@redhat.com> (raw)
In-Reply-To: <20200817152259.GP11402@linux.fritz.box>

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

On 17.08.20 17:22, Kevin Wolf wrote:
> Am 17.08.2020 um 16:56 hat Max Reitz geschrieben:
>> On 13.08.20 18:29, Kevin Wolf wrote:
>>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>>> ---
>>>  include/block/export.h |  6 ++++++
>>>  nbd/server.c           | 26 +++++++++++++-------------
>>>  2 files changed, 19 insertions(+), 13 deletions(-)
>>
>> Reviewed-by: Max Reitz <mreitz@redhat.com>
>>
>>> diff --git a/include/block/export.h b/include/block/export.h
>>> index f44290a4a2..5459f79469 100644
>>> --- a/include/block/export.h
>>> +++ b/include/block/export.h
>>> @@ -33,6 +33,12 @@ struct BlockExport {
>>>       * the export.
>>>       */
>>>      int refcount;
>>> +
>>> +    /*
>>> +     * The AioContex whose lock needs to be held while calling
>>
>> *AioContext
>>
>>> +     * BlockExportDriver callbacks.
>>
>> Hm.  But other blk_exp_* functions (i.e. the refcount manipulation
>> functions) are fair game?
> 
> Hmm... The assumption was the ref/unref are only called from the main
> thread, but maybe that's not true? So maybe blk_exp_*() shouldn't lock
> the AioContext internally, but require that the lock is already held, so
> that they can be called both from within the AioContext (where we don't
> want to lock a second tim) and from the main context.
> 
> I also guess we need a separate mutex to protect the exports list if
> unref can be called from different threads.
> 
> And probably the existing NBD server code has already the same problems
> with respect to different AioContexts.
> 
>>> +     */
>>> +    AioContext *ctx;
>>>  };
>>>  
>>>  extern const BlockExportDriver blk_exp_nbd;
>>> diff --git a/nbd/server.c b/nbd/server.c
>>> index 2bf30bb731..b735a68429 100644
>>> --- a/nbd/server.c
>>> +++ b/nbd/server.c
>>
>> [...]
>>
>>> @@ -1466,7 +1464,7 @@ static void blk_aio_attached(AioContext *ctx, void *opaque)
>>>  
>>>      trace_nbd_blk_aio_attached(exp->name, ctx);
>>>  
>>> -    exp->ctx = ctx;
>>> +    exp->common.ctx = ctx;
>>
>> (Not sure if Ḯ’m missing anything to that regard), but perhaps after
>> patch 21 we can move this part to the common block export code, and
>> maybe make it call a BlockExportDriver callback (that handles the rest
>> of this function).
> 
> Could probably be done. Not every export driver may support switching
> AioContexts, but we can make it conditional on having the callback.

Good point.

> So do I understand right from your comments to the series in general
> that you would prefer to make this series more complete, even if that
> means that it becomes quite a bit longer?

I’m not necessarily asking for this now, I’m mostly asking whether you
have the same idea as me on things like this.  I don’t mind too much
leaving this in an unfinished state as long as we both agree that it’s
kind of unfinished.

Sorry if this is a bit frustrating to you because you wrote in the cover
letter that indeed you are unsure about how complete you want to do
this.  The problem is that I don’t know exactly what things you’re
referring to, so I just point out everything that stands out to me.  If
you’re aware of those things, and we can work on them later, then that’s OK.

OTOH...  Yes, from a design standpoint, I think it makes sense to pull
out as much specialized code as possible from NBD into the generalized
block export code.  But I say that as a reviewer.  You would have to do
that, so I want to leave it to you how much work you think is reasonable
to put into that.  Leaving a couple of rough edges here and there
shouldn’t be a problem.  (Or maybe leaving something to me for when I
add fuse export code.)

Max

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