Re: [Qemu-devel] [PATCH 3/7] block: raw-posix image file reopen

[Kevin Wolf]

Am 30.08.2012 20:47, schrieb Jeff Cody:
> This is derived from the Supriya Kannery's reopen patches.
> 
> This contains the raw-posix driver changes for the bdrv_reopen_*
> functions.  All changes are staged into a temporary scratch buffer
> during the prepare() stage, and copied over to the live structure
> during commit().  Upon abort(), all changes are abandoned, and the
> live structures are unmodified.
> 
> The _prepare() will create an extra fd - either by means of a dup,
> if possible, or opening a new fd if not (for instance, access
> control changes).  Upon _commit(), the original fd is closed and
> the new fd is used.  Upon _abort(), the duplicate/new fd is closed.
> 
> Signed-off-by: Jeff Cody <address@hidden>
> ---
>  block/raw-posix.c | 153 
> +++++++++++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 139 insertions(+), 14 deletions(-)
> 
> diff --git a/block/raw-posix.c b/block/raw-posix.c
> index 6be20b1..48086d7 100644
> --- a/block/raw-posix.c
> +++ b/block/raw-posix.c
> @@ -140,6 +140,15 @@ typedef struct BDRVRawState {
>  #endif
>  } BDRVRawState;
>  
> +typedef struct BDRVRawReopenState {
> +    BDRVReopenState reopen_state;
> +    int fd;
> +    int open_flags;
> +    uint8_t *aligned_buf;
> +    unsigned aligned_buf_size;
> +    BDRVRawState *stash_s;
> +} BDRVRawReopenState;
> +
>  static int fd_open(BlockDriverState *bs);
>  static int64_t raw_getlength(BlockDriverState *bs);
>  
> @@ -185,6 +194,28 @@ static int raw_normalize_devicepath(const char 
> **filename)
>  }
>  #endif
>  
> +static void raw_parse_flags(int bdrv_flags, int *open_flags)
> +{
> +    assert(open_flags != NULL);
> +
> +    *open_flags |= O_BINARY;
> +    *open_flags &= ~O_ACCMODE;
> +    if (bdrv_flags & BDRV_O_RDWR) {
> +        *open_flags |= O_RDWR;
> +    } else {
> +        *open_flags |= O_RDONLY;
> +    }
> +
> +    /* Use O_DSYNC for write-through caching, no flags for write-back 
> caching,
> +     * and O_DIRECT for no caching. */
> +    if ((bdrv_flags & BDRV_O_NOCACHE)) {
> +        *open_flags |= O_DIRECT;
> +    }
> +    if (!(bdrv_flags & BDRV_O_CACHE_WB)) {
> +        *open_flags |= O_DSYNC;
> +    }
> +}

The code motion would ideally be a separate patch.

