[PATCH 1/4]: Fix blkfront to accept expanded devices

[PATCH 1/4]: Fix blkfront to accept expanded devices

[Chris Lalancette]

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

This patch implements the new extended allocation scheme on the blkfront end.
Note that there is a new xenbus node for the extended stuff, called
"virtual-device-ext".  On a blkfront_probe(), if nothing is found in
"virtual-device", then it will go looking in "virtual-device-ext".  This
prevents old guest kernels from potentially mis-interpreting data in
"virtual-device" (and then subsequently crashing).

Signed-off-by: Chris Lalancette <clalance@redhat.com>

[-- Attachment #2: xen-unstable-greater-16-vbd-kernel-blkfront-fixes.patch --]
[-- Type: text/x-patch, Size: 4378 bytes --]

diff -r 5201a184f513 drivers/xen/blkfront/blkfront.c
--- a/drivers/xen/blkfront/blkfront.c	Fri Jun 20 17:43:16 2008 +0100
+++ b/drivers/xen/blkfront/blkfront.c	Mon Jun 23 17:56:00 2008 +0200
@@ -92,8 +92,13 @@ static int blkfront_probe(struct xenbus_
 	err = xenbus_scanf(XBT_NIL, dev->nodename,
 			   "virtual-device", "%i", &vdevice);
 	if (err != 1) {
-		xenbus_dev_fatal(dev, err, "reading virtual-device");
-		return err;
+		/* go looking in the extended area instead */
+		err = xenbus_scanf(XBT_NIL, dev->nodename, "virtual-device-ext",
+			"%i", &vdevice);
+		if (err != 1) {
+			xenbus_dev_fatal(dev, err, "reading virtual-device");
+			return err;
+		}
 	}
 
 	info = kzalloc(sizeof(*info), GFP_KERNEL);
diff -r 5201a184f513 drivers/xen/blkfront/vbd.c
--- a/drivers/xen/blkfront/vbd.c	Fri Jun 20 17:43:16 2008 +0100
+++ b/drivers/xen/blkfront/vbd.c	Mon Jun 23 17:56:00 2008 +0200
@@ -43,6 +43,9 @@
 #define BLKIF_MAJOR(dev) ((dev)>>8)
 #define BLKIF_MINOR(dev) ((dev) & 0xff)
 
+#define VDEV_IS_EXTENDED(dev) (dev & (1<<28))
+#define BLKIF_MINOR_EXT(dev) ((dev)&0x0FFFFFFF)
+
 /*
  * For convenience we distinguish between ide, scsi and 'other' (i.e.,
  * potentially combinations of the two) in the naming scheme and in a few other
@@ -84,10 +87,6 @@ static struct xlbd_major_info *major_inf
 #define XLBD_MAJOR_IDE_RANGE	XLBD_MAJOR_IDE_START ... XLBD_MAJOR_SCSI_START - 1
 #define XLBD_MAJOR_SCSI_RANGE	XLBD_MAJOR_SCSI_START ... XLBD_MAJOR_VBD_START - 1
 #define XLBD_MAJOR_VBD_RANGE	XLBD_MAJOR_VBD_START ... XLBD_MAJOR_VBD_START + NUM_VBD_MAJORS - 1
-
-/* Information about our VBDs. */
-#define MAX_VBDS 64
-static LIST_HEAD(vbds_list);
 
 static struct block_device_operations xlvbd_block_fops =
 {
@@ -144,8 +143,14 @@ xlbd_get_major_info(int vdevice)
 	struct xlbd_major_info *mi;
 	int major, minor, index;
 
-	major = BLKIF_MAJOR(vdevice);
-	minor = BLKIF_MINOR(vdevice);
+	if (!VDEV_IS_EXTENDED(vdevice)) {
+		major = BLKIF_MAJOR(vdevice);
+		minor = BLKIF_MINOR(vdevice);
+	}
+	else {
+		major = 202;
+		minor = BLKIF_MINOR_EXT(vdevice);
+	}
 
 	switch (major) {
 	case IDE0_MAJOR: index = 0; break;
@@ -231,6 +236,7 @@ xlvbd_alloc_gendisk(int minor, blkif_sec
 	int nr_minors = 1;
 	int err = -ENODEV;
 	unsigned int offset;
+	int part_shift;
 
 	BUG_ON(info->gd != NULL);
 	BUG_ON(info->mi != NULL);
@@ -241,15 +247,19 @@ xlvbd_alloc_gendisk(int minor, blkif_sec
 		goto out;
 	info->mi = mi;
 
-	if ((minor & ((1 << mi->type->partn_shift) - 1)) == 0)
-		nr_minors = 1 << mi->type->partn_shift;
+	part_shift = mi->type->partn_shift;
+	if (VDEV_IS_EXTENDED(vdevice))
+		part_shift += 4;
+
+	if ((minor & ((1 << part_shift) - 1)) == 0)
+		nr_minors = 1 << part_shift;
 
 	gd = alloc_disk(nr_minors);
 	if (gd == NULL)
 		goto out;
 
-	offset =  mi->index * mi->type->disks_per_major +
-			(minor >> mi->type->partn_shift);
+	offset =  mi->index * mi->type->disks_per_major + (minor >> part_shift);
+
 	if (nr_minors > 1) {
 		if (offset < 26) {
 			sprintf(gd->disk_name, "%s%c",
@@ -266,13 +276,13 @@ xlvbd_alloc_gendisk(int minor, blkif_sec
 			sprintf(gd->disk_name, "%s%c%d",
 				mi->type->diskname,
 				'a' + offset,
-				minor & ((1 << mi->type->partn_shift) - 1));
+				minor & ((1 << part_shift) - 1));
 		}
 		else {
 			sprintf(gd->disk_name, "%s%c%c%d",
 				mi->type->diskname,
 				'a' + ((offset/26)-1), 'a' + (offset%26),
-				minor & ((1 << mi->type->partn_shift) - 1));
+				minor & ((1 << part_shift) - 1));
 		}
 	}
 
@@ -319,14 +329,26 @@ xlvbd_add(blkif_sector_t capacity, int v
 	struct block_device *bd;
 	int err = 0;
 
-	info->dev = MKDEV(BLKIF_MAJOR(vdevice), BLKIF_MINOR(vdevice));
+	if (!VDEV_IS_EXTENDED(vdevice)) {
+		info->dev = MKDEV(BLKIF_MAJOR(vdevice), BLKIF_MINOR(vdevice));
+		bd = bdget(info->dev);
+		if (bd == NULL)
+			return -ENODEV;
 
-	bd = bdget(info->dev);
-	if (bd == NULL)
-		return -ENODEV;
+		err = xlvbd_alloc_gendisk(BLKIF_MINOR(vdevice), capacity,
+					  vdevice, vdisk_info, sector_size,
+					  info);
+	}
+	else {
+		info->dev = MKDEV(202, BLKIF_MINOR_EXT(vdevice));
+		bd = bdget(info->dev);
+		if (bd == NULL)
+			return -ENODEV;
 
-	err = xlvbd_alloc_gendisk(BLKIF_MINOR(vdevice), capacity, vdevice,
-				  vdisk_info, sector_size, info);
+		err = xlvbd_alloc_gendisk(BLKIF_MINOR_EXT(vdevice), capacity,
+					  vdevice, vdisk_info, sector_size,
+					  info);
+	}
 
 	bdput(bd);
 	return err;

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

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

[PATCH 0/4]: Expand xvd to support > 16 devices

[Chris Lalancette]

All,
     Current blktap and blkfront are limited to 16 xvd devices (xvda -> xvdp).
This is enforced in the userland dom0 tools, but is also hard-coded into the
blkfront kernel code (even though modern dev_t can hold many more than 256
minors).  Based on the discussion that we had here:

http://lists.xensource.com/archives/html/xen-devel/2008-05/msg00128.html

I ended up implementing Ian Jackson's suggestion here:

http://lists.xensource.com/archives/html/xen-devel/2008-05/msg00231.html

Basically, I left the old format alone, but added a new format that looks like:

 1 << 28 | disk << 8 | partition       xvd, disks or partitions 16 onwards

This format is used for any disks xvdq onward.  Note that blktap has a hardcoded
limit of 100 devices that I did not change with this patch series; if that ends
up being a problem, then that's just a simple #define to change.

I did not expand the number of partitions available (it's still 15), although
there is space in the allocation to do that if someone wishes.  More details are
in each individual patch.

Note that I developed this against RHEL-5 kernels and ported it over to
xen-unstable, and only compile tested it there.

Chris Lalancette

Re: [PATCH 0/4]: Expand xvd to support > 16 devices

[Ian Jackson]

Chris Lalancette writes ("[Xen-devel] [PATCH 0/4]: Expand xvd to support > 16 devices"):
> I ended up implementing Ian Jackson's suggestion here:
>   http://lists.xensource.com/archives/html/xen-devel/2008-05/msg00231.html
> Basically, I left the old format alone, but added a new format that
> looks like:
> 
>  1 << 28 | disk << 8 | partition       xvd, disks or partitions 16 onwards

I approve of this, obviously.  But I think your patch lacks some error
checks.  These are most critical in the guest, as the guest's
interpretation of the interface will effectively be frozen.

When the guest is enumerating the devices, it should be sure to
check that the block device number integer matches one of the expected
forms, as I wrote in my message.  If the number does not, then that
vbd should be ignored with a warning message.

This applies also to the partition numbers which you are currently
limiting to 15.  That's fine but you should put in a check so that
out-of-range partition numbers are ignored rather than causing
unexpected behaviours.  (I'll admit that I haven't analysed your code
in detail to determine exactly what the behaviour would be ...)

(Obviously even adding this check now won't prevent attempts to
specify out of range devices from totally breaking even older guests
which lack proper checking.  But there's no reason to perpetuate these
bugs.)

Also I think you should include an API changelog entry.

Thanks,
Ian.

Re: [PATCH 0/4]: Expand xvd to support > 16 devices

[Chris Lalancette]

(sorry for the delay in responding)

Ian Jackson wrote:
> When the guest is enumerating the devices, it should be sure to
> check that the block device number integer matches one of the expected
> forms, as I wrote in my message.  If the number does not, then that
> vbd should be ignored with a warning message.

OK, yes, I see, that makes sense.  I'll make the appropriate change in xlvbd_add().

> 
> This applies also to the partition numbers which you are currently
> limiting to 15.  That's fine but you should put in a check so that
> out-of-range partition numbers are ignored rather than causing
> unexpected behaviours.  (I'll admit that I haven't analysed your code
> in detail to determine exactly what the behaviour would be ...)

I'm not sure that this one is a problem (although I could be wrong).  During
xlvbd_add() time, we end up doing an alloc_disk() with the number of minors that
we can use.  So I don't think that the rest of the system will allow us to go
beyond that value; empirical evidence seems to support this, as attaching a disk
with 16 partitions to /dev/xvdb only shows the first 15 partitions.

Incidentally, the comment I made in my initial posting about expanding the
partitions is wrong, looking back at the code.  I *did* expand the number of
entries that blkfront will pick up (i.e. increased nr_minors when doing the
alloc_disk()), but I did not change the tools side to accept partitions > 15.
Again, something that can easily be done in the future.

> Also I think you should include an API changelog entry.

Do you mean on the Xen Wiki?  I did find a page about API changes, so if/when
these patches go in, I'm happy to add an entry there.

Thanks for looking,
Chris Lalancette

Re: [PATCH 0/4]: Expand xvd to support > 16 devices

[Keir Fraser]

On 25/6/08 10:45, "Chris Lalancette" <clalance@redhat.com> wrote:

>> Also I think you should include an API changelog entry.
> 
> Do you mean on the Xen Wiki?  I did find a page about API changes, so if/when
> these patches go in, I'm happy to add an entry there.

docs/ChangeLog in the repository.

 -- Keir