Le Tuesday 04 Feb 2014 à 14:57:22 (+0100), Kevin Wolf a écrit :
> Am 03.02.2014 um 22:51 hat Benoît Canet geschrieben:
> > From: Benoît Canet <[email protected]>
> >
> > Signed-off-by: Benoit Canet <[email protected]>
> > ---
> > block/quorum.c | 104
> > +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 104 insertions(+)
>
> Starting with writes before the driver can even open an image is a weird
> order to do things in. It also doesn't make the review any easier when
> you don't know how things are initialised.
I have done it in this way for better git bisectability: if the driver cannot
open the quorum the commit is a preparation work that should be excluded of git
bisect commit range.
For the write before read order it's this way because quorum writes are simpler.
Best regards
Benoît
>
> > diff --git a/block/quorum.c b/block/quorum.c
> > index 157efdf..81bffdd 100644
> > --- a/block/quorum.c
> > +++ b/block/quorum.c
> > @@ -64,11 +64,115 @@ struct QuorumAIOCB {
> > int vote_ret;
> > };
> >
> > +static void quorum_aio_cancel(BlockDriverAIOCB *blockacb)
> > +{
> > + QuorumAIOCB *acb = container_of(blockacb, QuorumAIOCB, common);
> > + BDRVQuorumState *s = acb->bqs;
> > + int i;
> > +
> > + /* cancel all callback */
>
> "callbacks"
>
> > + for (i = 0; i < s->total; i++) {
> > + bdrv_aio_cancel(acb->aios[i].aiocb);
> > + }
> > +}
>
> Don't you want to free acb and similar cleanup?
>
> > +
> > +static AIOCBInfo quorum_aiocb_info = {
> > + .aiocb_size = sizeof(QuorumAIOCB),
> > + .cancel = quorum_aio_cancel,
> > +};
> > +
> > +static void quorum_aio_finalize(QuorumAIOCB *acb)
> > +{
> > + BDRVQuorumState *s = acb->bqs;
>
> block/quorum.c: In function 'quorum_aio_finalize':
> block/quorum.c:86:22: error: unused variable 's' [-Werror=unused-variable]
>
> > + int ret = 0;
> > +
> > + acb->common.cb(acb->common.opaque, ret);
> > + if (acb->finished) {
> > + *acb->finished = true;
> > + }
> > + g_free(acb->aios);
> > + qemu_aio_release(acb);
> > +}
> > +
> > +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->bqs = s;
>
> Noticed it only here, but it's really in patch 2 (and should be in
> patch 1):
>
> What is acb->bqs good for? Isn't it always the same as
> acb->common.bs->opaque?
>
> > + acb->sector_num = sector_num;
> > + acb->nb_sectors = nb_sectors;
> > + acb->qiov = qiov;
> > + acb->aios = g_new0(QuorumSingleAIOCB, s->total);
> > + acb->count = 0;
> > + acb->success_count = 0;
> > + acb->finished = NULL;
> > + acb->is_read = false;
> > + acb->vote_ret = 0;
> > +
> > + for (i = 0; i < s->total; i++) {
> > + acb->aios[i].buf = NULL;
> > + acb->aios[i].ret = 0;
> > + acb->aios[i].parent = acb;
> > + }
> >
> > +
> > + return acb;
> > +}
>
> Kevin
