Patchwork [RFC,4/7] Qemu: Framework for reopening image files safely

login
register
mail settings
Submitter Supriya Kannery
Date Feb. 1, 2012, 3:06 a.m.
Message ID <20120201030658.2990.15176.sendpatchset@skannery.in.ibm.com>
Download mbox | patch
Permalink /patch/138894/
State New
Headers show

Comments

Supriya Kannery - Feb. 1, 2012, 3:06 a.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>
Stefan Hajnoczi - Feb. 7, 2012, 10:08 a.m.
On Wed, Feb 01, 2012 at 08:36:58AM +0530, 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>
> 
> Index: qemu/block.c
> ===================================================================
> --- qemu.orig/block.c
> +++ qemu/block.c
> @@ -808,10 +808,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);

Indentation should be 4 spaces.  I suggest configuring your editor so
this is always correct and done automatically.

> +}
> +
> +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();
> @@ -820,17 +842,32 @@ 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_REOPEN_FILE_FAILED, bs->filename);
> -        ret = bdrv_open(bs, bs->filename, open_flags, drv);
> +    /* Use driver specific reopen() if available */
> +    if (drv->bdrv_reopen_prepare) {
> +        ret = bdrv_reopen_prepare(bs, &reopen_state, bdrv_flags);
> +         if (ret < 0) {

Indentation

> +            bdrv_reopen_abort(bs, reopen_state);
> +            qerror_report(QERR_REOPEN_FILE_FAILED, bs->filename);
> +            return ret;
> +        }
> +
> +        bdrv_reopen_commit(bs, reopen_state);
> +        bs->open_flags = bdrv_flags;

Is bs->open_flags not assigned inside bdrv_reopen_*()?

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

This should be a post-condition of bdrv_open().  If you have found a
case where bs->drv != NULL after bdrv_open() fails, then please fix that
code path instead of assigning NULL here.
Kevin Wolf - Feb. 8, 2012, 3:07 p.m.
Am 01.02.2012 04:06, 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
> @@ -808,10 +808,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();
> @@ -820,17 +842,32 @@ 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_REOPEN_FILE_FAILED, bs->filename);
> -        ret = bdrv_open(bs, bs->filename, open_flags, drv);
> +    /* 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_REOPEN_FILE_FAILED, bs->filename);
> +            return ret;
> +        }
> +
> +        bdrv_reopen_commit(bs, reopen_state);
> +        bs->open_flags = bdrv_flags;
> +
> +    } else {
> +       open_flags = bs->open_flags;
> +       bdrv_close(bs);
> +
> +       ret = bdrv_open(bs, bs->filename, bdrv_flags, drv);
>          if (ret < 0) {
> -            /* Reopen failed with orig and modified flags */
> -            abort();
> +            /* Reopen failed. Try to open with original flags */
> +            qerror_report(QERR_REOPEN_FILE_FAILED, bs->filename);
> +            ret = bdrv_open(bs, bs->filename, open_flags, drv);
> +            if (ret < 0) {
> +                /* Reopen failed with orig and modified flags */
> +                bs->drv = NULL;
> +            }
>          }

Most image formats don't have a bdrv_reopen_* implementation after this
series, so usually you'll have something like qcow2 on top of file. This
code uses bdrv_close/open for the whole stack, even though the file
layer could actually make use of a bdrv_reopen_* implementation and the
qcow2 open isn't likely to fail if the image file could be opened.

I think we can use drv->bdrv_close/open to reopen only one layer and try
using bdrv_reopen_* for the lower layer again.

This is an improvement that can be done in a separate patch, though.

Kevin
Supriya Kannery - Feb. 13, 2012, 1:49 p.m.
On 02/08/2012 08:37 PM, Kevin Wolf wrote:
> Am 01.02.2012 04:06, 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.

