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 <kw...@redhat.com>
>>> ---
>>> include/block/export.h | 6 ++++++
>>> nbd/server.c | 26 +++++++++++++-------------
>>> 2 files changed, 19 insertions(+), 13 deletions(-)
>>
>> Reviewed-by: Max Reitz <mre...@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 Ḯ’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
signature.asc
Description: OpenPGP digital signature
