[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH v5 11/12] qed: Implement .bdrv_drain
From: |
Fam Zheng |
Subject: |
Re: [Qemu-block] [PATCH v5 11/12] qed: Implement .bdrv_drain |
Date: |
Thu, 22 Oct 2015 10:59:20 +0800 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
On Wed, 10/21 22:20, Jeff Cody wrote:
> On Wed, Oct 21, 2015 at 10:06:48AM +0800, Fam Zheng wrote:
> > The "need_check_timer" is used to clear the "NEED_CHECK" flag in the
> > image header after a grace period once metadata update has finished. In
> > compliance to the bdrv_drain semantics we should make sure it remains
> > deleted once .bdrv_drain is called.
> >
> > Call the qed_need_check_timer_cb manually to update the header
> > immediately.
> >
> > Signed-off-by: Fam Zheng <address@hidden>
> > ---
> > block/qed.c | 7 +++++++
> > 1 file changed, 7 insertions(+)
> >
> > diff --git a/block/qed.c b/block/qed.c
> > index 5ea05d4..e9dcb4d 100644
> > --- a/block/qed.c
> > +++ b/block/qed.c
> > @@ -375,6 +375,12 @@ static void
> > bdrv_qed_attach_aio_context(BlockDriverState *bs,
> > }
> > }
> >
> > +static void bdrv_qed_drain(BlockDriverState *bs)
> > +{
> > + qed_cancel_need_check_timer(bs->opaque);
> > + qed_need_check_timer_cb(bs->opaque);
> > +}
> > +
>
> Uh oh.
>
> This causes a segfault sometimes, and other times an abort:
>
>
> # ./qemu-img create -f qed test.qed 512M
> Formatting 'test.qed', fmt=qed size=536870912 cluster_size=65536
>
> # ./qemu-io -c "read 0 512M" test.qed
> read 536870912/536870912 bytes at offset 0
> 512 MiB, 1 ops; 0.0556 sec (8.988 GiB/sec and 17.9759 ops/sec)
> Segmentation fault (core dumped)
>
>
> If I run the above qemu-io command with gdb, it will abort in
> qed_plug_allocating_write_reqs().
>
> I'd hazard a guess (I have not verified) that it is due to the
> qed_header_write() call triggered by the aio flush callback function
> qed_clear_need_check(). The aio flush is done inside the
> qed_need_check_timer_cb() call.
Good catch, I think it's because of the second bdrv_drain in bdrv_close(),
when the first bdrv_aio_flush in qed_need_check_timer_cb hasn't finished.
We need a different bdrv_qed_drain implementation here.
Fam
>
>
>
> > static int bdrv_qed_open(BlockDriverState *bs, QDict *options, int flags,
> > Error **errp)
> > {
> > @@ -1676,6 +1682,7 @@ static BlockDriver bdrv_qed = {
> > .bdrv_check = bdrv_qed_check,
> > .bdrv_detach_aio_context = bdrv_qed_detach_aio_context,
> > .bdrv_attach_aio_context = bdrv_qed_attach_aio_context,
> > + .bdrv_drain = bdrv_qed_drain,
> > };
> >
> > static void bdrv_qed_init(void)
> > --
> > 2.4.3
> >
> >
- Re: [Qemu-block] [PATCH v5 07/12] block: Add "drained begin/end" for transactional backup, (continued)
- [Qemu-block] [PATCH v5 08/12] block: Add "drained begin/end" for transactional blockdev-backup, Fam Zheng, 2015/10/20
- [Qemu-block] [PATCH v5 09/12] block: Add "drained begin/end" for internal snapshot, Fam Zheng, 2015/10/20
- [Qemu-block] [PATCH v5 10/12] block: Introduce BlockDriver.bdrv_drain callback, Fam Zheng, 2015/10/20
- [Qemu-block] [PATCH v5 11/12] qed: Implement .bdrv_drain, Fam Zheng, 2015/10/20
- [Qemu-block] [PATCH v5 12/12] tests: Add test case for aio_disable_external, Fam Zheng, 2015/10/20
- Re: [Qemu-block] [PATCH v5 00/12] block: Protect nested event loop with bdrv_drained_begin and bdrv_drained_end, Kevin Wolf, 2015/10/21