diff mbox

[PATCHv2,1/2] Xen PV backend: Move call to bdrv_new from blk_init to blk_connect

Message ID 1365157905-22987-1-git-send-email-alex@alex.org.uk
State New
Headers show

Commit Message

Alex Bligh April 5, 2013, 10:31 a.m. UTC 
This commit delays the point at which bdrv_new (and hence blk_open
on the underlying device) is called from blk_init to blk_connect.
This ensures that in an inbound live migrate, the block device is
not opened until it has been closed at the other end. This is in
preparation for supporting devices with open/close consistency
without using O_DIRECT. This commit does NOT itself change O_DIRECT
semantics.

Commit f3903bbac78a81fcbce1350cdce860764a62783a (in xen's qemu-upstream
repo but not in qemu's repo) should be reverted prior to applying
this commit.

Signed-off-by: Alex Bligh <alex@alex.org.uk>
---
 hw/xen_disk.c |   71 ++++++++++++++++++++++++++++++++-------------------------
 1 file changed, 40 insertions(+), 31 deletions(-)

Comments

Stefano Stabellini April 5, 2013, 2:22 p.m. UTC | #1 
On Fri, 5 Apr 2013, Alex Bligh wrote:
> This commit delays the point at which bdrv_new (and hence blk_open
> on the underlying device) is called from blk_init to blk_connect.
> This ensures that in an inbound live migrate, the block device is
> not opened until it has been closed at the other end. This is in
> preparation for supporting devices with open/close consistency
> without using O_DIRECT. This commit does NOT itself change O_DIRECT
> semantics.
> 
> Commit f3903bbac78a81fcbce1350cdce860764a62783a (in xen's qemu-upstream
> repo but not in qemu's repo) should be reverted prior to applying
> this commit.
> 
> Signed-off-by: Alex Bligh <alex@alex.org.uk>
> ---
>  hw/xen_disk.c |   71 ++++++++++++++++++++++++++++++++-------------------------
>  1 file changed, 40 insertions(+), 31 deletions(-)
> 
> diff --git a/hw/xen_disk.c b/hw/xen_disk.c
> index 69e1d9d..dd5a711 100644
> --- a/hw/xen_disk.c
> +++ b/hw/xen_disk.c
> @@ -701,7 +701,7 @@ static void blk_alloc(struct XenDevice *xendev)
>  static int blk_init(struct XenDevice *xendev)
>  {
>      struct XenBlkDev *blkdev = container_of(xendev, struct XenBlkDev, xendev);
> -    int index, qflags, info = 0;
> +    int info = 0;
>  
>      /* read xenstore entries */
>      if (blkdev->params == NULL) {
> @@ -744,10 +744,7 @@ static int blk_init(struct XenDevice *xendev)
>      }
>  
>      /* read-only ? */
> -    qflags = BDRV_O_NOCACHE | BDRV_O_CACHE_WB | BDRV_O_NATIVE_AIO;
> -    if (strcmp(blkdev->mode, "w") == 0) {
> -        qflags |= BDRV_O_RDWR;
> -    } else {
> +    if (strcmp(blkdev->mode, "w")) {
>          info  |= VDISK_READONLY;
>      }
>  
> @@ -756,6 +753,41 @@ static int blk_init(struct XenDevice *xendev)
>          info  |= VDISK_CDROM;
>      }
>  
> +    blkdev->file_blk  = BLOCK_SIZE;

This is useless, you are setting file_blk twice. Just remove this please.
Alex Bligh April 5, 2013, 3:43 p.m. UTC | #2 
Stefano,

--On 5 April 2013 15:22:16 +0100 Stefano Stabellini 
<stefano.stabellini@eu.citrix.com> wrote:

>> @@ -756,6 +753,41 @@ static int blk_init(struct XenDevice *xendev)
>>          info  |= VDISK_CDROM;
>>      }
>>
>> +    blkdev->file_blk  = BLOCK_SIZE;
>
> This is useless, you are setting file_blk twice. Just remove this please.

Actually the problem was I failed to remove it from blk_connect on the
qemu-upstream-unstable patch set. The qemu-upstream-4.2 patch set is fine.

I've thus resent, with it still in blk_init, but removed from blk_connect.
diff mbox

Patch

diff --git a/hw/xen_disk.c b/hw/xen_disk.c
index 69e1d9d..dd5a711 100644
--- a/hw/xen_disk.c
+++ b/hw/xen_disk.c
@@ -701,7 +701,7 @@  static void blk_alloc(struct XenDevice *xendev)
 static int blk_init(struct XenDevice *xendev)
 {
     struct XenBlkDev *blkdev = container_of(xendev, struct XenBlkDev, xendev);
-    int index, qflags, info = 0;
+    int info = 0;
 
     /* read xenstore entries */
     if (blkdev->params == NULL) {
@@ -744,10 +744,7 @@  static int blk_init(struct XenDevice *xendev)
     }
 
     /* read-only ? */
-    qflags = BDRV_O_NOCACHE | BDRV_O_CACHE_WB | BDRV_O_NATIVE_AIO;
-    if (strcmp(blkdev->mode, "w") == 0) {
-        qflags |= BDRV_O_RDWR;
-    } else {
+    if (strcmp(blkdev->mode, "w")) {
         info  |= VDISK_READONLY;
     }
 
@@ -756,6 +753,41 @@  static int blk_init(struct XenDevice *xendev)
         info  |= VDISK_CDROM;
     }
 
+    blkdev->file_blk  = BLOCK_SIZE;
+
+    /* fill info
+     * blk_connect supplies sector-size and sectors
+     */
+    xenstore_write_be_int(&blkdev->xendev, "feature-flush-cache", 1);
+    xenstore_write_be_int(&blkdev->xendev, "feature-persistent", 1);
+    xenstore_write_be_int(&blkdev->xendev, "info", info);
+    return 0;
+
+out_error:
+    g_free(blkdev->params);
+    blkdev->params = NULL;
+    g_free(blkdev->mode);
+    blkdev->mode = NULL;
+    g_free(blkdev->type);
+    blkdev->type = NULL;
+    g_free(blkdev->dev);
+    blkdev->dev = NULL;
+    g_free(blkdev->devtype);
+    blkdev->devtype = NULL;
+    return -1;
+}
+
+static int blk_connect(struct XenDevice *xendev)
+{
+    struct XenBlkDev *blkdev = container_of(xendev, struct XenBlkDev, xendev);
+    int pers, index, qflags;
+
+    /* read-only ? */
+    qflags = BDRV_O_NOCACHE | BDRV_O_CACHE_WB | BDRV_O_NATIVE_AIO;
+    if (strcmp(blkdev->mode, "w") == 0) {
+        qflags |= BDRV_O_RDWR;
+    }
+
     /* init qemu block driver */
     index = (blkdev->xendev.dev - 202 * 256) / 16;
     blkdev->dinfo = drive_get(IF_XEN, 0, index);
@@ -771,7 +803,7 @@  static int blk_init(struct XenDevice *xendev)
             }
         }
         if (!blkdev->bs) {
-            goto out_error;
+            return -1;
         }
     } else {
         /* setup via qemu cmdline -> already setup for us */
@@ -793,33 +825,10 @@  static int blk_init(struct XenDevice *xendev)
                   blkdev->type, blkdev->fileproto, blkdev->filename,
                   blkdev->file_size, blkdev->file_size >> 20);
 
