Re: [Qemu-devel] [PATCH 1/6] block: Keep track of devices' I/O status

[Luiz Capitulino]

On Fri, 23 Sep 2011 17:36:53 +0200
Markus Armbruster <address@hidden> wrote:

> Luiz Capitulino <address@hidden> writes:
> 
> > On Fri, 23 Sep 2011 09:51:24 +0200
> > Markus Armbruster <address@hidden> wrote:
> >
> >> Luiz Capitulino <address@hidden> 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 <address@hidden>
> >> > ---
> >> >  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.
> 
> [...]
>