qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Markus Armbruster
Subject: Re: [Qemu-devel] [PATCH 1/6] block: Keep track of devices' I/O status
Date: Fri, 23 Sep 2011 09:51:24 +0200
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/23.2 (gnu/linux)

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().

> +
> +/* 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)?

>  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?

> +
> +void bdrv_iostatus_enable(BlockDriverState *bs);
> +void bdrv_iostatus_reset(BlockDriverState *bs);
> +bool bdrv_iostatus_is_enabled(const BlockDriverState *bs);
> +void bdrv_iostatus_set_err(BlockDriverState *bs, int error);
>  void bdrv_mon_event(const BlockDriverState *bdrv,
>                      BlockMonEventAction action, int is_read);
>  void bdrv_info_print(Monitor *mon, const QObject *data);
> diff --git a/block_int.h b/block_int.h
> index 8c3b863..f2f4f2d 100644
> --- a/block_int.h
> +++ b/block_int.h
> @@ -199,6 +199,7 @@ struct BlockDriverState {
>         drivers. They are not used by the block driver */
>      int cyls, heads, secs, translation;
>      BlockErrorAction on_read_error, on_write_error;
> +    BlockIOStatus iostatus;
>      char device_name[32];
>      unsigned long *dirty_bitmap;
>      int64_t dirty_count;
> diff --git a/monitor.c b/monitor.c
> index 8ec2c5e..88d8228 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -1304,6 +1304,11 @@ struct bdrv_iterate_context {
>      int err;
>  };
>  
> +static void iostatus_bdrv_it(void *opaque, BlockDriverState *bs)
> +{
> +    bdrv_iostatus_reset(bs);
> +}
> +
>  /**
>   * do_cont(): Resume emulation.
>   */
> @@ -1320,6 +1325,7 @@ static int do_cont(Monitor *mon, const QDict *qdict, 
> QObject **ret_data)
>          return -1;
>      }
>  
> +    bdrv_iterate(iostatus_bdrv_it, NULL);
>      bdrv_iterate(encrypted_bdrv_it, &context);
>      /* only resume the vm if all keys are set and valid */
>      if (!context.err) {



reply via email to

[Prev in Thread] Current Thread [Next in Thread]