Re: [Qemu-devel] [RFC] Safely reopening image files by stashing fds

[Supriya Kannery <supriyak@linux.vnet.ibm.com>]

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

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);
      int (*bdrv_read)(BlockDriverState *bs, int64_t sector_num,
                       uint8_t *buf, int nb_sectors);