brd: partition fixes

From: Ross Zwisler <[email protected]>

Some programs require HDIO_GETGEO work, which requires we implement getgeo. Based off of the work done to the NVMe driver in this commit:

4cc09e2dc4cb NVMe: Add getgeo to block ops

[Boaz] Converted original work done for prd.c for here.
This is needed if we want to support partitions, fdisk calls this.

Signed-off-by Ross Zwisler <[email protected]>

Signed-off-by Boaz Harrosh <[email protected]>

---
 drivers/block/brd.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

drivers/block/brd.c +11 -0

index a10a0a9..3f07cb4 100644
@@ -19,6 +19,7 @@
 #include <linux/radix-tree.h>
 #include <linux/fs.h>
 #include <linux/slab.h>
+#include <linux/hdreg.h>
 
 #include <asm/uaccess.h>
 

@@ -424,6 +425,15 @@ static int brd_ioctl(struct block_device *bdev, fmode_t mode,
 	return error;
 }
 
+static int brd_getgeo(struct block_device *bd, struct hd_geometry *geo)
+{
+	/* some standard values */
+	geo->heads = 1 << 6;
+	geo->sectors = 1 << 5;
+	geo->cylinders = get_capacity(bd->bd_disk) >> 11;
+	return 0;
+}
+
 static const struct block_device_operations brd_fops = {
 	.owner =		THIS_MODULE,
 	.rw_page =		brd_rw_page,

@@ -431,6 +441,7 @@ static const struct block_device_operations brd_fops = {
 #ifdef CONFIG_BLK_DEV_XIP
 	.direct_access =	brd_direct_access,
 #endif
+	.getgeo =		brd_getgeo,
 };
 
 /*

Signature · 1 line

1.9.3

Quoted 5 lines · Boaz Harrosh

From: Ross Zwisler <[email protected]>

Some programs require HDIO_GETGEO work, which requires we implement getgeo. Based off of the work done to the NVMe driver in this commit:

drivers/block/brd.c static int brd_ioctl(struct block_device *bdev, fmode_t mode, · line 436

Quoted 9 lines

drivers/block/brd.c +9 -0

+static int brd_getgeo(struct block_device *bd, struct hd_geometry *geo)
+{
+	/* some standard values */
+	geo->heads = 1 << 6;
+	geo->sectors = 1 << 5;
+	geo->cylinders = get_capacity(bd->bd_disk) >> 11;
+	return 0;
+}
+

This looks good.

From: Boaz Harrosh <[email protected]>

Some programs like fdisk, require HDIO_GETGEO to work, which requires we implement getgeo.

We set all hd_geometry members to 1, because this way fdisk math will not try its crazy geometry math and get stuff totally wrong.

drivers/block/brd.c +5 -0

