[PATCH] make qemu handle drbd properly

[PATCH] make qemu handle drbd properly

[James Harper]

This attached patch allows qemu to accept a drbd: prefix and interpret
it as a drbd resource. It then searches for it in /dev/drbd/by-res/
(which must be mapped via udev). It's against 4.0-testing.

Also, the block scripts make the drbd resource primary but don't want
for it to actually become available. A secondary drbd resource returns
EMEDIUMTYPE so if we get that result, wait half a second and retry, up
to a total of 5 retries. Fixing this in the block scripts might be
better but I can't see how to do it cleanly. 

James


diff --git a/xenstore.c b/xenstore.c
index 13b8d57..f6e2170 100644
--- a/xenstore.c
+++ b/xenstore.c
@@ -413,6 +413,9 @@ void xenstore_parse_domain_config(int hvm_domid)
     char *danger_buf = NULL;
     char *danger_type = NULL;

+    int retries = 1; /* number of attempts to open the block device -
drbd can take a second to become available */
+    int status;
+
     for(i = 0; i < MAX_DRIVES + 1; i++)
         media_filename[i] = NULL;

@@ -519,6 +522,14 @@ void xenstore_parse_domain_config(int hvm_domid)
            format = &bdrv_raw;
         }

+        if (!strcmp(drv, "drbd")) {
+            char *newparams = malloc(17 + strlen(params) + 1);
+            sprintf(newparams, "/dev/drbd/by-res/%s", params);
+            free(params);
+            params = newparams;
+           format = &bdrv_raw;
+            retries = 5;
+        }
 #if 0
        /* Phantom VBDs are disabled because the use of paths
         * from guest-controlled areas in xenstore is unsafe.
@@ -617,7 +628,14 @@ void xenstore_parse_domain_config(int hvm_domid)

             fprintf(stderr, "Using file %s in read-%s mode\n",
bs->filename, is_readonly ? "only" : "write");

-            if (bdrv_open2(bs, params, flags, format) < 0)
+            do {
+                int status = bdrv_open2(bs, params, flags, format);
+                if (status != -EMEDIUMTYPE)
+                    break;
+                usleep(500000); /* 500ms */
+                retries--;
+            } while (retries);
+            if (status < 0)
                 fprintf(stderr, "qemu: could not open vbd '%s' or hard
disk image '%s' (drv '%s' format '%s')\n", buf, params, drv ? drv : "?",
format ? format->format_name : "
         }

Re: [PATCH] make qemu handle drbd properly

[Ian Jackson]

James Harper writes ("[Xen-devel] [PATCH] make qemu handle drbd properly"):
> This attached patch allows qemu to accept a drbd: prefix and interpret
> it as a drbd resource. It then searches for it in /dev/drbd/by-res/
> (which must be mapped via udev). It's against 4.0-testing.

Jim, did any of your drbd tests involve HVM domains ?

The code in qemu-xen in 4.0-testing is going to be very similar to
that in xen-unstable.  I'm not sure whether we should be looking to
apply this to 4.1 ?

Ian.

Re: [PATCH] make qemu handle drbd properly

[Jim Fehlig]

