diff mbox series

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

Message ID 20200813162935.210070-15-kwolf@redhat.com
State New, archived
Headers show
Series block/export: Add infrastructure and QAPI for block exports | expand

Commit Message

Kevin Wolf Aug. 13, 2020, 4:29 p.m. UTC
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 include/block/export.h |  6 ++++++
 nbd/server.c           | 26 +++++++++++++-------------
 2 files changed, 19 insertions(+), 13 deletions(-)

Comments

Max Reitz Aug. 17, 2020, 2:56 p.m. UTC | #1
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?

> +     */
> +    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).

>      QTAILQ_FOREACH(client, &exp->clients, next) {
>          qio_channel_attach_aio_context(client->ioc, ctx);
> @@ -1484,13 +1482,13 @@ static void blk_aio_detach(void *opaque)
>      NBDExport *exp = opaque;
>      NBDClient *client;
>  
> -    trace_nbd_blk_aio_detach(exp->name, exp->ctx);
> +    trace_nbd_blk_aio_detach(exp->name, exp->common.ctx);
>  
>      QTAILQ_FOREACH(client, &exp->clients, next) {
>          qio_channel_detach_aio_context(client->ioc);
>      }
>  
> -    exp->ctx = NULL;
> +    exp->common.ctx = NULL;

(same here)

Max
Kevin Wolf Aug. 17, 2020, 3:22 p.m. UTC | #2
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
Max Reitz Aug. 17, 2020, 3:47 p.m. UTC | #3
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 Ḯ’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
diff mbox series

Patch

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
+     * BlockExportDriver callbacks.
+     */
+    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
@@ -92,8 +92,6 @@  struct NBDExport {
     QTAILQ_HEAD(, NBDClient) clients;
     QTAILQ_ENTRY(NBDExport) next;
 
-    AioContext *ctx;
-
     BlockBackend *eject_notifier_blk;
     Notifier eject_notifier;
 
@@ -1333,8 +1331,8 @@  static coroutine_fn int nbd_negotiate(NBDClient *client, Error **errp)
     }
 
     /* Attach the channel to the same AioContext as the export */
-    if (client->exp && client->exp->ctx) {
-        qio_channel_attach_aio_context(client->ioc, client->exp->ctx);
+    if (client->exp && client->exp->common.ctx) {
+        qio_channel_attach_aio_context(client->ioc, client->exp->common.ctx);
     }
 
     assert(!client->optlen);
@@ -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;
 
     QTAILQ_FOREACH(client, &exp->clients, next) {
         qio_channel_attach_aio_context(client->ioc, ctx);
@@ -1484,13 +1482,13 @@  static void blk_aio_detach(void *opaque)
     NBDExport *exp = opaque;
     NBDClient *client;
 
-    trace_nbd_blk_aio_detach(exp->name, exp->ctx);
+    trace_nbd_blk_aio_detach(exp->name, exp->common.ctx);
 
     QTAILQ_FOREACH(client, &exp->clients, next) {
         qio_channel_detach_aio_context(client->ioc);
     }
 
-    exp->ctx = NULL;
+    exp->common.ctx = NULL;
 }
 
 static void nbd_eject_notifier(Notifier *n, void *data)
@@ -1498,7 +1496,7 @@  static void nbd_eject_notifier(Notifier *n, void *data)
     NBDExport *exp = container_of(n, NBDExport, eject_notifier);
     AioContext *aio_context;
 
-    aio_context = exp->ctx;
+    aio_context = exp->common.ctx;
     aio_context_acquire(aio_context);
     nbd_export_close(exp);
     aio_context_release(aio_context);
@@ -1534,10 +1532,13 @@  NBDExport *nbd_export_new(BlockDriverState *bs,
         return NULL;
     }
 
+    ctx = bdrv_get_aio_context(bs);
+
     exp = g_new0(NBDExport, 1);
     exp->common = (BlockExport) {
         .drv        = &blk_exp_nbd,
         .refcount   = 1,
+        .ctx        = ctx,
     };
 
     /*
@@ -1547,7 +1548,7 @@  NBDExport *nbd_export_new(BlockDriverState *bs,
      * ctx was acquired in the caller.
      */
     assert(name && strlen(name) <= NBD_MAX_STRING_SIZE);
-    ctx = bdrv_get_aio_context(bs);
+
     bdrv_invalidate_cache(bs, NULL);
 
     /* Don't allow resize while the NBD server is running, otherwise we don't
@@ -1622,7 +1623,6 @@  NBDExport *nbd_export_new(BlockDriverState *bs,
         assert(strlen(exp->export_bitmap_context) < NBD_MAX_STRING_SIZE);
     }
 
-    exp->ctx = ctx;
     blk_add_aio_context_notifier(blk, blk_aio_attached, blk_aio_detach, exp);
 
     blk_exp_ref(&exp->common);
@@ -1653,7 +1653,7 @@  NBDExport *nbd_export_find(const char *name)
 AioContext *
 nbd_export_aio_context(NBDExport *exp)
 {
-    return exp->ctx;
+    return exp->common.ctx;
 }
 
 void nbd_export_close(NBDExport *exp)
@@ -1738,7 +1738,7 @@  void nbd_export_close_all(void)
     AioContext *aio_context;
 
     QTAILQ_FOREACH_SAFE(exp, &exports, next, next) {
-        aio_context = exp->ctx;
+        aio_context = exp->common.ctx;
         aio_context_acquire(aio_context);
         nbd_export_close(exp);
         aio_context_release(aio_context);
@@ -2519,7 +2519,7 @@  static void nbd_client_receive_next_request(NBDClient *client)
     if (!client->recv_coroutine && client->nb_requests < MAX_NBD_REQUESTS) {
         nbd_client_get(client);
         client->recv_coroutine = qemu_coroutine_create(nbd_trip, client);
-        aio_co_schedule(client->exp->ctx, client->recv_coroutine);
+        aio_co_schedule(client->exp->common.ctx, client->recv_coroutine);
     }
 }