[Qemu-devel] [PATCH v3 00/19] block: bdrv_reopen() patches

[Qemu-devel] [PATCH v3 00/19] block: bdrv_reopen() patches

[Jeff Cody]

These patches are based off Supriya Kannery's original bdrv_reopen()
patches as part of the hostcache series.

This provides support for safe reopen of a single image, or transactional
reopening of multiple images atomically.


Changes form v2 -> v3:

Patch 03/19: [Paolo] BlockReopenQueue is now only passed to the _prepare
                     functions.
Patch 06/19: [Kevin, Paolo] New. Purge imporper usage of BDRV_O_CACHE_WB.
Patch 07/19: [Paolo] New. Use BDRV_O_NOCACHE instead of align_buf.
Patch 08/19: [Paolo] New. Purge aligned_buf and aligned_buf_size.
Patch 09/19: [Paolo] reopen no longer uses aligned_buf. BlockReopenQueue is
                     now passed only to the _prepare() functions.
Patch 10/19 - 14/19: [Paolo] Removed unneeded _commit() and _abort() stubs
Patch 15/19: [Paolo] BlockReopenQueue is now only passed to the _prepare
                     functions.
Patch 16/19: [Paolo] New.  Support new image format.
Patch 17/19: [Paolo] New.  Support new image format.



Changes from v1 -> v2:

Patch 01/16: None

Patch 02/16: New patch

Patch 03/16: [Kevin] Use QSIMPLEQ_FOREACH_SAFE
             [Kevin] Use BDRV_O_ALLOW_RDWR flag instead of keep_read_only
             [Kevin] Preserve error on failure of bdrv_flush()
             [Kevin] bdrv_reopen_prepare() no longer calls bdrv_reopen_abort()
             [Kevin] Directly embed BDRVReopenState field in
                     BlockReopenQueueEntry, rather than by reference
             [Jeff]  Add BlockReopenQueue field to the BDRVReopenState struct
 
             Feedback not incorporated:
             --------------------------
             [Kevin] Sharing 3 of the BDS flag setting lines with bdrv_open(). 
                     I didn't see a great way of doing this.

Patch 04/16: New patch, aio init code motion 

Patch 05/16: [Kevin] New patch, raw_parse_flags code motion 

Patch 06/16: [Kevin] New patch.  Do not parse BDRV_O_CACHE_WB in
                     raw_posix/raw_win32

Patch 07/16: [Kevin] New patch. Code motion - move aligned_buf allocation
                     to helper function.

Patch 08/16: [Kevin] See patches 4-7
             [Kevin] Init AIO, if appropriate
             [Kevin] Fallback to qemu_open, if fcntl fails
             [Eric]  Remove spurious mode in qemu_open()
             [Corey] Use qemu_close() instead of qemu_open()

             Feedback not incorporated:
             --------------------------
             Moving from fcntl + fcntl_setfl to qemu_dup_flags or equivalent.
             Ideally I think a separate patch series creating a wrapper
             function for fcntl, and then update this code with the new wrapper.
            
Patch 09/16: None
Patch 10/16: None
Patch 11/16: None
Patch 12/16: None

Patch 13/16: New patch, VMDK driver for reopen

Patch 14/16: New patch, raw-win32 driver for reopen.  Note, not necessarily 
             safe.  If anyone knows a better way to do it under win32, without
             potentially having to close the handle first, please let me know.

Patch 15/16: New patch, move bdrv_commit to use bdrv_reopen()

Patch 16/16: [Kevin] Get rid of keep_read_only flag completely

Jeff Cody (19):
  block: correctly set the keep_read_only flag
  block: make bdrv_set_enable_write_cache() modify open_flags
  block: Framework for reopening files safely
  block: move aio initialization into a helper function
  block: move open flag parsing in raw block drivers to helper
    functions
  block: do not parse BDRV_O_CACHE_WB in block drivers
  block: use BDRV_O_NOCACHE instead of s->aligned_buf in raw-posix.c
  block: purge s->aligned_buf and s->aligned_buf_size from raw-posix.c
  block: raw-posix image file reopen
  block: raw image file reopen
  block: qed image file reopen
  block: qcow2 image file reopen
  block: qcow image file reopen
  block: vmdk image file reopen
  block: raw-win32 driver reopen support
  block: vdi image file reopen
  block: vpc image file reopen
  block: convert bdrv_commit() to use bdrv_reopen()
  block: remove keep_read_only flag from BlockDriverState struct

 block.c           | 299 +++++++++++++++++++++++++++++++++++++++++++++---------
 block.h           |  18 ++++
 block/iscsi.c     |   4 -
 block/qcow.c      |  10 ++
 block/qcow2.c     |  10 ++
 block/qed.c       |   9 ++
 block/raw-posix.c | 212 ++++++++++++++++++++++++++++----------
 block/raw-win32.c | 145 ++++++++++++++++++++++----
 block/raw.c       |  10 ++
 block/rbd.c       |   6 --
 block/sheepdog.c  |  14 ++-
 block/vdi.c       |   7 ++
 block/vmdk.c      |  35 +++++++
 block/vpc.c       |   7 ++
 block_int.h       |   9 +-
 qemu-common.h     |   1 +
 16 files changed, 652 insertions(+), 144 deletions(-)

-- 
1.7.11.4

[Qemu-devel] [PATCH v3 01/19] block: correctly set the keep_read_only flag

[Jeff Cody]

I believe the bs->keep_read_only flag is supposed to reflect
the initial open state of the device. If the device is initially
opened R/O, then commit operations, or reopen operations changing
to R/W, are prohibited.

Currently, the keep_read_only flag is only accurate for the active
layer, and its backing file. Subsequent images end up always having
the keep_read_only flag set.

For instance, what happens now:

[  base  ]  kro = 1, ro = 1
    |
    v
[ snap-1 ]  kro = 1, ro = 1
    |
    v
[ snap-2 ]  kro = 0, ro = 1
    |
    v
[ active ]  kro = 0, ro = 0

What we want:

[  base  ]  kro = 0, ro = 1
    |
    v
[ snap-1 ]  kro = 0, ro = 1
    |
    v
[ snap-2 ]  kro = 0, ro = 1
    |
    v
[ active ]  kro = 0, ro = 0

Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block.c | 14 +++++++-------
 block.h |  1 +
 2 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/block.c b/block.c
index 470bdcc..c3eb585 100644
--- a/block.c
+++ b/block.c
@@ -664,7 +664,7 @@ static int bdrv_open_common(BlockDriverState *bs, const char *filename,
         open_flags |= BDRV_O_RDWR;
     }
 
