Re: [Qemu-devel] [PATCH 2/7] block: Framework for reopening files safely

[Jeff Cody]

On 08/30/2012 02:47 PM, Jeff Cody wrote:
> This is based heavily on Supriya Kannery's bdrv_reopen()
> patch series.
> 
> This provides a transactional method to reopen multiple
> images files safely.
> 
> Image files are queue for reopen via bdrv_reopen_queue(), and the
> reopen occurs when bdrv_reopen_multiple() is called.  Changes are
> staged in bdrv_reopen_prepare() and in the equivalent driver level
> functions.  If any of the staged images fails a prepare, then all
> of the images left untouched, and the staged changes for each image
> abandoned.
>

Open question (my assumption is yes):

Is it safe to assume that reopen() should always enable BDRV_O_CACHE_WB
(without affecting enable_write_cache), so as to not undo what was done
by Paolo's commit e1e9b0ac?

> Signed-off-by: Jeff Cody <address@hidden>
> ---
>  block.c       | 226 
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  block.h       |  15 ++++
>  block_int.h   |  13 ++++
>  qemu-common.h |   1 +
>  4 files changed, 255 insertions(+)
> 
> diff --git a/block.c b/block.c
> index e31b76f..9470319 100644
> --- a/block.c
> +++ b/block.c
> @@ -857,6 +857,232 @@ unlink_and_fail:
>      return ret;
>  }
>  
> +/*
> + * Adds a BlockDriverState to a simple queue for an atomic, transactional
> + * reopen of multiple devices.
> + *
> + * bs_queue can either be an existing BlockReopenQueue that has had 
> QSIMPLE_INIT
> + * already performed, or alternatively may be NULL a new BlockReopenQueue 
> will
> + * be created and initialized. This newly created BlockReopenQueue should be
> + * passed back in for subsequent calls that are intended to be of the same
> + * atomic 'set'.
> + *
> + * bs is the BlockDriverState to add to the reopen queue.
> + *
> + * flags contains the open flags for the associated bs
> + *
> + * returns a pointer to bs_queue, which is either the newly allocated
> + * bs_queue, or the existing bs_queue being used.
> + *
> + */
> +BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue,
> +                                    BlockDriverState *bs, int flags)
> +{
> +    assert(bs != NULL);
> +
> +    BlockReopenQueueEntry *bs_entry;
> +    if (bs_queue == NULL) {
> +        bs_queue = g_new0(BlockReopenQueue, 1);
> +        QSIMPLEQ_INIT(bs_queue);
> +    }
> +
> +    if (bs->file) {
> +        bdrv_reopen_queue(bs_queue, bs->file, flags);
> +    }
> +
> +    bs_entry = g_new0(BlockReopenQueueEntry, 1);
> +    QSIMPLEQ_INSERT_TAIL(bs_queue, bs_entry, entry);
> +
> +    bs_entry->state = g_new0(BDRVReopenState, 1);
> +    bs_entry->state->bs = bs;
> +    bs_entry->state->flags = flags;
> +
> +    return bs_queue;
> +}
> +
> +/*
> + * Reopen multiple BlockDriverStates atomically & transactionally.
> + *
> + * The queue passed in (bs_queue) must have been built up previous
> + * via bdrv_reopen_queue().
> + *
> + * Reopens all BDS specified in the queue, with the appropriate
> + * flags.  All devices are prepared for reopen, and failure of any
> + * device will cause all device changes to be abandonded, and intermediate
> + * data cleaned up.
> + *
> + * If all devices prepare successfully, then the changes are committed
> + * to all devices.
> + *
> + */
> +int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, Error **errp)
> +{
> +    int ret = -1;
> +    BlockReopenQueueEntry *bs_entry;
> +    Error *local_err = NULL;
> +
> +    assert(bs_queue != NULL);
> +
> +    bdrv_drain_all();
> +
> +    QSIMPLEQ_FOREACH(bs_entry, bs_queue, entry) {
> +        if (bdrv_reopen_prepare(bs_entry->state, &local_err)) {
> +            error_propagate(errp, local_err);
> +            goto cleanup;
> +        }
> +        bs_entry->prepared = true;
> +    }
> +
> +    /* If we reach this point, we have success and just need to apply the
> +     * changes
> +     */
> +    QSIMPLEQ_FOREACH(bs_entry, bs_queue, entry) {
> +        bdrv_reopen_commit(bs_entry->state);
> +    }
> +
> +    ret = 0;
> +
> +cleanup:
> +    QSIMPLEQ_FOREACH(bs_entry, bs_queue, entry) {
> +        if (ret && bs_entry->prepared) {
> +            bdrv_reopen_abort(bs_entry->state);
> +        }
> +        g_free(bs_entry->state);
> +        g_free(bs_entry);
> +    }
> +    g_free(bs_queue);
> +    return ret;
> +}
> +
> +
> +/* Reopen a single BlockDriverState with the specified flags. */
> +int bdrv_reopen(BlockDriverState *bs, int bdrv_flags, Error **errp)
> +{
> +    int ret = -1;
> +    Error *local_err = NULL;
> +    BlockReopenQueue *queue = bdrv_reopen_queue(NULL, bs, bdrv_flags);
> +
> +    ret = bdrv_reopen_multiple(queue, &local_err);
> +    if (local_err != NULL) {
> +        error_propagate(errp, local_err);
> +    }
> +    return ret;
> +}
> +
> +
> +/*
> + * Prepares a BlockDriverState for reopen. All changes are staged in the
> + * 'reopen_state' field of the BlockDriverState, which must be NULL when
> + * entering (all previous reopens must have completed for the BDS).
> + *
> + * bs is the BlockDriverState to reopen
> + * flags are the new open flags
> + *
> + * Returns 0 on success, non-zero on error.  On error errp will be set
> + * as well.
> + *
> + * On failure, bdrv_reopen_abort() will be called to clean up any data.
> + * It is the responsibility of the caller to then call the abort() or
> + * commit() for any other BDS that have been left in a prepare() state
> + *
> + */
> +int bdrv_reopen_prepare(BDRVReopenState *reopen_state, Error **errp)
> +{
> +    int ret = -1;
> +    Error *local_err = NULL;
> +    BlockDriver *drv;
> +
> +    assert(reopen_state != NULL);
> +    assert(reopen_state->bs->drv != NULL);
> +    drv = reopen_state->bs->drv;
> +
> +    /* if we are to stay read-only, do not allow permission change
> +     * to r/w */
> +    if (reopen_state->bs->keep_read_only &&
> +        reopen_state->flags & BDRV_O_RDWR) {
> +        error_set(errp, QERR_DEVICE_IS_READ_ONLY,
> +                  reopen_state->bs->device_name);
> +        goto error;
> +    }
> +
> +
> +    ret = bdrv_flush(reopen_state->bs);
> +    if (ret) {
> +        error_set(errp, QERR_IO_ERROR);
> +        goto error;
> +    }
> +
> +    if (drv->bdrv_reopen_prepare) {
> +        ret = drv->bdrv_reopen_prepare(reopen_state, &local_err);
> +        if (ret) {
> +            if (local_err != NULL) {
> +                error_propagate(errp, local_err);
> +            } else {
> +                error_set(errp, QERR_OPEN_FILE_FAILED,
> +                          reopen_state->bs->filename);
> +            }
> +            goto error;
> +        }
> +    } else {
> +        /* It is currently mandatory to have a bdrv_reopen_prepare()
> +         * handler for each supported drv. */
> +        error_set(errp, QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED,
> +                  drv->format_name, reopen_state->bs->device_name,
> +                 "reopening of file");
> +        ret = -1;
> +        goto error;
> +    }
> +
> +    return 0;
> +
> +error:
> +    bdrv_reopen_abort(reopen_state);
> +    return ret;
> +}
> +
> +/*
> + * Takes the staged changes for the reopen from bdrv_reopen_prepare(), and
> + * makes them final by swapping the staging BlockDriverState contents into
> + * the active BlockDriverState contents.
> + */
> +void bdrv_reopen_commit(BDRVReopenState *reopen_state)
> +{
> +    BlockDriver *drv;
> +
> +    assert(reopen_state != NULL);
> +    drv = reopen_state->bs->drv;
> +    assert(drv != NULL);
> +
> +    /* If there are any driver level actions to take */
> +    if (drv->bdrv_reopen_commit) {
> +        drv->bdrv_reopen_commit(reopen_state);
> +    }
> +
> +    /* set BDS specific flags now */
> +    reopen_state->bs->open_flags         = reopen_state->flags;
> +    reopen_state->bs->enable_write_cache = !!(reopen_state->flags &
> +                                              BDRV_O_CACHE_WB);
> +    reopen_state->bs->read_only = !(reopen_state->flags & BDRV_O_RDWR);
> +}
> +
> +/*
> + * Abort the reopen, and delete and free the staged changes in
> + * reopen_state
> + */
> +void bdrv_reopen_abort(BDRVReopenState *reopen_state)
> +{
> +    BlockDriver *drv;
> +
> +    assert(reopen_state != NULL);
> +    drv = reopen_state->bs->drv;
> +    assert(drv != NULL);
> +
> +    if (drv->bdrv_reopen_abort) {
> +        drv->bdrv_reopen_abort(reopen_state);
> +    }
> +}
> +
> +
>  void bdrv_close(BlockDriverState *bs)
>  {
>      bdrv_flush(bs);
> diff --git a/block.h b/block.h
> index 4d919c2..db812b1 100644
> --- a/block.h
> +++ b/block.h
> @@ -97,6 +97,14 @@ typedef enum {
>      BDRV_ACTION_REPORT, BDRV_ACTION_IGNORE, BDRV_ACTION_STOP
>  } BlockQMPEventAction;
>  
> +typedef struct BlockReopenQueueEntry {
> +     bool prepared;
> +     BDRVReopenState *state;
> +     QSIMPLEQ_ENTRY(BlockReopenQueueEntry) entry;
> +} BlockReopenQueueEntry;
> +
> +typedef QSIMPLEQ_HEAD(BlockReopenQueue, BlockReopenQueueEntry) 
> BlockReopenQueue;
> +
>  void bdrv_iostatus_enable(BlockDriverState *bs);
>  void bdrv_iostatus_reset(BlockDriverState *bs);
>  void bdrv_iostatus_disable(BlockDriverState *bs);
> @@ -131,6 +139,13 @@ int bdrv_parse_cache_flags(const char *mode, int *flags);
>  int bdrv_file_open(BlockDriverState **pbs, const char *filename, int flags);
>  int bdrv_open(BlockDriverState *bs, const char *filename, int flags,
>                BlockDriver *drv);
> +BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue,
> +                                    BlockDriverState *bs, int flags);
> +int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, Error **errp);
> +int bdrv_reopen(BlockDriverState *bs, int bdrv_flags, Error **errp);
> +int bdrv_reopen_prepare(BDRVReopenState *reopen_state, Error **errp);
> +void bdrv_reopen_commit(BDRVReopenState *reopen_state);
> +void bdrv_reopen_abort(BDRVReopenState *reopen_state);
>  void bdrv_close(BlockDriverState *bs);
>  int bdrv_attach_dev(BlockDriverState *bs, void *dev);
>  void bdrv_attach_dev_nofail(BlockDriverState *bs, void *dev);
> diff --git a/block_int.h b/block_int.h
> index 4452f6f..7a4e226 100644
> --- a/block_int.h
> +++ b/block_int.h
> @@ -139,6 +139,12 @@ struct BlockDriver {
>      int instance_size;
>      int (*bdrv_probe)(const uint8_t *buf, int buf_size, const char 
> *filename);
>      int (*bdrv_probe_device)(const char *filename);
> +
> +    /* For handling image reopen for split or non-split files */
> +    int (*bdrv_reopen_prepare)(BDRVReopenState *reopen_state, Error **errp);
> +    void (*bdrv_reopen_commit)(BDRVReopenState *reopen_state);
> +    void (*bdrv_reopen_abort)(BDRVReopenState *reopen_state);
> +
>      int (*bdrv_open)(BlockDriverState *bs, int flags);
>      int (*bdrv_file_open)(BlockDriverState *bs, const char *filename, int 
> flags);
>      int (*bdrv_read)(BlockDriverState *bs, int64_t sector_num,
> @@ -336,6 +342,13 @@ struct BlockDriverState {
>  
>      /* long-running background operation */
>      BlockJob *job;
> +
> +};
> +
> +struct BDRVReopenState {
> +    BlockDriverState *bs;
> +    int flags;
> +    void *opaque;
>  };
>  
>  int get_tmp_filename(char *filename, int size);
> diff --git a/qemu-common.h b/qemu-common.h
> index e5c2bcd..6a6181c 100644
> --- a/qemu-common.h
> +++ b/qemu-common.h
> @@ -245,6 +245,7 @@ typedef struct NICInfo NICInfo;
>  typedef struct HCIInfo HCIInfo;
>  typedef struct AudioState AudioState;
>  typedef struct BlockDriverState BlockDriverState;
> +typedef struct BDRVReopenState BDRVReopenState;
>  typedef struct DriveInfo DriveInfo;
>  typedef struct DisplayState DisplayState;
>  typedef struct DisplayChangeListener DisplayChangeListener;
>