[Qemu-devel] [PATCH v2 00/16] block: bdrv_reopen() patches

[Qemu-devel] [PATCH v2 00/16] 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 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 (16):
  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 raw block drivers
  block: move allocating aligned_buf into a helper function in
    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: convert bdrv_commit() to use bdrv_reopen()
  block: remove keep_read_only flag from BlockDriverState struct

 block.c           | 292 ++++++++++++++++++++++++++++++++++++++++++++----------
 block.h           |  24 +++++
 block/qcow.c      |  23 +++++
 block/qcow2.c     |  22 ++++
 block/qed.c       |  20 ++++
 block/raw-posix.c | 243 +++++++++++++++++++++++++++++++++++++--------
 block/raw-win32.c | 144 +++++++++++++++++++++++----
 block/raw.c       |  22 ++++
 block/vmdk.c      |  47 +++++++++
 block_int.h       |   8 +-
 qemu-common.h     |   1 +
 11 files changed, 732 insertions(+), 114 deletions(-)

-- 
1.7.11.4

[Qemu-devel] [PATCH v2 01/16] 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 v2 02/16] 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 v2 03/16] 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       | 225 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 block.h       |  23 ++++++
 block_int.h   |   7 ++
 qemu-common.h |   1 +
 4 files changed, 256 insertions(+)

diff --git a/block.c b/block.c
index 2b96c8e..a54022b 100644
--- a/block.c
+++ b/block.c
@@ -859,6 +859,231 @@ 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.bs = bs;
+    bs_entry->state.flags = flags;
+    bs_entry->state.queue = bs_queue;
+
+    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, &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
+ * '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->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, &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..e51704a 100644
--- a/block.h
+++ b/block.h
@@ -97,6 +97,22 @@ 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;
+    BlockReopenQueue *queue;
+} BDRVReopenState;
+
+typedef struct BlockReopenQueueEntry {
+     bool prepared;
+     BDRVReopenState state;
+     QSIMPLEQ_ENTRY(BlockReopenQueueEntry) entry;
+} BlockReopenQueueEntry;
+
+
 void bdrv_iostatus_enable(BlockDriverState *bs);
 void bdrv_iostatus_reset(BlockDriverState *bs);
 void bdrv_iostatus_disable(BlockDriverState *bs);
@@ -131,6 +147,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..67f8858 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,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

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

[Paolo Bonzini]

Il 13/09/2012 17:49, Jeff Cody ha scritto:
> +    reopen_state->bs->open_flags         = reopen_state->flags;
> +    reopen_state->bs->enable_write_cache = !!(reopen_state->flags &
> +                                              BDRV_O_CACHE_WB);

I think setting enable_write_cache is wrong here because this is really
part of the guest state, not the host state (yes, it's a mess).
Instead, you could proceed like this:

1) change bdrv_enable_write_cache like this:

int bdrv_enable_write_cache(BlockDriverState *bs)
{
    return bs->enable_write_cache && (bs->open_flags & BDRV_O_CACHE_WB);
}

and use it in block.c instead of checking bs->enable_write_cache directly;

2) always initialize bs->enable_write_cache to true, rather than to the
value of BDRV_O_CACHE_WB;

This way bs->enable_write_cache is _really_ guest state rather than a mix.

3) never touch enable_write_cache in bdrv_reopen.

Paolo

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

[Paolo Bonzini]

Il 13/09/2012 17:49, Jeff Cody ha scritto:
> +typedef QSIMPLEQ_HEAD(BlockReopenQueue, BlockReopenQueueEntry) BlockReopenQueue;
> +
> +typedef struct BDRVReopenState {
> +    BlockDriverState *bs;
> +    int flags;
> +    void *opaque;
> +    BlockReopenQueue *queue;

Do we need the queue pointer here?  Or it can be a separate argument to
prepare?  Commit and abort don't need it, and it may mess things up a
bit if commit calls bdrv_reopen_queue (because then the newly-added
element will get a commit without being prepared).

> +} BDRVReopenState;
> +
> +typedef struct BlockReopenQueueEntry {
> +     bool prepared;
> +     BDRVReopenState state;
> +     QSIMPLEQ_ENTRY(BlockReopenQueueEntry) entry;
> +} BlockReopenQueueEntry;
> +

A small change: please move struct BlockReopenQueueEntry to block.c.

Paolo

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

[Jeff Cody]

On 09/13/2012 04:35 PM, Paolo Bonzini wrote:
> Il 13/09/2012 17:49, Jeff Cody ha scritto:
>> +typedef QSIMPLEQ_HEAD(BlockReopenQueue, BlockReopenQueueEntry) BlockReopenQueue;
>> +
>> +typedef struct BDRVReopenState {
>> +    BlockDriverState *bs;
>> +    int flags;
>> +    void *opaque;
>> +    BlockReopenQueue *queue;
> 
> Do we need the queue pointer here?  Or it can be a separate argument to
> prepare?  Commit and abort don't need it, and it may mess things up a
> bit if commit calls bdrv_reopen_queue (because then the newly-added
> element will get a commit without being prepared).
> 

It could, and should, be a separate argument to prepare(), as you are
correct - commit() and abort() should never use it.

>> +} BDRVReopenState;
>> +
>> +typedef struct BlockReopenQueueEntry {
>> +     bool prepared;
>> +     BDRVReopenState state;
>> +     QSIMPLEQ_ENTRY(BlockReopenQueueEntry) entry;
>> +} BlockReopenQueueEntry;
>> +
> 
> A small change: please move struct BlockReopenQueueEntry to block.c.
> 

OK

[Qemu-devel] [PATCH v2 04/16] 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 v2 05/16] 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 v2 06/16] block: do not parse BDRV_O_CACHE_WB in raw block drivers