+static int brd_getgeo(struct block_device *bd, struct hd_geometry *geo)
+{
+	/* Just tell fdisk to get out of the way. The math here is so
+	 * convoluted and does not make any sense at all. With all 1s
+	 * The math just gets out of the way.

Quoted 2 lines · On Thu, 07 Aug 2014 17:03:08 +0300 Boaz Harrosh <[email protected]> wrote:

From: Boaz Harrosh <[email protected]>

Quoted 5 lines

Some programs like fdisk, require HDIO_GETGEO to work, which requires we implement getgeo.

We set all hd_geometry members to 1, because this way fdisk math will not try its crazy geometry math and get stuff totally wrong.

If you are running a new storage system for god sake don't use DOS partitioning, use GPT or something sane.

Quoted 12 lines

drivers/block/brd.c +12 -0

+static int brd_getgeo(struct block_device *bd, struct hd_geometry *geo)
+{
+     /* Just tell fdisk to get out of the way. The math here is so
+      * convoluted and does not make any sense at all. With all 1s
+      * The math just gets out of the way.
+      * NOTE: I was trying to get some values that will make fdisk
+      * Want to align first sector on 4K (like 8, 16, 20, ... sectors) but
+      * nothing worked, I searched the net the math is not your regular
+      * simple multiplication at all. If you managed to get these please
+      * fix here. For now we use 4k physical sectors for this
+      */
+     geo->heads = 1;

drivers/block/brd.c static int brd_ioctl(struct block_device *bdev, fmode_t mode, · line 442

Quoted 3 lines

drivers/block/brd.c +3 -0

+     geo->sectors = 1;
+     geo->cylinders = 1;
+     return 0;

This is then going to blow up on your with some other tool. Fix fdisk instead. Lying to apps generally ends up like children lying to parents - the lie gets more complicated to keep up each case you find until it breaks.

Alan

Quoted 11 lines · One Thousand Gnomes

Quoted 7 lines · On Thu, 07 Aug 2014 17:03:08 +0300 Boaz Harrosh <[email protected]> wrote:

From: Boaz Harrosh <[email protected]>

Some programs like fdisk, require HDIO_GETGEO to work, which requires we implement getgeo.

We set all hd_geometry members to 1, because this way fdisk math will not try its crazy geometry math and get stuff totally wrong.

Quoted 3 lines

If you are running a new storage system for god sake don't use DOS partitioning, use GPT or something sane.

I am always using GPT, on all my systems for few years now. Fdisk works exactly the same, regardless of format, regarding alignment and start sectors.

This patch fixes up brd's partitions scheme, now enjoying all worlds.

The MAIN fix here is that currently if one fdisks some partitions, a BAD bug will make all partitions point to the same start-end sector
ie: 0 - brd_size And an mkfs of any partition would trash the partition table and the other partition.

Another fix is that "mount -U uuid" did not work, because of the GENHD_FL_SUPPRESS_PARTITION_INFO flag.

So NOW the logic goes like this:
* max_part - Just says how many minors to reserve between devices But in any way, there can be as many partition as requested. If minors between devices ends, then dynamic 259-major ids will be allocated on the fly.
The default is now max_part=1, which means all partitions devt will be from the dynamic major-range.
(If persistent partition minors is needed use max_part=)

* Creation of new devices on the fly still/always work:
mknod /path/devnod b 1 X
fdisk -l /path/devnod
Will create a new device if (X / max_part) was not already created before. (Just as before)

partitions on the dynamically created device will work as well Same logic applies with minors as with the pre-created ones.

TODO: dynamic grow of device size, maybe through sysfs. So each
device can have it's own size.

Signed-off-by Boaz Harrosh <[email protected]>

---
 drivers/block/brd.c | 93 +++++++++++++++++++++--------------------------------
 1 file changed, 36 insertions(+), 57 deletions(-)

drivers/block/brd.c +36 -57

index 3f07cb4..9673704 100644
@@ -447,16 +447,18 @@ static const struct block_device_operations brd_fops = {
 /*
  * And now the modules code and kernel interface.
  */
-static int rd_nr;
-int rd_size = CONFIG_BLK_DEV_RAM_SIZE;
-static int max_part;
-static int part_shift;
+static int rd_nr = CONFIG_BLK_DEV_RAM_COUNT;
 module_param(rd_nr, int, S_IRUGO);
 MODULE_PARM_DESC(rd_nr, "Maximum number of brd devices");
+
+int rd_size = CONFIG_BLK_DEV_RAM_SIZE;
 module_param(rd_size, int, S_IRUGO);
 MODULE_PARM_DESC(rd_size, "Size of each RAM disk in kbytes.");
+
+static int max_part = 1;
 module_param(max_part, int, S_IRUGO);
-MODULE_PARM_DESC(max_part, "Maximum number of partitions per RAM disk");
+MODULE_PARM_DESC(max_part, "Num Minors to reserve between devices");
+
 MODULE_LICENSE("GPL");
 MODULE_ALIAS_BLOCKDEV_MAJOR(RAMDISK_MAJOR);
 MODULE_ALIAS("rd");

@@ -502,15 +504,15 @@ static struct brd_device *brd_alloc(int i)
 	brd->brd_queue->limits.discard_zeroes_data = 1;
 	queue_flag_set_unlocked(QUEUE_FLAG_DISCARD, brd->brd_queue);
 
-	disk = brd->brd_disk = alloc_disk(1 << part_shift);
+	disk = brd->brd_disk = alloc_disk(max_part);
 	if (!disk)
 		goto out_free_queue;
 	disk->major		= RAMDISK_MAJOR;
-	disk->first_minor	= i << part_shift;
+	disk->first_minor	= i * max_part;
 	disk->fops		= &brd_fops;
 	disk->private_data	= brd;
 	disk->queue		= brd->brd_queue;
-	disk->flags |= GENHD_FL_SUPPRESS_PARTITION_INFO;
+	disk->flags		= GENHD_FL_EXT_DEVT;
 	sprintf(disk->disk_name, "ram%d", i);
 	set_capacity(disk, rd_size * 2);
 

@@ -532,10 +534,11 @@ static void brd_free(struct brd_device *brd)
 	kfree(brd);
 }
 
-static struct brd_device *brd_init_one(int i)
+static struct brd_device *brd_init_one(int i, bool *new)
 {
 	struct brd_device *brd;
 
+	*new = false;
 	list_for_each_entry(brd, &brd_devices, brd_list) {
 		if (brd->brd_number == i)
 			goto out;

@@ -546,6 +549,7 @@ static struct brd_device *brd_init_one(int i)
 		add_disk(brd->brd_disk);
 		list_add_tail(&brd->brd_list, &brd_devices);
 	}
+	*new = true;
 out:
 	return brd;
 }

@@ -561,70 +565,46 @@ static struct kobject *brd_probe(dev_t dev, int *part, void *data)
 {
 	struct brd_device *brd;
 	struct kobject *kobj;
+	bool new;
 
 	mutex_lock(&brd_devices_mutex);
-	brd = brd_init_one(MINOR(dev) >> part_shift);
+	brd = brd_init_one(MINOR(dev) / max_part, &new);
 	kobj = brd ? get_disk(brd->brd_disk) : NULL;
 	mutex_unlock(&brd_devices_mutex);
 
-	*part = 0;
+	if (new)
+		*part = 0;
+
 	return kobj;
 }
 
 static int __init brd_init(void)
 {
-	int i, nr;
-	unsigned long range;
 	struct brd_device *brd, *next;
+	int i;
 
 	/*
 	 * brd module now has a feature to instantiate underlying device
 	 * structure on-demand, provided that there is an access dev node.
-	 * However, this will not work well with user space tool that doesn't
-	 * know about such "feature".  In order to not break any existing
-	 * tool, we do the following:
 	 *
-	 * (1) if rd_nr is specified, create that many upfront, and this
-	 *     also becomes a hard limit.
-	 * (2) if rd_nr is not specified, create CONFIG_BLK_DEV_RAM_COUNT
-	 *     (default 16) rd device on module load, user can further
-	 *     extend brd device by create dev node themselves and have
-	 *     kernel automatically instantiate actual device on-demand.
+	 * (1) if rd_nr is specified, create that many upfront. else
+	 *     it defaults to CONFIG_BLK_DEV_RAM_COUNT
+	 * (2) User can further extend brd devices by create dev node themselves
+	 *     and have kernel automatically instantiate actual device
+	 *     on-demand. Example:
+	 *		mknod /path/devnod_name b 1 X	# 1 is the rd major
+	 *		fdisk -l /path/devnod_name
+	 *	If (X / max_part) was not already created it will be created
+	 *	dynamically.
 	 */
 
-	part_shift = 0;
-	if (max_part > 0) {
-		part_shift = fls(max_part);
-
-		/*
-		 * Adjust max_part according to part_shift as it is exported
-		 * to user space so that user can decide correct minor number
-		 * if [s]he want to create more devices.
-		 *
-		 * Note that -1 is required because partition 0 is reserved
-		 * for the whole disk.
-		 */
-		max_part = (1UL << part_shift) - 1;
-	}
-
-	if ((1UL << part_shift) > DISK_MAX_PARTS)
-		return -EINVAL;
-
-	if (rd_nr > 1UL << (MINORBITS - part_shift))
-		return -EINVAL;
-
-	if (rd_nr) {
-		nr = rd_nr;
-		range = rd_nr << part_shift;
-	} else {
-		nr = CONFIG_BLK_DEV_RAM_COUNT;
-		range = 1UL << MINORBITS;
-	}
-
 	if (register_blkdev(RAMDISK_MAJOR, "ramdisk"))
 		return -EIO;
 
-	for (i = 0; i < nr; i++) {
+	if (unlikely(!max_part))
+		max_part = 1;
+
+	for (i = 0; i < rd_nr; i++) {
 		brd = brd_alloc(i);
 		if (!brd)
 			goto out_free;

@@ -636,7 +616,7 @@ static int __init brd_init(void)
 	list_for_each_entry(brd, &brd_devices, brd_list)
 		add_disk(brd->brd_disk);
 
-	blk_register_region(MKDEV(RAMDISK_MAJOR, 0), range,
+	blk_register_region(MKDEV(RAMDISK_MAJOR, 0), 1UL << MINORBITS,
 				  THIS_MODULE, brd_probe, NULL, NULL);
 
 	printk(KERN_INFO "brd: module loaded\n");

@@ -649,21 +629,20 @@ out_free:
 	}
 	unregister_blkdev(RAMDISK_MAJOR, "ramdisk");
 
+	printk(KERN_INFO "brd: module NOT loaded !!!\n");
 	return -ENOMEM;
 }
 
 static void __exit brd_exit(void)
 {
-	unsigned long range;
 	struct brd_device *brd, *next;
 
-	range = rd_nr ? rd_nr << part_shift : 1UL << MINORBITS;
-
 	list_for_each_entry_safe(brd, next, &brd_devices, brd_list)
 		brd_del_one(brd);
 
-	blk_unregister_region(MKDEV(RAMDISK_MAJOR, 0), range);
+	blk_unregister_region(MKDEV(RAMDISK_MAJOR, 0), 1UL << MINORBITS);
 	unregister_blkdev(RAMDISK_MAJOR, "ramdisk");
+	printk(KERN_INFO "brd: module unloaded\n");
 }
 
 module_init(brd_init);

On Wed, 2014-08-06 at 14:33 +0300, Boaz Harrosh wrote:

With this patch we end up in what feels like a weird place where we're half using the old scheme of major/minor allocation, and half in the world of dynamic major/minors. Devices have a major of 1 and minors that increment by 1, but partitions have a major of 259 (BLOCK_EXT_MAJOR):

	brw-rw---- 1 root disk   1, 0 Aug  6 14:10 /dev/ram0
	brw-rw---- 1 root disk   1, 1 Aug  6 14:13 /dev/ram1
	brw-rw---- 1 root disk 259, 0 Aug  6 14:14 /dev/ram1p1
	brw-rw---- 1 root disk 259, 1 Aug  6 14:13 /dev/ram1p2
	brw-rw---- 1 root disk 259, 2 Aug  6 14:14 /dev/ram1p51

For NVMe and PRD you get a major of 259 all around:

	brw-rw---- 1 root disk 259, 0 Aug  6 16:55 /dev/pmem0
	brw-rw---- 1 root disk 259, 2 Aug  6 16:55 /dev/pmem0p1
	brw-rw---- 1 root disk 259, 3 Aug  6 16:55 /dev/pmem0p2
	brw-rw---- 1 root disk 259, 1 Aug  6 16:54 /dev/pmem1

It could be that this is fine, but it just smells fishy to me I guess.

Also, it looks like you can still create a new device with this patch, but you can't create partitions on that device. Not sure if this is just what you get when you dynamically create a device on the fly, or if it's a symptom of something larger.

Quoted 5 lines · Ross Zwisler

Also, it looks like you can still create a new device with this patch, but you can't create partitions on that device. Not sure if this is just what you get when you dynamically create a device on the fly, or if it's a symptom of something larger.

What? I just tried again this all works fine for me, here with fdisk.

$ modprobe brd      # will create ram0-7
$ mknod /dev/ram8 b 1 8
$ fdisk /dev/ram8
  g, n, , , +2M, n, , , , , w 

I create 2 partitions 2M each and press w and it is all there.

What numbers did you use ? rd_nr, max_part, and the mknod numbers. Here it just works fine. What did you try?

This patch fixes up brd's partitions scheme, now enjoying all worlds.

The MAIN fix here is that currently if one fdisks some partitions, a BAD bug will make all partitions point to the same start-end sector
ie: 0 - brd_size And an mkfs of any partition would trash the partition table and the other partition.

Another fix is that "mount -U uuid" did not work, because of the GENHD_FL_SUPPRESS_PARTITION_INFO flag.

So NOW the logic goes like this:
* max_part - Just says how many minors to reserve between devices But in any way, there can be as many partition as requested. If minors between devices ends, then dynamic 259-major ids will be allocated on the fly.
The default is now max_part=1, which means all partitions devt will be from the dynamic major-range.
(If persistent partition minors is needed use max_part=)

* Creation of new devices on the fly still/always work:
mknod /path/devnod b 1 X
fdisk -l /path/devnod
Will create a new device if (X / max_part) was not already created before. (Just as before)

partitions on the dynamically created device will work as well Same logic applies with minors as with the pre-created ones.

TODO: dynamic grow of device size, maybe through sysfs. So each
device can have it's own size.

Signed-off-by Boaz Harrosh <[email protected]>

Because of the direct_access() API which returns a PFN. partitions better start on 4K boundary, else offset ZERO of a partition will not be aligned and blk_direct_access() will fail the call.

By setting blk_queue_physical_block_size(PAGE_SIZE) we can communicate this to fdisk and friends.
Note that blk_queue_physical_block_size() also trashes io_min, but we can leave this one to be 512.

Signed-off-by Boaz Harrosh <[email protected]>

---
 drivers/block/brd.c | 7 +++++++
 1 file changed, 7 insertions(+)

drivers/block/brd.c +1 -0

index 9673704..514cfe1 100644
@@ -495,10 +495,17 @@ static struct brd_device *brd_alloc(int i)
 	brd->brd_queue = blk_alloc_queue(GFP_KERNEL);
 	if (!brd->brd_queue)
 		goto out_free_dev;
+
 	blk_queue_make_request(brd->brd_queue, brd_make_request);
 	blk_queue_max_hw_sectors(brd->brd_queue, 1024);
 	blk_queue_bounce_limit(brd->brd_queue, BLK_BOUNCE_ANY);

Quoted 27 lines · Boaz Harrosh

Because of the direct_access() API which returns a PFN. partitions better start on 4K boundary, else offset ZERO of a partition will not be aligned and blk_direct_access() will fail the call.

By setting blk_queue_physical_block_size(PAGE_SIZE) we can communicate this to fdisk and friends.
Note that blk_queue_physical_block_size() also trashes io_min, but we can leave this one to be 512.

Signed-off-by Boaz Harrosh <[email protected]>

---
 drivers/block/brd.c | 7 +++++++
 1 file changed, 7 insertions(+)

drivers/block/brd.c +1 -0

index 9673704..514cfe1 100644
@@ -495,10 +495,17 @@ static struct brd_device *brd_alloc(int i)
 	brd->brd_queue = blk_alloc_queue(GFP_KERNEL);
 	if (!brd->brd_queue)
 		goto out_free_dev;
+
 	blk_queue_make_request(brd->brd_queue, brd_make_request);
 	blk_queue_max_hw_sectors(brd->brd_queue, 1024);
 	blk_queue_bounce_limit(brd->brd_queue, BLK_BOUNCE_ANY);
 

drivers/block/brd.c static struct brd_device *brd_alloc(int i) · line 511

Quoted 9 lines

drivers/block/brd.c +6 -0

+	/* This is so fdisk will align partitions on 4k, because of
+	 * direct_access API needing 4k alignment, returning a PFN
+	 */
+	blk_queue_physical_block_size(brd->brd_queue, PAGE_SIZE);
+	brd->brd_queue->limits.io_min = 512; /* Don't use the accessor */
+
 	brd->brd_queue->limits.discard_granularity = PAGE_SIZE;
 	brd->brd_queue->limits.max_discard_sectors = UINT_MAX;
 	brd->brd_queue->limits.discard_zeroes_data = 1;

Is there an error case that this patch fixes? I've had page alignment checks in my PRD direct_access code forever, and I don't know if they've ever tripped.

Also, blk_queue_physical_block_size() seems wrong - here's the comment for that function:

/**
* blk_queue_physical_block_size - set physical block size for the queue
* @q: the request queue for the device
* @size: the physical block size, in bytes
*
* Description:
* This should be set to the lowest possible sector size that the
* hardware can operate on without reverting to read-modify-write
* operations.
*/

It doesn't sound like this is what you're after? It sounds like instead you want to control the alignment, not minimum natural I/O size? It seems like if you did want to do this, blk_queue_alignment_offset() would be more what you were after?

- Ross

Quoted 4 lines

Is there an error case that this patch fixes? I've had page alignment checks in my PRD direct_access code forever, and I don't know if they've ever tripped.

Yes! as I said above fix fdisk. You never tripped on it because partitions never worked and you never tried them. With current code fdisk is very trigger happy

to miss-align my partitions. Depending on size maybe not the very first one but the consecutive ones easily.

Quoted 7 lines · Boaz Harrosh

Quoted 4 lines

Is there an error case that this patch fixes? I've had page alignment checks in my PRD direct_access code forever, and I don't know if they've ever tripped.

Yes! as I said above fix fdisk. You never tripped on it because partitions never worked and you never tried them. With current code fdisk is very trigger happy

What do you mean with fdisk? which version?

Quoted 5 lines · Boaz Harrosh

Quoted 4 lines

Is there an error case that this patch fixes? I've had page alignment checks in my PRD direct_access code forever, and I don't know if they've ever tripped.

Quoted 2 lines

Yes! as I said above fix fdisk. You never tripped on it because partitions never worked and you never tried them. With current code fdisk is very trigger happy

What do you mean with fdisk? which version?

The current fdisk (and cfdisk) follows I/O limits it has no problem with 4K devices. All you need is to provide all necessary information by /sys (or ioctls).

Quoted 2 lines

to miss-align my partitions. Depending on size maybe not the very first one but the consecutive ones easily.

it would be nice to have usable bug report...

Karel

I was not saying that fdisk is wrong. I was saying that if my block driver was *not* exporting 4K physical sectors through limits.physical_block_size

then fdisk would be happy to not align my partition start on 4k and would give me funny values like 34 for first sector which makes my device unusable because in direct_access() API we must absolutely have 4K aligned partitions.

Quoted 5 lines

Quoted 2 lines

to miss-align my partitions. Depending on size maybe not the very first one but the consecutive ones easily.

it would be nice to have usable bug report...

Setting limits.physical_block_size = 4k; was the only way I found that could cause fdisk to default to 4k alignment.

I was trying to play with the heads, sectors, cylinders; values but none I tried

would cause an alignment of 4k, not even of the first partition start.

Please advise what I can do?

fdisk uses physical sector size or minimal I/O size (greater value wins)

Quoted 1 line

I was trying to play with the heads, sectors, cylinders; values but none I tried

don't play with CHS, that's waste of time and it's completely ignored by fdisk by default

Quoted 3 lines

would cause an alignment of 4k, not even of the first partition start.

Please advise what I can do?

IMHO you're right with your patch (alignment offset is IMHO bad way). It's all (brd) about pages, is there any reason to use something else for I/O limits?

It would be also nice to set minimal and optimal io size, zero values in this case means (for userspace) that the device does not provide any I/O information to system. It's normal for old hw disks and then we use some built-in defaults, but I don't see a reason to do the same for virtual devices.

Karel