[Qemu-devel] [PATCH] quorum: don't share qiov

[Qemu-devel] [PATCH] quorum: don't share qiov

[Wen Congyang]

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

Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
---
 block/quorum.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/block/quorum.c b/block/quorum.c
index cdc026c..ef0c1e9 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -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]);
     }
-- 
2.1.0

Re: [Qemu-devel] [PATCH] quorum: don't share qiov

[Kevin Wolf]

Am 30.01.2015 um 09:07 hat Wen Congyang geschrieben:
> If the child touches qiov->iov, it will cause unexpected results.
> 
> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>

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

Re: [Qemu-devel] [PATCH] quorum: don't share qiov

[Wen Congyang]

On 01/30/2015 09:39 PM, Kevin Wolf wrote:
> Am 30.01.2015 um 09:07 hat Wen Congyang geschrieben:
>> If the child touches qiov->iov, it will cause unexpected results.
>>
>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
> 
> 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.

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()

Thanks
Wen Congyang

> 
> Kevin
> 

Re: [Qemu-devel] [PATCH] quorum: don't share qiov

[Kevin Wolf]

Am 02.02.2015 um 02:19 hat Wen Congyang geschrieben:
> On 01/30/2015 09:39 PM, Kevin Wolf wrote:
> > Am 30.01.2015 um 09:07 hat Wen Congyang geschrieben:
> >> If the child touches qiov->iov, it will cause unexpected results.
> >>
> >> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
> > 
> > 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.
> 
> 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

Re: [Qemu-devel] [PATCH] quorum: don't share qiov

[Paolo Bonzini]

On 03/02/2015 10:22, Kevin Wolf wrote:
> 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. :)

> 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

Re: [Qemu-devel] [PATCH] quorum: don't share qiov

[Wen Congyang]

On 02/03/2015 07:01 PM, Paolo Bonzini wrote:
> 
> 
> On 03/02/2015 10:22, Kevin Wolf wrote:
>> 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. :)
> 
>> 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.

Hi, kevin

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

Thanks
Wen Congyang

> 
> Paolo
> .
>