[Jeff Cody]

Block drivers should always open the files in writeback mode (see commit
e1e9b0ac), so raw-posix/raw-win32 should not parse the BDRV_O_CACHE_WB
flag.

Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block/raw-posix.c | 3 ---
 block/raw-win32.c | 3 ---
 2 files changed, 6 deletions(-)

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)
-- 
1.7.11.4

Re: [Qemu-devel] [PATCH v2 06/16] block: do not parse BDRV_O_CACHE_WB in raw block drivers

[Paolo Bonzini]

Il 13/09/2012 17:49, Jeff Cody ha scritto:
> Block drivers should always open the files in writeback mode (see commit
> e1e9b0ac), so raw-posix/raw-win32 should not parse the BDRV_O_CACHE_WB
> flag.
> 
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>  block/raw-posix.c | 3 ---
>  block/raw-win32.c | 3 ---
>  2 files changed, 6 deletions(-)
> 
> 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)
> 

Why does this matter?  If raw-posix was opened directly (i.e. without
the bs->file indirection) this would cause a writethrough file to be
incorrectly opened as writeback.

Paolo

Re: [Qemu-devel] [PATCH v2 06/16] block: do not parse BDRV_O_CACHE_WB in raw block drivers

[Jeff Cody]

On 09/13/2012 12:12 PM, Paolo Bonzini wrote:
> Il 13/09/2012 17:49, Jeff Cody ha scritto:
>> Block drivers should always open the files in writeback mode (see commit
>> e1e9b0ac), so raw-posix/raw-win32 should not parse the BDRV_O_CACHE_WB
>> flag.
>>
>> Signed-off-by: Jeff Cody <jcody@redhat.com>
>> ---
>>  block/raw-posix.c | 3 ---
>>  block/raw-win32.c | 3 ---
>>  2 files changed, 6 deletions(-)
>>
>> 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)
>>
> 
> Why does this matter?  If raw-posix was opened directly (i.e. without
> the bs->file indirection) this would cause a writethrough file to be
> incorrectly opened as writeback.
> 
> Paolo
> 

The problem this patch was trying to work around is that
bdrv_open_common() forces BDRV_O_CACHE_WB (commit e1e9b0ac), but that
setting is not preserved in bs->open_flags, so it is lost on a reopen.

Is there a scenario currently that has raw-posix opened directly as a
writethrough file, or were you more concerned with future use?

Re: [Qemu-devel] [PATCH v2 06/16] block: do not parse BDRV_O_CACHE_WB in raw block drivers

[Paolo Bonzini]

Il 13/09/2012 19:17, Jeff Cody ha scritto:
>> > 
>> > Why does this matter?  If raw-posix was opened directly (i.e. without
>> > the bs->file indirection) this would cause a writethrough file to be
>> > incorrectly opened as writeback.
>> > 
>> > Paolo
>> > 
> The problem this patch was trying to work around is that
> bdrv_open_common() forces BDRV_O_CACHE_WB (commit e1e9b0ac), but that
> setting is not preserved in bs->open_flags, so it is lost on a reopen.

Perhaps we _should_ preserve that in bs->open_flags, while still using
the initial value of BDRV_O_CACHE_WB to initialize bs->enable_write_cache.

> Is there a scenario currently that has raw-posix opened directly as a
> writethrough file, or were you more concerned with future use?

Not for raw-posix, but IIRC some other protocol is opened directly
without a format on top.  rbd perhaps?  I'm concerned of having to work
around what seems like a bug elsewhere, in multiple protocols.

Paolo

Re: [Qemu-devel] [PATCH v2 06/16] block: do not parse BDRV_O_CACHE_WB in raw block drivers

[Jeff Cody]

On 09/13/2012 02:56 PM, Paolo Bonzini wrote:
> Il 13/09/2012 19:17, Jeff Cody ha scritto:
>>>>
>>>> Why does this matter?  If raw-posix was opened directly (i.e. without
>>>> the bs->file indirection) this would cause a writethrough file to be
>>>> incorrectly opened as writeback.
>>>>
>>>> Paolo
>>>>
>> The problem this patch was trying to work around is that
>> bdrv_open_common() forces BDRV_O_CACHE_WB (commit e1e9b0ac), but that
>> setting is not preserved in bs->open_flags, so it is lost on a reopen.
> 
> Perhaps we _should_ preserve that in bs->open_flags, while still using
> the initial value of BDRV_O_CACHE_WB to initialize bs->enable_write_cache.

That would work.  Part of the problem is that BDRV_O_CACHE_WB seems
overloaded; maybe bdrv_parse_cache_flags() should set a new flag, called
BDRV_O_CACHE_WCE, which can be used in lieu of enable_write_cache
(similar to getting rid of keep_read_only in favor of
 BDRV_O_ALLOW_RDWR).  And then bdrv_parse_cache_flags() would just not
set BDRV_O_CACHE_WB, which can then be used exclusively by the lower
layers for their parsing (and bdrv_open_common would just set
bs->open_flags to always have BDRV_O_CACHE_WB).

Then patch 2/16 would change to having bdrv_set_enable_write_cache()
toggle BDRV_O_CACHE_WCE.

> 
>> Is there a scenario currently that has raw-posix opened directly as a
>> writethrough file, or were you more concerned with future use?
> 
> Not for raw-posix, but IIRC some other protocol is opened directly
> without a format on top.  rbd perhaps?  I'm concerned of having to work
> around what seems like a bug elsewhere, in multiple protocols.
> 
> Paolo
> 

Re: [Qemu-devel] [PATCH v2 06/16] block: do not parse BDRV_O_CACHE_WB in raw block drivers

[Paolo Bonzini]

