On Thu, 04 Aug 2011 11:19:31 +0200 Markus Armbruster <arm...@redhat.com> wrote:
> Kevin Wolf <kw...@redhat.com> writes: > > > Am 03.08.2011 20:08, schrieb Luiz Capitulino: > >> On Wed, 03 Aug 2011 17:39:04 +0200 > >> Kevin Wolf <kw...@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)?