> +
>  static int raw_open_common(BlockDriverState *bs, const char *filename,
>                             int bdrv_flags, int open_flags)
>  {
> @@ -196,20 +227,8 @@ static int raw_open_common(BlockDriverState *bs, const 
> char *filename,
>          return ret;
>      }
>  
> -    s->open_flags = open_flags | O_BINARY;
> -    s->open_flags &= ~O_ACCMODE;
> -    if (bdrv_flags & BDRV_O_RDWR) {
> -        s->open_flags |= O_RDWR;
> -    } else {
> -        s->open_flags |= O_RDONLY;
> -    }
> -
> -    /* Use O_DSYNC for write-through caching, no flags for write-back 
> caching,
> -     * and O_DIRECT for no caching. */
> -    if ((bdrv_flags & BDRV_O_NOCACHE))
> -        s->open_flags |= O_DIRECT;
> -    if (!(bdrv_flags & BDRV_O_CACHE_WB))
> -        s->open_flags |= O_DSYNC;
> +    s->open_flags = open_flags;
> +    raw_parse_flags(bdrv_flags, &s->open_flags);
>  
>      s->fd = -1;
>      fd = qemu_open(filename, s->open_flags, 0644);
> @@ -283,6 +302,109 @@ static int raw_open(BlockDriverState *bs, const char 
> *filename, int flags)
>      return raw_open_common(bs, filename, flags, 0);
>  }
>  
> +static int raw_reopen_prepare(BDRVReopenState *state, Error **errp)
> +{
> +    BDRVRawState *s;
> +    BDRVRawReopenState *raw_s;
> +    int ret = 0;
> +
> +    assert(state != NULL);
> +    assert(state->bs != NULL);
> +
> +    s = state->bs->opaque;
> +
> +    state->opaque = g_malloc0(sizeof(BDRVRawReopenState));
> +    raw_s = state->opaque;
> +
> +    raw_parse_flags(state->flags, &raw_s->open_flags);
> +
> +    /*
> +     * If we didn't have BDRV_O_NOCACHE set before, we may not have allocated
> +     * aligned_buf
> +     */
> +    if ((state->flags & BDRV_O_NOCACHE)) {
> +        /*
> +         * Allocate a buffer for read/modify/write cycles.  Choose the size
> +         * pessimistically as we don't know the block size yet.
> +         */
> +        raw_s->aligned_buf_size = 32 * MAX_BLOCKSIZE;
> +        raw_s->aligned_buf = qemu_memalign(MAX_BLOCKSIZE,
> +                                           raw_s->aligned_buf_size);
> +
> +        if (raw_s->aligned_buf == NULL) {
> +            ret = -1;
> +            goto error;
> +        }

Even though it's pretty small, I think I would factor this out into a
small static helper to make sure it's kept in sync with raw_open_common().

> +    }
> +
> +    int fcntl_flags = O_APPEND | O_ASYNC | O_NONBLOCK;
> +#ifdef O_NOATIME
> +    fcntl_flags |= O_NOATIME;
> +#endif
> +    if ((raw_s->open_flags & ~fcntl_flags) == (s->open_flags & 
> ~fcntl_flags)) {
> +        /* dup the original fd */
> +        /* TODO: use qemu fcntl wrapper */
> +        raw_s->fd = fcntl(s->fd, F_DUPFD_CLOEXEC, 0);
> +        if (raw_s->fd == -1) {
> +            ret = -1;
> +            goto error;
> +        }
> +        ret = fcntl_setfl(raw_s->fd, raw_s->open_flags);
> +    } else {
> +        raw_s->fd = qemu_open(state->bs->filename, raw_s->open_flags, 0644);
> +        if (raw_s->fd == -1) {
> +            ret = -1;
> +        }

Ignoring this part for now, with qemu_dup_flags() it's going to look a
bit different. In particular, I'm hoping that we don't get a second
fcntl_flags enumeration here, but can just fall back to qemu_open()
whenever qemu_dup_flags() fails.

If we do need to keep fcntl_flags here, we'll probably want to add
O_DIRECT to it.

> +    }
> +error:
> +    return ret;
> +}
> +
> +
> +static void raw_reopen_commit(BDRVReopenState *state)
> +{
> +    BDRVRawReopenState *raw_s = state->opaque;
> +    BDRVRawState *s = state->bs->opaque;
> +
> +    if (raw_s->aligned_buf != NULL) {
> +        if (s->aligned_buf) {
> +            qemu_vfree(s->aligned_buf);
> +        }
> +        s->aligned_buf      = raw_s->aligned_buf;
> +        s->aligned_buf_size = raw_s->aligned_buf_size;
> +    }
> +
> +    s->open_flags = raw_s->open_flags;
> +
> +    close(s->fd);
> +    s->fd = raw_s->fd;
> +
> +    g_free(state->opaque);
> +    state->opaque = NULL;
> +}

I think s->use_aio must be changed as well, it depends on
BDRV_O_NOCACHE. Maybe we even need to do it in prepare, laio_init() may
need to be called.

Kevin