Il 13/09/2012 21:04, Jeff Cody ha scritto:
>> > Perhaps we _should_ preserve that in bs->open_flags, while still using
>> > the initial value of BDRV_O_CACHE_WB to initialize bs->enable_write_cache.
> That would work.  Part of the problem is that BDRV_O_CACHE_WB seems
> overloaded; maybe bdrv_parse_cache_flags() should set a new flag, called
> BDRV_O_CACHE_WCE, which can be used in lieu of enable_write_cache
> (similar to getting rid of keep_read_only in favor of
>  BDRV_O_ALLOW_RDWR).  And then bdrv_parse_cache_flags() would just not
> set BDRV_O_CACHE_WB, which can then be used exclusively by the lower
> layers for their parsing (and bdrv_open_common would just set
> bs->open_flags to always have BDRV_O_CACHE_WB).
> 
> Then patch 2/16 would change to having bdrv_set_enable_write_cache()
> toggle BDRV_O_CACHE_WCE.
> 

Yeah, that would work.

Alternatively, we can keep this patch and leave bdrv_open_common as is;
but then I would also prefer if raw-{posix,win32} took care of setting
BDRV_O_CACHE_WB in bs->open_flags, so that the flags are consistent.
This setting of BDRV_O_CACHE_WB can be extended later to other formats.

Considering this and my comments to patch 3/16 we would have:

- after opening with cache=writethrough:

                           bs           bs->file
enable_write_cache        true           true
BDRV_O_CACHE_WB           false          true
bdrv_enable_write_cache() false          true


- after opening with cache=writeback:

                           bs           bs->file
enable_write_cache        true           true
BDRV_O_CACHE_WB           true           true
bdrv_enable_write_cache() true           true

Paolo

Re: [Qemu-devel] [PATCH v2 06/16] block: do not parse BDRV_O_CACHE_WB in raw block drivers

[Paolo Bonzini]

Il 13/09/2012 21:44, Paolo Bonzini ha scritto:
> Il 13/09/2012 21:04, Jeff Cody ha scritto:
>>>> Perhaps we _should_ preserve that in bs->open_flags, while still using
>>>> the initial value of BDRV_O_CACHE_WB to initialize bs->enable_write_cache.
>> That would work.  Part of the problem is that BDRV_O_CACHE_WB seems
>> overloaded; maybe bdrv_parse_cache_flags() should set a new flag, called
>> BDRV_O_CACHE_WCE, which can be used in lieu of enable_write_cache
>> (similar to getting rid of keep_read_only in favor of
>>  BDRV_O_ALLOW_RDWR).  And then bdrv_parse_cache_flags() would just not
>> set BDRV_O_CACHE_WB, which can then be used exclusively by the lower
>> layers for their parsing (and bdrv_open_common would just set
>> bs->open_flags to always have BDRV_O_CACHE_WB).
>>
>> Then patch 2/16 would change to having bdrv_set_enable_write_cache()
>> toggle BDRV_O_CACHE_WCE.
>>
> 
> Yeah, that would work.
> 
> Alternatively, we can keep this patch and leave bdrv_open_common as is;
> but then I would also prefer if raw-{posix,win32} took care of setting
> BDRV_O_CACHE_WB in bs->open_flags, so that the flags are consistent.
> This setting of BDRV_O_CACHE_WB can be extended later to other formats.

Hmm, no, what was I thinking...

But there is a very simple thing we can do:

- patch 2 can be left as is

- in patch 3 bdrv_reopen_queue, always add BDRV_O_CACHE_WB to
bs_entry->state.flags

- in patch 3 bdrv_reopen_commit, always leave BDRV_O_CACHE_WB aside:

    reopen_state->bs->open_flags         =
         (bs->open_flags & BDRV_O_CACHE_WB) |
         (reopen_state->flags & ~BDRV_O_CACHE_WB);

- this patch can be dropped completely.

Paolo

Re: [Qemu-devel] [PATCH v2 06/16] block: do not parse BDRV_O_CACHE_WB in raw block drivers

[Jeff Cody]

On 09/13/2012 04:29 PM, Paolo Bonzini wrote:
> Il 13/09/2012 21:44, Paolo Bonzini ha scritto:
>> Il 13/09/2012 21:04, Jeff Cody ha scritto:
>>>>> Perhaps we _should_ preserve that in bs->open_flags, while still using
>>>>> the initial value of BDRV_O_CACHE_WB to initialize bs->enable_write_cache.
>>> That would work.  Part of the problem is that BDRV_O_CACHE_WB seems
>>> overloaded; maybe bdrv_parse_cache_flags() should set a new flag, called
>>> BDRV_O_CACHE_WCE, which can be used in lieu of enable_write_cache
>>> (similar to getting rid of keep_read_only in favor of
>>>  BDRV_O_ALLOW_RDWR).  And then bdrv_parse_cache_flags() would just not
>>> set BDRV_O_CACHE_WB, which can then be used exclusively by the lower
>>> layers for their parsing (and bdrv_open_common would just set
>>> bs->open_flags to always have BDRV_O_CACHE_WB).
>>>
>>> Then patch 2/16 would change to having bdrv_set_enable_write_cache()
>>> toggle BDRV_O_CACHE_WCE.
>>>
>>
>> Yeah, that would work.
>>
>> Alternatively, we can keep this patch and leave bdrv_open_common as is;
>> but then I would also prefer if raw-{posix,win32} took care of setting
>> BDRV_O_CACHE_WB in bs->open_flags, so that the flags are consistent.
>> This setting of BDRV_O_CACHE_WB can be extended later to other formats.
> 
> Hmm, no, what was I thinking...
> 
> But there is a very simple thing we can do:
> 
> - patch 2 can be left as is
> 
> - in patch 3 bdrv_reopen_queue, always add BDRV_O_CACHE_WB to
> bs_entry->state.flags
> 
> - in patch 3 bdrv_reopen_commit, always leave BDRV_O_CACHE_WB aside:
> 
>     reopen_state->bs->open_flags         =
>          (bs->open_flags & BDRV_O_CACHE_WB) |
>          (reopen_state->flags & ~BDRV_O_CACHE_WB);
> 
> - this patch can be dropped completely.
>

