Message ID | 54CB3BB4.80406@cn.fujitsu.com |
---|---|
State | New |
Headers | show |
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
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 >
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
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
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 > . >
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]); }
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(-)