-    bs->keep_read_only = bs->read_only = !(open_flags & BDRV_O_RDWR);
+    bs->read_only = !(open_flags & BDRV_O_RDWR);
 
     /* Open the image, either directly or using a protocol */
     if (drv->bdrv_file_open) {
@@ -804,6 +804,12 @@ int bdrv_open(BlockDriverState *bs, const char *filename, int flags,
         goto unlink_and_fail;
     }
 
+    if (flags & BDRV_O_RDWR) {
+        flags |= BDRV_O_ALLOW_RDWR;
+    }
+
+    bs->keep_read_only = !(flags & BDRV_O_ALLOW_RDWR);
+
     /* Open the image */
     ret = bdrv_open_common(bs, filename, flags, drv);
     if (ret < 0) {
@@ -833,12 +839,6 @@ int bdrv_open(BlockDriverState *bs, const char *filename, int flags,
             bdrv_close(bs);
             return ret;
         }
-        if (bs->is_temporary) {
-            bs->backing_hd->keep_read_only = !(flags & BDRV_O_RDWR);
-        } else {
-            /* base image inherits from "parent" */
-            bs->backing_hd->keep_read_only = bs->keep_read_only;
-        }
     }
 
     if (!bdrv_key_required(bs)) {
diff --git a/block.h b/block.h
index 2e2be11..4d919c2 100644
--- a/block.h
+++ b/block.h
@@ -80,6 +80,7 @@ typedef struct BlockDevOps {
 #define BDRV_O_COPY_ON_READ 0x0400 /* copy read backing sectors into image */
 #define BDRV_O_INCOMING    0x0800  /* consistency hint for incoming migration */
 #define BDRV_O_CHECK       0x1000  /* open solely for consistency check */
+#define BDRV_O_ALLOW_RDWR  0x2000  /* allow reopen to change from r/o to r/w */
 
 #define BDRV_O_CACHE_MASK  (BDRV_O_NOCACHE | BDRV_O_CACHE_WB | BDRV_O_NO_FLUSH)
 
-- 
1.7.11.4

[Qemu-devel] [PATCH v3 02/19] block: make bdrv_set_enable_write_cache() modify open_flags

[Jeff Cody]

bdrv_set_enable_write_cache() sets the bs->enable_write_cache flag,
but without the flag recorded in bs->open_flags, then next time
a reopen() is performed the enable_write_cache setting may be
inadvertently lost.

This will set the flag in open_flags, so it is preserved across
reopens.

Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/block.c b/block.c
index c3eb585..2b96c8e 100644
--- a/block.c
+++ b/block.c
@@ -2164,6 +2164,13 @@ int bdrv_enable_write_cache(BlockDriverState *bs)
 void bdrv_set_enable_write_cache(BlockDriverState *bs, bool wce)
 {
     bs->enable_write_cache = wce;
+
+    /* so a reopen() will preserve wce */
+    if (wce) {
+        bs->open_flags |= BDRV_O_CACHE_WB;
+    } else {
+        bs->open_flags &= ~BDRV_O_CACHE_WB;
+    }
 }
 
 int bdrv_is_encrypted(BlockDriverState *bs)
-- 
1.7.11.4

[Qemu-devel] [PATCH v3 03/19] block: Framework for reopening files safely

[Jeff Cody]

This is based 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.

Block drivers are passed a reopen state structure, that contains:
    * BDS to reopen
    * flags for the reopen
    * opaque pointer for any driver-specific data that needs to be
      persistent from _prepare to _commit/_abort
    * reopen queue pointer, if the driver needs to queue additional
      BDS for a reopen

Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block.c       | 232 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 block.h       |  17 +++++
 block_int.h   |   8 ++
 qemu-common.h |   1 +
 4 files changed, 258 insertions(+)

diff --git a/block.c b/block.c
index 2b96c8e..d625703 100644
--- a/block.c
+++ b/block.c
@@ -859,6 +859,238 @@ unlink_and_fail:
     return ret;
 }
 
+typedef struct BlockReopenQueueEntry {
+     bool prepared;
+     BDRVReopenState state;
+     QSIMPLEQ_ENTRY(BlockReopenQueueEntry) entry;
+} BlockReopenQueueEntry;
+
+/*
+ * 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.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, *next;
+    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, bs_queue, &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_SAFE(bs_entry, bs_queue, entry, next) {
+        if (ret && bs_entry->prepared) {
+            bdrv_reopen_abort(&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
+ * 'opaque' field of the BDRVReopenState, which is used and allocated by
+ * the block driver layer .bdrv_reopen_prepare()
+ *
+ * bs is the BlockDriverState to reopen
+ * flags are the new open flags
+ * queue is the reopen queue
+ *
+ * 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, BlockReopenQueue *queue,
+                        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->open_flags & BDRV_O_ALLOW_RDWR) &&
+        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, ERROR_CLASS_GENERIC_ERROR, "Error (%s) flushing drive",
+                  strerror(-ret));
+        goto error;
+    }
+
+    if (drv->bdrv_reopen_prepare) {
+        ret = drv->bdrv_reopen_prepare(reopen_state, queue, &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;
+    }
+
+    ret = 0;
+
+error:
+    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..b1095d8 100644
--- a/block.h
+++ b/block.h
@@ -97,6 +97,15 @@ typedef enum {
     BDRV_ACTION_REPORT, BDRV_ACTION_IGNORE, BDRV_ACTION_STOP
 } BlockQMPEventAction;
 
+typedef QSIMPLEQ_HEAD(BlockReopenQueue, BlockReopenQueueEntry) BlockReopenQueue;
+
+typedef struct BDRVReopenState {
+    BlockDriverState *bs;
+    int flags;
+    void *opaque;
+} BDRVReopenState;
+
+
 void bdrv_iostatus_enable(BlockDriverState *bs);
 void bdrv_iostatus_reset(BlockDriverState *bs);
 void bdrv_iostatus_disable(BlockDriverState *bs);
@@ -131,6 +140,14 @@ 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,
+                        BlockReopenQueue *queue, 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..22b3d93 100644
--- a/block_int.h
+++ b/block_int.h
@@ -139,6 +139,13 @@ 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,
+                               BlockReopenQueue *queue, 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 +343,7 @@ struct BlockDriverState {
 
     /* long-running background operation */
     BlockJob *job;
+
 };
 
 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;
-- 
1.7.11.4

[Qemu-devel] [PATCH v3 04/19] block: move aio initialization into a helper function

[Jeff Cody]

Move AIO initialization for raw-posix block driver into a helper function.

In addition to just code motion, the aio_ctx pointer is checked for NULL,
prior to calling laio_init(), to make sure laio_init() is only run once.

Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block/raw-posix.c | 55 +++++++++++++++++++++++++++++++++++--------------------
 1 file changed, 35 insertions(+), 20 deletions(-)

diff --git a/block/raw-posix.c b/block/raw-posix.c
index 6be20b1..ee55f79 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -185,6 +185,40 @@ static int raw_normalize_devicepath(const char **filename)
 }
 #endif
 
+static int raw_set_aio(void **aio_ctx, int *use_aio, int bdrv_flags)
+{
+#ifdef CONFIG_LINUX_AIO
+    int ret = -1;
+    assert(aio_ctx != NULL);
+    assert(use_aio != NULL);
+    /*
+     * Currently Linux do AIO only for files opened with O_DIRECT
+     * specified so check NOCACHE flag too
+     */
+    if ((bdrv_flags & (BDRV_O_NOCACHE|BDRV_O_NATIVE_AIO)) ==
+                      (BDRV_O_NOCACHE|BDRV_O_NATIVE_AIO)) {
+
+        /* if non-NULL, laio_init() has already been run */
+        if (*aio_ctx == NULL) {
+            *aio_ctx = laio_init();
+            if (!*aio_ctx) {
+                goto error;
+            }
+        }
+        *use_aio = 1;
+    } else {
+        *use_aio = 0;
+    }
+
+    ret = 0;
+
+error:
+    return ret;
+#else
+    return 0;
+#endif
+}
+
 static int raw_open_common(BlockDriverState *bs, const char *filename,
                            int bdrv_flags, int open_flags)
 {
@@ -239,26 +273,7 @@ static int raw_open_common(BlockDriverState *bs, const char *filename,
         goto out_free_buf;
     }
 
-#ifdef CONFIG_LINUX_AIO
-    /*
-     * Currently Linux do AIO only for files opened with O_DIRECT
-     * specified so check NOCACHE flag too
-     */
-    if ((bdrv_flags & (BDRV_O_NOCACHE|BDRV_O_NATIVE_AIO)) ==
-                      (BDRV_O_NOCACHE|BDRV_O_NATIVE_AIO)) {
-
-        s->aio_ctx = laio_init();
-        if (!s->aio_ctx) {
-            goto out_free_buf;
-        }
-        s->use_aio = 1;
-    } else
-#endif
-    {
-#ifdef CONFIG_LINUX_AIO
-        s->use_aio = 0;
-#endif
-    }
+    raw_set_aio(&s->aio_ctx, &s->use_aio, bdrv_flags);
 
 #ifdef CONFIG_XFS
     if (platform_test_xfs_fd(s->fd)) {
-- 
1.7.11.4

[Qemu-devel] [PATCH v3 05/19] block: move open flag parsing in raw block drivers to helper functions

[Jeff Cody]

Code motion, to move parsing of open flags into a helper function.

Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block/raw-posix.c | 38 ++++++++++++++++++++++++--------------
 block/raw-win32.c | 43 +++++++++++++++++++++++--------------------
 2 files changed, 47 insertions(+), 34 deletions(-)

diff --git a/block/raw-posix.c b/block/raw-posix.c
index ee55f79..7d3ac9d 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -185,6 +185,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;
+    }
+}
+
 static int raw_set_aio(void **aio_ctx, int *use_aio, int bdrv_flags)
 {
 #ifdef CONFIG_LINUX_AIO
@@ -230,20 +252,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);
diff --git a/block/raw-win32.c b/block/raw-win32.c
index c56bf83..335c06a 100644
--- a/block/raw-win32.c
+++ b/block/raw-win32.c
@@ -77,6 +77,26 @@ static int set_sparse(int fd)
 				 NULL, 0, NULL, 0, &returned, NULL);
 }
 
+static void raw_parse_flags(int flags, int *access_flags, DWORD *overlapped)
+{
+    assert(access_flags != NULL);
+    assert(overlapped != NULL);
+
+    if (flags & BDRV_O_RDWR) {
+        *access_flags = GENERIC_READ | GENERIC_WRITE;
+    } else {
+        *access_flags = GENERIC_READ;
+    }
+
+    *overlapped = FILE_ATTRIBUTE_NORMAL;
+    if (flags & BDRV_O_NOCACHE) {
+        *overlapped |= FILE_FLAG_NO_BUFFERING;
+    }
+    if (!(flags & BDRV_O_CACHE_WB)) {
+        *overlapped |= FILE_FLAG_WRITE_THROUGH;
+    }
+}
+
 static int raw_open(BlockDriverState *bs, const char *filename, int flags)
 {
     BDRVRawState *s = bs->opaque;
@@ -85,17 +105,8 @@ static int raw_open(BlockDriverState *bs, const char *filename, int flags)
 
     s->type = FTYPE_FILE;
 
-    if (flags & BDRV_O_RDWR) {
-        access_flags = GENERIC_READ | GENERIC_WRITE;
-    } else {
-        access_flags = GENERIC_READ;
-    }
+    raw_parse_flags(flags, &access_flags, &overlapped);
 
-    overlapped = FILE_ATTRIBUTE_NORMAL;
-    if (flags & BDRV_O_NOCACHE)
-        overlapped |= FILE_FLAG_NO_BUFFERING;
-    if (!(flags & BDRV_O_CACHE_WB))
-        overlapped |= FILE_FLAG_WRITE_THROUGH;
     s->hfile = CreateFile(filename, access_flags,
                           FILE_SHARE_READ, NULL,
                           OPEN_EXISTING, overlapped, NULL);
@@ -374,18 +385,10 @@ static int hdev_open(BlockDriverState *bs, const char *filename, int flags)
     }
     s->type = find_device_type(bs, filename);
 
