Alex Bligh
2013-Apr-01 15:32 UTC
[PATCH] [RFC] Xen PV backend: Move call to bdrv_new from blk_init to blk_connect
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.
Note this patch is compile-tested only.
Signed-off-by: Alex Bligh <alex@alex.org.uk>
---
hw/xen_disk.c | 73 +++++++++++++++++++++++++++++++++------------------------
1 file changed, 42 insertions(+), 31 deletions(-)
diff --git a/hw/xen_disk.c b/hw/xen_disk.c
index 69e1d9d..3cccea8 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,44 @@ static int blk_init(struct XenDevice *xendev)
info |= VDISK_CDROM;
}
+ blkdev->file_blk = BLOCK_SIZE;
+
+ /* fill info
+ * Temporarily write zero sectors as we won''t know file size until
+ * bdrv_new has been called. blk_connect corrects this.
+ */
+ 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",
BLOCK_SIZE);
+ xenstore_write_be_int(&blkdev->xendev, "sectors", 0);
+ 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 +806,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 +828,9 @@ 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 sectors */
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;
--
1.7.9.5
Stefano Stabellini
2013-Apr-01 15:44 UTC
Re: [PATCH] [RFC] Xen PV backend: Move call to bdrv_new from blk_init to blk_connect
On Mon, 1 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. > > Note this patch is compile-tested only.I think the patch looks good, just a minor comment.> Signed-off-by: Alex Bligh <alex@alex.org.uk> > --- > hw/xen_disk.c | 73 +++++++++++++++++++++++++++++++++------------------------ > 1 file changed, 42 insertions(+), 31 deletions(-) > > diff --git a/hw/xen_disk.c b/hw/xen_disk.c > index 69e1d9d..3cccea8 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,44 @@ static int blk_init(struct XenDevice *xendev) > info |= VDISK_CDROM; > } > > + blkdev->file_blk = BLOCK_SIZE; > + > + /* fill info > + * Temporarily write zero sectors as we won''t know file size until > + * bdrv_new has been called. blk_connect corrects this. > + */ > + 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", BLOCK_SIZE); > + xenstore_write_be_int(&blkdev->xendev, "sectors", 0); > + return 0;There is no need to fill the sector-size and sectors info here, you can do it later in blk_connect.
Alex Bligh
2013-Apr-01 20:56 UTC
Re: [PATCH] [RFC] Xen PV backend: Move call to bdrv_new from blk_init to blk_connect
Stefano, --On 1 April 2013 16:44:05 +0100 Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote:>> Note this patch is compile-tested only. > > I think the patch looks good, just a minor comment.Thanks. I guess I ought to actually test it works then :-)>> + /* fill info >> + * Temporarily write zero sectors as we won''t know file size until >> + * bdrv_new has been called. blk_connect corrects this. >> + */ >> + 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", BLOCK_SIZE); >> + xenstore_write_be_int(&blkdev->xendev, "sectors", 0); >> + return 0; > > There is no need to fill the sector-size and sectors info here, you can > do it later in blk_connect.My concern (not knowing how xenstore works) was the possibility of leaving them as uninitialized values. I''m taking it that if I don''t initialise them, the key is just absent - correct? I''ll redo and move setting sector-size and sectors into blk_connect, and then add a second commit which removes BDRV_O_NOCACHE. Oh, and test it ... -- Alex Bligh
Stefano Stabellini
2013-Apr-02 11:08 UTC
Re: [PATCH] [RFC] Xen PV backend: Move call to bdrv_new from blk_init to blk_connect
On Mon, 1 Apr 2013, Alex Bligh wrote:> Stefano, > > --On 1 April 2013 16:44:05 +0100 Stefano Stabellini > <stefano.stabellini@eu.citrix.com> wrote: > > >> Note this patch is compile-tested only. > > > > I think the patch looks good, just a minor comment. > > Thanks. I guess I ought to actually test it works then :-) > > >> + /* fill info > >> + * Temporarily write zero sectors as we won''t know file size until > >> + * bdrv_new has been called. blk_connect corrects this. > >> + */ > >> + 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", BLOCK_SIZE); > >> + xenstore_write_be_int(&blkdev->xendev, "sectors", 0); > >> + return 0; > > > > There is no need to fill the sector-size and sectors info here, you can > > do it later in blk_connect. > > My concern (not knowing how xenstore works) was the possibility of leaving > them as uninitialized values. I''m taking it that if I don''t initialise > them, the key is just absent - correct?Right> I''ll redo and move setting sector-size and sectors into blk_connect, and > then add a second commit which removes BDRV_O_NOCACHE. Oh, and test it ...Good :)
Alex Bligh
2013-Apr-05 10:34 UTC
Re: [PATCH] [RFC] Xen PV backend: Move call to bdrv_new from blk_init to blk_connect
Stefano, --On 2 April 2013 12:08:25 +0100 Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote:>> I''ll redo and move setting sector-size and sectors into blk_connect, and >> then add a second commit which removes BDRV_O_NOCACHE. Oh, and test it >> ... > > Good :)I''ve just sent patches for qemu-upstream-unstable and qemu-upstream-4.2-testing to do this. These are pretty lightly tested but should be fine as it''s just moving lines around really. -- Alex Bligh