>> +    } else {
>> +       open_flags = bs->open_flags;
>> +       bdrv_close(bs);
>> +
>> +       ret = bdrv_open(bs, bs->filename, bdrv_flags, drv);
>>           if (ret<  0) {
>> -            /* Reopen failed with orig and modified flags */
>> -            abort();
>> +            /* Reopen failed. Try to open with original flags */
>> +            qerror_report(QERR_REOPEN_FILE_FAILED, bs->filename);
>> +            ret = bdrv_open(bs, bs->filename, open_flags, drv);
>> +            if (ret<  0) {
>> +                /* Reopen failed with orig and modified flags */
>> +                bs->drv = NULL;
>> +            }
>>           }
>
> Most image formats don't have a bdrv_reopen_* implementation after this
> series, so usually you'll have something like qcow2 on top of file. This
> code uses bdrv_close/open for the whole stack, even though the file
> layer could actually make use of a bdrv_reopen_* implementation and the
> qcow2 open isn't likely to fail if the image file could be opened.
>
> I think we can use drv->bdrv_close/open to reopen only one layer and try
> using bdrv_reopen_* for the lower layer again.
>
> This is an improvement that can be done in a separate patch, though.

What I understood is, in the enhancement patch, we will have something 
like (taking qcow2 as an example)

Implement bdrv_reopen_qcow2(image file) which reopens only the qcow2
image file

Then,  drv->bdrv_open(qcow2 driver) will reopen qcow2 driver
        => calls bdrv_reopen_qcow2(qcow2 image file) if image file has
           to be reopen

Can you please explain a bit more, it this is not what you meant.

-thanks, Supriya
Supriya Kannery - Feb. 14, 2012, 1:34 p.m.
On 02/07/2012 03:38 PM, Stefan Hajnoczi wrote:
> On Wed, Feb 01, 2012 at 08:36:58AM +0530, 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.
>>

>> +int bdrv_reopen_prepare(BlockDriverState *bs, BDRVReopenState **prs, int flags)
>> +{
>> +     BlockDriver *drv = bs->drv;
>> +
>> +     return drv->bdrv_reopen_prepare(bs, prs, flags);
>
> Indentation should be 4 spaces.  I suggest configuring your editor so
> this is always correct and done automatically.
>

ok

>> -    ret = bdrv_open(bs, bs->filename, bdrv_flags, drv);
>> -    if (ret<  0) {
>> -        /* Reopen failed. Try to open with original flags */
>> -        qerror_report(QERR_REOPEN_FILE_FAILED, bs->filename);
>> -        ret = bdrv_open(bs, bs->filename, open_flags, drv);
>> +    /* Use driver specific reopen() if available */
>> +    if (drv->bdrv_reopen_prepare) {
>> +        ret = bdrv_reopen_prepare(bs,&reopen_state, bdrv_flags);
>> +         if (ret<  0) {
>
> Indentation
>
>> +            bdrv_reopen_abort(bs, reopen_state);
>> +            qerror_report(QERR_REOPEN_FILE_FAILED, bs->filename);
>> +            return ret;
>> +        }
>> +
>> +        bdrv_reopen_commit(bs, reopen_state);
>> +        bs->open_flags = bdrv_flags;
>
> Is bs->open_flags not assigned inside bdrv_reopen_*()?
>

No, Since it is generic for all the drivers, placed it here.

>> +
>> +    } else {
>> +       open_flags = bs->open_flags;
>> +       bdrv_close(bs);
>> +
>> +       ret = bdrv_open(bs, bs->filename, bdrv_flags, drv);
>>           if (ret<  0) {
>> -            /* Reopen failed with orig and modified flags */
>> -            abort();
>> +            /* Reopen failed. Try to open with original flags */
>> +            qerror_report(QERR_REOPEN_FILE_FAILED, bs->filename);
>> +            ret = bdrv_open(bs, bs->filename, open_flags, drv);
>> +            if (ret<  0) {
>> +                /* Reopen failed with orig and modified flags */
>> +                bs->drv = NULL;
>
> This should be a post-condition of bdrv_open().  If you have found a
> case where bs->drv != NULL after bdrv_open() fails, then please fix that
> code path instead of assigning NULL here.
>

ok, will check on this

-thanks for reviewing, Supriya

Patch

Index: qemu/block.c
===================================================================
--- qemu.orig/block.c
+++ qemu/block.c
@@ -808,10 +808,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();
@@ -820,17 +842,32 @@  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_REOPEN_FILE_FAILED, bs->filename);
-        ret = bdrv_open(bs, bs->filename, open_flags, drv);
+    /* 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_REOPEN_FILE_FAILED, bs->filename);
+            return ret;
+        }
+
+        bdrv_reopen_commit(bs, reopen_state);
+        bs->open_flags = bdrv_flags;
+
+    } else {
+       open_flags = bs->open_flags;
+       bdrv_close(bs);
+
+       ret = bdrv_open(bs, bs->filename, bdrv_flags, drv);
         if (ret < 0) {
-            /* Reopen failed with orig and modified flags */
-            abort();
+            /* Reopen failed. Try to open with original flags */
+            qerror_report(QERR_REOPEN_FILE_FAILED, bs->filename);
+            ret = bdrv_open(bs, bs->filename, open_flags, drv);
+            if (ret < 0) {
+                /* Reopen failed with orig and modified flags */
+                bs->drv = NULL;
+            }
         }
     }
 
Index: qemu/block_int.h
===================================================================
--- qemu.orig/block_int.h
+++ qemu/block_int.h
@@ -105,6 +105,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);
@@ -299,6 +306,10 @@  struct BlockDriverState {
     BlockJob *job;
 };
 
+struct BDRVReopenState {
+    BlockDriverState *bs;
+};
+
 struct BlockDriverAIOCB {
     AIOPool *pool;
     BlockDriverState *bs;
Index: qemu/qemu-common.h
===================================================================
--- qemu.orig/qemu-common.h
+++ qemu/qemu-common.h
@@ -210,6 +210,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
@@ -120,6 +120,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);