On Mon, 26 Sep 2011 11:34:34 +0200
Markus Armbruster <arm...@redhat.com> wrote:
> Luiz Capitulino <lcapitul...@redhat.com> writes:
>
> > On Fri, 23 Sep 2011 17:36:53 +0200
> > Markus Armbruster <arm...@redhat.com> wrote:
> >
> >> Luiz Capitulino <lcapitul...@redhat.com> writes:
> >>
> >> > On Fri, 23 Sep 2011 09:51:24 +0200
> >> > Markus Armbruster <arm...@redhat.com> wrote:
> >> >
> >> >> Luiz Capitulino <lcapitul...@redhat.com> writes:
> >> >>
> >> >> > This commit adds support to the BlockDriverState type to keep track
> >> >> > of devices' I/O status.
> >> >> >
> >> >> > There are three possible status: BDRV_IOS_OK (no error),
> >> >> > BDRV_IOS_ENOSPC
> >> >> > (no space error) and BDRV_IOS_FAILED (any other error). The
> >> >> > distinction
> >> >> > between no space and other errors is important because a management
> >> >> > application may want to watch for no space in order to extend the
> >> >> > space assigned to the VM and put it to run again.
> >> >> >
> >> >> > Qemu devices supporting the I/O status feature have to enable it
> >> >> > explicitly by calling bdrv_iostatus_enable() _and_ have to be
> >> >> > configured to stop the VM on errors (ie. werror=stop|enospc or
> >> >> > rerror=stop).
> >> >> >
> >> >> > In case of multiple errors being triggered in sequence only the first
> >> >> > one is stored. The I/O status is always reset to BDRV_IOS_OK when the
> >> >> > 'cont' command is issued.
> >> >> >
> >> >> > Next commits will add support to some devices and extend the
> >> >> > query-block/info block commands to return the I/O status information.
> >> >> >
> >> >> > Signed-off-by: Luiz Capitulino <lcapitul...@redhat.com>
> >> >> > ---
> >> >> > block.c | 32 ++++++++++++++++++++++++++++++++
> >> >> > block.h | 9 +++++++++
> >> >> > block_int.h | 1 +
> >> >> > monitor.c | 6 ++++++
> >> >> > 4 files changed, 48 insertions(+), 0 deletions(-)
> >> >> >
> >> >> > diff --git a/block.c b/block.c
> >> >> > index e3fe97f..fbd90b4 100644
> >> >> > --- a/block.c
> >> >> > +++ b/block.c
> >> >> > @@ -221,6 +221,7 @@ BlockDriverState *bdrv_new(const char
> >> >> > *device_name)
> >> >> > if (device_name[0] != '\0') {
> >> >> > QTAILQ_INSERT_TAIL(&bdrv_states, bs, list);
> >> >> > }
> >> >> > + bs->iostatus = BDRV_IOS_INVAL;
> >> >> > return bs;
> >> >> > }
> >> >> >
> >> >> > @@ -3181,6 +3182,37 @@ int bdrv_in_use(BlockDriverState *bs)
> >> >> > return bs->in_use;
> >> >> > }
> >> >> >
> >> >> > +void bdrv_iostatus_enable(BlockDriverState *bs)
> >> >> > +{
> >> >> > + assert(bs->iostatus == BDRV_IOS_INVAL);
> >> >> > + bs->iostatus = BDRV_IOS_OK;
> >> >> > +}
> >> >>
> >> >> bdrv_new() creates the BDS with I/O status tracking disabled. Devices
> >> >> that do tracking declare that by calling this function during
> >> >> initialization. Enables tracking if the BDS has suitable on_write_error
> >> >> and on_read_error.
> >> >>
> >> >> If a device gets hot unplugged, tracking remains enabled. If the BDS
> >> >> then gets reused with a device that doesn't do tracking, I/O status
> >> >> becomes incorrect. Can't happen right now, because we automatically
> >> >> delete the BDS on hot unplug, but it's a trap.
> >> >>
> >> >> Suggest to disable tracking in bdrv_detach_dev() or bdrv_attach_dev().
> >> >
> >> > Ok, and in bdrv_close() as Zhi Yong said.
> >>
> >> Disabling in bdrv_close() makes the status go away on eject. Makes some
> >> sense, in a way. Also complicates the simple rule "status persists from
> >> stop to cont". Hmm, consider the following race:
> >>
> >> 1. Management app sends eject command
> >>
> >> 2. qemu gets read error on the same drive, VM stops, I/O status set,
> >> QEVENT_BLOCK_IO_ERROR sent to management app.
> >>
> >> 3. qemu receives eject command, bdrv_close() resets I/O status
> >>
> >> 4. Management receives QEVENT_BLOCK_IO_ERROR, and would like to find out
> >> which drive caused it.
> >>
> >> If 2 happens before 3, I/O status is lost.
> >
> > Honestly speaking, I didn't intend to make it persistent on eject. Do we
> > really
> > want to support this? I find it a bit confusing to have the I/O status on
> > something that has no media. In that case we should only return the
> > io-status
> > info if the media is inserted.
> >
> > This way, if the VM stops due to an I/O error and the media is ejected, a
> > mngt
> > application would:
> >
> > 1. Issue the query-block and find no guilt
> > 2. Issue cont
> > 3. The VM would execute normally
> >
> > Of course that we have to be clear about it in the documentation and a
> > mngt app has to be prepared to deal with it.
>
> The management app may well want to know that we hit errors on the
> media, even when the error happens close to an eject. In particular,
> they probably want to alert their users on write errors. Just because
> you eject a medium doesn't mean you're no longer interested in the data
> on it ;)
Ok, so I'll have to reset the I/O status on bdrv_open() too, so that
a new inserted media won't contain the previous media status.
