* [Qemu-devel] [PATCH] quorum: don't share qiov
@ 2015-01-30 8:07 Wen Congyang
2015-01-30 13:39 ` Kevin Wolf
0 siblings, 1 reply; 6+ messages in thread
From: Wen Congyang @ 2015-01-30 8:07 UTC (permalink / raw)
To: qemu-devl, Kevin Wolf, Stefan Hajnoczi
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] quorum: don't share qiov
2015-01-30 8:07 [Qemu-devel] [PATCH] quorum: don't share qiov Wen Congyang
@ 2015-01-30 13:39 ` Kevin Wolf
2015-02-02 1:19 ` Wen Congyang
0 siblings, 1 reply; 6+ messages in thread
From: Kevin Wolf @ 2015-01-30 13:39 UTC (permalink / raw)
To: Wen Congyang; +Cc: qemu-devl, Stefan Hajnoczi
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] quorum: don't share qiov
2015-01-30 13:39 ` Kevin Wolf
@ 2015-02-02 1:19 ` Wen Congyang
2015-02-03 9:22 ` Kevin Wolf
0 siblings, 1 reply; 6+ messages in thread
From: Wen Congyang @ 2015-02-02 1:19 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-devl, Stefan Hajnoczi
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] quorum: don't share qiov
2015-02-02 1:19 ` Wen Congyang
@ 2015-02-03 9:22 ` Kevin Wolf
2015-02-03 11:01 ` Paolo Bonzini
0 siblings, 1 reply; 6+ messages in thread
From: Kevin Wolf @ 2015-02-03 9:22 UTC (permalink / raw)
To: Wen Congyang; +Cc: pbonzini, qemu-devl, Stefan Hajnoczi
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] quorum: don't share qiov
2015-02-03 9:22 ` Kevin Wolf
@ 2015-02-03 11:01 ` Paolo Bonzini
2015-02-09 2:10 ` Wen Congyang
0 siblings, 1 reply; 6+ messages in thread
From: Paolo Bonzini @ 2015-02-03 11:01 UTC (permalink / raw)
To: Kevin Wolf, Wen Congyang; +Cc: qemu-devl, Stefan Hajnoczi
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] quorum: don't share qiov
2015-02-03 11:01 ` Paolo Bonzini
@ 2015-02-09 2:10 ` Wen Congyang
0 siblings, 0 replies; 6+ messages in thread
From: Wen Congyang @ 2015-02-09 2:10 UTC (permalink / raw)
To: Kevin Wolf; +Cc: Paolo Bonzini, qemu-devl, Stefan Hajnoczi
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
> .
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-02-09 2:08 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-01-30 8:07 [Qemu-devel] [PATCH] quorum: don't share qiov Wen Congyang
2015-01-30 13:39 ` Kevin Wolf
2015-02-02 1:19 ` Wen Congyang
2015-02-03 9:22 ` Kevin Wolf
2015-02-03 11:01 ` Paolo Bonzini
2015-02-09 2:10 ` Wen Congyang
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.