[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [Qemu-devel] [PATCH v4 13/13] block/mirror: Block "devi
From: |
Fam Zheng |
Subject: |
Re: [Qemu-block] [Qemu-devel] [PATCH v4 13/13] block/mirror: Block "device IO" during mirror exit |
Date: |
Wed, 20 May 2015 10:23:06 +0800 |
User-agent: |
Mutt/1.5.23 (2014-03-12) |
On Tue, 05/19 12:57, Paolo Bonzini wrote:
>
>
> On 19/05/2015 20:37, Fam Zheng wrote:
> > On Tue, 05/19 10:49, Paolo Bonzini wrote:
> >>
> >>
> >> On 19/05/2015 18:48, Fam Zheng wrote:
> >>>>>
> >>>>> This is too late. As a rule, the blocker must be established before
> >>>>> calling bdrv_drain, and removed on the next yield (in this case, before
> >>>>> the assignment to last_pause_ns).
> >>> I don't understand. If the blocker is removed before mirror_run returns,
> >>> wouldn't more device IO already hit source image by the time mirror_exit
> >>> runs?
> >>
> >> If you go to mirror_exit, you won't reach the assignment (so you have to
> >> remove the blocker in mirror_exit too).
> >>
> >> But if you don't go to mirror_exit because cnt != 0, you must remove the
> >> blocker before the next I/O.
> >>
> >
> > OK, but I'm still not clear how is it too late in this patch? Although the
> > blocker is set after bdrv_drain, we know there is no dirty data because cnt
> > is
> > 0, and we'll be holding a blocker when releasing the AioContext, no new IO
> > is
> > allowed.
>
> So you rely on the caller of mirror_run not calling aio_context_release
> between bdrv_drain and block_job_defer_to_main_loop? That indeed should
> work, but why not stick to a common pattern of blocking I/O before
> bdrv_drain? That's how bdrv_drain is almost always used in the code, so
> it's a safe thing to do and the preemption points are then documented
> more clearly.
>
> I think it would be nice to have all bdrv_drain calls:
>
> - either preceded by a device I/O blocker
>
> - or preceded by a comment explaining why the call is there and why it
> doesn't need the blocker
>
> This is not a NACK, but I would like to understand the disadvantages of
> what I am suggesting here.
That makes sense now, I was just trying to get your idea. Of course the patten
as you suggested is more advanced!
Fam
- [Qemu-block] [PATCH v4 06/13] virtio-scsi-dataplane: Add "device IO" op blocker listener, (continued)
- [Qemu-block] [PATCH v4 06/13] virtio-scsi-dataplane: Add "device IO" op blocker listener, Fam Zheng, 2015/05/18
- [Qemu-block] [PATCH v4 12/13] block: Block "device IO" during bdrv_drain and bdrv_drain_all, Fam Zheng, 2015/05/18
- [Qemu-block] [PATCH v4 01/13] block: Add op blocker type "device IO", Fam Zheng, 2015/05/18
- [Qemu-block] [PATCH v4 11/13] blockdev: Block device IO during blockdev-backup transaction, Fam Zheng, 2015/05/18
- [Qemu-block] [PATCH v4 13/13] block/mirror: Block "device IO" during mirror exit, Fam Zheng, 2015/05/18
[Qemu-block] [PATCH v4 10/13] blockdev: Block device IO during drive-backup transaction, Fam Zheng, 2015/05/18