Stubdom and blktap

Stubdom and blktap

[Yang, Xiaowei]

Recently we had a try of stubdom, whose general status is good.

One issue we found is with aio/sync blktap as backend, disk performance 
is obviously slower, and there are some IDE kernel error messages (like 
irq timeout) at boot time.

Another issue (not stubdom specific) comes from blktap's support for 
QCOW image which is based on a backing file. We never tried this 
configure before. The root cause is that in tdqcow_get_parent_id(), the 
backing file is hard-coded to be QCOW type and tdqcow_validate_parent() 
also presumes it, though the backing file is a raw image normally. With 
the image type set correctly and a good method for image validation, 
QCOW works.

Thanks,
Xiaowei

Re: Stubdom and blktap

[Stefano Stabellini]

Yang, Xiaowei wrote:

> Recently we had a try of stubdom, whose general status is good.
> 
> One issue we found is with aio/sync blktap as backend, disk performance
> is obviously slower, and there are some IDE kernel error messages (like
> irq timeout) at boot time.

I'll try to reproduce the problem, thanks for reporting it.

> Another issue (not stubdom specific) comes from blktap's support for
> QCOW image which is based on a backing file. We never tried this
> configure before. The root cause is that in tdqcow_get_parent_id(), the
> backing file is hard-coded to be QCOW type and tdqcow_validate_parent()
> also presumes it, though the backing file is a raw image normally. With
> the image type set correctly and a good method for image validation,
> QCOW works.
> 

I am not sure to understand what the issue is exactly: there is no image
format guessing using blktap, the user is supposed to specify correctly
the format in the VM configuration file.

RE: Stubdom and blktap

[Yang, Xiaowei]

>-----Original Message-----
>From: Stefano Stabellini [mailto:stefano.stabellini@eu.citrix.com]
>Sent: Monday, September 29, 2008 7:32 PM
>To: Yang, Xiaowei
>Cc: xen-devel@lists.xensource.com
>Subject: Re: [Xen-devel] Stubdom and blktap
>
>Yang, Xiaowei wrote:
>
>> Recently we had a try of stubdom, whose general status is good.
>>
>> One issue we found is with aio/sync blktap as backend, disk performance
>> is obviously slower, and there are some IDE kernel error messages (like
>> irq timeout) at boot time.
>
>I'll try to reproduce the problem, thanks for reporting it.
>
>> Another issue (not stubdom specific) comes from blktap's support for
>> QCOW image which is based on a backing file. We never tried this
>> configure before. The root cause is that in tdqcow_get_parent_id(), the
>> backing file is hard-coded to be QCOW type and tdqcow_validate_parent()
>> also presumes it, though the backing file is a raw image normally. With
>> the image type set correctly and a good method for image validation,
>> QCOW works.
>>
>
>I am not sure to understand what the issue is exactly: there is no image
>format guessing using blktap, the user is supposed to specify correctly
>the format in the VM configuration file.

Sorry for later response. I was on vacation.

Typically a QCOW image only presents changes made to a backing image. 
When a QCOW image is specified to be used in VM configuration file, 
blktap uses QCOW driver for it indeed. However, blktap also needs to 
access the backing file and before that it needs to choose a driver for 
it. As I said, the backing file is a raw one normally, and Qemu assumes it.

Here's a possibly simple (though with less flexibility) patch to it. Any 
comments are welcome!