Yes, I think that would work.  The only thing I don't like is that
BDRV_O_CACHE_WB still remains a bit confusing when looking through the
code... bs->open_flags does not actually represent the open flags.

With what I proposed above, here are the steps needed:

- bdrv_parse_cache_flags() sets BDRV_O_CACHE_WCE instead of
  BDRV_O_CACHE_WB

- BDRV_O_CACHE_WCE is used everywhere enable_write_cache was used

- bs->enable_write_cache is removed

- this patch is dropped

- bdrv_open_common() sets bs->open_flags to have BDRV_O_CACHE_WB enabled
  by default.

- The only way BDRV_O_CACHE_WB would get cleared from bs->open_flags now
  would be if someone explicitly cleared it during a bdrv_reopen().

While there are more changes this way, I think it cleans up the code a
bit.  The advantage is that bs->open_flags actually reflects the open
flags that are currently in use.  One disadvantage I see is that it
seems a bit odd to have BDRV_O_CACHE_WCE cleared and BDRV_O_CACHE_WB
set, until you think about them being intended for different layers.

Maybe that is part of the underlying problem - there is one open_flags
variable in the BDS, that has some flags intended for all layers in the
block stack, and some flags specific to a layer.

Re: [Qemu-devel] [PATCH v2 06/16] block: do not parse BDRV_O_CACHE_WB in raw block drivers

[Paolo Bonzini]

Il 13/09/2012 23:45, Jeff Cody ha scritto:
> While there are more changes this way, I think it cleans up the code a
> bit.  The advantage is that bs->open_flags actually reflects the open
> flags that are currently in use.  One disadvantage I see is that it
> seems a bit odd to have BDRV_O_CACHE_WCE cleared and BDRV_O_CACHE_WB
> set, until you think about them being intended for different layers.

It's more weird to see BDRV_O_CACHE_WCE set and BDRV_O_CACHE_WB cleared. :)

> Maybe that is part of the underlying problem - there is one open_flags
> variable in the BDS, that has some flags intended for all layers in the
> block stack, and some flags specific to a layer.

Yes, that's true.

Paolo

Re: [Qemu-devel] [PATCH v2 06/16] block: do not parse BDRV_O_CACHE_WB in raw block drivers

[Kevin Wolf]

Am 13.09.2012 23:45, schrieb Jeff Cody:
> On 09/13/2012 04:29 PM, Paolo Bonzini wrote:
>> Il 13/09/2012 21:44, Paolo Bonzini ha scritto:
>>> Il 13/09/2012 21:04, Jeff Cody ha scritto:
>>>>>> Perhaps we _should_ preserve that in bs->open_flags, while still using
>>>>>> the initial value of BDRV_O_CACHE_WB to initialize bs->enable_write_cache.
>>>> That would work.  Part of the problem is that BDRV_O_CACHE_WB seems
>>>> overloaded; maybe bdrv_parse_cache_flags() should set a new flag, called
>>>> BDRV_O_CACHE_WCE, which can be used in lieu of enable_write_cache
>>>> (similar to getting rid of keep_read_only in favor of
>>>>  BDRV_O_ALLOW_RDWR).  And then bdrv_parse_cache_flags() would just not
>>>> set BDRV_O_CACHE_WB, which can then be used exclusively by the lower
>>>> layers for their parsing (and bdrv_open_common would just set
>>>> bs->open_flags to always have BDRV_O_CACHE_WB).
>>>>
>>>> Then patch 2/16 would change to having bdrv_set_enable_write_cache()
>>>> toggle BDRV_O_CACHE_WCE.
>>>>
>>>
>>> Yeah, that would work.
>>>
>>> Alternatively, we can keep this patch and leave bdrv_open_common as is;
>>> but then I would also prefer if raw-{posix,win32} took care of setting
>>> BDRV_O_CACHE_WB in bs->open_flags, so that the flags are consistent.
>>> This setting of BDRV_O_CACHE_WB can be extended later to other formats.
>>
>> Hmm, no, what was I thinking...
>>
>> But there is a very simple thing we can do:
>>
>> - patch 2 can be left as is
>>
>> - in patch 3 bdrv_reopen_queue, always add BDRV_O_CACHE_WB to
>> bs_entry->state.flags
>>
>> - in patch 3 bdrv_reopen_commit, always leave BDRV_O_CACHE_WB aside:
>>
>>     reopen_state->bs->open_flags         =
>>          (bs->open_flags & BDRV_O_CACHE_WB) |
>>          (reopen_state->flags & ~BDRV_O_CACHE_WB);
>>
>> - this patch can be dropped completely.
>>
> 
> Yes, I think that would work.  The only thing I don't like is that
> BDRV_O_CACHE_WB still remains a bit confusing when looking through the
> code... bs->open_flags does not actually represent the open flags.
> 
> With what I proposed above, here are the steps needed:
> 
> - bdrv_parse_cache_flags() sets BDRV_O_CACHE_WCE instead of
>   BDRV_O_CACHE_WB
> 
> - BDRV_O_CACHE_WCE is used everywhere enable_write_cache was used
> 
> - bs->enable_write_cache is removed
> 
> - this patch is dropped
> 
> - bdrv_open_common() sets bs->open_flags to have BDRV_O_CACHE_WB enabled
>   by default.
> 
> - The only way BDRV_O_CACHE_WB would get cleared from bs->open_flags now
>   would be if someone explicitly cleared it during a bdrv_reopen().

