diff mbox

[RFC] Safely reopening image files by stashing fds

Message ID 4E4AC29F.7010601@linux.vnet.ibm.com
State New
Headers show

Commit Message

Supriya Kannery Aug. 16, 2011, 7:18 p.m. UTC 
On 08/09/2011 03:02 PM, supriya kannery wrote:
 > Kevin Wolf wrote:
 >> Am 09.08.2011 11:22, schrieb supriya kannery:
 >>> Kevin Wolf wrote:
 >>
 >> What I meant is that in the end, with a generic bdrv_reopen(), we can
 >> have raw-posix only call dup() and fcntl() instead of doing a
 >> close()/open() sequence if it can satisfy the new flags this way. But
 >> this would be an implementation detail and not be visible in the
 >> interface.
 >>
 >> Kevin
 >
 > ok
 > - thanks, Supriya
 >

Though I started the RFC patch with defining BDRVReopenState, ended up
in enhancing struct BlockDriver with .bdrv_reopen. bdrv_reopen 
mplementation specific to respective driver is assigned to this
function pointer.

Please find the implementation of O_DIRECT flag change, done in
raw-posix.c. Similar implementation can be done for vmdk (with
bdrv_reopen_commit and bdrv_reopen_abort as internal functions in
vmdk.c for opening split files), sheepdog, nbd etc..

Request for comments on this approach.

Note: Following patch is for demonstrating the approach using
raw-posix  as sample driver. This patch is not complete.

- thanks, Supriya

---
  block.c           |   33 +++++++++++++++++++++++----------
  block/raw-posix.c |   34 ++++++++++++++++++++++++++++++++++
  block_int.h       |    1 +
  3 files changed, 58 insertions(+), 10 deletions(-)

      int (*bdrv_read)(BlockDriverState *bs, int64_t sector_num,
                       uint8_t *buf, int nb_sectors);

Comments

Kevin Wolf Aug. 17, 2011, 2:35 p.m. UTC | #1 
Am 16.08.2011 21:18, schrieb Supriya Kannery:
> On 08/09/2011 03:02 PM, supriya kannery wrote:
>  > Kevin Wolf wrote:
>  >> Am 09.08.2011 11:22, schrieb supriya kannery:
>  >>> Kevin Wolf wrote:
>  >>
>  >> What I meant is that in the end, with a generic bdrv_reopen(), we can
>  >> have raw-posix only call dup() and fcntl() instead of doing a
>  >> close()/open() sequence if it can satisfy the new flags this way. But
>  >> this would be an implementation detail and not be visible in the
>  >> interface.
>  >>
>  >> Kevin
>  >
>  > ok
>  > - thanks, Supriya
>  >
> 
> Though I started the RFC patch with defining BDRVReopenState, ended up
> in enhancing struct BlockDriver with .bdrv_reopen. bdrv_reopen 
> mplementation specific to respective driver is assigned to this
> function pointer.
> 
> Please find the implementation of O_DIRECT flag change, done in
> raw-posix.c. Similar implementation can be done for vmdk (with
> bdrv_reopen_commit and bdrv_reopen_abort as internal functions in
> vmdk.c for opening split files), sheepdog, nbd etc..

I haven't looked at the code yet, but this can't work. I have thought of
BDRVReopenState for a reason. If you have multiple files like VMDK can
have, then you want an all-or-nothing semantics of reopen.

With only .bdrv_reopen, I can't see a way to guarantee this. If
reopening the first file succeeds and the second one goes wrong, you
have the new flags on the first file, but the old flags on the second
one. You would have to re-reopen the first file with the old flags, but
then this is not guaranteed to succeed.

Kevin
diff mbox

Patch

Index: qemu/block/raw-posix.c
===================================================================
--- qemu.orig/block/raw-posix.c
+++ qemu/block/raw-posix.c
@@ -276,6 +276,39 @@  static int raw_open(BlockDriverState *bs
      return raw_open_common(bs, filename, flags, 0);
  }

+static int raw_reopen(BlockDriverState *bs, int flags)
+{
+    BDRVRawState *s = bs->opaque;
+    int new_fd;
+    int ret = 0;
+
+    /* Handle request for toggling O_DIRECT */
+    if ((bs->open_flags | BDRV_O_NOCACHE) ^
+        (flags | BDRV_O_NOCACHE))
+    {
+        if ((new_fd = dup(s->fd)) <=  0) {
+            return -1;
+        }
+
+        if ((ret = fcntl_setfl(new_fd, flags)) < 0) {
+            close(new_fd);
+            return ret;
+        }
+
+        /* Set new flags, so replace old fd with new one */
+        close(s->fd);
+        s->fd = new_fd;
+        s->open_flags = flags;
+        bs->open_flags = flags;
+    }
+
+    /*
+     * TBD: handle O_DSYNC and other flags for which image
+     * file has to be reopened
+     */
+    return 0;
+}
+
  /* XXX: use host sector size if necessary with:
  #ifdef DIOCGSECTORSIZE
          {
@@ -886,6 +919,7 @@  static BlockDriver bdrv_file = {
      .instance_size = sizeof(BDRVRawState),
      .bdrv_probe = NULL, /* no probe for protocols */
      .bdrv_file_open = raw_open,
+    .bdrv_reopen = raw_reopen,
      .bdrv_read = raw_read,
      .bdrv_write = raw_write,
      .bdrv_close = raw_close,
Index: qemu/block.c
===================================================================
--- qemu.orig/block.c
+++ qemu/block.c
@@ -687,20 +687,33 @@  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) {
+        if ((ret = drv->bdrv_reopen(bs, bdrv_flags)) < 0) {
+            qerror_report(QERR_REOPEN_FILE_FAILED, bs->filename);
+            return ret;
+        }
+    } else {
+        /* Use reopen procedure common for drivers */
+        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 with orig and modified flags failed
+                 */
+                abort();
+            }
          }
      }
-
      return ret;
  }

Index: qemu/block_int.h
===================================================================
--- qemu.orig/block_int.h
+++ qemu/block_int.h
@@ -54,6 +54,7 @@  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);
+    int (*bdrv_reopen)(BlockDriverState *bs, int flags);
      int (*bdrv_file_open)(BlockDriverState *bs, const char *filename, 
int flags);