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

[Kevin Wolf]

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 Ḯ’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.

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?

Kevin

signature.asc
Description: PGP signature