[Qemu-devel] [PATCH] quorum: Fix crash in quorum_aio_cb()

[Qemu-devel] [PATCH] quorum: Fix crash in quorum_aio_cb()

[Alberto Garcia]

quorum_aio_cb() emits the QUORUM_REPORT_BAD event if there's
an I/O error in a Quorum child. However sacb->aiocb must be
correctly initialized for this to happen. read_quorum_children() and
read_fifo_child() are not doing this, which results in a QEMU crash.

Signed-off-by: Alberto Garcia <berto@igalia.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 block/quorum.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/block/quorum.c b/block/quorum.c
index b9ba028..e640688 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -646,8 +646,9 @@ static BlockAIOCB *read_quorum_children(QuorumAIOCB *acb)
     }
 
     for (i = 0; i < s->num_children; i++) {
-        bdrv_aio_readv(s->children[i]->bs, acb->sector_num, &acb->qcrs[i].qiov,
-                       acb->nb_sectors, quorum_aio_cb, &acb->qcrs[i]);
+        acb->qcrs[i].aiocb = bdrv_aio_readv(s->children[i]->bs, acb->sector_num,
+                                            &acb->qcrs[i].qiov, acb->nb_sectors,
+                                            quorum_aio_cb, &acb->qcrs[i]);
     }
 
     return &acb->common;
@@ -662,9 +663,10 @@ static BlockAIOCB *read_fifo_child(QuorumAIOCB *acb)
     qemu_iovec_init(&acb->qcrs[acb->child_iter].qiov, acb->qiov->niov);
     qemu_iovec_clone(&acb->qcrs[acb->child_iter].qiov, acb->qiov,
                      acb->qcrs[acb->child_iter].buf);
-    bdrv_aio_readv(s->children[acb->child_iter]->bs, acb->sector_num,
-                   &acb->qcrs[acb->child_iter].qiov, acb->nb_sectors,
-                   quorum_aio_cb, &acb->qcrs[acb->child_iter]);
+    acb->qcrs[acb->child_iter].aiocb =
+        bdrv_aio_readv(s->children[acb->child_iter]->bs, acb->sector_num,
+                       &acb->qcrs[acb->child_iter].qiov, acb->nb_sectors,
+                       quorum_aio_cb, &acb->qcrs[acb->child_iter]);
 
     return &acb->common;
 }
-- 
2.7.0

Re: [Qemu-devel] [PATCH] quorum: Fix crash in quorum_aio_cb()

[Wen Congyang]

On 03/10/2016 08:13 PM, Alberto Garcia wrote:
> quorum_aio_cb() emits the QUORUM_REPORT_BAD event if there's
> an I/O error in a Quorum child. However sacb->aiocb must be
> correctly initialized for this to happen. read_quorum_children() and
> read_fifo_child() are not doing this, which results in a QEMU crash.

If we use FIFO mode, we don't call quorum_report_bad() in quorum_aio_cb().
But it is OK to iniialize sacb->aiocb for it.

> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>

Reviewed-by: Wen Congyang <wency@cn.fujitsu.com>

> ---
>  block/quorum.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/block/quorum.c b/block/quorum.c
> index b9ba028..e640688 100644
> --- a/block/quorum.c
> +++ b/block/quorum.c
> @@ -646,8 +646,9 @@ static BlockAIOCB *read_quorum_children(QuorumAIOCB *acb)
>      }
>  
>      for (i = 0; i < s->num_children; i++) {
> -        bdrv_aio_readv(s->children[i]->bs, acb->sector_num, &acb->qcrs[i].qiov,
> -                       acb->nb_sectors, quorum_aio_cb, &acb->qcrs[i]);
> +        acb->qcrs[i].aiocb = bdrv_aio_readv(s->children[i]->bs, acb->sector_num,
> +                                            &acb->qcrs[i].qiov, acb->nb_sectors,
> +                                            quorum_aio_cb, &acb->qcrs[i]);
>      }
>  
>      return &acb->common;
> @@ -662,9 +663,10 @@ static BlockAIOCB *read_fifo_child(QuorumAIOCB *acb)
>      qemu_iovec_init(&acb->qcrs[acb->child_iter].qiov, acb->qiov->niov);
>      qemu_iovec_clone(&acb->qcrs[acb->child_iter].qiov, acb->qiov,
>                       acb->qcrs[acb->child_iter].buf);
> -    bdrv_aio_readv(s->children[acb->child_iter]->bs, acb->sector_num,
> -                   &acb->qcrs[acb->child_iter].qiov, acb->nb_sectors,
> -                   quorum_aio_cb, &acb->qcrs[acb->child_iter]);
> +    acb->qcrs[acb->child_iter].aiocb =
> +        bdrv_aio_readv(s->children[acb->child_iter]->bs, acb->sector_num,
> +                       &acb->qcrs[acb->child_iter].qiov, acb->nb_sectors,
> +                       quorum_aio_cb, &acb->qcrs[acb->child_iter]);
>  
>      return &acb->common;
>  }
> 

Re: [Qemu-devel] [PATCH] quorum: Fix crash in quorum_aio_cb()

[Alberto Garcia]

On Fri 11 Mar 2016 02:31:31 AM CET, Wen Congyang wrote:
> On 03/10/2016 08:13 PM, Alberto Garcia wrote:
>> quorum_aio_cb() emits the QUORUM_REPORT_BAD event if there's
>> an I/O error in a Quorum child. However sacb->aiocb must be
>> correctly initialized for this to happen. read_quorum_children() and
>> read_fifo_child() are not doing this, which results in a QEMU crash.
>
> If we use FIFO mode, we don't call quorum_report_bad() in
> quorum_aio_cb().  But it is OK to iniialize sacb->aiocb for it.

You're right. I still think it's a good idea to leave it initialized in
case we change that in the future.

And now that we're at it, shouldn't we call quorum_report_bad() in FIFO
mode as well? Or is there any reason not to do it?

Berto

Re: [Qemu-devel] [PATCH] quorum: Fix crash in quorum_aio_cb()

[Changlong Xie]

On 03/11/2016 04:25 PM, Alberto Garcia wrote:
> On Fri 11 Mar 2016 02:31:31 AM CET, Wen Congyang wrote:
>> On 03/10/2016 08:13 PM, Alberto Garcia wrote:
>>> quorum_aio_cb() emits the QUORUM_REPORT_BAD event if there's
>>> an I/O error in a Quorum child. However sacb->aiocb must be
>>> correctly initialized for this to happen. read_quorum_children() and
>>> read_fifo_child() are not doing this, which results in a QEMU crash.
>>
>> If we use FIFO mode, we don't call quorum_report_bad() in
>> quorum_aio_cb().  But it is OK to iniialize sacb->aiocb for it.
>

Hi betro

> You're right. I still think it's a good idea to leave it initialized in
> case we change that in the future.

Yes.

>
> And now that we're at it, shouldn't we call quorum_report_bad() in FIFO
> mode as well? Or is there any reason not to do it?

IMO, no reason not to do it.

Thanks
	-Xie

>
> Berto
>
>
>
>

Re: [Qemu-devel] [PATCH] quorum: Fix crash in quorum_aio_cb()

[Alberto Garcia]

On Mon 14 Mar 2016 02:57:31 AM CET, Changlong Xie wrote:
>> And now that we're at it, shouldn't we call quorum_report_bad() in
>> FIFO mode as well? Or is there any reason not to do it?
>
> IMO, no reason not to do it.

I'll send a patch to fix that.

Thanks,

Berto