Re: [Qemu-devel] [PATCH V17 05/12] quorum: Add quorum_aio_readv.

[Kevin Wolf <kwolf@redhat.com>]

Am 15.02.2014 um 02:40 hat Max Reitz geschrieben:
> On 12.02.2014 23:06, Benoît Canet wrote:
> >From: Benoît Canet <benoit@irqsave.net>
> >
> >Add code to do num_children reads in parallel and cleanup the structures
> >afterward.
> 
> "afterwards"
> 
> >Signed-off-by: Benoit Canet <benoit@irqsave.net>
> >Reviewed-by: Max Reitz <mreitz@redhat.com>
> >---
> >  block/quorum.c | 39 ++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 38 insertions(+), 1 deletion(-)
> >
> >diff --git a/block/quorum.c b/block/quorum.c
> >index 197cdca..c7a5d79 100644
> >--- a/block/quorum.c
> >+++ b/block/quorum.c
> >@@ -91,10 +91,18 @@ static AIOCBInfo quorum_aiocb_info = {
> >  static void quorum_aio_finalize(QuorumAIOCB *acb)
> >  {
> >-    int ret = 0;
> >+    BDRVQuorumState *s = acb->common.bs->opaque;
> >+    int i, ret = 0;
> >      acb->common.cb(acb->common.opaque, ret);
> >+    if (acb->is_read) {
> >+        for (i = 0; i < s->num_children; i++) {
> >+            qemu_vfree(acb->qcrs[i].buf);
> >+            qemu_iovec_destroy(&acb->qcrs[i].qiov);
> >+        }
> >+    }
> >+
> >      g_free(acb->qcrs);
> >      qemu_aio_release(acb);
> >  }
> >@@ -149,6 +157,34 @@ static void quorum_aio_cb(void *opaque, int ret)
> >      quorum_aio_finalize(acb);
> >  }
> >+static BlockDriverAIOCB *quorum_aio_readv(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;
> >+
> >+    acb->is_read = true;
> >+
> >+    for (i = 0; i < s->num_children; i++) {
> >+        acb->qcrs[i].buf = qemu_blockalign(s->bs[i], qiov->size);
> >+        qemu_iovec_init(&acb->qcrs[i].qiov, qiov->niov);
> >+        qemu_iovec_clone(&acb->qcrs[i].qiov, qiov, acb->qcrs[i].buf);
> >+    }
> >+
> >+    for (i = 0; i < s->num_children; i++) {
> >+        bdrv_aio_readv(s->bs[i], sector_num, &acb->qcrs[i].qiov, nb_sectors,
> >+                       quorum_aio_cb, &acb->qcrs[i]);
> 
> I know Kevin told you to, but using the child's own QIOV here means
> quorum_aio_readv() won't work after this patch but only after patch
> 6. Just pointing it out, I don't like it, but Kevin told you to, so:

Sorry, but this is the entirely wrong attitude. Just because I am a
maintainer that doesn't mean that I don't get things wrong. When I talk
bullshit (and probably I do more often than others), just point it out.

In this case, I didn't even tell him to do the change, but I asked in
patch 6:

> Why don't you do this from the beginning?

The answer appears to be that the read data isn't copied back to the
original qiov yet, but this happens only in the quorum_copy_qiov() calls
in quorum_vote().

So this is a bug in this patch, which can be fixed either by reverting
to the old state of using the original qiov here, or (perhaps more
nicely) by introducing quorum_copy_qiov() already in this patch.

Kevin