Re: [Qemu-devel] [RFC PATCH] qemu-io: add drain/undrain cmd

[Kevin Wolf]

Am 15.05.2017 um 14:52 hat Fam Zheng geschrieben:
> On Mon, 05/15 14:32, Peter Lieven wrote:
> > Am 15.05.2017 um 14:28 schrieb Fam Zheng:
> > > On Mon, 05/15 13:58, Peter Lieven wrote:
> > > > Am 15.05.2017 um 13:53 schrieb Fam Zheng:
> > > > > On Mon, 05/15 13:26, Peter Lieven wrote:
> > > > > > Am 15.05.2017 um 12:50 schrieb Fam Zheng:
> > > > > > > On Mon, 05/15 12:02, Peter Lieven wrote:
> > > > > > > > Hi Block developers,
> > > > > > > > 
> > > > > > > > I would like to add a feature to Qemu to drain all traffic from 
> > > > > > > > a block so that
> > > > > > > > I can take external snaphosts without the risk to that in the 
> > > > > > > > middle of a write
> > > > > > > > operation. Its meant for cases where where QGA freeze/thaw is 
> > > > > > > > not available.
> > > > > > > > 
> > > > > > > > For me its enough to have this through qemu-io, but Kevin asked 
> > > > > > > > me to check
> > > > > > > > if its not worth to have a stable API for it and present it via 
> > > > > > > > QMP/HMP.
> > > > > > > > 
> > > > > > > > What are your thoughts?
> > > > > > > For debugging purpose or a "hacky" usage where you know what you 
> > > > > > > are doing, it
> > > > > > > may be fine to have this. The only issue is it should be a 
> > > > > > > separate flag, like
> > > > > > > BlockJob.user_paused.
> > > > > > How can I add, remove such a flag?
> > > > > Like bs->user_drained. Set it in "drain" command, then increment
> > > > > bs->quiesce_counter if toggled; vise versa.
> > > > Ah okay. You wouldn't use bdrv_drained_begin/end? Because in these 
> > > > functions
> > > > the counter is incremented already.
> > > You're right, calling bdrv_drained_begin() is better.
> > > 
> > > > 
> > > > 
> > > > > > > What happens from guest perspective? In the case of virtio, the 
> > > > > > > request queue is
> > > > > > > not handled and -ETIMEDOUT may happen. With IDE, I/O commands are 
> > > > > > > still handled,
> > > > > > > the command is not effective (or rather the implementation is not 
> > > > > > > complete).
> > > > > > That it only works with virtio is fine. However, the thing it does 
> > > > > > not work correctly
> > > > > > apply then also to all other users of the drained_begin/end 
> > > > > > functions, right?
> > > > > > As for the timeout I only plan to drain the device for about 1 
> > > > > > second.
> > > > > It didn't matter because for IDE, the invariant (staying quiesced as 
> > > > > long as
> > > > > necessary) is already ensured by BQL.  Virtio is different because it 
> > > > > supports
> > > > > ioeventfd and data plane.
> > > > Okay understood. So my use of bdrv_drained_begin/end is more an abuse of
> > > > these functions?
> > > Sort of. But it's not unreasonable to "extend" bdrv_drained_begin/end to 
> > > cover
> > > IDE, I just haven't thought about "how".
> > > 
> > > > Do you have another idea how to achieve what I want? I was thinking of 
> > > > throttle
> > > > the I/O to zero. It would be enough to do this for writes, reading 
> > > > doesn't hurt in
> > > > my case.
> > > Maybe add a block filter on top of the drained node, drain it when doing 
> > > so,
> > > then queue all further requests with a CoQueue until "undrain".  (It is 
> > > then not
> > > quite to "drain" but to "halt" or "pause", though.)
> > 
> > To get the drain for free was why I was looking at this approach. If I read 
> > correctly
> > if I keep using bdrv_drained_begin/end its too hacky to implement it in QMP?
> 
> I think so.
> 
> > If yes, would support adding it to qemu-io?
> 
> I'm under the impression that you are looking to a real use case, I don't 
> think
> I like the idea. Also, accessing the image from other processes while QEMU is
> using it is strongly discouraged, and there is the coming image locking
> mechanism to prevent this from happening.

Thinking a bit more about this, it looks to me as if what we really want
is inactivating the image. Should we add an option to the 'stop' command
(or introduce another command to be used on an already stopped VM) that
inactivates all/some images? And then 'cont' regains control over the
images, just like after migration.

Automatically stopping the VM while the snapshot is taken also makes it
work with IDE and prevents guests running into timeouts, which makes it
look much more like a proper solution to me.

But then, stop/cont would already achieve something very similar today
(as 'stop' calls bdrv_drain_all(); just the locking part wouldn't be
covered), so maybe there is a reason not to use it in your case, Peter?

Kevin