diff -r f4552d9f6afb tools/blktap/drivers/block-qcow.c
--- a/tools/blktap/drivers/block-qcow.c	Tue Sep 23 17:11:33 2008 +0100
+++ b/tools/blktap/drivers/block-qcow.c	Wed Oct 08 05:45:29 2008 +0800
@@ -1385,7 +1385,7 @@ static int tdqcow_get_parent_id(struct d
  	filename[len]  = '\0';

  	id->name       = strdup(filename);
-	id->drivertype = DISK_TYPE_QCOW;
+	id->drivertype = DISK_TYPE_AIO;
  	err            = 0;
   out:
  	free(buf);
@@ -1397,17 +1397,15 @@ static int tdqcow_validate_parent(struct
  {
  	struct stat stats;
  	uint64_t psize, csize;
-	struct tdqcow_state *c = (struct tdqcow_state *)child->private;
-	struct tdqcow_state *p = (struct tdqcow_state *)parent->private;
-	
-	if (stat(p->name, &stats))
+	
+	if (stat(parent->name, &stats))
  		return -EINVAL;
-	if (get_filesize(p->name, &psize, &stats))
+	if (get_filesize(parent->name, &psize, &stats))
  		return -EINVAL;

-	if (stat(c->name, &stats))
+	if (stat(child->name, &stats))
  		return -EINVAL;
-	if (get_filesize(c->name, &csize, &stats))
+	if (get_filesize(child->name, &csize, &stats))
  		return -EINVAL;

  	if (csize != psize)


Thanks,
Xiaowei

[PATCH] Stubdom and blktap

[Stefano Stabellini]

Yang, Xiaowei wrote:

> Typically a QCOW image only presents changes made to a backing image.
> When a QCOW image is specified to be used in VM configuration file,
> blktap uses QCOW driver for it indeed. However, blktap also needs to
> access the backing file and before that it needs to choose a driver for
> it. As I said, the backing file is a raw one normally, and Qemu assumes it.
> 
> Here's a possibly simple (though with less flexibility) patch to it. Any
> comments are welcome!




You are right about the backing file driver issue in blktap and I think
your patch is correct too.
I am sending a new patch to include an equivalent fix for
block-qcow2 too and also to disable O_DIRECT on block-qcow since our
reads and writes are not always aligned to 512 mb.

It has to be said that this patch still doesn't solve all the qcow and
stubdoms related issues found in the last weekly status report.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

---

diff -r 4129f0f2f2ba tools/blktap/drivers/block-qcow.c
--- a/tools/blktap/drivers/block-qcow.c	Fri Oct 17 14:15:37 2008 +0100
+++ b/tools/blktap/drivers/block-qcow.c	Mon Oct 20 13:03:02 2008 +0100
@@ -734,8 +734,8 @@ static int tdqcow_open (struct disk_driv
 
  	DPRINTF("QCOW: Opening %s\n",name);
 
-	o_flags = O_DIRECT | O_LARGEFILE | 
-		((flags == TD_RDONLY) ? O_RDONLY : O_RDWR);
+	/* Since we don't handle O_DIRECT correctly, don't use it */
+	o_flags = O_LARGEFILE | ((flags == TD_RDONLY) ? O_RDONLY : O_RDWR);
 	fd = open(name, o_flags);
 	if (fd < 0) {
 		DPRINTF("Unable to open %s (%d)\n",name,0 - errno);
@@ -1385,7 +1385,7 @@ static int tdqcow_get_parent_id(struct d
 	filename[len]  = '\0';
 
 	id->name       = strdup(filename);
-	id->drivertype = DISK_TYPE_QCOW;
+	id->drivertype = DISK_TYPE_AIO;
 	err            = 0;
  out:
 	free(buf);
@@ -1397,17 +1397,15 @@ static int tdqcow_validate_parent(struct
 {
 	struct stat stats;
 	uint64_t psize, csize;
-	struct tdqcow_state *c = (struct tdqcow_state *)child->private;
-	struct tdqcow_state *p = (struct tdqcow_state *)parent->private;
-	
-	if (stat(p->name, &stats))
+	
+	if (stat(parent->name, &stats))
 		return -EINVAL;
-	if (get_filesize(p->name, &psize, &stats))
+	if (get_filesize(parent->name, &psize, &stats))
 		return -EINVAL;
 
-	if (stat(c->name, &stats))
+	if (stat(child->name, &stats))
 		return -EINVAL;
-	if (get_filesize(c->name, &csize, &stats))
+	if (get_filesize(child->name, &csize, &stats))
 		return -EINVAL;
 
 	if (csize != psize)
diff -r 4129f0f2f2ba tools/blktap/drivers/block-qcow2.c
--- a/tools/blktap/drivers/block-qcow2.c	Fri Oct 17 14:15:37 2008 +0100
+++ b/tools/blktap/drivers/block-qcow2.c	Mon Oct 20 13:00:18 2008 +0100
@@ -34,6 +34,7 @@
 #include "tapdisk.h"
 #include "tapaio.h"
 #include "bswap.h"
+#include "blk.h"
 
 #define USE_AIO
 
@@ -1902,6 +1903,42 @@ repeat:
 
 #endif	
 
+static int get_filesize(char *filename, uint64_t *size, struct stat *st)
+{
+	int fd;
+	QCowHeader header;
+
+	/*Set to the backing file size*/
+	fd = open(filename, O_RDONLY);
+	if (fd < 0)
+		return -1;
+	if (read(fd, &header, sizeof(header)) < sizeof(header)) {
+		close(fd);
+		return -1;
+	}
+	close(fd);
+	
+	be32_to_cpus(&header.magic);
+	be32_to_cpus(&header.version);
+	be64_to_cpus(&header.size);
+	if (header.magic == QCOW_MAGIC && header.version == QCOW_VERSION) {
+		*size = header.size >> SECTOR_SHIFT;
+		return 0;
+	}
+
+	if(S_ISBLK(st->st_mode)) {
+		fd = open(filename, O_RDONLY);
+		if (fd < 0)
+			return -1;
+		if (blk_getimagesize(fd, size) != 0) {
+			close(fd);
+			return -1;
+		}
+		close(fd);
+	} else *size = (st->st_size >> SECTOR_SHIFT);	
+	return 0;
+}
+
 /**
  * @return 
  *	   0 if parent id successfully retrieved;
@@ -1916,7 +1953,7 @@ static int qcow_get_parent_id(struct dis
 		return TD_NO_PARENT;
 
 	id->name = strdup(s->backing_file);
-	id->drivertype = DISK_TYPE_QCOW2;
+	id->drivertype = DISK_TYPE_AIO;
 
 	return 0;
 }
@@ -1924,15 +1961,22 @@ static int qcow_validate_parent(struct d
 static int qcow_validate_parent(struct disk_driver *child, 
 		struct disk_driver *parent, td_flag_t flags)
 {
-	struct BDRVQcowState *cs = (struct BDRVQcowState*) child->private;
-	struct BDRVQcowState *ps = (struct BDRVQcowState*) parent->private;
-
-	if (ps->total_sectors != cs->total_sectors) {
-		DPRINTF("qcow_validate_parent(): %#"PRIx64" != %#"PRIx64"\n",
-			ps->total_sectors, cs->total_sectors);
+	struct stat stats;
+	uint64_t psize, csize;
+	
+	if (stat(parent->name, &stats))
 		return -EINVAL;
-	}
-	
+	if (get_filesize(parent->name, &psize, &stats))
+		return -EINVAL;
+
+	if (stat(child->name, &stats))
+		return -EINVAL;
+	if (get_filesize(child->name, &csize, &stats))
+		return -EINVAL;
+
+	if (csize != psize)
+		return -EINVAL;
+
 	return 0;
 }