Re: [PATCH v2 2/2] nbd/server: Use drained block ops to quiesce the server

[Sergio Lopez]

On Wed, Jun 02, 2021 at 03:06:02PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> 02.06.2021 09:05, Sergio Lopez wrote:
> > Before switching between AioContexts we need to make sure that we're
> > fully quiesced ("nb_requests == 0" for every client) when entering the
> > drained section.
> > 
> > To do this, we set "quiescing = true" for every client on
> > ".drained_begin" to prevent new coroutines from being created, and
> > check if "nb_requests == 0" on ".drained_poll". Finally, once we're
> > exiting the drained section, on ".drained_end" we set "quiescing =
> > false" and call "nbd_client_receive_next_request()" to resume the
> > processing of new requests.
> > 
> > With these changes, "blk_aio_attach()" and "blk_aio_detach()" can be
> > reverted to be as simple as they were before f148ae7d36.
> > 
> > RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1960137
> > Suggested-by: Kevin Wolf <kwolf@redhat.com>
> > Signed-off-by: Sergio Lopez <slp@redhat.com>
> > ---
> >   nbd/server.c | 82 ++++++++++++++++++++++++++++++++++++++--------------
> >   1 file changed, 61 insertions(+), 21 deletions(-)
> > 
> > diff --git a/nbd/server.c b/nbd/server.c
> > index 86a44a9b41..b60ebc3ab6 100644
> > --- a/nbd/server.c
> > +++ b/nbd/server.c
> > @@ -1513,6 +1513,11 @@ static void nbd_request_put(NBDRequestData *req)
> >       g_free(req);
> >       client->nb_requests--;
> > +
> > +    if (client->quiescing && client->nb_requests == 0) {
> > +        aio_wait_kick();
> > +    }
> > +
> >       nbd_client_receive_next_request(client);
> >       nbd_client_put(client);
> > @@ -1530,49 +1535,68 @@ static void blk_aio_attached(AioContext *ctx, void 
> > *opaque)
> >       QTAILQ_FOREACH(client, &exp->clients, next) {
> >           qio_channel_attach_aio_context(client->ioc, ctx);
> > +        assert(client->nb_requests == 0);
> >           assert(client->recv_coroutine == NULL);
> >           assert(client->send_coroutine == NULL);
> > -
> > -        if (client->quiescing) {
> > -            client->quiescing = false;
> > -            nbd_client_receive_next_request(client);
> > -        }
> >       }
> >   }
> > -static void nbd_aio_detach_bh(void *opaque)
> > +static void blk_aio_detach(void *opaque)
> >   {
> >       NBDExport *exp = opaque;
> >       NBDClient *client;
> > +    trace_nbd_blk_aio_detach(exp->name, exp->common.ctx);
> > +
> >       QTAILQ_FOREACH(client, &exp->clients, next) {
> >           qio_channel_detach_aio_context(client->ioc);
> > +    }
> > +
> > +    exp->common.ctx = NULL;
> > +}
> > +
> > +static void nbd_drained_begin(void *opaque)
> > +{
> > +    NBDExport *exp = opaque;
> > +    NBDClient *client;
> > +
> > +    QTAILQ_FOREACH(client, &exp->clients, next) {
> >           client->quiescing = true;
> > +    }
> > +}
> > -        if (client->recv_coroutine) {
> > -            if (client->read_yielding) {
> > -                qemu_aio_coroutine_enter(exp->common.ctx,
> > -                                         client->recv_coroutine);
> > -            } else {
> > -                AIO_WAIT_WHILE(exp->common.ctx, client->recv_coroutine != 
> > NULL);
> > -            }
> > -        }
> > +static void nbd_drained_end(void *opaque)
> > +{
> > +    NBDExport *exp = opaque;
> > +    NBDClient *client;
> > -        if (client->send_coroutine) {
> > -            AIO_WAIT_WHILE(exp->common.ctx, client->send_coroutine != 
> > NULL);
> > -        }
> > +    QTAILQ_FOREACH(client, &exp->clients, next) {
> > +        client->quiescing = false;
> > +        nbd_client_receive_next_request(client);
> >       }
> >   }
> > -static void blk_aio_detach(void *opaque)
> > +static bool nbd_drained_poll(void *opaque)
> >   {
> >       NBDExport *exp = opaque;
> > +    NBDClient *client;
> > -    trace_nbd_blk_aio_detach(exp->name, exp->common.ctx);
> > +    QTAILQ_FOREACH(client, &exp->clients, next) {
> > +        if (client->nb_requests != 0) {
> > +            /*
> > +             * If there's a coroutine waiting for a request on 
> > nbd_read_eof()
> > +             * enter it here so we don't depend on the client to wake it 
> > up.
> > +             */
> > +            if (client->recv_coroutine != NULL && client->read_yielding) {
> > +                qemu_aio_coroutine_enter(exp->common.ctx,
> > +                                         client->recv_coroutine);
> > +            }
> > -    aio_wait_bh_oneshot(exp->common.ctx, nbd_aio_detach_bh, exp);
> > +            return true;
> > +        }
> > +    }
> > -    exp->common.ctx = NULL;
> > +    return false;
> >   }
> >   static void nbd_eject_notifier(Notifier *n, void *data)
> > @@ -1594,6 +1618,12 @@ void nbd_export_set_on_eject_blk(BlockExport *exp, 
> > BlockBackend *blk)
> >       blk_add_remove_bs_notifier(blk, &nbd_exp->eject_notifier);
> >   }
> > +static const BlockDevOps nbd_block_ops = {
> > +    .drained_begin = nbd_drained_begin,
> > +    .drained_end = nbd_drained_end,
> > +    .drained_poll = nbd_drained_poll,
> > +};
> > +
> >   static int nbd_export_create(BlockExport *blk_exp, BlockExportOptions 
> > *exp_args,
> >                                Error **errp)
> >   {
> > @@ -1715,8 +1745,17 @@ static int nbd_export_create(BlockExport *blk_exp, 
> > BlockExportOptions *exp_args,
> >       exp->allocation_depth = arg->allocation_depth;
> > +    /*
> > +     * We need to inhibit request queuing in the block layer to ensure we 
> > can
> > +     * be properly quiesced when entering a drained section, as our 
> > coroutines
> > +     * servicing pending requests might enter blk_pread().
> > +     */
> 
> Not very understandable to me :(. What's bad in queuing requests at blk layer 
> during drained section?

We need to make sure that all coroutines in the NBD server have
finished (client->nb_requests == 0) before detaching from the
AioContext. If we don't inhibit request queuing, some coroutines may
get stuck in blk_pread()->...->blk_wait_while_drained(), causing
nbd_drained_poll() to always return that we're busy.

> > +    blk_set_disable_request_queuing(blk, true);
> > +
> >       blk_add_aio_context_notifier(blk, blk_aio_attached, blk_aio_detach, 
> > exp);
> > +    blk_set_dev_ops(blk, &nbd_block_ops, exp);
> > +
> >       QTAILQ_INSERT_TAIL(&exports, exp, next);
> >       return 0;
> > @@ -1788,6 +1827,7 @@ static void nbd_export_delete(BlockExport *blk_exp)
> >           }
> >           blk_remove_aio_context_notifier(exp->common.blk, blk_aio_attached,
> >                                           blk_aio_detach, exp);
> > +        blk_set_disable_request_queuing(exp->common.blk, false);
> >       }
> >       for (i = 0; i < exp->nr_export_bitmaps; i++) {
> > 
> 
> I don't follow the whole logic of clients quiescing, but overall looks good 
> to me. As I understand, prior to patch we have a kind of drain_begin 
> realization in blk_aio_detach handler, and after patch we instead utilize 
> generic code of drained sections with help of appropriate devops handlers.

Doing that in blk_aio_detach() was a bit too late since we were
already in the drained section, and as stated above, we need it alive
to ensure that all of the NBD server coroutines finish properly.

Also, .drained_poll allows us to align quiescing the NBD server with
the block layer.

Thanks,
Sergio.

> weak:
> 
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> 
> 
> -- 
> Best regards,
> Vladimir
>

signature.asc
Description: PGP signature