diff mbox

[1/3] pmem: Initial version of persistent memory driver

Message ID 1430757781.5434.6.camel@theros.lm.intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Ross Zwisler May 4, 2015, 4:43 p.m. UTC 
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 <ross.zwisler@linux.intel.com>
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 <ross.zwisler@linux.intel.com>
---
 drivers/block/nd/pmem.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

Comments

Christoph Hellwig May 7, 2015, 7:26 a.m. UTC | #1 
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Boaz Harrosh May 7, 2015, 8:35 a.m. UTC | #2 
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

--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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;