[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [RFC V8 03/13] quorum: Add quorum_aio_writev and its de
From: |
Kevin Wolf |
Subject: |
Re: [Qemu-devel] [RFC V8 03/13] quorum: Add quorum_aio_writev and its dependencies. |
Date: |
Fri, 27 Sep 2013 12:03:07 +0200 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
Am 26.09.2013 um 18:29 hat Benoît Canet geschrieben:
> Le Friday 08 Feb 2013 à 11:38:38 (+0100), Kevin Wolf a écrit :
> > Am 28.01.2013 18:07, schrieb Benoît Canet:
> > > Signed-off-by: Benoit Canet <address@hidden>
> > > ---
> > > block/quorum.c | 111
> > > ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > > 1 file changed, 111 insertions(+)
> > >
> > > diff --git a/block/quorum.c b/block/quorum.c
> > > index d8fffbe..5d8470b 100644
> > > --- a/block/quorum.c
> > > +++ b/block/quorum.c
> > > @@ -52,11 +52,122 @@ struct QuorumAIOCB {
> > > int vote_ret;
> > > };
> > >
> > > +static void quorum_aio_cancel(BlockDriverAIOCB *blockacb)
> > > +{
> > > + QuorumAIOCB *acb = container_of(blockacb, QuorumAIOCB, common);
> > > + bool finished = false;
> > > +
> > > + /* Wait for the request to finish */
> > > + acb->finished = &finished;
> > > + while (!finished) {
> > > + qemu_aio_wait();
> > > + }
> > > +}
> > > +
> > > +static AIOCBInfo quorum_aiocb_info = {
> > > + .aiocb_size = sizeof(QuorumAIOCB),
> > > + .cancel = quorum_aio_cancel,
> > > +};
> > > +
> > > +static void quorum_aio_bh(void *opaque)
> > > +{
> > > + QuorumAIOCB *acb = opaque;
> > > + BDRVQuorumState *s = acb->bqs;
> > > + int ret;
> > > +
> > > + ret = s->threshold <= acb->success_count ? 0 : -EIO;
> >
> > It would be very much preferable if you stored the actual error code
> > instead of turning everything into -EIO.
> >
> > > +
> > > + qemu_bh_delete(acb->bh);
> > > + acb->common.cb(acb->common.opaque, ret);
> > > + if (acb->finished) {
> > > + *acb->finished = true;
> > > + }
> > > + g_free(acb->aios);
> > > + qemu_aio_release(acb);
> > > +}
> >
> > Move this down so that it's next to the function using the bottom half.
> >
> > > +
> > > +static QuorumAIOCB *quorum_aio_get(BDRVQuorumState *s,
> > > + BlockDriverState *bs,
> > > + QEMUIOVector *qiov,
> > > + uint64_t sector_num,
> > > + int nb_sectors,
> > > + BlockDriverCompletionFunc *cb,
> > > + void *opaque)
> > > +{
> > > + QuorumAIOCB *acb = qemu_aio_get(&quorum_aiocb_info, bs, cb, opaque);
> > > + int i;
> > > +
> > > + acb->aios = g_new0(QuorumSingleAIOCB, s->total);
> > > +
> > > + acb->bqs = s;
> > > + acb->qiov = qiov;
> > > + acb->bh = NULL;
> > > + acb->count = 0;
> > > + acb->success_count = 0;
> > > + acb->sector_num = sector_num;
> > > + acb->nb_sectors = nb_sectors;
> > > + acb->vote = NULL;
> > > + acb->vote_ret = 0;
> > > + acb->finished = NULL;
> > > +
> > > + for (i = 0; i < s->total; i++) {
> > > + acb->aios[i].buf = NULL;
> > > + acb->aios[i].ret = 0;
> > > + acb->aios[i].parent = acb;
> > > + }
> >
> > Would you mind to reorder the initialisation of the fields according to
> > the order that is used in the struct definition?
> >
> > > +
> > > + return acb;
> > > +}
> > > +
> > > +static void quorum_aio_cb(void *opaque, int ret)
> > > +{
> > > + QuorumSingleAIOCB *sacb = opaque;
> > > + QuorumAIOCB *acb = sacb->parent;
> > > + BDRVQuorumState *s = acb->bqs;
> > > +
> > > + sacb->ret = ret;
> > > + acb->count++;
> > > + if (ret == 0) {
> > > + acb->success_count++;
> > > + }
> > > + assert(acb->count <= s->total);
> > > + assert(acb->success_count <= s->total);
> > > + if (acb->count < s->total) {
> > > + return;
> > > + }
> > > +
> > > + acb->bh = qemu_bh_new(quorum_aio_bh, acb);
> > > + qemu_bh_schedule(acb->bh);
> >
> > What's the reason for using a bottom half here? Worth a comment?
> >
> > multiwrite_cb() in block.c doesn't use one to achieve something similar.
> > Is it buggy when you need one here?
> >
>
> I tried the code without bh and it doesn't work.
It's long ago tbat I wrote that comment, but the remark about
multiwrite_cb() concerns me. Do you know _why_ it doesn't work without
the BH, and whether the same problem affects multiwrite_cb()? I'd prefer
if we understood what we're doing over just basing the code on
experiments.
Kevin