-    /* fill info */
-    xenstore_write_be_int(&blkdev->xendev, "feature-flush-cache", 1);
-    xenstore_write_be_int(&blkdev->xendev, "feature-persistent", 1);
-    xenstore_write_be_int(&blkdev->xendev, "info",            info);
-    xenstore_write_be_int(&blkdev->xendev, "sector-size",     blkdev->file_blk);
+    /* Fill in number of sector size and number of sectors */
+    xenstore_write_be_int(&blkdev->xendev, "sector-size", blkdev->file_blk);
     xenstore_write_be_int(&blkdev->xendev, "sectors",
                           blkdev->file_size / blkdev->file_blk);
-    return 0;
-
-out_error:
-    g_free(blkdev->params);
-    blkdev->params = NULL;
-    g_free(blkdev->mode);
-    blkdev->mode = NULL;
-    g_free(blkdev->type);
-    blkdev->type = NULL;
-    g_free(blkdev->dev);
-    blkdev->dev = NULL;
-    g_free(blkdev->devtype);
-    blkdev->devtype = NULL;
-    return -1;
-}
-
-static int blk_connect(struct XenDevice *xendev)
-{
-    struct XenBlkDev *blkdev = container_of(xendev, struct XenBlkDev, xendev);
-    int pers;
 
     if (xenstore_read_fe_int(&blkdev->xendev, "ring-ref", &blkdev->ring_ref) == -1) {
         return -1;