| Message ID | 1391464280-25627-4-git-send-email-benoit.canet@irqsave.net |
|---|---|
| State | New |
| Headers | show |
Am 03.02.2014 um 22:51 hat Benoît Canet geschrieben: > From: Benoît Canet <benoit@irqsave.net> > > Signed-off-by: Benoit Canet <benoit@irqsave.net> > --- > 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. > 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
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 <benoit@irqsave.net> > > > > Signed-off-by: Benoit Canet <benoit@irqsave.net> > > --- > > 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
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 */ + for (i = 0; i < s->total; i++) { + bdrv_aio_cancel(acb->aios[i].aiocb); + } +} + +static AIOCBInfo quorum_aiocb_info = { + .aiocb_size = sizeof(QuorumAIOCB), + .cancel = quorum_aio_cancel, +}; + +static void quorum_aio_finalize(QuorumAIOCB *acb) +{ + BDRVQuorumState *s = acb->bqs; + 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; + 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; +} + +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; + } + + quorum_aio_finalize(acb); +} + +static BlockDriverAIOCB *quorum_aio_writev(BlockDriverState *bs, + int64_t sector_num, + QEMUIOVector *qiov, + int nb_sectors, + BlockDriverCompletionFunc *cb, + void *opaque) +{ + BDRVQuorumState *s = bs->opaque; + QuorumAIOCB *acb = quorum_aio_get(s, bs, qiov, sector_num, nb_sectors, + cb, opaque); + int i; + + for (i = 0; i < s->total; i++) { + acb->aios[i].aiocb = bdrv_aio_writev(s->bs[i], sector_num, qiov, + nb_sectors, &quorum_aio_cb, + &acb->aios[i]); + } + + return &acb->common; +} + static BlockDriver bdrv_quorum = { .format_name = "quorum", .protocol_name = "quorum", .instance_size = sizeof(BDRVQuorumState), + + .bdrv_aio_writev = quorum_aio_writev, }; static void bdrv_quorum_init(void)