This feels totally wrong. Your BDRV_O_CACHE_WCE is what BDRV_O_CACHE_WB
should be; removing bs->enable_write_cache in favour of a fix
BDRV_O_CACHE_WB makes sense, though (maybe we need to consider renaming
bs->open_flags then, it really hasn't anything to do with open and
more). All block drivers, including raw-{posix,win32}, should completely
ignore the flag in their .bdrv_open implementation, this flag is purely
for the generic block layer. Drivers always open everything writeback
and provide a flush function. They already do today, because today's
broken BDRV_O_CACHE_WB is always set.

Kevin

Re: [Qemu-devel] [PATCH v2 06/16] block: do not parse BDRV_O_CACHE_WB in raw block drivers

[Kevin Wolf]

Am 13.09.2012 18:12, schrieb Paolo Bonzini:
> Il 13/09/2012 17:49, Jeff Cody ha scritto:
>> Block drivers should always open the files in writeback mode (see commit
>> e1e9b0ac), so raw-posix/raw-win32 should not parse the BDRV_O_CACHE_WB
>> flag.
>>
>> Signed-off-by: Jeff Cody <jcody@redhat.com>
>> ---
>>  block/raw-posix.c | 3 ---
>>  block/raw-win32.c | 3 ---
>>  2 files changed, 6 deletions(-)
>>
>> 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)
>>
> 
> Why does this matter?  If raw-posix was opened directly (i.e. without
> the bs->file indirection) this would cause a writethrough file to be
> incorrectly opened as writeback.

--verbose, please.

I can't see how bs->file is needed here for writethrough semantics.
bdrv_open_common() sets bs->enable_write_cache to false and
bdrv_co_do_writev() checks it and flushes if necessary. Looks fine to me.

In fact, bdrv_open_common() even removes BDRV_O_CACHE_WB, so what Jeff
removes here is really dead code (checked with strace: The file isn't
opened with O_SYNC even when using -drive format=file).

Kevin

Re: [Qemu-devel] [PATCH v2 06/16] block: do not parse BDRV_O_CACHE_WB in raw block drivers

[Paolo Bonzini]

Il 14/09/2012 09:27, Kevin Wolf ha scritto:
> I can't see how bs->file is needed here for writethrough semantics.
> bdrv_open_common() sets bs->enable_write_cache to false and
> bdrv_co_do_writev() checks it and flushes if necessary. Looks fine to me.

You're right.

> In fact, bdrv_open_common() even removes BDRV_O_CACHE_WB, so what Jeff
> removes here is really dead code (checked with strace: The file isn't
> opened with O_SYNC even when using -drive format=file).

Yes, it's dead, on the other hand we still honor BDRV_O_CACHE_WB in all
the other protocols.  Either we go and touch all the protocols
(effectively removing BDRV_O_CACHE_WB from the BlockDriver
specification), or treating raw-{posix,win32} specially means we leave
bugs everywhere else.

Paolo

Re: [Qemu-devel] [PATCH v2 06/16] block: do not parse BDRV_O_CACHE_WB in raw block drivers

[Kevin Wolf]

Am 14.09.2012 09:50, schrieb Paolo Bonzini:
> Il 14/09/2012 09:27, Kevin Wolf ha scritto:
>> I can't see how bs->file is needed here for writethrough semantics.
>> bdrv_open_common() sets bs->enable_write_cache to false and
>> bdrv_co_do_writev() checks it and flushes if necessary. Looks fine to me.
> 
> You're right.
> 
>> In fact, bdrv_open_common() even removes BDRV_O_CACHE_WB, so what Jeff
>> removes here is really dead code (checked with strace: The file isn't
>> opened with O_SYNC even when using -drive format=file).
> 
> Yes, it's dead, on the other hand we still honor BDRV_O_CACHE_WB in all
> the other protocols.  Either we go and touch all the protocols
> (effectively removing BDRV_O_CACHE_WB from the BlockDriver
> specification), or treating raw-{posix,win32} specially means we leave
> bugs everywhere else.

Yes, touch all protocols and fix them. BDRV_O_CACHE_WB should be for
block.c only.

Kevin

Re: [Qemu-devel] [PATCH v2 06/16] block: do not parse BDRV_O_CACHE_WB in raw block drivers

[Paolo Bonzini]

Il 14/09/2012 09:55, Kevin Wolf ha scritto:
>> Either we go and touch all the protocols
>> > (effectively removing BDRV_O_CACHE_WB from the BlockDriver
>> > specification), or treating raw-{posix,win32} specially means we leave
>> > bugs everywhere else.
> Yes, touch all protocols and fix them. BDRV_O_CACHE_WB should be for
> block.c only.

That's also fine, of course.

Paolo

[Qemu-devel] [PATCH v2 07/16] block: move allocating aligned_buf into a helper function in raw_posix.c

[Jeff Cody]

Code motion, to move allocating aligned_buf and setting aligned_buf_size
into a helper function.

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

diff --git a/block/raw-posix.c b/block/raw-posix.c
index 4a1047c..47cab9f 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -238,6 +238,26 @@ error:
 #endif
 }
 