[-- Attachment #1: Type: text/plain, Size: 756 bytes --]

Ian Jackson wrote:
> James Harper writes ("[Xen-devel] [PATCH] make qemu handle drbd properly"):
>   
>> This attached patch allows qemu to accept a drbd: prefix and interpret
>> it as a drbd resource. It then searches for it in /dev/drbd/by-res/
>> (which must be mapped via udev). It's against 4.0-testing.
>>     
>
> Jim, did any of your drbd tests involve HVM domains ?
>   

No, they did not :(.

But we have a similar hack (err, patch - attached for reference) for
other external block scripts that we support. Interestingly, drbd is
missing from the patch. I stumbled across this patch several months ago,
but the original author is gone and I have not had time to consider a
solution that will accommodate arbitrary external block scripts.

Jim



[-- Attachment #2: xen-qemu-iscsi-fix.patch --]
[-- Type: text/x-diff, Size: 2313 bytes --]

Index: xen-4.0.1-testing/tools/ioemu-qemu-xen/xenstore.c
===================================================================
--- xen-4.0.1-testing.orig/tools/ioemu-qemu-xen/xenstore.c
+++ xen-4.0.1-testing/tools/ioemu-qemu-xen/xenstore.c
@@ -399,7 +399,7 @@ void xenstore_parse_domain_config(int hv
     char *buf = NULL;
     char *fpath = NULL, *bpath = NULL, *btype = NULL,
         *dev = NULL, *params = NULL, *drv = NULL;
-    int i, ret, is_tap;
+    int i, j, ret, is_tap;
     unsigned int len, num, hd_index, pci_devid = 0;
     BlockDriverState *bs;
     BlockDriver *format;
@@ -491,12 +491,7 @@ void xenstore_parse_domain_config(int hv
             continue;
         free(danger_type);
         danger_type = xs_read(xsh, XBT_NULL, danger_buf, &len);
-        if (pasprintf(&buf, "%s/params", bpath) == -1)
-            continue;
-        free(params);
-        params = xs_read(xsh, XBT_NULL, buf, &len);
-        if (params == NULL)
-            continue;
+
         /* read the name of the device */
         if (pasprintf(&buf, "%s/type", bpath) == -1)
             continue;
@@ -504,6 +499,35 @@ void xenstore_parse_domain_config(int hv
         drv = xs_read(xsh, XBT_NULL, buf, &len);
         if (drv == NULL)
             continue;
+
+        free(params);
+        if (!strcmp(drv,"iscsi") || !strcmp(drv, "npiv") ||
+            !strcmp(drv,"dmmd")) {
+          if (pasprintf(&buf, "%s/node", bpath) == -1)
+             continue;
+
+          /* wait for block-[iscsi|npiv|dmmd] script to complete and populate the
+           * node entry.  try 30 times (30 secs) */
+          for (j = 0; j < 30; j++) {
+            params = xs_read(xsh, XBT_NULL, buf, &len);
+            if (params != NULL)
+               break;
+	    sleep(1);
+          }
+          if (params == NULL) {
+             fprintf(stderr, "qemu: %s device not found -- timed out \n", drv);
+            continue;
+          }
+        }
+        else
+        {
+          if (pasprintf(&buf, "%s/params", bpath) == -1)
+              continue;
+           params = xs_read(xsh, XBT_NULL, buf, &len);
+           if (params == NULL)
+             continue;
+        }
+
         /* Obtain blktap sub-type prefix */
         if (!strcmp(drv, "tap") && params[0]) {
             char *offset = strchr(params, ':'); 

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH] make qemu handle drbd properly

[Ian Jackson]

Jim Fehlig writes ("Re: [Xen-devel] [PATCH] make qemu handle drbd properly"):
> Ian Jackson wrote:
> > Jim, did any of your drbd tests involve HVM domains ?
> 
> No, they did not :(.
> 
> But we have a similar hack (err, patch - attached for reference) for
> other external block scripts that we support. Interestingly, drbd is
> missing from the patch. I stumbled across this patch several months ago,
> but the original author is gone and I have not had time to consider a
> solution that will accommodate arbitrary external block scripts.

Hrm.

In 4.2, with the deprecation of xend, I think we are going to revisit
the way block devices are set up and things will become a lot simpler
and more tractable.

Ian.

Re: [PATCH] make qemu handle drbd properly

[Shriram Rajagopalan]

[-- Attachment #1.1: Type: text/plain, Size: 2733 bytes --]

On Wed, Jan 26, 2011 at 11:43 PM, James Harper <
james.harper@bendigoit.com.au> wrote:

> This attached patch allows qemu to accept a drbd: prefix and interpret
> it as a drbd resource. It then searches for it in /dev/drbd/by-res/
> (which must be mapped via udev). It's against 4.0-testing.
>
> Also, the block scripts make the drbd resource primary but don't want
> for it to actually become available. A secondary drbd resource returns
> EMEDIUMTYPE so if we get that result, wait half a second and retry, up
> to a total of 5 retries. Fixing this in the block scripts might be
> better but I can't see how to do it cleanly.
>
> James
>
>
> diff --git a/xenstore.c b/xenstore.c
> index 13b8d57..f6e2170 100644
> --- a/xenstore.c
> +++ b/xenstore.c
> @@ -413,6 +413,9 @@ void xenstore_parse_domain_config(int hvm_domid)
>     char *danger_buf = NULL;
>     char *danger_type = NULL;
>
> +    int retries = 1; /* number of attempts to open the block device -
> drbd can take a second to become available */
> +    int status;
> +
>     for(i = 0; i < MAX_DRIVES + 1; i++)
>         media_filename[i] = NULL;
>
> @@ -519,6 +522,14 @@ void xenstore_parse_domain_config(int hvm_domid)
>            format = &bdrv_raw;
>         }
>
> +        if (!strcmp(drv, "drbd")) {
> +            char *newparams = malloc(17 + strlen(params) + 1);
> +            sprintf(newparams, "/dev/drbd/by-res/%s", params);
> +            free(params);
> +            params = newparams;
> +           format = &bdrv_raw;
> +            retries = 5;
> +        }
>  #if 0
>        /* Phantom VBDs are disabled because the use of paths
>         * from guest-controlled areas in xenstore is unsafe.
> @@ -617,7 +628,14 @@ void xenstore_parse_domain_config(int hvm_domid)
>
>             fprintf(stderr, "Using file %s in read-%s mode\n",
> bs->filename, is_readonly ? "only" : "write");
>
> -            if (bdrv_open2(bs, params, flags, format) < 0)
> +            do {
> +                int status = bdrv_open2(bs, params, flags, format);
> +                if (status != -EMEDIUMTYPE)
> +                    break;
> +                usleep(500000); /* 500ms */
> +                retries--;
> +            } while (retries);
> +            if (status < 0)
>                 fprintf(stderr, "qemu: could not open vbd '%s' or hard
> disk image '%s' (drv '%s' format '%s')\n", buf, params, drv ? drv : "?",
> format ? format->format_name : "
>         }
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel
>

Sorry to dig up an old thread. This patch doesnt seem to be in any of the
qemu-xen-*, xen-*-testing or xen-unstable trees. Any reason why?

shriram

[-- Attachment #1.2: Type: text/html, Size: 3467 bytes --]

[-- Attachment #2: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH] make qemu handle drbd properly

[Ian Jackson]

Shriram Rajagopalan writes ("Re: [Xen-devel] [PATCH] make qemu handle drbd properly"):
> On Wed, Jan 26, 2011 at 11:43 PM, James Harper <
> james.harper@bendigoit.com.au> wrote:
> > This attached patch allows qemu to accept a drbd: prefix and interpret
> > it as a drbd resource. It then searches for it in /dev/drbd/by-res/
> > (which must be mapped via udev). It's against 4.0-testing.
...
> Sorry to dig up an old thread. This patch doesnt seem to be in any of the
> qemu-xen-*, xen-*-testing or xen-unstable trees. Any reason why?

I don't think we're going to introduce a new feature of this kind into
qemu-xen-unstable at this stage.

Sorry,
Ian.

Re: [PATCH] make qemu handle drbd properly

[Shriram Rajagopalan]

On Tue, Jun 28, 2011 at 8:54 AM, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote:
> Shriram Rajagopalan writes ("Re: [Xen-devel] [PATCH] make qemu handle drbd properly"):
>> On Wed, Jan 26, 2011 at 11:43 PM, James Harper <
>> james.harper@bendigoit.com.au> wrote:
>> > This attached patch allows qemu to accept a drbd: prefix and interpret
>> > it as a drbd resource. It then searches for it in /dev/drbd/by-res/
>> > (which must be mapped via udev). It's against 4.0-testing.
> ...
>> Sorry to dig up an old thread. This patch doesnt seem to be in any of the
>> qemu-xen-*, xen-*-testing or xen-unstable trees. Any reason why?
>
> I don't think we're going to introduce a new feature of this kind into
> qemu-xen-unstable at this stage.
>
> Sorry,
> Ian.
>

Hi Ian,
 can you please include this patch into qemu-xen-unstable ?
 The last time I requested that this patch be included into qemu-xen-unstable,
it was too late. I hope I am not late again this time. :)
Please let me know if you need a more formal version of this patch.

thanks
shriram

RE: [PATCH] make qemu handle drbd properly

[James Harper]

> -----Original Message-----
> From: Shriram Rajagopalan [mailto:rshriram@cs.ubc.ca]
> Sent: Tuesday, 30 August 2011 14:11
> To: Ian Jackson
> Cc: James Harper; xen-devel@lists.xensource.com
> Subject: Re: [Xen-devel] [PATCH] make qemu handle drbd properly
> 
> On Tue, Jun 28, 2011 at 8:54 AM, Ian Jackson
<Ian.Jackson@eu.citrix.com>
> wrote:
> > Shriram Rajagopalan writes ("Re: [Xen-devel] [PATCH] make qemu
handle drbd
> properly"):
> >> On Wed, Jan 26, 2011 at 11:43 PM, James Harper <
> >> james.harper@bendigoit.com.au> wrote:
> >> > This attached patch allows qemu to accept a drbd: prefix and
interpret
> >> > it as a drbd resource. It then searches for it in
/dev/drbd/by-res/
> >> > (which must be mapped via udev). It's against 4.0-testing.
> > ...
> >> Sorry to dig up an old thread. This patch doesnt seem to be in any
of the
> >> qemu-xen-*, xen-*-testing or xen-unstable trees. Any reason why?
> >
> > I don't think we're going to introduce a new feature of this kind
into
> > qemu-xen-unstable at this stage.
> >
> > Sorry,
> > Ian.
> >
> 
> Hi Ian,
>  can you please include this patch into qemu-xen-unstable ?
>  The last time I requested that this patch be included into
qemu-xen-unstable,
> it was too late. I hope I am not late again this time. :)
> Please let me know if you need a more formal version of this patch.
> 

While this patch works great, I'd like to see a more generic mechanism
for passing different backends into qemu when they are fundamentally the
same (just /dev entries) and the setup is handled by the block-*
scripts. A block-iscsi would need the same futzing in qemu to make it
happen properly.

I think it's wrong for qemu to use 'type' - the access method should be
stated explicitly. drbd/iscsi should be set up in block-drbd/iscsi and
then qemu should handle them the same way it handles phy: (with the
exception that drbd may need a delay while it comes online). So maybe
another setting in the xenstore like qemu-type or access-method or
something which, if present, overrides the type that qemu uses when
deciding how to access the underlying block device. I don't really like
the name access-method, and qemu-type sounds wrong too, but can't think
of anything better.

So the changes required would be:

1. The block-drbd script would need to write an entry access-method=phy
into xenstore into the appropriate place. Maybe also write a flag in
there to tell qemu that the device may take a few seconds to come online

2. qemu would need to first read access-method, and then type if
access-type doesn't exist. It would also retry the open of the device if
the delay flag was specified. Or maybe it wouldn't hurt to always retry.

If the decision makers agree with that approach I think I could come up
with a patch...

James

RE: [PATCH] make qemu handle drbd properly

[Stefano Stabellini]

On Tue, 30 Aug 2011, James Harper wrote:
> While this patch works great, I'd like to see a more generic mechanism
> for passing different backends into qemu when they are fundamentally the
> same (just /dev entries) and the setup is handled by the block-*
> scripts. A block-iscsi would need the same futzing in qemu to make it
> happen properly.

I agree


> I think it's wrong for qemu to use 'type' - the access method should be
> stated explicitly. drbd/iscsi should be set up in block-drbd/iscsi and
> then qemu should handle them the same way it handles phy: (with the
> exception that drbd may need a delay while it comes online). So maybe
> another setting in the xenstore like qemu-type or access-method or
> something which, if present, overrides the type that qemu uses when
> deciding how to access the underlying block device. I don't really like
> the name access-method, and qemu-type sounds wrong too, but can't think
> of anything better.
> 
> So the changes required would be:
> 
> 1. The block-drbd script would need to write an entry access-method=phy
> into xenstore into the appropriate place. Maybe also write a flag in
> there to tell qemu that the device may take a few seconds to come online
> 
> 2. qemu would need to first read access-method, and then type if
> access-type doesn't exist. It would also retry the open of the device if
> the delay flag was specified. Or maybe it wouldn't hurt to always retry.
> 
> If the decision makers agree with that approach I think I could come up
> with a patch...

Upstream qemu does not use xenstore to read the disk configuration and
any mechanisms we introduce at this point should work on upstream qemu
too.
I don't think qemu needs any knowledge of the backend type: the
toolstack (block scripts and/or libxenlight) should just setup a phy
backend and pass to qemu the right device name from the start.
Regarding the retries, it is probably better to handle them in the block
scripts, qemu doesn't seem the right place to put them.

RE: [PATCH] make qemu handle drbd properly

[James Harper]

> I don't think qemu needs any knowledge of the backend type: the
> toolstack (block scripts and/or libxenlight) should just setup a phy
> backend and pass to qemu the right device name from the start.
> Regarding the retries, it is probably better to handle them in the
block
> scripts, qemu doesn't seem the right place to put them.

Does xen wait for the block-* scripts to complete before starting qemu?
When I tested where to put the delay last time it didn't seem to work
without putting it in qemu-dm.

James

RE: [PATCH] make qemu handle drbd properly

[Stefano Stabellini]

On Tue, 30 Aug 2011, James Harper wrote:
> > I don't think qemu needs any knowledge of the backend type: the
> > toolstack (block scripts and/or libxenlight) should just setup a phy
> > backend and pass to qemu the right device name from the start.
> > Regarding the retries, it is probably better to handle them in the
> block
> > scripts, qemu doesn't seem the right place to put them.
> 
> Does xen wait for the block-* scripts to complete before starting qemu?
> When I tested where to put the delay last time it didn't seem to work
> without putting it in qemu-dm.
 
Currently libxenlight does not support block-* script, but it should, it
is one of the missing features.