-    if (flags & BDRV_O_RDWR) {
-        access_flags = GENERIC_READ | GENERIC_WRITE;
-    } else {
-        access_flags = GENERIC_READ;
-    }
+    raw_parse_flags(flags, &access_flags, &overlapped);
+
     create_flags = OPEN_EXISTING;
 
-    overlapped = FILE_ATTRIBUTE_NORMAL;
-    if (flags & BDRV_O_NOCACHE)
-        overlapped |= FILE_FLAG_NO_BUFFERING;
-    if (!(flags & BDRV_O_CACHE_WB))
-        overlapped |= FILE_FLAG_WRITE_THROUGH;
     s->hfile = CreateFile(filename, access_flags,
                           FILE_SHARE_READ, NULL,
                           create_flags, overlapped, NULL);
-- 
1.7.11.4

[Qemu-devel] [PATCH v3 06/19] block: do not parse BDRV_O_CACHE_WB in block drivers

[Jeff Cody]

Block drivers should ignore BDRV_O_CACHE_WB in .bdrv_open flags,
and in the bs->open_flags.

This patch removes the code, leaving the behaviour behind as if
BDRV_O_CACHE_WB was set.

Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block/iscsi.c     |  4 ----
 block/raw-posix.c |  3 ---
 block/raw-win32.c |  3 ---
 block/rbd.c       |  6 ------
 block/sheepdog.c  | 14 ++++++--------
 5 files changed, 6 insertions(+), 24 deletions(-)

diff --git a/block/iscsi.c b/block/iscsi.c
index 0b96165..70cf700 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -268,10 +268,6 @@ iscsi_aio_writev(BlockDriverState *bs, int64_t sector_num,
     acb->task->xfer_dir = SCSI_XFER_WRITE;
     acb->task->cdb_size = 16;
     acb->task->cdb[0] = 0x8a;
-    if (!(bs->open_flags & BDRV_O_CACHE_WB)) {
-        /* set FUA on writes when cache mode is write through */
-        acb->task->cdb[1] |= 0x04;
-    }
     lba = sector_qemu2lun(sector_num, iscsilun);
     *(uint32_t *)&acb->task->cdb[2]  = htonl(lba >> 32);
     *(uint32_t *)&acb->task->cdb[6]  = htonl(lba & 0xffffffff);
diff --git a/block/raw-posix.c b/block/raw-posix.c
index 7d3ac9d..4a1047c 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -202,9 +202,6 @@ static void raw_parse_flags(int bdrv_flags, int *open_flags)
     if ((bdrv_flags & BDRV_O_NOCACHE)) {
         *open_flags |= O_DIRECT;
     }
-    if (!(bdrv_flags & BDRV_O_CACHE_WB)) {
-        *open_flags |= O_DSYNC;
-    }
 }
 
 static int raw_set_aio(void **aio_ctx, int *use_aio, int bdrv_flags)
diff --git a/block/raw-win32.c b/block/raw-win32.c
index 335c06a..78c8306 100644
--- a/block/raw-win32.c
+++ b/block/raw-win32.c
@@ -92,9 +92,6 @@ static void raw_parse_flags(int flags, int *access_flags, DWORD *overlapped)
     if (flags & BDRV_O_NOCACHE) {
         *overlapped |= FILE_FLAG_NO_BUFFERING;
     }
-    if (!(flags & BDRV_O_CACHE_WB)) {
-        *overlapped |= FILE_FLAG_WRITE_THROUGH;
-    }
 }
 
 static int raw_open(BlockDriverState *bs, const char *filename, int flags)
diff --git a/block/rbd.c b/block/rbd.c
index 5a0f79f..015a9db 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -487,12 +487,6 @@ static int qemu_rbd_open(BlockDriverState *bs, const char *filename, int flags)
         rados_conf_set(s->cluster, "rbd_cache", "false");
     } else {
         rados_conf_set(s->cluster, "rbd_cache", "true");
-        if (!(flags & BDRV_O_CACHE_WB)) {
-            r = rados_conf_set(s->cluster, "rbd_cache_max_dirty", "0");
-            if (r < 0) {
-                rados_conf_set(s->cluster, "rbd_cache", "false");
-            }
-        }
     }
 
     if (strstr(conf, "conf=") == NULL) {
diff --git a/block/sheepdog.c b/block/sheepdog.c
index df4f441..5b553db 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -1114,14 +1114,12 @@ static int sd_open(BlockDriverState *bs, const char *filename, int flags)
         goto out;
     }
 
