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 I/O status is only enabled if the drive explicitly
> >> > + * enables it _and_ the VM is configured to stop on errors */
> >> > +bool bdrv_iostatus_is_enabled(const BlockDriverState *bs)
> >> > +{
> >> > + return (bs->iostatus != BDRV_IOS_INVAL &&
> >> > + (bs->on_write_error == BLOCK_ERR_STOP_ENOSPC ||
> >> > + bs->on_write_error == BLOCK_ERR_STOP_ANY ||
> >> > + bs->on_read_error == BLOCK_ERR_STOP_ANY));
> >> > +}
> >> > +
> >> > +void bdrv_iostatus_reset(BlockDriverState *bs)
> >> > +{
> >> > + if (bdrv_iostatus_is_enabled(bs)) {
> >> > + bs->iostatus = BDRV_IOS_OK;
> >> > + }
> >> > +}
> >> > +
> >> > +void bdrv_iostatus_set_err(BlockDriverState *bs, int error)
> >> > +{
> >> > + if (bdrv_iostatus_is_enabled(bs) && bs->iostatus == BDRV_IOS_OK) {
> >> > + bs->iostatus = (abs(error) == ENOSPC) ? BDRV_IOS_ENOSPC :
> >> > + BDRV_IOS_FAILED;
> >> > + }
> >> > +}
> >> > +
> >>
> >> abs(error) feels... unusual.
> >>
> >> If you want to guard against callers passing wrongly signed values, why
> >> not simply assert(error >= 0)?
> >
> > Yes. Actually, I thought that there were existing devices that could pass
> > a positive value (while others passed a negative one), but by taking a look
> > at the code now it seems that all supported devices pass a negative value,
> > so your suggestion makes sense.
> >
> >>
> >> > void
> >> > bdrv_acct_start(BlockDriverState *bs, BlockAcctCookie *cookie, int64_t
> >> > bytes,
> >> > enum BlockAcctType type)
> >> > diff --git a/block.h b/block.h
> >> > index 16bfa0a..de74af0 100644
> >> > --- a/block.h
> >> > +++ b/block.h
> >> > @@ -77,6 +77,15 @@ typedef enum {
> >> > BDRV_ACTION_REPORT, BDRV_ACTION_IGNORE, BDRV_ACTION_STOP
> >> > } BlockMonEventAction;
> >> >
> >> > +typedef enum {
> >> > + BDRV_IOS_INVAL, BDRV_IOS_OK, BDRV_IOS_FAILED, BDRV_IOS_ENOSPC,
> >> > + BDRV_IOS_MAX
> >> > +} BlockIOStatus;
> >>
> >> Why is this in block.h?
> >
> > I followed BlockErrorAction, you suggest block_int.h?
>
> I guess I'd put it in block_int.h, just because I can. No biggie,
> though.
>
> [...]
>
