On Thu, 2015-03-26 at 09:06 +0100, Christoph Hellwig wrote:
> On Wed, Mar 25, 2015 at 02:21:53PM -0600, Ross Zwisler wrote:
> > What needed to be fixed with the partition support? I used to have real
> > numbers for first_minor and passed into alloc_disk(), but simplified it based
> > on code found in this commit in the nvme driver:
> >
> > 469071a37afc NVMe: Dynamically allocate partition numbers
> >
> > This has worked fine for me - is there some test case in which it breaks?
>
> Yes, if CONFIG_DEBUG_BLOCK_EXT_DEVT isn't set that code doesn't work at all.
I can't figure out a use case that breaks when using dynamically allocated
minors without CONFIG_DEBUG_BLOCK_EXT_DEVT. The patch that I've been testing
against is at the bottom of this mail.
Here are the minors that I get when creating a bunch of partitions using the
current code with PMEM_MINORS=16, with CONFIG_DEBUG_BLOCK_EXT_DEVT turned off:
pmem0 249:0 0 63.5G 0 rom
├─pmem0p1 249:1 0 1G 0 part
├─pmem0p2 249:2 0 1G 0 part
├─pmem0p3 249:3 0 1G 0 part
├─pmem0p4 249:4 0 1G 0 part
├─pmem0p5 249:5 0 1G 0 part
├─pmem0p6 249:6 0 1G 0 part
├─pmem0p7 249:7 0 1G 0 part
├─pmem0p8 249:8 0 1G 0 part
├─pmem0p9 249:9 0 1G 0 part
├─pmem0p10 249:10 0 1G 0 part
├─pmem0p11 249:11 0 1G 0 part
├─pmem0p12 249:12 0 1G 0 part
├─pmem0p13 249:13 0 1G 0 part
├─pmem0p14 249:14 0 1G 0 part
├─pmem0p15 249:15 0 1G 0 part
├─pmem0p16 259:0 0 1G 0 part
├─pmem0p17 259:1 0 1G 0 part
└─pmem0p18 259:2 0 1G 0 part
With dynamic minor allocation, with CONFIG_DEBUG_BLOCK_EXT_DEVT turned off:
pmem0 259:0 0 63.5G 0 rom
├─pmem0p1 259:1 0 1G 0 part
├─pmem0p2 259:2 0 1G 0 part
├─pmem0p3 259:3 0 1G 0 part
├─pmem0p4 259:4 0 1G 0 part
├─pmem0p5 259:5 0 1G 0 part
├─pmem0p6 259:6 0 1G 0 part
├─pmem0p7 259:7 0 1G 0 part
├─pmem0p8 259:8 0 1G 0 part
├─pmem0p9 259:9 0 1G 0 part
├─pmem0p10 259:10 0 1G 0 part
├─pmem0p11 259:11 0 1G 0 part
├─pmem0p12 259:12 0 1G 0 part
├─pmem0p13 259:13 0 1G 0 part
├─pmem0p14 259:14 0 1G 0 part
├─pmem0p15 259:15 0 1G 0 part
├─pmem0p16 259:16 0 1G 0 part
├─pmem0p17 259:17 0 1G 0 part
└─pmem0p18 259:18 0 1G 0 part
And with CONFIG_DEBUG_BLOCK_EXT_DEVT turned on:
pmem0 259:262144 0 63.5G 0 rom
├─pmem0p1 259:786432 0 1G 0 part
├─pmem0p2 259:131072 0 1G 0 part
├─pmem0p3 259:655360 0 1G 0 part
├─pmem0p4 259:393216 0 1G 0 part
├─pmem0p5 259:917504 0 1G 0 part
├─pmem0p6 259:65536 0 1G 0 part
├─pmem0p7 259:589824 0 1G 0 part
├─pmem0p8 259:327680 0 1G 0 part
├─pmem0p9 259:851968 0 1G 0 part
├─pmem0p10 259:196608 0 1G 0 part
├─pmem0p11 259:720896 0 1G 0 part
├─pmem0p12 259:458752 0 1G 0 part
├─pmem0p13 259:983040 0 1G 0 part
├─pmem0p14 259:32768 0 1G 0 part
├─pmem0p15 259:557056 0 1G 0 part
├─pmem0p16 259:294912 0 1G 0 part
├─pmem0p17 259:819200 0 1G 0 part
└─pmem0p18 259:163840 0 1G 0 part
With CONFIG_DEBUG_BLOCK_EXT_DEVT the minors are all mangled due to
blk_mangle_minor(), but I think that all three configs work?
Was there maybe confusion between that config option and the GENHD_FL_EXT_DEVT
gendisk flag, which AFAIK are independent?
Is there a use case that breaks when using dynamic minors without
CONFIG_DEBUG_BLOCK_EXT_DEVT?
Thanks,
- Ross
--- >8 ---
>From 6202dc7c1ef765faebb905161860c6b9ab19cc8a Mon Sep 17 00:00:00 2001
From: Ross Zwisler <[email protected]>
Date: Mon, 4 May 2015 10:26:54 -0600
Subject: [PATCH] pmem: Dynamically allocate partition numbers
Dynamically allocate minor numbers for partitions instead of statically
preallocating them.
Inspired by this commit:
469071a37afc NVMe: Dynamically allocate partition numbers
Signed-off-by: Ross Zwisler <[email protected]>
---
drivers/block/nd/pmem.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/block/nd/pmem.c b/drivers/block/nd/pmem.c
index 900dad61a6b9..b977def8981e 100644
--- a/drivers/block/nd/pmem.c
+++ b/drivers/block/nd/pmem.c
@@ -26,8 +26,6 @@
#include <linux/nd.h>
#include "nd.h"
-#define PMEM_MINORS 16
-
struct pmem_device {
struct request_queue *pmem_queue;
struct gendisk *pmem_disk;
@@ -185,12 +183,12 @@ static struct pmem_device *pmem_alloc(struct device *dev, struct resource *res)
blk_queue_max_hw_sectors(pmem->pmem_queue, 1024);
blk_queue_bounce_limit(pmem->pmem_queue, BLK_BOUNCE_ANY);
- disk = alloc_disk(PMEM_MINORS);
+ disk = alloc_disk(0);
if (!disk)
goto out_free_queue;
disk->major = pmem_major;
- disk->first_minor = PMEM_MINORS * pmem->id;
+ disk->first_minor = 0;
disk->fops = &pmem_fops;
disk->private_data = pmem;
disk->queue = pmem->pmem_queue;
--
1.9.3
On Mon, May 04, 2015 at 10:43:01AM -0600, Ross Zwisler wrote:
> > Yes, if CONFIG_DEBUG_BLOCK_EXT_DEVT isn't set that code doesn't work at all.
>
> I can't figure out a use case that breaks when using dynamically allocated
> minors without CONFIG_DEBUG_BLOCK_EXT_DEVT. The patch that I've been testing
> against is at the bottom of this mail.
>
> Here are the minors that I get when creating a bunch of partitions using the
> current code with PMEM_MINORS=16, with CONFIG_DEBUG_BLOCK_EXT_DEVT turned off:
FYI, your patch below works fine for me, but the original one certainly
didn't. One big difference was that it also removed the register_blkdev
call and thus assigning a major number.
On 05/07/2015 10:26 AM, Christoph Hellwig wrote:
> On Mon, May 04, 2015 at 10:43:01AM -0600, Ross Zwisler wrote:
>>> Yes, if CONFIG_DEBUG_BLOCK_EXT_DEVT isn't set that code doesn't work at all.
>>
>> I can't figure out a use case that breaks when using dynamically allocated
>> minors without CONFIG_DEBUG_BLOCK_EXT_DEVT. The patch that I've been testing
>> against is at the bottom of this mail.
>>
>> Here are the minors that I get when creating a bunch of partitions using the
>> current code with PMEM_MINORS=16, with CONFIG_DEBUG_BLOCK_EXT_DEVT turned off:
>
> FYI, your patch below works fine for me, but the original one certainly
> didn't. One big difference was that it also removed the register_blkdev
> call and thus assigning a major number.
>
assigning a major number for what?
That "assigned major number" is then never used *anywhere* in the code at all
until it is unregistered. All the devices come up with the dynamic (259) major
the register_blkdev is just dead code. I have experimented with this a lot,
I have audited the all code stack, that major is never used, when doing the:
alloc_disk(0)
disk->flags |= GENHD_FL_EXT_DEVT
The:
disk->major = X
Is completely ignored and is immediately over written. The only relic of that
major registration is the pmem_major global member collecting dust.
Sigh, bad habits die hard, I don't really care you can keep it. Sorry for
the noise.
Cheers
Boaz