| Message ID | 20200813162935.210070-15-kwolf@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | block/export: Add infrastructure and QAPI for block exports | expand |
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
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
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 --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); } }
Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- include/block/export.h | 6 ++++++ nbd/server.c | 26 +++++++++++++------------- 2 files changed, 19 insertions(+), 13 deletions(-)