Re: [Qemu-devel] [PATCH v2 0/5]: QMP: Proper thin provisioning support

[Luiz Capitulino <lcapitulino@redhat.com>]

On Thu, 04 Aug 2011 11:19:31 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Kevin Wolf <kwolf@redhat.com> writes:
> 
> > Am 03.08.2011 20:08, schrieb Luiz Capitulino:
> >> On Wed, 03 Aug 2011 17:39:04 +0200
> >> Kevin Wolf <kwolf@redhat.com> wrote:
> >> 
> >>> Am 03.08.2011 17:19, schrieb Luiz Capitulino:
> >>>> Roughly speaking, thin provisioning is a feature where the VM is started with
> >>>> a small disk and space is allocated on demand.
> >>>>
> >>>> It's already possible for QMP clients to implement this feature by using
> >>>> the BLOCK_IO_ERROR event. However, the event can be missed. When this
> >>>> happens QMP clients need a way to query if any block device has hit a
> >>>> no space condition.
> >>>>
> >>>> This is what this series is about: it extends the query-block command to
> >>>> contain the disk's I/O status.
> >>>>
> >>>> Please, note that this series depends on the following two other series:
> >>>>
> >>>>  1. [PATCH 00/55] Block layer cleanup & fixes (by Markus)
> >>>>  2. [PATCH 0/7]: Introduce the QemuState type (by me)
> >>>
> >>> It feels strange that device models are involved with this. The block
> >>> layer already knows the error number, it just tells the device model so
> >>> that it can ask it back. Wouldn't it be easier to store it in the
> >>> BlockDriverState?
> >> 
> >> My first version did it, but looking at it now maybe I did it wrong,
> >> because I was setting the iostatus in the devices (instead of setting
> >> it in the block layer itself). Here's an example:
> >> 
> >>  http://lists.nongnu.org/archive/html/qemu-devel/2011-07/msg00421.html
> >> 
> >> But I'm not sure what the best place is. I'm hoping that you and/or
> >> Markus can help me here. Also note that Markus already told us what
> >> he thinks:
> >> 
> >> """
> >> Actually, this is a symptom of the midlayer disease.  I suspect things
> >> would be simpler if we hold the status in its rightful owner, the device
> >> model.  Need a getter for it.  I'm working on a patch series that moves
> >> misplaced state out of the block layer into device models and block
> >> drivers, and a I/O status getter will fit in easily there.
> >> """
> >> 
> >>  http://lists.nongnu.org/archive/html/qemu-devel/2011-07/msg00937.html
> >> 
> >> Can you guys get in agreement?
> >
> > I think you can argue either way, but the main point is that we should
> > clearly assign it to either host or guest state, but not involve both sides.
> 
> Indeed.
> 
> > For me, an ENOSPC is clearly host state, so I would prefer to see it
> > done in the block layer. An I/O status property of a device I would
> > expect to provide whatever the guest sees and that's not an ENOSPC.
> 
> If it can be done in the block layer without involving device models,
> and the status is not guest visible, then it's most probably host state,
> and I retract the stupid things I said earlier :)

ACK.

It's also important to note that I'm considering to do what Christoph
suggested in the other thread: only set ENOSPC. Perhaps, only when it's
set to be reported by the werror option.

This would simplify things immensely. I just have to be sure it would
be sufficient for the use-case.

> 
> >>> Also, as I already commented on v1, I think it's a bad idea to let
> >>> requests that complete later overwrite the error code with "success".
> >>> Quite possible in an ENOSPC case, I think.
> >> 
> >> Really? I'm under the assumption that all pending I/O will be completed
> >> as soon as the vm is stopped. Am I wrong?
> >
> > Yes, vm_stop() will flush all requests. And of course this means that
> > any request that completes successfully during this flush will reset the
> > error value.
> 
> Looks like a show stopper.
> 
> What if multiple different errors occur?  Do we need a set of errors, or
> should the first one stick, or should the most important one (definition
> needed) stick?

Good point. I'd choose to stick with the first one, but this is not an issue
if we implement the idea described above.

> >> What we need here is a way to "reset" the io-status field. That's, the vm will
> >> stop due to ENOSPC, the mngt application will handle it and put the vm to run
> >> again. At this point the io-status has to be reset otherwise it will contain
> >> stale data.
> >> 
> >> By doing the overwrite this "reset" is done automatically as soon as the
> >> vm is put to run again and doesn't hit the same (or other) errors.
> >> 
> >> The other option would be to allow the mngt application to manually reset
> >> the field.
> >> 
> >> More ideas?
> >
> > Maybe we could do it automatically in 'cont'. Not sure if this is too
> > much magic, but if the VM isn't stopped you can't do anything useful
> > with the value anyway.
> 
> Sounds workable to me.

I thought about doing this too, but I feared I'd be coupling unrelated things.

Maybe the block layer could register a vm state handler which resets the
field for all BSs (at vm_start() time)?