+static int raw_allocate_aligned_buf(uint8_t **aligned_buf,
+                                    unsigned *aligned_buf_size, int bdrv_flags)
+{
+    assert(aligned_buf != NULL);
+    assert(aligned_buf_size != 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.
+         */
+        *aligned_buf_size = 32 * MAX_BLOCKSIZE;
+        *aligned_buf = qemu_memalign(MAX_BLOCKSIZE, *aligned_buf_size);
+        if (*aligned_buf == NULL) {
+            return -1;
+        }
+    }
+    return 0;
+}
+
 static int raw_open_common(BlockDriverState *bs, const char *filename,
                            int bdrv_flags, int open_flags)
 {
@@ -263,16 +283,9 @@ static int raw_open_common(BlockDriverState *bs, const char *filename,
     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) {
+    if (raw_allocate_aligned_buf(&s->aligned_buf, &s->aligned_buf_size,
+                                 bdrv_flags)) {
             goto out_close;
-        }
     }
 
     /* We're falling back to POSIX AIO in some cases so init always */
-- 
1.7.11.4

Re: [Qemu-devel] [PATCH v2 07/16] block: move allocating aligned_buf into a helper function in raw_posix.c

[Eric Blake]

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

On 09/13/2012 09:49 AM, Jeff Cody wrote:
> Code motion, to move allocating aligned_buf and setting aligned_buf_size
> into a helper function.
> 
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>  block/raw-posix.c | 31 ++++++++++++++++++++++---------
>  1 file changed, 22 insertions(+), 9 deletions(-)
> 

> +    if ((bdrv_flags & BDRV_O_NOCACHE)) {
> +        /*
> +         * Allocate a buffer for read/modify/write cycles.  Chose the size

Pre-existing typo, but as long as you are moving it,

s/Chose/Choose/

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

[Qemu-devel] [PATCH v2 08/16] 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 | 123 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 123 insertions(+)

diff --git a/block/raw-posix.c b/block/raw-posix.c
index 47cab9f..2407eeb 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -140,6 +140,19 @@ typedef struct BDRVRawState {
 #endif
 } BDRVRawState;
 
+typedef struct BDRVRawReopenState {
+    BDRVReopenState reopen_state;
+    int fd;
+    int open_flags;
+#ifdef CONFIG_LINUX_AIO
+    int use_aio;
+    void *aio_ctx;
+#endif
+    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);
 
@@ -318,6 +331,113 @@ 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_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);
+
+    /*
+     * If we didn't have BDRV_O_NOCACHE set before, we may not have allocated
+     * aligned_buf
+     */
+    ret = raw_allocate_aligned_buf(&raw_s->aligned_buf,
+                                   &raw_s->aligned_buf_size, state->flags);
+    if (ret) {
+        goto error;
+    }
+
+    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;
+        }
+    }
+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;
+
+    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;
+        if (raw_s->aligned_buf != NULL) {
+            qemu_vfree(raw_s->aligned_buf);
+        }
+    }
+    g_free(state->opaque);
+    state->opaque = NULL;
+}
+
+
 /* XXX: use host sector size if necessary with:
 #ifdef DIOCGSECTORSIZE
         {
@@ -770,6 +890,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

Re: [Qemu-devel] [PATCH v2 08/16] block: raw-posix image file reopen

[Paolo Bonzini]

Il 13/09/2012 17:49, Jeff Cody ha scritto:
> +
> +    /*
> +     * If we didn't have BDRV_O_NOCACHE set before, we may not have allocated
> +     * aligned_buf
> +     */
> +    ret = raw_allocate_aligned_buf(&raw_s->aligned_buf,
> +                                   &raw_s->aligned_buf_size, state->flags);

Aligned_buf is unused, except for checking if it exists here:

    if (s->aligned_buf) {
        if (!qiov_is_aligned(bs, qiov)) {
            type |= QEMU_AIO_MISALIGNED;
#ifdef CONFIG_LINUX_AIO
        } else if (s->use_aio) {
            return laio_submit(bs, s->aio_ctx, s->fd, sector_num, qiov,
                               nb_sectors, cb, opaque, type);
#endif
        }
    }

You can instead check BDRV_O_NOCACHE and kill aligned_buf completely.

Paolo

Re: [Qemu-devel] [PATCH v2 08/16] block: raw-posix image file reopen

[Jeff Cody]

On 09/13/2012 12:02 PM, Paolo Bonzini wrote:
> Il 13/09/2012 17:49, Jeff Cody ha scritto:
>> +
>> +    /*
>> +     * If we didn't have BDRV_O_NOCACHE set before, we may not have allocated
>> +     * aligned_buf
>> +     */
>> +    ret = raw_allocate_aligned_buf(&raw_s->aligned_buf,
>> +                                   &raw_s->aligned_buf_size, state->flags);
> 
> Aligned_buf is unused, except for checking if it exists here:
> 
>     if (s->aligned_buf) {
>         if (!qiov_is_aligned(bs, qiov)) {
>             type |= QEMU_AIO_MISALIGNED;
> #ifdef CONFIG_LINUX_AIO
>         } else if (s->use_aio) {
>             return laio_submit(bs, s->aio_ctx, s->fd, sector_num, qiov,
>                                nb_sectors, cb, opaque, type);
> #endif
>         }
>     }
> 
> You can instead check BDRV_O_NOCACHE and kill aligned_buf completely.
> 
> Paolo
>

Hmm - yes, it looks like that is so.  This patch will transform into
checking for BDRV_O_NOCACHE as you suggest, and killing aligned_buf.

Thanks,
Jeff

Re: [Qemu-devel] [PATCH v2 08/16] block: raw-posix image file reopen

[Kevin Wolf]

