Patchwork [v1,4/10] Qemu: Framework for reopening image files safely

login
register
mail settings
Submitter Supriya Kannery
Date June 15, 2012, 8:47 p.m.
Message ID <20120615204744.9853.62830.sendpatchset@skannery.in.ibm.com>
Download mbox | patch
Permalink /patch/165221/
State New
Headers show

Comments

Supriya Kannery - June 15, 2012, 8:47 p.m.
Struct BDRVReopenState along with three reopen related functions
introduced for handling reopening of images safely. This can be
extended by each of the block drivers to reopen respective
image files.

Signed-off-by: Supriya Kannery <supriyak@linux.vnet.ibm.com>
Eric Blake - June 15, 2012, 10:02 p.m.
On 06/15/2012 02:47 PM, Supriya Kannery wrote:
> Struct BDRVReopenState along with three reopen related functions
> introduced for handling reopening of images safely. This can be
> extended by each of the block drivers to reopen respective
> image files.
> 
> Signed-off-by: Supriya Kannery <supriyak@linux.vnet.ibm.com>
> 

> +    /* Use driver specific reopen() if available */
> +    if (drv->bdrv_reopen_prepare) {
> +        ret = bdrv_reopen_prepare(bs, &reopen_state, bdrv_flags);
> +         if (ret < 0) {
> +            bdrv_reopen_abort(bs, reopen_state);
> +            qerror_report(QERR_OPEN_FILE_FAILED, bs->filename);
> +            return ret;
> +        }
> +
> +        bdrv_reopen_commit(bs, reopen_state);
> +        bs->open_flags = bdrv_flags;

Good.

> +
> +    } else {
> +
> +        /* Try reopening the image using protocol or directly */
> +        if (bs->file) {
> +            open_flags = bs->open_flags;
> +            drv->bdrv_close(bs);
> +            if (drv->bdrv_file_open) {
> +                ret = drv->bdrv_file_open(bs, bs->filename, bdrv_flags);

Not so good.  Are we sure we can't flush all data, so that we can then
attempt the open before the close, and avoid losing the original file on
error?

> +            } else {
> +                ret = bdrv_file_open(&bs->file, bs->filename, bdrv_flags);
> +            }
> +            if (ret < 0) {
> +                /* Reopen failed. Try to open with original flags */
> +                qerror_report(QERR_OPEN_FILE_FAILED, bs->filename);
> +                if (drv->bdrv_file_open) {
> +                    ret = drv->bdrv_file_open(bs, bs->filename, open_flags);
> +                } else {
> +                    ret = bdrv_file_open(&bs->file, bs->filename, open_flags);
> +                }
> +                if (ret < 0) {
> +                    /* Reopen failed with orig and modified flags */
> +                    bdrv_delete(bs->file);
> +                    bs->drv = NULL;

You dropped the abort() here, but now the device is silently gone.  But
since the error of QERR_OPEN_FILE_FAILED is the same as in the safe
case, how does the management app know that things were corrupted?
Kevin Wolf - July 9, 2012, 3:06 p.m.
Am 15.06.2012 22:47, schrieb Supriya Kannery:
> Struct BDRVReopenState along with three reopen related functions
> introduced for handling reopening of images safely. This can be
> extended by each of the block drivers to reopen respective
> image files.
> 
> Signed-off-by: Supriya Kannery <supriyak@linux.vnet.ibm.com>
> 
> Index: qemu/block.c
> ===================================================================
> --- qemu.orig/block.c
> +++ qemu/block.c
> @@ -858,10 +858,32 @@ unlink_and_fail:
>      return ret;
>  }
>  
> +int bdrv_reopen_prepare(BlockDriverState *bs, BDRVReopenState **prs, int flags)
> +{
> +     BlockDriver *drv = bs->drv;
> +
> +     return drv->bdrv_reopen_prepare(bs, prs, flags);
> +}
> +
> +void bdrv_reopen_commit(BlockDriverState *bs, BDRVReopenState *rs)
> +{
> +    BlockDriver *drv = bs->drv;
> +
> +    drv->bdrv_reopen_commit(bs, rs);
> +}
> +
> +void bdrv_reopen_abort(BlockDriverState *bs, BDRVReopenState *rs)
> +{
> +    BlockDriver *drv = bs->drv;
> +
> +    drv->bdrv_reopen_abort(bs, rs);
> +}
> +
>  int bdrv_reopen(BlockDriverState *bs, int bdrv_flags)
>  {
>      BlockDriver *drv = bs->drv;
>      int ret = 0, open_flags;
> +    BDRVReopenState *reopen_state = NULL;
>  
>      /* Quiesce IO for the given block device */
>      qemu_aio_flush();
> @@ -870,17 +892,44 @@ int bdrv_reopen(BlockDriverState *bs, in
>          qerror_report(QERR_DATA_SYNC_FAILED, bs->device_name);
>          return ret;
>      }
> -    open_flags = bs->open_flags;
> -    bdrv_close(bs);
>  
> -    ret = bdrv_open(bs, bs->filename, bdrv_flags, drv);
> -    if (ret < 0) {
> -        /* Reopen failed. Try to open with original flags */
> -        qerror_report(QERR_OPEN_FILE_FAILED, bs->filename);
> -        ret = bdrv_open(bs, bs->filename, open_flags, drv);
> -        if (ret < 0) {
> -            /* Reopen failed with orig and modified flags */
> -            abort();
> +    /* Use driver specific reopen() if available */
> +    if (drv->bdrv_reopen_prepare) {
> +        ret = bdrv_reopen_prepare(bs, &reopen_state, bdrv_flags);
> +         if (ret < 0) {
> +            bdrv_reopen_abort(bs, reopen_state);
> +            qerror_report(QERR_OPEN_FILE_FAILED, bs->filename);
> +            return ret;
> +        }
> +
> +        bdrv_reopen_commit(bs, reopen_state);
> +        bs->open_flags = bdrv_flags;
> +
> +    } else {
> +
> +        /* Try reopening the image using protocol or directly */
> +        if (bs->file) {
> +            open_flags = bs->open_flags;
> +            drv->bdrv_close(bs);
> +            if (drv->bdrv_file_open) {
> +                ret = drv->bdrv_file_open(bs, bs->filename, bdrv_flags);
> +            } else {
> +                ret = bdrv_file_open(&bs->file, bs->filename, bdrv_flags);

Doesn't this forget to reopen bs itself? And bs->file wasn't even
closed. If think we need something more along the lines of:

if (bs->file) {
    bdrv_reopen(bs->file)
}

if (drv->bdrv_file_open) {
    drv->bdrv_file_open(bs)
} else {
   drv->bdrv_open(bs)
}

In fact we would really want to be able to commit/abort the bdrv_reopen
of bs->file only after we know if bdrv_open(bs) has succeeded, but with
the current design we can't because bdrv_open wants to read from the new fd.

Maybe it would make sense to require bdrv_reopen_prepare() to do the
switch without throwing the old state away yet, but it sounds as if it
would make implementations for image formats quite a bit harder.

Maybe we should completely avoid this default implementation and just
fail bdrv_reopen if a driver doesn't support the explicit,
transactionable reopen functions.

Kevin

Patch

Index: qemu/block.c
===================================================================
--- qemu.orig/block.c
+++ qemu/block.c
@@ -858,10 +858,32 @@  unlink_and_fail:
     return ret;
 }
 
+int bdrv_reopen_prepare(BlockDriverState *bs, BDRVReopenState **prs, int flags)
+{
+     BlockDriver *drv = bs->drv;
+
+     return drv->bdrv_reopen_prepare(bs, prs, flags);
+}
+
+void bdrv_reopen_commit(BlockDriverState *bs, BDRVReopenState *rs)
+{
+    BlockDriver *drv = bs->drv;
+
+    drv->bdrv_reopen_commit(bs, rs);
+}
+
+void bdrv_reopen_abort(BlockDriverState *bs, BDRVReopenState *rs)
+{
+    BlockDriver *drv = bs->drv;
+
+    drv->bdrv_reopen_abort(bs, rs);
+}
+
 int bdrv_reopen(BlockDriverState *bs, int bdrv_flags)
 {
     BlockDriver *drv = bs->drv;
     int ret = 0, open_flags;
+    BDRVReopenState *reopen_state = NULL;
 
     /* Quiesce IO for the given block device */
     qemu_aio_flush();
@@ -870,17 +892,44 @@  int bdrv_reopen(BlockDriverState *bs, in
         qerror_report(QERR_DATA_SYNC_FAILED, bs->device_name);
         return ret;
     }
-    open_flags = bs->open_flags;
-    bdrv_close(bs);
 
-    ret = bdrv_open(bs, bs->filename, bdrv_flags, drv);
-    if (ret < 0) {
-        /* Reopen failed. Try to open with original flags */
-        qerror_report(QERR_OPEN_FILE_FAILED, bs->filename);
-        ret = bdrv_open(bs, bs->filename, open_flags, drv);
-        if (ret < 0) {
-            /* Reopen failed with orig and modified flags */
-            abort();
+    /* Use driver specific reopen() if available */
+    if (drv->bdrv_reopen_prepare) {
+        ret = bdrv_reopen_prepare(bs, &reopen_state, bdrv_flags);
+         if (ret < 0) {
+            bdrv_reopen_abort(bs, reopen_state);
+            qerror_report(QERR_OPEN_FILE_FAILED, bs->filename);
+            return ret;
+        }
+
+        bdrv_reopen_commit(bs, reopen_state);
+        bs->open_flags = bdrv_flags;
+
+    } else {
+
+        /* Try reopening the image using protocol or directly */
+        if (bs->file) {
+            open_flags = bs->open_flags;
+            drv->bdrv_close(bs);
+            if (drv->bdrv_file_open) {
+                ret = drv->bdrv_file_open(bs, bs->filename, bdrv_flags);
+            } else {
+                ret = bdrv_file_open(&bs->file, bs->filename, bdrv_flags);
+            }
+            if (ret < 0) {
+                /* Reopen failed. Try to open with original flags */
+                qerror_report(QERR_OPEN_FILE_FAILED, bs->filename);
+                if (drv->bdrv_file_open) {
+                    ret = drv->bdrv_file_open(bs, bs->filename, open_flags);
+                } else {
+                    ret = bdrv_file_open(&bs->file, bs->filename, open_flags);
+                }
+                if (ret < 0) {
+                    /* Reopen failed with orig and modified flags */
+                    bdrv_delete(bs->file);
+                    bs->drv = NULL;
+                }
+            }
         }
     }
 
Index: qemu/block_int.h
===================================================================
--- qemu.orig/block_int.h
+++ qemu/block_int.h
@@ -137,6 +137,13 @@  struct BlockDriver {
     int (*bdrv_probe)(const uint8_t *buf, int buf_size, const char *filename);
     int (*bdrv_probe_device)(const char *filename);
     int (*bdrv_open)(BlockDriverState *bs, int flags);
+
+    /* For handling image reopen for split or non-split files */
+    int (*bdrv_reopen_prepare)(BlockDriverState *bs,
+                               BDRVReopenState **prs,
+                               int flags);
+    void (*bdrv_reopen_commit)(BlockDriverState *bs, BDRVReopenState *rs);
+    void (*bdrv_reopen_abort)(BlockDriverState *bs, BDRVReopenState *rs);
     int (*bdrv_file_open)(BlockDriverState *bs, const char *filename, int flags);
     int (*bdrv_read)(BlockDriverState *bs, int64_t sector_num,
                      uint8_t *buf, int nb_sectors);
@@ -335,6 +342,10 @@  struct BlockDriverState {
     BlockJob *job;
 };
 
+struct BDRVReopenState {
+    BlockDriverState *bs;
+};
+
 int get_tmp_filename(char *filename, int size);
 
 void bdrv_set_io_limits(BlockDriverState *bs,
Index: qemu/qemu-common.h
===================================================================
--- qemu.orig/qemu-common.h
+++ qemu/qemu-common.h
@@ -225,6 +225,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;
Index: qemu/block.h
===================================================================
--- qemu.orig/block.h
+++ qemu/block.h
@@ -129,6 +129,9 @@  int bdrv_file_open(BlockDriverState **pb
 int bdrv_open(BlockDriverState *bs, const char *filename, int flags,
               BlockDriver *drv);
 int bdrv_reopen(BlockDriverState *bs, int bdrv_flags);
+int bdrv_reopen_prepare(BlockDriverState *bs, BDRVReopenState **prs, int flags);
+void bdrv_reopen_commit(BlockDriverState *bs, BDRVReopenState *rs);
+void bdrv_reopen_abort(BlockDriverState *bs, BDRVReopenState *rs);
 void bdrv_close(BlockDriverState *bs);
 int bdrv_attach_dev(BlockDriverState *bs, void *dev);
 void bdrv_attach_dev_nofail(BlockDriverState *bs, void *dev);