-    if (flags & BDRV_O_CACHE_WB) {
-        s->cache_enabled = 1;
-        s->flush_fd = connect_to_sdog(s->addr, s->port);
-        if (s->flush_fd < 0) {
-            error_report("failed to connect");
-            ret = s->flush_fd;
-            goto out;
-        }
+    s->cache_enabled = 1;
+    s->flush_fd = connect_to_sdog(s->addr, s->port);
+    if (s->flush_fd < 0) {
+        error_report("failed to connect");
+        ret = s->flush_fd;
+        goto out;
     }
 
     if (snapid || tag[0] != '\0') {
-- 
1.7.11.4

[Qemu-devel] [PATCH v3 07/19] block: use BDRV_O_NOCACHE instead of s->aligned_buf in raw-posix.c

[Jeff Cody]

Rather than check for a non-NULL aligned_buf to determine if
raw_aio_submit needs to check for alignment, check for the presence
of BDRV_O_NOCACHE in the bs->open_flags.

Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block/raw-posix.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/raw-posix.c b/block/raw-posix.c
index 4a1047c..1ea09c0 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -352,7 +352,7 @@ static BlockDriverAIOCB *raw_aio_submit(BlockDriverState *bs,
      * boundary.  Check if this is the case or tell the low-level
      * driver that it needs to copy the buffer.
      */
-    if (s->aligned_buf) {
+    if ((bs->open_flags & BDRV_O_NOCACHE)) {
         if (!qiov_is_aligned(bs, qiov)) {
             type |= QEMU_AIO_MISALIGNED;
 #ifdef CONFIG_LINUX_AIO
-- 
1.7.11.4

[Qemu-devel] [PATCH v3 08/19] block: purge s->aligned_buf and s->aligned_buf_size from raw-posix.c

[Jeff Cody]

The aligned_buf pointer and aligned_buf size are no longer used in
raw_posix.c, so remove all references to them.

Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block/raw-posix.c | 21 +--------------------
 1 file changed, 1 insertion(+), 20 deletions(-)

diff --git a/block/raw-posix.c b/block/raw-posix.c
index 1ea09c0..6bf5480 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -133,8 +133,6 @@ typedef struct BDRVRawState {
     int use_aio;
     void *aio_ctx;
 #endif
-    uint8_t *aligned_buf;
-    unsigned aligned_buf_size;
 #ifdef CONFIG_XFS
     bool is_xfs : 1;
 #endif
@@ -261,23 +259,10 @@ static int raw_open_common(BlockDriverState *bs, const char *filename,
         return ret;
     }
     s->fd = fd;
-    s->aligned_buf = NULL;
-
-    if ((bdrv_flags & BDRV_O_NOCACHE)) {
-        /*
-         * Allocate a buffer for read/modify/write cycles.  Chose the size
-         * pessimistically as we don't know the block size yet.
-         */
-        s->aligned_buf_size = 32 * MAX_BLOCKSIZE;
-        s->aligned_buf = qemu_memalign(MAX_BLOCKSIZE, s->aligned_buf_size);
-        if (s->aligned_buf == NULL) {
-            goto out_close;
-        }
-    }
 
     /* We're falling back to POSIX AIO in some cases so init always */
     if (paio_init() < 0) {
-        goto out_free_buf;
+        goto out_close;
     }
 
     raw_set_aio(&s->aio_ctx, &s->use_aio, bdrv_flags);
@@ -290,8 +275,6 @@ static int raw_open_common(BlockDriverState *bs, const char *filename,
 
     return 0;
 
-out_free_buf:
-    qemu_vfree(s->aligned_buf);
 out_close:
     qemu_close(fd);
     return -errno;
@@ -400,8 +383,6 @@ static void raw_close(BlockDriverState *bs)
     if (s->fd >= 0) {
         qemu_close(s->fd);
         s->fd = -1;
-        if (s->aligned_buf != NULL)
-            qemu_vfree(s->aligned_buf);
     }
 }
 
-- 
1.7.11.4

[Qemu-devel] [PATCH v3 09/19] block: raw-posix image file reopen

[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 <jcody@redhat.com>
---
 block/raw-posix.c | 99 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 99 insertions(+)

diff --git a/block/raw-posix.c b/block/raw-posix.c
index 6bf5480..edd1eca 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -138,6 +138,15 @@ typedef struct BDRVRawState {
 #endif
 } BDRVRawState;
 
+typedef struct BDRVRawReopenState {
+    int fd;
+    int open_flags;
+#ifdef CONFIG_LINUX_AIO
+    int use_aio;
+    void *aio_ctx;
+#endif
+} BDRVRawReopenState;
+
 static int fd_open(BlockDriverState *bs);
 static int64_t raw_getlength(BlockDriverState *bs);
 
@@ -288,6 +297,93 @@ 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,
+                              BlockReopenQueue *queue, 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_s->use_aio = s->use_aio;
+    raw_s->aio_ctx = s->aio_ctx;
+
+    raw_parse_flags(state->flags, &raw_s->open_flags);
+    raw_set_aio(&raw_s->aio_ctx, &raw_s->use_aio, state->flags);
+
+    raw_s->fd = -1;
+
+    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 >= 0) {
+            ret = fcntl_setfl(raw_s->fd, raw_s->open_flags);
+            if (ret) {
+                qemu_close(raw_s->fd);
+                raw_s->fd = -1;
+            }
+        }
+    }
+
+    /* If we cannot use fctnl, or fcntl failed, fall back to qemu_open() */
+    if (raw_s->fd == -1) {
+        assert(!(raw_s->open_flags & O_CREAT));
+        raw_s->fd = qemu_open(state->bs->filename, raw_s->open_flags);
+        if (raw_s->fd == -1) {
+            ret = -1;
+        }
+    }
+
+    return ret;
+}
+
+
+static void raw_reopen_commit(BDRVReopenState *state)
+{
+    BDRVRawReopenState *raw_s = state->opaque;
+    BDRVRawState *s = state->bs->opaque;
+
+    s->open_flags = raw_s->open_flags;
+
+    qemu_close(s->fd);
+    s->fd = raw_s->fd;
+    s->use_aio = raw_s->use_aio;
+    s->aio_ctx = raw_s->aio_ctx;
+
+    g_free(state->opaque);
+    state->opaque = NULL;
+}
+
+
+static void raw_reopen_abort(BDRVReopenState *state)
+{
+    BDRVRawReopenState *raw_s = state->opaque;
+
+     /* nothing to do if NULL, we didn't get far enough */
+    if (raw_s == NULL) {
+        return;
+    }
+
+    if (raw_s->fd >= 0) {
+        qemu_close(raw_s->fd);
+        raw_s->fd = -1;
+    }
+    g_free(state->opaque);
+    state->opaque = NULL;
+}
+
+
 /* XXX: use host sector size if necessary with:
 #ifdef DIOCGSECTORSIZE
         {
@@ -738,6 +834,9 @@ static BlockDriver bdrv_file = {
     .instance_size = sizeof(BDRVRawState),
     .bdrv_probe = NULL, /* no probe for protocols */
     .bdrv_file_open = raw_open,
+    .bdrv_reopen_prepare = raw_reopen_prepare,
+    .bdrv_reopen_commit = raw_reopen_commit,
+    .bdrv_reopen_abort = raw_reopen_abort,
     .bdrv_close = raw_close,
     .bdrv_create = raw_create,
     .bdrv_co_discard = raw_co_discard,
-- 
1.7.11.4

[Qemu-devel] [PATCH v3 10/19] block: raw image file reopen

[Jeff Cody]

These are the stubs for the file reopen drivers for the raw format.

There is currently nothing that needs to be done by the raw driver
in reopen.

Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block/raw.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/block/raw.c b/block/raw.c
index ff34ea4..253e949 100644
--- a/block/raw.c
+++ b/block/raw.c
@@ -9,6 +9,14 @@ static int raw_open(BlockDriverState *bs, int flags)
     return 0;
 }
 
+/* We have nothing to do for raw reopen, stubs just return
+ * success */
+static int raw_reopen_prepare(BDRVReopenState *state,
+                              BlockReopenQueue *queue,  Error **errp)
+{
+    return 0;
+}
+
 static int coroutine_fn raw_co_readv(BlockDriverState *bs, int64_t sector_num,
                                      int nb_sectors, QEMUIOVector *qiov)
 {
@@ -115,6 +123,8 @@ static BlockDriver bdrv_raw = {
     .bdrv_open          = raw_open,
     .bdrv_close         = raw_close,
 
+    .bdrv_reopen_prepare  = raw_reopen_prepare,
+
     .bdrv_co_readv          = raw_co_readv,
     .bdrv_co_writev         = raw_co_writev,
     .bdrv_co_is_allocated   = raw_co_is_allocated,
-- 
1.7.11.4

[Qemu-devel] [PATCH v3 11/19] block: qed image file reopen

[Jeff Cody]

These are the stubs for the file reopen drivers for the qed format.

There is currently nothing that needs to be done by the qed driver
in reopen.

Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block/qed.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/block/qed.c b/block/qed.c
index 21cb239..6c182ca 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -505,6 +505,14 @@ out:
     return ret;
 }
 
+/* We have nothing to do for QED reopen, stubs just return
+ * success */
+static int bdrv_qed_reopen_prepare(BDRVReopenState *state,
+                                   BlockReopenQueue *queue, Error **errp)
+{
+    return 0;
+}
+
 static void bdrv_qed_close(BlockDriverState *bs)
 {
     BDRVQEDState *s = bs->opaque;
@@ -1564,6 +1572,7 @@ static BlockDriver bdrv_qed = {
     .bdrv_rebind              = bdrv_qed_rebind,
     .bdrv_open                = bdrv_qed_open,
     .bdrv_close               = bdrv_qed_close,
+    .bdrv_reopen_prepare      = bdrv_qed_reopen_prepare,
     .bdrv_create              = bdrv_qed_create,
     .bdrv_co_is_allocated     = bdrv_qed_co_is_allocated,
     .bdrv_make_empty          = bdrv_qed_make_empty,
-- 
1.7.11.4

[Qemu-devel] [PATCH v3 12/19] block: qcow2 image file reopen

[Jeff Cody]

These are the stubs for the file reopen drivers for the qcow2 format.

There is currently nothing that needs to be done by the qcow2 driver
in reopen.

Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block/qcow2.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/block/qcow2.c b/block/qcow2.c
index 8f183f1..aa5e603 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -52,6 +52,7 @@ typedef struct {
     uint32_t magic;
     uint32_t len;
 } QCowExtension;
+
 #define  QCOW2_EXT_MAGIC_END 0
 #define  QCOW2_EXT_MAGIC_BACKING_FORMAT 0xE2792ACA
 #define  QCOW2_EXT_MAGIC_FEATURE_TABLE 0x6803f857
@@ -558,6 +559,14 @@ static int qcow2_set_key(BlockDriverState *bs, const char *key)
     return 0;
 }
 
+/* We have nothing to do for QCOW2 reopen, stubs just return
+ * success */
+static int qcow2_reopen_prepare(BDRVReopenState *state,
+                                BlockReopenQueue *queue, Error **errp)
+{
+    return 0;
+}
+
 static int coroutine_fn qcow2_co_is_allocated(BlockDriverState *bs,
         int64_t sector_num, int nb_sectors, int *pnum)
 {
@@ -1679,6 +1688,7 @@ static BlockDriver bdrv_qcow2 = {
     .bdrv_probe         = qcow2_probe,
     .bdrv_open          = qcow2_open,
     .bdrv_close         = qcow2_close,
+    .bdrv_reopen_prepare  = qcow2_reopen_prepare,
     .bdrv_create        = qcow2_create,
     .bdrv_co_is_allocated = qcow2_co_is_allocated,
     .bdrv_set_key       = qcow2_set_key,
-- 
1.7.11.4

[Qemu-devel] [PATCH v3 13/19] block: qcow image file reopen

[Jeff Cody]

These are the stubs for the file reopen drivers for the qcow format.

There is currently nothing that needs to be done by the qcow driver
in reopen.

Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block/qcow.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/block/qcow.c b/block/qcow.c
index 7b5ab87..b239c82 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -197,6 +197,15 @@ static int qcow_open(BlockDriverState *bs, int flags)
     return ret;
 }
 
+
+/* We have nothing to do for QCOW reopen, stubs just return
+ * success */
+static int qcow_reopen_prepare(BDRVReopenState *state,
+                               BlockReopenQueue *queue, Error **errp)
+{
+    return 0;
+}
+
 static int qcow_set_key(BlockDriverState *bs, const char *key)
 {
     BDRVQcowState *s = bs->opaque;
@@ -868,6 +877,7 @@ static BlockDriver bdrv_qcow = {
     .bdrv_probe		= qcow_probe,
     .bdrv_open		= qcow_open,
     .bdrv_close		= qcow_close,
+    .bdrv_reopen_prepare = qcow_reopen_prepare,
     .bdrv_create	= qcow_create,
 
     .bdrv_co_readv          = qcow_co_readv,
-- 
1.7.11.4

[Qemu-devel] [PATCH v3 14/19] block: vmdk image file reopen

[Jeff Cody]

This patch supports reopen for VMDK image files.  VMDK extents are added
to the existing reopen queue, so that the transactional model of reopen
is maintained with multiple image files.

Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block/vmdk.c | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/block/vmdk.c b/block/vmdk.c
index bba4c61..f2e861b 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -300,6 +300,40 @@ static int vmdk_is_cid_valid(BlockDriverState *bs)
     return 1;
 }
 
+/* Queue extents, if any, for reopen() */
+static int vmdk_reopen_prepare(BDRVReopenState *state,
+                               BlockReopenQueue *queue, Error **errp)
+{
+    BDRVVmdkState *s;
+    int ret = -1;
+    int i;
+    VmdkExtent *e;
+
+    assert(state != NULL);
+    assert(state->bs != NULL);
+
+    if (queue == NULL) {
+        error_set(errp, ERROR_CLASS_GENERIC_ERROR,
+                 "No reopen queue for VMDK extents");
+        goto exit;
+    }
+
+    s = state->bs->opaque;
+
+    assert(s != NULL);
+
+    for (i = 0; i < s->num_extents; i++) {
+        e = &s->extents[i];
+        if (e->file != state->bs->file) {
+            bdrv_reopen_queue(queue, e->file, state->flags);
+        }
+    }
+    ret = 0;
+
+exit:
+    return ret;
+}
+
 static int vmdk_parent_open(BlockDriverState *bs)
 {
     char *p_name;
@@ -1646,6 +1680,7 @@ static BlockDriver bdrv_vmdk = {
     .instance_size  = sizeof(BDRVVmdkState),
     .bdrv_probe     = vmdk_probe,
     .bdrv_open      = vmdk_open,
+    .bdrv_reopen_prepare = vmdk_reopen_prepare,
     .bdrv_read      = vmdk_co_read,
     .bdrv_write     = vmdk_co_write,
     .bdrv_close     = vmdk_close,
-- 
1.7.11.4

[Qemu-devel] [PATCH v3 15/19] block: raw-win32 driver reopen support

[Jeff Cody]

This is the support for reopen, for win32.  Note that there is a key
difference in the win32 reopen, namely that it is not guaranteed safe
like all the other drivers.  Attempts are made to reopen the file, or
open the file in a new handle prior to closing the old handle.  However,
this is not always feasible, and there are times when we must close the
existing file and then open the new file, breaking the transactional nature
of the reopen.

Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block/raw-win32.c | 105 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 105 insertions(+)

diff --git a/block/raw-win32.c b/block/raw-win32.c
index 78c8306..8a698d3 100644
--- a/block/raw-win32.c
+++ b/block/raw-win32.c
@@ -31,6 +31,7 @@
 #define FTYPE_FILE 0
 #define FTYPE_CD     1
 #define FTYPE_HARDDISK 2
+#define WINDOWS_VISTA 6
 
 typedef struct BDRVRawState {
     HANDLE hfile;
@@ -38,6 +39,10 @@ typedef struct BDRVRawState {
     char drive_path[16]; /* format: "d:\" */
 } BDRVRawState;
 
+typedef struct BDRVRawReopenState {
+    HANDLE hfile;
+} BDRVRawReopenState;
+
 int qemu_ftruncate64(int fd, int64_t length)
 {
     LARGE_INTEGER li;
@@ -117,6 +122,103 @@ static int raw_open(BlockDriverState *bs, const char *filename, int flags)
     return 0;
 }
 
+static int raw_reopen_prepare(BDRVReopenState *state,
+                              BlockReopenQueue *queue, Error **errp)
+{
+    BDRVRawState *s;
+    BDRVRawReopenState *raw_s;
+    int ret = 0;
+    int access_flags;
+    DWORD overlapped;
+    OSVERSIONINFO osvi;
+
+    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, &access_flags, &overlapped);
+
+    ZeroMemory(&osvi, sizeof(OSVERSIONINFO));
+    osvi.dwOSVersionInfoSize = sizeof(OSVERSIONINFO);
+
+    GetVersionEx(&osvi);
+    raw_s->hfile = INVALID_HANDLE_VALUE;
+
+    if (osvi.dwMajorVersion >= WINDOWS_VISTA) {
+        raw_s->hfile = ReOpenFile(s->hfile, access_flags, FILE_SHARE_READ,
+                                  overlapped);
+    }
+
+    /* could not reopen the file handle, so fall back to opening
+     * new file (CreateFile) */
+    if (raw_s->hfile == INVALID_HANDLE_VALUE) {
+        raw_s->hfile = CreateFile(state->bs->filename, access_flags,
+                                  FILE_SHARE_READ, NULL, OPEN_EXISTING,
+                                  overlapped, NULL);
+        if (raw_s->hfile == INVALID_HANDLE_VALUE) {
+            /* this could happen because the access_flags requested are
+             * incompatible with the existing share mode of s->hfile,
+             * so our only option now is to close s->hfile, and try again.
+             * This could end badly */
+            CloseHandle(s->hfile);
+            s->hfile = INVALID_HANDLE_VALUE;
+            raw_s->hfile = CreateFile(state->bs->filename, access_flags,
+                                      FILE_SHARE_READ, NULL, OPEN_EXISTING,
+                                      overlapped, NULL);
+            if (raw_s->hfile == INVALID_HANDLE_VALUE) {
+                /* Unrecoverable error */
+                error_set(errp, ERROR_CLASS_GENERIC_ERROR,
+                          "Fatal error reopening %s file; file closed and cannot be opened\n",
+                          state->bs->filename);
+                ret = -1;
+            } else{
+                /* since we had to close the original, go ahead and
+                 * re-assign here */
+                s->hfile = raw_s->hfile;
+                raw_s->hfile = INVALID_HANDLE_VALUE;
+            }
+        }
+    }
+
+    return ret;
+}
+
+
+static void raw_reopen_commit(BDRVReopenState *state)
+{
+    BDRVRawReopenState *raw_s = state->opaque;
+    BDRVRawState *s = state->bs->opaque;
+
+    if (raw_s->hfile != INVALID_HANDLE_VALUE) {
+        CloseHandle(s->hfile);
+        s->hfile = raw_s->hfile;
+    }
+
+    g_free(state->opaque);
+    state->opaque = NULL;
+}
+
+
+static void raw_reopen_abort(BDRVReopenState *state)
+{
+    BDRVRawReopenState *raw_s = state->opaque;
+
+     /* nothing to do if NULL, we didn't get far enough */
+    if (raw_s == NULL) {
+        return;
+    }
+
+    if (raw_s->hfile != INVALID_HANDLE_VALUE) {
+        CloseHandle(raw_s->hfile);
+    }
+    g_free(state->opaque);
+    state->opaque = NULL;
+}
+
 static int raw_read(BlockDriverState *bs, int64_t sector_num,
                     uint8_t *buf, int nb_sectors)
 {
@@ -287,6 +389,9 @@ static BlockDriver bdrv_file = {
     .protocol_name	= "file",
     .instance_size	= sizeof(BDRVRawState),
     .bdrv_file_open	= raw_open,
+    .bdrv_reopen_prepare = raw_reopen_prepare,
+    .bdrv_reopen_commit = raw_reopen_commit,
+    .bdrv_reopen_abort = raw_reopen_abort,
     .bdrv_close		= raw_close,
     .bdrv_create	= raw_create,
 
-- 
1.7.11.4

[Qemu-devel] [PATCH v3 16/19] block: vdi image file reopen

[Jeff Cody]

There is currently nothing that needs to be done for VDI reopen.

Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block/vdi.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/block/vdi.c b/block/vdi.c
index c4f1529..ce38395 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -454,6 +454,12 @@ static int vdi_open(BlockDriverState *bs, int flags)
     return -1;
 }
 
+static int vdi_reopen_prepare(BDRVReopenState *state,
+                              BlockReopenQueue *queue, Error **errp)
+{
+    return 0;
+}
+
 static int coroutine_fn vdi_co_is_allocated(BlockDriverState *bs,
         int64_t sector_num, int nb_sectors, int *pnum)
 {
@@ -762,6 +768,7 @@ static BlockDriver bdrv_vdi = {
     .bdrv_probe = vdi_probe,
     .bdrv_open = vdi_open,
     .bdrv_close = vdi_close,
+    .bdrv_reopen_prepare = vdi_reopen_prepare,
     .bdrv_create = vdi_create,
     .bdrv_co_is_allocated = vdi_co_is_allocated,
     .bdrv_make_empty = vdi_make_empty,
-- 
1.7.11.4

[Qemu-devel] [PATCH v3 17/19] block: vpc image file reopen

[Jeff Cody]

There is currently nothing that needs to be done for VPC image
file reopen.

Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block/vpc.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/block/vpc.c b/block/vpc.c
index c0b82c4..b6bf52f 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -265,6 +265,12 @@ static int vpc_open(BlockDriverState *bs, int flags)
     return err;
 }
 
+static int vpc_reopen_prepare(BDRVReopenState *state,
+                              BlockReopenQueue *queue, Error **errp)
+{
+    return 0;
+}
+
 /*
  * Returns the absolute byte offset of the given sector in the image file.
  * If the sector is not allocated, -1 is returned instead.
@@ -783,6 +789,7 @@ static BlockDriver bdrv_vpc = {
     .bdrv_probe     = vpc_probe,
     .bdrv_open      = vpc_open,
     .bdrv_close     = vpc_close,
+    .bdrv_reopen_prepare = vpc_reopen_prepare,
     .bdrv_create    = vpc_create,
 
     .bdrv_read              = vpc_co_read,
-- 
1.7.11.4

[Qemu-devel] [PATCH v3 18/19] block: convert bdrv_commit() to use bdrv_reopen()

[Jeff Cody]

Currently, bdrv_commit() reopens images r/w itself, via risky
_delete() and _open() calls. Use the new safe method for drive reopen.

Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block.c | 48 +++++-------------------------------------------
 1 file changed, 5 insertions(+), 43 deletions(-)

diff --git a/block.c b/block.c
index d625703..ebdbf61 100644
--- a/block.c
+++ b/block.c
@@ -1497,13 +1497,11 @@ int bdrv_check(BlockDriverState *bs, BdrvCheckResult *res, BdrvCheckMode fix)
 int bdrv_commit(BlockDriverState *bs)
 {
     BlockDriver *drv = bs->drv;
-    BlockDriver *backing_drv;
     int64_t sector, total_sectors;
     int n, ro, open_flags;
-    int ret = 0, rw_ret = 0;
+    int ret = 0;
     uint8_t *buf;
     char filename[1024];
-    BlockDriverState *bs_rw, *bs_ro;
 
     if (!drv)
         return -ENOMEDIUM;
@@ -1512,42 +1510,18 @@ int bdrv_commit(BlockDriverState *bs)
         return -ENOTSUP;
     }
 
-    if (bs->backing_hd->keep_read_only) {
-        return -EACCES;
-    }
-
     if (bdrv_in_use(bs) || bdrv_in_use(bs->backing_hd)) {
         return -EBUSY;
     }
 
-    backing_drv = bs->backing_hd->drv;
     ro = bs->backing_hd->read_only;
     strncpy(filename, bs->backing_hd->filename, sizeof(filename));
     open_flags =  bs->backing_hd->open_flags;
 
     if (ro) {
-        /* re-open as RW */
-        bdrv_delete(bs->backing_hd);
-        bs->backing_hd = NULL;
-        bs_rw = bdrv_new("");
-        rw_ret = bdrv_open(bs_rw, filename, open_flags | BDRV_O_RDWR,
-            backing_drv);
-        if (rw_ret < 0) {
-            bdrv_delete(bs_rw);
-            /* try to re-open read-only */
-            bs_ro = bdrv_new("");
-            ret = bdrv_open(bs_ro, filename, open_flags & ~BDRV_O_RDWR,
-                backing_drv);
-            if (ret < 0) {
-                bdrv_delete(bs_ro);
-                /* drive not functional anymore */
-                bs->drv = NULL;
-                return ret;
-            }
-            bs->backing_hd = bs_ro;
-            return rw_ret;
+        if (bdrv_reopen(bs->backing_hd, open_flags | BDRV_O_RDWR, NULL)) {
+            return -EACCES;
         }
-        bs->backing_hd = bs_rw;
     }
 
     total_sectors = bdrv_getlength(bs) >> BDRV_SECTOR_BITS;
@@ -1584,20 +1558,8 @@ ro_cleanup:
     g_free(buf);
 
     if (ro) {
-        /* re-open as RO */
-        bdrv_delete(bs->backing_hd);
-        bs->backing_hd = NULL;
-        bs_ro = bdrv_new("");
-        ret = bdrv_open(bs_ro, filename, open_flags & ~BDRV_O_RDWR,
-            backing_drv);
-        if (ret < 0) {
-            bdrv_delete(bs_ro);
-            /* drive not functional anymore */
-            bs->drv = NULL;
-            return ret;
-        }
-        bs->backing_hd = bs_ro;
-        bs->backing_hd->keep_read_only = 0;
+        /* ignoring error return here */
+        bdrv_reopen(bs->backing_hd, open_flags & ~BDRV_O_RDWR, NULL);
     }
 
     return ret;
-- 
1.7.11.4

[Qemu-devel] [PATCH v3 19/19] block: remove keep_read_only flag from BlockDriverState struct

[Jeff Cody]

The keep_read_only flag is no longer used, in favor of the bdrv
flag BDRV_O_ALLOW_RDWR.

Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block.c     | 2 --
 block_int.h | 1 -
 2 files changed, 3 deletions(-)

diff --git a/block.c b/block.c
index ebdbf61..5db1df1 100644
--- a/block.c
+++ b/block.c
@@ -808,8 +808,6 @@ int bdrv_open(BlockDriverState *bs, const char *filename, int flags,
         flags |= BDRV_O_ALLOW_RDWR;
     }
 
-    bs->keep_read_only = !(flags & BDRV_O_ALLOW_RDWR);
-
     /* Open the image */
     ret = bdrv_open_common(bs, filename, flags, drv);
     if (ret < 0) {
diff --git a/block_int.h b/block_int.h
index 22b3d93..ac4245c 100644
--- a/block_int.h
+++ b/block_int.h
@@ -275,7 +275,6 @@ struct BlockDriverState {
     int64_t total_sectors; /* if we are reading a disk image, give its
                               size in sectors */
     int read_only; /* if true, the media is read only */
-    int keep_read_only; /* if true, the media was requested to stay read only */
     int open_flags; /* flags used to open the file, re-used for re-open */
     int encrypted; /* if true, the media is encrypted */
     int valid_key; /* if true, a valid encryption key has been set */
-- 
1.7.11.4

Re: [Qemu-devel] [PATCH v3 09/19] block: raw-posix image file reopen

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2857 bytes --]

On 09/18/2012 12:53 PM, Jeff Cody wrote:
> 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 <jcody@redhat.com>
> ---
>  block/raw-posix.c | 99 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 99 insertions(+)
> 
> diff --git a/block/raw-posix.c b/block/raw-posix.c
> index 6bf5480..edd1eca 100644
> --- a/block/raw-posix.c
> +++ b/block/raw-posix.c
> @@ -138,6 +138,15 @@ typedef struct BDRVRawState {
>  #endif
>  } BDRVRawState;
>  
> +typedef struct BDRVRawReopenState {
> +    int fd;
> +    int open_flags;
> +#ifdef CONFIG_LINUX_AIO
> +    int use_aio;
> +    void *aio_ctx;
> +#endif

These members are conditional...

> +static int raw_reopen_prepare(BDRVReopenState *state,
> +                              BlockReopenQueue *queue, 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_s->use_aio = s->use_aio;
> +    raw_s->aio_ctx = s->aio_ctx;

...but you are unconditionally assigning into them.  This will introduce
compile failures on other platforms.

> +    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);

F_DUPFD_CLOEXEC is not defined everywhere yet; you still need to address
your TODO.

> +
> +    /* If we cannot use fctnl, or fcntl failed, fall back to qemu_open() */

s/fctnl/fcntl/

> +static void raw_reopen_commit(BDRVReopenState *state)
> +{
> +    BDRVRawReopenState *raw_s = state->opaque;
> +    BDRVRawState *s = state->bs->opaque;
> +
> +    s->open_flags = raw_s->open_flags;
> +
> +    qemu_close(s->fd);
> +    s->fd = raw_s->fd;
> +    s->use_aio = raw_s->use_aio;
> +    s->aio_ctx = raw_s->aio_ctx;

Again, more unconditional use of conditional members.

-- 
Eric Blake   eblake@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 617 bytes --]

Re: [Qemu-devel] [PATCH v3 09/19] block: raw-posix image file reopen

[Jeff Cody]

On 09/18/2012 05:20 PM, Eric Blake wrote:
> On 09/18/2012 12:53 PM, Jeff Cody wrote:
>> 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 <jcody@redhat.com>
>> ---
>>  block/raw-posix.c | 99 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 99 insertions(+)
>>
>> diff --git a/block/raw-posix.c b/block/raw-posix.c
>> index 6bf5480..edd1eca 100644
>> --- a/block/raw-posix.c
>> +++ b/block/raw-posix.c
>> @@ -138,6 +138,15 @@ typedef struct BDRVRawState {
>>  #endif
>>  } BDRVRawState;
>>  
>> +typedef struct BDRVRawReopenState {
>> +    int fd;
>> +    int open_flags;
>> +#ifdef CONFIG_LINUX_AIO
>> +    int use_aio;
>> +    void *aio_ctx;
>> +#endif
> 
> These members are conditional...
> 
>> +static int raw_reopen_prepare(BDRVReopenState *state,
>> +                              BlockReopenQueue *queue, 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_s->use_aio = s->use_aio;
>> +    raw_s->aio_ctx = s->aio_ctx;
> 
> ...but you are unconditionally assigning into them.  This will introduce
> compile failures on other platforms.

Argh.  Thanks.

> 
>> +    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);
> 
> F_DUPFD_CLOEXEC is not defined everywhere yet; you still need to address
> your TODO.
> 
>> +
>> +    /* If we cannot use fctnl, or fcntl failed, fall back to qemu_open() */
> 
> s/fctnl/fcntl/
> 
>> +static void raw_reopen_commit(BDRVReopenState *state)
>> +{
>> +    BDRVRawReopenState *raw_s = state->opaque;
>> +    BDRVRawState *s = state->bs->opaque;
>> +
>> +    s->open_flags = raw_s->open_flags;
>> +
>> +    qemu_close(s->fd);
>> +    s->fd = raw_s->fd;
>> +    s->use_aio = raw_s->use_aio;
>> +    s->aio_ctx = raw_s->aio_ctx;
> 
> Again, more unconditional use of conditional members.
> 

Re: [Qemu-devel] [PATCH v3 03/19] block: Framework for reopening files safely

[Kevin Wolf]

Am 18.09.2012 20:53, schrieb Jeff Cody:
> This is based 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.
> 
> Block drivers are passed a reopen state structure, that contains:
>     * BDS to reopen
>     * flags for the reopen
>     * opaque pointer for any driver-specific data that needs to be
>       persistent from _prepare to _commit/_abort
>     * reopen queue pointer, if the driver needs to queue additional
>       BDS for a reopen
> 
> Signed-off-by: Jeff Cody <jcody@redhat.com>

> diff --git a/block.h b/block.h
> index 4d919c2..b1095d8 100644
> --- a/block.h
> +++ b/block.h
> @@ -97,6 +97,15 @@ typedef enum {
>      BDRV_ACTION_REPORT, BDRV_ACTION_IGNORE, BDRV_ACTION_STOP
>  } BlockQMPEventAction;
>  
> +typedef QSIMPLEQ_HEAD(BlockReopenQueue, BlockReopenQueueEntry) BlockReopenQueue;
> +
> +typedef struct BDRVReopenState {
> +    BlockDriverState *bs;
> +    int flags;
> +    void *opaque;
> +} BDRVReopenState;

Doesn't compile, it should be typedefed only once:

In file included from monitor.h:8,
                 from qemu-timer.c:27:
block.h:106: Fehler: Redefinition des typedef »BDRVReopenState«
qemu-common.h:248: Anmerkung: Vorherige Deklaration von
»BDRVReopenState« war hier

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

Kevin

Re: [Qemu-devel] [PATCH v3 03/19] block: Framework for reopening files safely

[Jeff Cody]

On 09/20/2012 07:24 AM, Kevin Wolf wrote:
> Am 18.09.2012 20:53, schrieb Jeff Cody:
>> This is based 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.
>>
>> Block drivers are passed a reopen state structure, that contains:
>>     * BDS to reopen
>>     * flags for the reopen
>>     * opaque pointer for any driver-specific data that needs to be
>>       persistent from _prepare to _commit/_abort
>>     * reopen queue pointer, if the driver needs to queue additional
>>       BDS for a reopen
>>
>> Signed-off-by: Jeff Cody <jcody@redhat.com>
> 
>> diff --git a/block.h b/block.h
>> index 4d919c2..b1095d8 100644
>> --- a/block.h
>> +++ b/block.h
>> @@ -97,6 +97,15 @@ typedef enum {
>>      BDRV_ACTION_REPORT, BDRV_ACTION_IGNORE, BDRV_ACTION_STOP
>>  } BlockQMPEventAction;
>>  
>> +typedef QSIMPLEQ_HEAD(BlockReopenQueue, BlockReopenQueueEntry) BlockReopenQueue;
>> +
>> +typedef struct BDRVReopenState {
>> +    BlockDriverState *bs;
>> +    int flags;
>> +    void *opaque;
>> +} BDRVReopenState;
> 
> Doesn't compile, it should be typedefed only once:
> 
> In file included from monitor.h:8,
>                  from qemu-timer.c:27:
> block.h:106: Fehler: Redefinition des typedef »BDRVReopenState«
> qemu-common.h:248: Anmerkung: Vorherige Deklaration von
> »BDRVReopenState« war hier
> 

Looks like this has been relaxed in later versions of gcc - in 4.7.0 (what I am using under F17), it
compiled fine.  I'll change this though, so that it compiles under previous versions of gcc (and check
for any other such errors), and verify that under an older version of gcc.


>> --- 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;
> 
> Kevin
> 

Re: [Qemu-devel] [PATCH v3 04/19] block: move aio initialization into a helper function

[Kevin Wolf]

Am 18.09.2012 20:53, schrieb Jeff Cody:
> Move AIO initialization for raw-posix block driver into a helper function.
> 
> In addition to just code motion, the aio_ctx pointer is checked for NULL,
> prior to calling laio_init(), to make sure laio_init() is only run once.
> 
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>  block/raw-posix.c | 55 +++++++++++++++++++++++++++++++++++--------------------
>  1 file changed, 35 insertions(+), 20 deletions(-)
> 
> diff --git a/block/raw-posix.c b/block/raw-posix.c
> index 6be20b1..ee55f79 100644
> --- a/block/raw-posix.c
> +++ b/block/raw-posix.c
> @@ -185,6 +185,40 @@ static int raw_normalize_devicepath(const char **filename)
>  }
>  #endif
>  
> +static int raw_set_aio(void **aio_ctx, int *use_aio, int bdrv_flags)
> +{
> +#ifdef CONFIG_LINUX_AIO
> +    int ret = -1;
> +    assert(aio_ctx != NULL);
> +    assert(use_aio != NULL);
> +    /*
> +     * Currently Linux do AIO only for files opened with O_DIRECT
> +     * specified so check NOCACHE flag too
> +     */
> +    if ((bdrv_flags & (BDRV_O_NOCACHE|BDRV_O_NATIVE_AIO)) ==
> +                      (BDRV_O_NOCACHE|BDRV_O_NATIVE_AIO)) {
> +
> +        /* if non-NULL, laio_init() has already been run */
> +        if (*aio_ctx == NULL) {
> +            *aio_ctx = laio_init();
> +            if (!*aio_ctx) {
> +                goto error;
> +            }
> +        }
> +        *use_aio = 1;
> +    } else {
> +        *use_aio = 0;
> +    }
> +
> +    ret = 0;
> +
> +error:
> +    return ret;
> +#else
> +    return 0;
> +#endif
> +}
> +
>  static int raw_open_common(BlockDriverState *bs, const char *filename,
>                             int bdrv_flags, int open_flags)
>  {
> @@ -239,26 +273,7 @@ static int raw_open_common(BlockDriverState *bs, const char *filename,
>          goto out_free_buf;
>      }
>  
> -#ifdef CONFIG_LINUX_AIO
> -    /*
> -     * Currently Linux do AIO only for files opened with O_DIRECT
> -     * specified so check NOCACHE flag too
> -     */
> -    if ((bdrv_flags & (BDRV_O_NOCACHE|BDRV_O_NATIVE_AIO)) ==
> -                      (BDRV_O_NOCACHE|BDRV_O_NATIVE_AIO)) {
> -
> -        s->aio_ctx = laio_init();
> -        if (!s->aio_ctx) {
> -            goto out_free_buf;
> -        }
> -        s->use_aio = 1;
> -    } else
> -#endif
> -    {
> -#ifdef CONFIG_LINUX_AIO
> -        s->use_aio = 0;
> -#endif
> -    }
> +    raw_set_aio(&s->aio_ctx, &s->use_aio, bdrv_flags);

This should check the return value to handle laio_init() failure properly.

Kevin

Re: [Qemu-devel] [PATCH v3 09/19] block: raw-posix image file reopen

[Kevin Wolf]

Am 18.09.2012 20:53, 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 <jcody@redhat.com>

> +static int raw_reopen_prepare(BDRVReopenState *state,
> +                              BlockReopenQueue *queue, 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_s->use_aio = s->use_aio;
> +    raw_s->aio_ctx = s->aio_ctx;

You can immediately set s->aio_ctx instead of going through
BDRVRawReopenState with it. It seems to be valid to have it present
while use_aio = 0. If it wasn't valid, you'd have to free the context
when reopening without Linux AIO.

> +
> +    raw_parse_flags(state->flags, &raw_s->open_flags);
> +    raw_set_aio(&raw_s->aio_ctx, &raw_s->use_aio, state->flags);

At least you're consistently omitting the error check. :-)

> +
> +    raw_s->fd = -1;
> +
> +    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 */

Hm, still not addressed?

Kevin

Re: [Qemu-devel] [PATCH v3 14/19] block: vmdk image file reopen

[Kevin Wolf]

Am 18.09.2012 20:53, schrieb Jeff Cody:
> This patch supports reopen for VMDK image files.  VMDK extents are added
> to the existing reopen queue, so that the transactional model of reopen
> is maintained with multiple image files.
> 
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>  block/vmdk.c | 35 +++++++++++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
> 
> diff --git a/block/vmdk.c b/block/vmdk.c
> index bba4c61..f2e861b 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -300,6 +300,40 @@ static int vmdk_is_cid_valid(BlockDriverState *bs)
>      return 1;
>  }
>  
> +/* Queue extents, if any, for reopen() */
> +static int vmdk_reopen_prepare(BDRVReopenState *state,
> +                               BlockReopenQueue *queue, Error **errp)
> +{
> +    BDRVVmdkState *s;
> +    int ret = -1;
> +    int i;
> +    VmdkExtent *e;
> +
> +    assert(state != NULL);
> +    assert(state->bs != NULL);
> +
> +    if (queue == NULL) {
> +        error_set(errp, ERROR_CLASS_GENERIC_ERROR,
> +                 "No reopen queue for VMDK extents");
> +        goto exit;
> +    }
> +
> +    s = state->bs->opaque;
> +
> +    assert(s != NULL);
> +
> +    for (i = 0; i < s->num_extents; i++) {
> +        e = &s->extents[i];
> +        if (e->file != state->bs->file) {
> +            bdrv_reopen_queue(queue, e->file, state->flags);

This is wrong, you can't abort the transaction if you do it like this.

VMDK needs to separately pass through each of prepare/commit/abort to
all extents, it can't use the all-in-one function.

Kevin

Re: [Qemu-devel] [PATCH v3 09/19] block: raw-posix image file reopen

[Jeff Cody]

On 09/20/2012 10:10 AM, Kevin Wolf wrote:
> Am 18.09.2012 20:53, 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 <jcody@redhat.com>
> 
>> +static int raw_reopen_prepare(BDRVReopenState *state,
>> +                              BlockReopenQueue *queue, 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_s->use_aio = s->use_aio;
>> +    raw_s->aio_ctx = s->aio_ctx;
> 
> You can immediately set s->aio_ctx instead of going through
> BDRVRawReopenState with it. It seems to be valid to have it present
> while use_aio = 0. If it wasn't valid, you'd have to free the context
> when reopening without Linux AIO.
>

Good catch, thanks.

>> +
>> +    raw_parse_flags(state->flags, &raw_s->open_flags);
>> +    raw_set_aio(&raw_s->aio_ctx, &raw_s->use_aio, state->flags);
> 
> At least you're consistently omitting the error check. :-)
> 

Thanks, fixed.

I guess I am an optimist at heart :)

>> +
>> +    raw_s->fd = -1;
>> +
>> +    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 */
> 
> Hm, still not addressed?
> 

No. I mentioned this in the cover letter for v2.  I'd rather see the
qemu fcntl wrapper happen as a separate series, and then come back and
update this, if that is OK.  I'm afraid changes to qemu_open or
qemu_dup_flags would delay getting this series in.

Although Eric is right, I do need to ifdef the F_DUPFD_CLOEXEC.


> Kevin
> 

Re: [Qemu-devel] [PATCH v3 14/19] block: vmdk image file reopen

[Jeff Cody]

On 09/20/2012 10:12 AM, Kevin Wolf wrote:
> Am 18.09.2012 20:53, schrieb Jeff Cody:
>> This patch supports reopen for VMDK image files.  VMDK extents are added
>> to the existing reopen queue, so that the transactional model of reopen
>> is maintained with multiple image files.
>>
>> Signed-off-by: Jeff Cody <jcody@redhat.com>
>> ---
>>  block/vmdk.c | 35 +++++++++++++++++++++++++++++++++++
>>  1 file changed, 35 insertions(+)
>>
>> diff --git a/block/vmdk.c b/block/vmdk.c
>> index bba4c61..f2e861b 100644
>> --- a/block/vmdk.c
>> +++ b/block/vmdk.c
>> @@ -300,6 +300,40 @@ static int vmdk_is_cid_valid(BlockDriverState *bs)
>>      return 1;
>>  }
>>  
>> +/* Queue extents, if any, for reopen() */
>> +static int vmdk_reopen_prepare(BDRVReopenState *state,
>> +                               BlockReopenQueue *queue, Error **errp)
>> +{
>> +    BDRVVmdkState *s;
>> +    int ret = -1;
>> +    int i;
>> +    VmdkExtent *e;
>> +
>> +    assert(state != NULL);
>> +    assert(state->bs != NULL);
>> +
>> +    if (queue == NULL) {
>> +        error_set(errp, ERROR_CLASS_GENERIC_ERROR,
>> +                 "No reopen queue for VMDK extents");
>> +        goto exit;
>> +    }
>> +
>> +    s = state->bs->opaque;
>> +
>> +    assert(s != NULL);
>> +
>> +    for (i = 0; i < s->num_extents; i++) {
>> +        e = &s->extents[i];
>> +        if (e->file != state->bs->file) {
>> +            bdrv_reopen_queue(queue, e->file, state->flags);
> 
> This is wrong, you can't abort the transaction if you do it like this.
> 
> VMDK needs to separately pass through each of prepare/commit/abort to
> all extents, it can't use the all-in-one function.
> 
> Kevin
> 
(Follow up from conversation on IRC)

This is actually correct, because it is adding the extents to the queue
in the prepare(). This will cause the extents to go through the same
transactional prepare/commit/abort cycle as the rest of the images in
the queue.

Jeff

Re: [Qemu-devel] [PATCH v3 04/19] block: move aio initialization into a helper function

[Jeff Cody]

On 09/20/2012 09:14 AM, Kevin Wolf wrote:
> Am 18.09.2012 20:53, schrieb Jeff Cody:
>> Move AIO initialization for raw-posix block driver into a helper function.
>>
>> In addition to just code motion, the aio_ctx pointer is checked for NULL,
>> prior to calling laio_init(), to make sure laio_init() is only run once.
>>
>> Signed-off-by: Jeff Cody <jcody@redhat.com>
>> ---
>>  block/raw-posix.c | 55 +++++++++++++++++++++++++++++++++++--------------------
>>  1 file changed, 35 insertions(+), 20 deletions(-)
>>
>> diff --git a/block/raw-posix.c b/block/raw-posix.c
>> index 6be20b1..ee55f79 100644
>> --- a/block/raw-posix.c
>> +++ b/block/raw-posix.c
>> @@ -185,6 +185,40 @@ static int raw_normalize_devicepath(const char **filename)
>>  }
>>  #endif
>>  
>> +static int raw_set_aio(void **aio_ctx, int *use_aio, int bdrv_flags)
>> +{
>> +#ifdef CONFIG_LINUX_AIO
>> +    int ret = -1;
>> +    assert(aio_ctx != NULL);
>> +    assert(use_aio != NULL);
>> +    /*
>> +     * Currently Linux do AIO only for files opened with O_DIRECT
>> +     * specified so check NOCACHE flag too
>> +     */
>> +    if ((bdrv_flags & (BDRV_O_NOCACHE|BDRV_O_NATIVE_AIO)) ==
>> +                      (BDRV_O_NOCACHE|BDRV_O_NATIVE_AIO)) {
>> +
>> +        /* if non-NULL, laio_init() has already been run */
>> +        if (*aio_ctx == NULL) {
>> +            *aio_ctx = laio_init();
>> +            if (!*aio_ctx) {
>> +                goto error;
>> +            }
>> +        }
>> +        *use_aio = 1;
>> +    } else {
>> +        *use_aio = 0;
>> +    }
>> +
>> +    ret = 0;
>> +
>> +error:
>> +    return ret;
>> +#else
>> +    return 0;
>> +#endif
>> +}
>> +
>>  static int raw_open_common(BlockDriverState *bs, const char *filename,
>>                             int bdrv_flags, int open_flags)
>>  {
>> @@ -239,26 +273,7 @@ static int raw_open_common(BlockDriverState *bs, const char *filename,
>>          goto out_free_buf;
>>      }
>>  
>> -#ifdef CONFIG_LINUX_AIO
>> -    /*
>> -     * Currently Linux do AIO only for files opened with O_DIRECT
>> -     * specified so check NOCACHE flag too
>> -     */
>> -    if ((bdrv_flags & (BDRV_O_NOCACHE|BDRV_O_NATIVE_AIO)) ==
>> -                      (BDRV_O_NOCACHE|BDRV_O_NATIVE_AIO)) {
>> -
>> -        s->aio_ctx = laio_init();
>> -        if (!s->aio_ctx) {
>> -            goto out_free_buf;
>> -        }
>> -        s->use_aio = 1;
>> -    } else
>> -#endif
>> -    {
>> -#ifdef CONFIG_LINUX_AIO
>> -        s->use_aio = 0;
>> -#endif
>> -    }
>> +    raw_set_aio(&s->aio_ctx, &s->use_aio, bdrv_flags);
> 
> This should check the return value to handle laio_init() failure properly.
> 
> Kevin
> 

Thanks, fixed for v4.