Am 13.09.2012 18:57, schrieb Jeff Cody:
> On 09/13/2012 12:02 PM, Paolo Bonzini wrote:
>> Il 13/09/2012 17:49, Jeff Cody ha scritto:
>>> +
>>> +    /*
>>> +     * If we didn't have BDRV_O_NOCACHE set before, we may not have allocated
>>> +     * aligned_buf
>>> +     */
>>> +    ret = raw_allocate_aligned_buf(&raw_s->aligned_buf,
>>> +                                   &raw_s->aligned_buf_size, state->flags);
>>
>> Aligned_buf is unused, except for checking if it exists here:
>>
>>     if (s->aligned_buf) {
>>         if (!qiov_is_aligned(bs, qiov)) {
>>             type |= QEMU_AIO_MISALIGNED;
>> #ifdef CONFIG_LINUX_AIO
>>         } else if (s->use_aio) {
>>             return laio_submit(bs, s->aio_ctx, s->fd, sector_num, qiov,
>>                                nb_sectors, cb, opaque, type);
>> #endif
>>         }
>>     }
>>
>> You can instead check BDRV_O_NOCACHE and kill aligned_buf completely.
>>
>> Paolo
>>
> 
> Hmm - yes, it looks like that is so.  This patch will transform into
> checking for BDRV_O_NOCACHE as you suggest, and killing aligned_buf.

Sounds good, but please make removing aligned_buf a separate patch.

Kevin

[Qemu-devel] [PATCH v2 09/16] 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 | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/block/raw.c b/block/raw.c
index ff34ea4..fa47ff1 100644
--- a/block/raw.c
+++ b/block/raw.c
@@ -9,6 +9,24 @@ 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, Error **errp)
+{
+    return 0;
+}
+
+static void raw_reopen_commit(BDRVReopenState *state)
+{
+    return;
+}
+
+static void raw_reopen_abort(BDRVReopenState *state)
+{
+    return;
+}
+
+
 static int coroutine_fn raw_co_readv(BlockDriverState *bs, int64_t sector_num,
                                      int nb_sectors, QEMUIOVector *qiov)
 {
@@ -115,6 +133,10 @@ static BlockDriver bdrv_raw = {
     .bdrv_open          = raw_open,
     .bdrv_close         = raw_close,
 
+    .bdrv_reopen_prepare  = raw_reopen_prepare,
+    .bdrv_reopen_commit   = raw_reopen_commit,
+    .bdrv_reopen_abort    = raw_reopen_abort,
+
     .bdrv_co_readv          = raw_co_readv,
     .bdrv_co_writev         = raw_co_writev,
     .bdrv_co_is_allocated   = raw_co_is_allocated,
-- 
1.7.11.4

Re: [Qemu-devel] [PATCH v2 09/16] block: raw image file reopen

[Paolo Bonzini]

Il 13/09/2012 17:49, Jeff Cody ha scritto:
> +/* We have nothing to do for raw reopen, stubs just return
> + * success */
> +static int raw_reopen_prepare(BDRVReopenState *state, Error **errp)
> +{
> +    return 0;
> +}
> +
> +static void raw_reopen_commit(BDRVReopenState *state)
> +{
> +    return;
> +}
> +
> +static void raw_reopen_abort(BDRVReopenState *state)
> +{
> +    return;
> +}
> +
> +

Commit and abort are optional, aren't they?

BTW, vdi and vpc should also support reopen.

Paolo

Re: [Qemu-devel] [PATCH v2 09/16] block: raw image file reopen

[Jeff Cody]

On 09/13/2012 12:05 PM, Paolo Bonzini wrote:
> Il 13/09/2012 17:49, Jeff Cody ha scritto:
>> +/* We have nothing to do for raw reopen, stubs just return
>> + * success */
>> +static int raw_reopen_prepare(BDRVReopenState *state, Error **errp)
>> +{
>> +    return 0;
>> +}
>> +
>> +static void raw_reopen_commit(BDRVReopenState *state)
>> +{
>> +    return;
>> +}
>> +
>> +static void raw_reopen_abort(BDRVReopenState *state)
>> +{
>> +    return;
>> +}
>> +
>> +
> 
> Commit and abort are optional, aren't they?
> 
> BTW, vdi and vpc should also support reopen.
> 
> Paolo
> 

The commit and abort block driver handlers are indeed optional, prepare
is the only required one.  I kept the stubs for them for completeness,
however (I can remove them if that causes heartburn for others).

A quick look at the vdi and vpc block drivers seems to indicate that
both of those can be supported via stubs as well; I'll add them to v3.

Thanks,
Jeff

Re: [Qemu-devel] [PATCH v2 09/16] block: raw image file reopen

[Paolo Bonzini]

Il 13/09/2012 19:02, Jeff Cody ha scritto:
> I kept the stubs for them for completeness,
> however (I can remove them if that causes heartburn for others).

The point of making them optional is to avoid stubs, isn't it? ;)

Paolo

Re: [Qemu-devel] [PATCH v2 09/16] block: raw image file reopen

[Jeff Cody]

On 09/13/2012 02:53 PM, Paolo Bonzini wrote:
> Il 13/09/2012 19:02, Jeff Cody ha scritto:
>> I kept the stubs for them for completeness,
>> however (I can remove them if that causes heartburn for others).
> 
> The point of making them optional is to avoid stubs, isn't it? ;)
> 
> Paolo
> 

Fair point - consider them removed for v3. :)

[Qemu-devel] [PATCH v2 10/16] 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 | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/block/qed.c b/block/qed.c
index 21cb239..ed154b1 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -505,6 +505,23 @@ out:
     return ret;
 }
 
