quorum: don't share qiov

PATCH In review Wen Congyang 3 participants 6 messages Jan 30, 2015 – Feb 9, 2015 Block layer

lore

Share & tools

lore

Wen Congyang

author

2015-01-30 08:05 UTC #0 lore raw

PATCH 1/1 quorum: don't share qiov

If the child touches qiov->iov, it will cause unexpected results.

Signed-off-by Wen Congyang <[email protected]>

---
 block/quorum.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

block/quorum.c +9 -1

index cdc026c..ef0c1e9 100644
@@ -165,6 +165,10 @@ static void quorum_aio_finalize(QuorumAIOCB *acb)
             qemu_vfree(acb->qcrs[i].buf);
             qemu_iovec_destroy(&acb->qcrs[i].qiov);
         }
+    } else {
+        for (i = 0; i <= acb->child_iter; i++) {
+            qemu_iovec_destroy(&acb->qcrs[i].qiov);
+        }
     }
 
     g_free(acb->qcrs);

@@ -709,8 +713,12 @@ static BlockAIOCB *quorum_aio_writev(BlockDriverState *bs,
                                       cb, opaque);
     int i;
 
+    acb->child_iter = s->num_children - 1;
     for (i = 0; i < s->num_children; i++) {
-        acb->qcrs[i].aiocb = bdrv_aio_writev(s->bs[i], sector_num, qiov,
+        qemu_iovec_init(&acb->qcrs[i].qiov, acb->qiov->niov);
+        qemu_iovec_concat(&acb->qcrs[i].qiov, acb->qiov, 0, acb->qiov->size);
+        acb->qcrs[i].aiocb = bdrv_aio_writev(s->bs[i], sector_num,
+                                             &acb->qcrs[i].qiov,
                                              nb_sectors, &quorum_aio_cb,
                                              &acb->qcrs[i]);
     }

Signature · 1 line

2.1.0

Kevin Wolf

2015-01-30 13:40 UTC #1 lore raw

Am 30.01.2015 um 09:07 hat Wen Congyang geschrieben:

Quoted 3 lines

If the child touches qiov->iov, it will cause unexpected results.
Signed-off-by Wen Congyang <[email protected]>

Any specific child you're thinking of?

I think children are not supposed to modify their qiov (which would also fail for init_external qiovs). Perhaps we should have made it const.

Kevin

Kevin Wolf

2015-02-03 09:22 UTC #3 lore raw

Am 02.02.2015 um 02:19 hat Wen Congyang geschrieben:

Quoted 11 lines · Kevin Wolf

Quoted 9 lines
Am 30.01.2015 um 09:07 hat Wen Congyang geschrieben:
Quoted 3 lines
If the child touches qiov->iov, it will cause unexpected results.
Signed-off-by Wen Congyang <[email protected]>
Any specific child you're thinking of?
I think children are not supposed to modify their qiov (which would also fail for init_external qiovs). Perhaps we should have made it const.

Quoted 8 lines

NBD client. The qiov will be modified in iov_send_recv():
nbd_co_writev()

    nbd_client_session_co_writev()
        nbd_co_writev_1()
            nbd_co_send_request()
                qemu_co_sendv()
                    qemu_co_sendv_recvv()
                        iov_send_recv()

Paolo, I think it's rather surprising that iov_send_recv() modifies its iov. The modification is undone at the end, so you seem to have considered that a caller might be reusing it after and you can't "use it up", but we still get problems with concurrent accesses.

Was it an intentional design decision that iov_send_recv() is the sole owner of the iov and the caller must duplicate it if it's used elsewhere concurrently?

Otherwise I would suggest to fix iov_send_recv(), and possibly try and make all the qiov/iov arguments in the block layer const.

Kevin

Paolo Bonzini

2015-02-03 11:02 UTC #4 lore raw

Quoted 4 lines · Kevin Wolf

Paolo, I think it's rather surprising that iov_send_recv() modifies its iov. The modification is undone at the end, so you seem to have considered that a caller might be reusing it after and you can't "use it up", but we still get problems with concurrent accesses.

Yes, I wasn't thinking of concurrent accesses indeed. But I wasn't the author of iov_send_recv(), I just took it from sheepdog. :)

Quoted 6 lines

Was it an intentional design decision that iov_send_recv() is the sole owner of the iov and the caller must duplicate it if it's used elsewhere concurrently?
Otherwise I would suggest to fix iov_send_recv(), and possibly try and make all the qiov/iov arguments in the block layer const.

I agree. However, it's not a small change. I think Wen's patch is okay with a FIXME comment added.

Paolo

Wen Congyang

author

2015-02-09 02:08 UTC #5 lore raw

Quoted 15 lines · Paolo Bonzini

Quoted 4 lines · Kevin Wolf
Paolo, I think it's rather surprising that iov_send_recv() modifies its iov. The modification is undone at the end, so you seem to have considered that a caller might be reusing it after and you can't "use it up", but we still get problems with concurrent accesses.
Yes, I wasn't thinking of concurrent accesses indeed. But I wasn't the author of iov_send_recv(), I just took it from sheepdog. :)
Quoted 4 lines
Was it an intentional design decision that iov_send_recv() is the sole owner of the iov and the caller must duplicate it if it's used elsewhere concurrently?

Quoted 5 lines

Quoted 2 lines
Otherwise I would suggest to fix iov_send_recv(), and possibly try and make all the qiov/iov arguments in the block layer const.
I agree. However, it's not a small change. I think Wen's patch is okay with a FIXME comment added.

Hi, kevin

What should I do next? Add a comment and resend it?

Thanks
Wen Congyang

Quoted 4 lines

Paolo
.