+/* We have nothing to do for QED reopen, stubs just return
+ * success */
+static int bdrv_qed_reopen_prepare(BDRVReopenState *state, Error **errp)
+{
+    return 0;
+}
+
+static void bdrv_qed_reopen_commit(BDRVReopenState *state)
+{
+    return;
+}
+
+static void bdrv_qed_reopen_abort(BDRVReopenState *state)
+{
+    return;
+}
+
 static void bdrv_qed_close(BlockDriverState *bs)
 {
     BDRVQEDState *s = bs->opaque;
@@ -1564,6 +1581,9 @@ 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_reopen_commit       = bdrv_qed_reopen_commit,
+    .bdrv_reopen_abort        = bdrv_qed_reopen_abort,
     .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 v2 11/16] 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 | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/block/qcow2.c b/block/qcow2.c
index 8f183f1..d462093 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,24 @@ 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, Error **errp)
+{
+    return 0;
+}
+
+static void qcow2_reopen_commit(BDRVReopenState *state)
+{
+    return;
+}
+
+static void qcow2_reopen_abort(BDRVReopenState *state)
+{
+    return;
+}
+
+
 static int coroutine_fn qcow2_co_is_allocated(BlockDriverState *bs,
         int64_t sector_num, int nb_sectors, int *pnum)
 {
@@ -1679,6 +1698,9 @@ static BlockDriver bdrv_qcow2 = {
     .bdrv_probe         = qcow2_probe,
     .bdrv_open          = qcow2_open,
     .bdrv_close         = qcow2_close,
+    .bdrv_reopen_prepare  = qcow2_reopen_prepare,
+    .bdrv_reopen_commit   = qcow2_reopen_commit,
+    .bdrv_reopen_abort    = qcow2_reopen_abort,
     .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 v2 12/16] 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 | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/block/qcow.c b/block/qcow.c
index 7b5ab87..f201575 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -197,6 +197,26 @@ 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, Error **errp)
+{
+    return 0;
+}
+
+static void qcow_reopen_commit(BDRVReopenState *state)
+{
+    return;
+}
+
+static void qcow_reopen_abort(BDRVReopenState *state)
+{
+    return;
+}
+
+
+
 static int qcow_set_key(BlockDriverState *bs, const char *key)
 {
     BDRVQcowState *s = bs->opaque;
@@ -868,6 +888,9 @@ static BlockDriver bdrv_qcow = {
     .bdrv_probe		= qcow_probe,
     .bdrv_open		= qcow_open,
     .bdrv_close		= qcow_close,
+    .bdrv_reopen_prepare = qcow_reopen_prepare,
+    .bdrv_reopen_commit  = qcow_reopen_commit,
+    .bdrv_reopen_abort   = qcow_reopen_abort,
     .bdrv_create	= qcow_create,
 
     .bdrv_co_readv          = qcow_co_readv,
-- 
1.7.11.4

[Qemu-devel] [PATCH v2 13/16] 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 | 47 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 47 insertions(+)

diff --git a/block/vmdk.c b/block/vmdk.c
index bba4c61..426dfc6 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -300,6 +300,50 @@ static int vmdk_is_cid_valid(BlockDriverState *bs)
     return 1;
 }
 
+/* Queue extents, if any, for reopen() */
+static int vmdk_reopen_prepare(BDRVReopenState *state, Error **errp)
+{
+    BDRVVmdkState *s;
+    int ret = -1;
+    int i;
+    VmdkExtent *e;
+
+    assert(state != NULL);
+    assert(state->bs != NULL);
+
+    if (state->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(state->queue, e->file, state->flags);
+        }
+    }
+    ret = 0;
+
+exit:
+    return ret;
+}
+
+/* No action for commit, or abort.  All VMDK has to do is
+ * queue the extents for reopen */
+static void vmdk_reopen_commit(BDRVReopenState *state)
+{
+}
+
+static void vmdk_reopen_abort(BDRVReopenState *state)
+{
+}
+
+
 static int vmdk_parent_open(BlockDriverState *bs)
 {
     char *p_name;
@@ -1646,6 +1690,9 @@ static BlockDriver bdrv_vmdk = {
     .instance_size  = sizeof(BDRVVmdkState),
     .bdrv_probe     = vmdk_probe,
     .bdrv_open      = vmdk_open,
+    .bdrv_reopen_prepare = vmdk_reopen_prepare,
+    .bdrv_reopen_commit  = vmdk_reopen_commit,
+    .bdrv_reopen_abort   = vmdk_reopen_abort,
     .bdrv_read      = vmdk_co_read,
     .bdrv_write     = vmdk_co_write,
     .bdrv_close     = vmdk_close,
-- 
1.7.11.4

[Qemu-devel] [PATCH v2 14/16] 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-posix.c |   1 -
 block/raw-win32.c | 104 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 104 insertions(+), 1 deletion(-)

diff --git a/block/raw-posix.c b/block/raw-posix.c
index 2407eeb..d0a931a 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -141,7 +141,6 @@ typedef struct BDRVRawState {
 } BDRVRawState;
 
 typedef struct BDRVRawReopenState {
-    BDRVReopenState reopen_state;
     int fd;
     int open_flags;
 #ifdef CONFIG_LINUX_AIO
diff --git a/block/raw-win32.c b/block/raw-win32.c
index 78c8306..cfa5def 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,102 @@ static int raw_open(BlockDriverState *bs, const char *filename, int flags)
     return 0;
 }
 
+static int raw_reopen_prepare(BDRVReopenState *state, 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 +388,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 v2 15/16] block: convert bdrv_commit() to use bdrv_reopen()

[Jeff Cody]

Currently, bdrv_commit() reopens images r/w itself, via bdrv_delete()
and bdrv_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 a54022b..1f24949 100644
--- a/block.c
+++ b/block.c
@@ -1490,13 +1490,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;
@@ -1505,42 +1503,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;
@@ -1577,20 +1551,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 v2 16/16] 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 1f24949..bc994df 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 67f8858..e533ca4 100644
--- a/block_int.h
+++ b/block_int.h
@@ -274,7 +274,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