Remove un-needed 'major' registration when alloc_disk(0) is used.

PATCH In review NeilBrown 5 participants 13 messages Mar 9, 2016 – Mar 16, 2016

Subsystems (4)

NVMe libnvdimm: non-volatile memory device Sony memorystick libnvdimm BTT: block translation table

lore

Share & tools

lore

NeilBrown

author

2016-03-09 21:59 UTC #0 lore raw

cover Remove un-needed 'major' registration when alloc_disk(0) is used.

When alloc_disk(0) is used, the ->major number is ignored and irrelevant. Yet several drivers register a major number anyway.

This series of patches removes the pointless registrations. The pmem driver also does this, but a patch has already been sent for that driver.

Note that I am not in a position to test these beyond simple compile testing.

Thanks,
NeilBrown

---

NeilBrown (4):
nvdimm/blk: don't allocate unused major device number nvdimm/btt: don't allocate unused major device number
memstick: don't allocate unused major for ms_block
NVMe: don't allocate unused nvme_major

 drivers/memstick/core/ms_block.c |   17 ++---------------
 drivers/nvdimm/blk.c             |   18 +-----------------
 drivers/nvdimm/btt.c             |   19 ++-----------------
 drivers/nvme/host/core.c         |   16 +---------------
 4 files changed, 6 insertions(+), 64 deletions(-)

--
Signature

NeilBrown

author

2016-03-09 21:59 UTC #1 lore raw

PATCH 1/4 nvdimm/blk: don't allocate unused major device number

When alloc_disk(0) is used ->major is completely ignored, all devices are allocated with a "major" of BLOCK_EXT_MAJOR.

So don't allocate nd_blk_major

Signed-off-by NeilBrown <neilb at suse.com>

---
 drivers/nvdimm/blk.c |   18 +-----------------
 1 file changed, 1 insertion(+), 17 deletions(-)

drivers/nvdimm/blk.c +1 ~~-17~~

index 91a336ea8c4f..e9ff9229d942 100644
@@ -31,8 +31,6 @@ struct nd_blk_device {
 	u32 internal_lbasize;
 };
 
-static int nd_blk_major;
-
 static u32 nd_blk_meta_size(struct nd_blk_device *blk_dev)
 {
 	return blk_dev->nsblk->lbasize - blk_dev->sector_size;

@@ -264,7 +262,6 @@ static int nd_blk_attach_disk(struct nd_namespace_common *ndns,
 	}
 
 	disk->driverfs_dev	= &ndns->dev;
-	disk->major		= nd_blk_major;
 	disk->first_minor	= 0;
 	disk->fops		= &nd_blk_fops;
 	disk->private_data	= blk_dev;

@@ -358,25 +355,12 @@ static struct nd_device_driver nd_blk_driver = {
 
 static int __init nd_blk_init(void)
 {
-	int rc;
-
-	rc = register_blkdev(0, "nd_blk");
-	if (rc < 0)
-		return rc;
-
-	nd_blk_major = rc;
-	rc = nd_driver_register(&nd_blk_driver);
-
-	if (rc < 0)
-		unregister_blkdev(nd_blk_major, "nd_blk");
-
-	return rc;
+	return nd_driver_register(&nd_blk_driver);
 }
 
 static void __exit nd_blk_exit(void)
 {
 	driver_unregister(&nd_blk_driver.drv);
-	unregister_blkdev(nd_blk_major, "nd_blk");
 }
 
 MODULE_AUTHOR("Ross Zwisler <ross.zwisler at linux.intel.com>");

Johannes Thumshirn

Reviewed-by on v1 1/4

2016-03-10 09:05 UTC #2 lore raw

Re: [PATCH 1/4] nvdimm/blk: don't allocate unused major device number

Quoted 6 lines · NeilBrown

When alloc_disk(0) is used ->major is completely ignored, all devices are allocated with a "major" of BLOCK_EXT_MAJOR.
So don't allocate nd_blk_major
Signed-off-by NeilBrown <neilb at suse.com>

Reviewed-by Johannes Thumshirn <jthumshirn at suse.de>

Signature · 6 lines

Johannes Thumshirn                                          Storage
jthumshirn at suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: Felix Imend?rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N?rnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

NeilBrown

author

2016-03-09 21:59 UTC #3 lore raw

PATCH 2/4 nvdimm/btt: don't allocate unused major device number

In reply to NeilBrown

alloc_disk(0) does not require or use a ->major number, all devices are allocated with a major of BLOCK_EXT_MAJOR.

So don't allocate btt_major.

Signed-off-by NeilBrown <neilb at suse.com>

---
 drivers/nvdimm/btt.c |   19 ++-----------------
 1 file changed, 2 insertions(+), 17 deletions(-)

drivers/nvdimm/btt.c +2 ~~-17~~

index efb2c1ceef98..c32cbb593600 100644
@@ -31,8 +31,6 @@ enum log_ent_request {
 	LOG_OLD_ENT
 };
 
-static int btt_major;
-
 static int arena_read_bytes(struct arena_info *arena, resource_size_t offset,
 		void *buf, size_t n)
 {

@@ -1246,7 +1244,6 @@ static int btt_blk_init(struct btt *btt)
 
 	nvdimm_namespace_disk_name(ndns, btt->btt_disk->disk_name);
 	btt->btt_disk->driverfs_dev = &btt->nd_btt->dev;
-	btt->btt_disk->major = btt_major;
 	btt->btt_disk->first_minor = 0;
 	btt->btt_disk->fops = &btt_fops;
 	btt->btt_disk->private_data = btt;

@@ -1423,22 +1420,11 @@ EXPORT_SYMBOL(nvdimm_namespace_detach_btt);
 
 static int __init nd_btt_init(void)
 {
-	int rc;
-
-	btt_major = register_blkdev(0, "btt");
-	if (btt_major < 0)
-		return btt_major;
+	int rc = 0;
 
 	debugfs_root = debugfs_create_dir("btt", NULL);
-	if (IS_ERR_OR_NULL(debugfs_root)) {
+	if (IS_ERR_OR_NULL(debugfs_root))
 		rc = -ENXIO;
-		goto err_debugfs;
-	}
-
-	return 0;
-
- err_debugfs:
-	unregister_blkdev(btt_major, "btt");
 
 	return rc;
 }

@@ -1446,7 +1432,6 @@ static int __init nd_btt_init(void)
 static void __exit nd_btt_exit(void)
 {
 	debugfs_remove_recursive(debugfs_root);
-	unregister_blkdev(btt_major, "btt");
 }
 
 MODULE_ALIAS_ND_DEVICE(ND_DEVICE_BTT);

Johannes Thumshirn

Reviewed-by on v1 2/4

2016-03-10 09:05 UTC #4 lore raw

Re: [PATCH 2/4] nvdimm/btt: don't allocate unused major device number

Quoted 6 lines · NeilBrown

alloc_disk(0) does not require or use a ->major number, all devices are allocated with a major of BLOCK_EXT_MAJOR.
So don't allocate btt_major.
Signed-off-by NeilBrown <neilb at suse.com>

Reviewed-by Johannes Thumshirn <jthumshirn at suse.de>

Signature · 6 lines

Johannes Thumshirn                                          Storage
jthumshirn at suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: Felix Imend?rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N?rnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

NeilBrown

author

2016-03-09 21:59 UTC #5 lore raw

PATCH 3/4 memstick: don't allocate unused major for ms_block

In reply to NeilBrown

When alloc_disk(0) is used the ->major number is completely ignored. All devices are allocated with a major of BLOCK_EXT_MAJOR.

So remove registration and deregistration of 'major'.

Signed-off-by NeilBrown <neilb at suse.com>

---
 drivers/memstick/core/ms_block.c |   17 ++---------------
 1 file changed, 2 insertions(+), 15 deletions(-)

drivers/memstick/core/ms_block.c +2 ~~-15~~

index 84abf9d3c24e..3d086f1d1234 100644
@@ -2340,23 +2340,11 @@ static struct memstick_driver msb_driver = {
 	.resume   = msb_resume
 };
 
-static int major;
-
 static int __init msb_init(void)
 {
-	int rc = register_blkdev(0, DRIVER_NAME);
-
-	if (rc < 0) {
-		pr_err("failed to register major (error %d)\n", rc);
-		return rc;
-	}
-
-	major = rc;
-	rc = memstick_register_driver(&msb_driver);
-	if (rc) {
-		unregister_blkdev(major, DRIVER_NAME);
+	int rc = memstick_register_driver(&msb_driver);
+	if (rc)
 		pr_err("failed to register memstick driver (error %d)\n", rc);
-	}
 
 	return rc;
 }

@@ -2364,7 +2352,6 @@ static int __init msb_init(void)
 static void __exit msb_exit(void)
 {
 	memstick_unregister_driver(&msb_driver);
-	unregister_blkdev(major, DRIVER_NAME);
 	idr_destroy(&msb_disk_idr);
 }

Johannes Thumshirn

Reviewed-by on v1 3/4

2016-03-10 09:06 UTC #6 lore raw

Re: [PATCH 3/4] memstick: don't allocate unused major for ms_block

Quoted 6 lines · NeilBrown

When alloc_disk(0) is used the ->major number is completely ignored. All devices are allocated with a major of BLOCK_EXT_MAJOR.
So remove registration and deregistration of 'major'.
Signed-off-by NeilBrown <neilb at suse.com>

Reviewed-by Johannes Thumshirn <jthumshirn at suse.de>

Signature · 6 lines

Johannes Thumshirn                                          Storage
jthumshirn at suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: Felix Imend?rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N?rnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

NeilBrown

author

2016-03-09 21:59 UTC #7 lore raw

PATCH 4/4 NVMe: don't allocate unused nvme_major

In reply to NeilBrown

When alloc_disk(0) is used, the ->major number is ignored. All device numbers are allocated with a major of BLOCK_EXT_MAJOR.

So remove all references to nvme_major.

Signed-off-by NeilBrown <neilb at suse.com>

---
 drivers/nvme/host/core.c |   16 +---------------
 1 file changed, 1 insertion(+), 15 deletions(-)

drivers/nvme/host/core.c +1 ~~-15~~

index 03c46412fff4..10e2435c84a9 100644
@@ -33,9 +33,6 @@
 
 #define NVME_MINORS		(1U << MINORBITS)
 
-static int nvme_major;
-module_param(nvme_major, int, 0);
-
 static int nvme_char_major;
 module_param(nvme_char_major, int, 0);

@@ -1170,8 +1167,6 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid)
 	blk_queue_logical_block_size(ns->queue, 1 << ns->lba_shift);
 	nvme_set_queue_limits(ctrl, ns->queue);
 
-	disk->major = nvme_major;
-	disk->first_minor = 0;
 	disk->fops = &nvme_fops;
 	disk->private_data = ns;
 	disk->queue = ns->queue;

@@ -1480,16 +1475,10 @@ int __init nvme_core_init(void)
 {
 	int result;
 
-	result = register_blkdev(nvme_major, "nvme");
-	if (result < 0)
-		return result;
-	else if (result > 0)
-		nvme_major = result;
-
 	result = __register_chrdev(nvme_char_major, 0, NVME_MINORS, "nvme",
 							&nvme_dev_fops);
 	if (result < 0)
-		goto unregister_blkdev;
+		return result;
 	else if (result > 0)
 		nvme_char_major = result;

@@ -1503,14 +1492,11 @@ int __init nvme_core_init(void)
 
  unregister_chrdev:
 	__unregister_chrdev(nvme_char_major, 0, NVME_MINORS, "nvme");
- unregister_blkdev:
-	unregister_blkdev(nvme_major, "nvme");
 	return result;
 }
 
 void nvme_core_exit(void)
 {
-	unregister_blkdev(nvme_major, "nvme");
 	class_destroy(nvme_class);
 	__unregister_chrdev(nvme_char_major, 0, NVME_MINORS, "nvme");
 }

Johannes Thumshirn

Reviewed-by on v1 4/4

2016-03-10 09:06 UTC #8 lore raw

Re: [PATCH 4/4] NVMe: don't allocate unused nvme_major

Quoted 6 lines · NeilBrown

When alloc_disk(0) is used, the ->major number is ignored. All device numbers are allocated with a major of BLOCK_EXT_MAJOR.
So remove all references to nvme_major.
Signed-off-by NeilBrown <neilb at suse.com>

Reviewed-by Johannes Thumshirn <jthumshirn at suse.de>

Signature · 6 lines

Johannes Thumshirn                                          Storage
jthumshirn at suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: Felix Imend?rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N?rnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

Dan Williams

2016-03-09 23:35 UTC #9 lore raw

In reply to NeilBrown

Quoted 14 lines · NeilBrown

When alloc_disk(0) is used, the ->major number is ignored and irrelevant. Yet several drivers register a major number anyway.
This series of patches removes the pointless registrations. The pmem driver also does this, but a patch has already been sent for that driver.
Note that I am not in a position to test these beyond simple compile testing.
Thanks,
NeilBrown

Quoted 5 lines

---
NeilBrown (4):
nvdimm/blk: don't allocate unused major device number nvdimm/btt: don't allocate unused major device number

The libnvdimm unit tests were fine with these, applied.

Btw, because you mentioned it, our unit test infrastructure does not require real hardware. See the ndctl readme [1], and if you have copious amounts of free time the lwn write up on how we're mocking resources [2].

[1]: https://github.com/pmem/ndctl/blob/master/README.md
[2]: https://lwn.net/Articles/654071/

Ross Zwisler

2016-03-14 22:50 UTC #10 lore raw

In reply to NeilBrown

Quoted 23 lines · NeilBrown

When alloc_disk(0) is used, the ->major number is ignored and irrelevant. Yet several drivers register a major number anyway.
This series of patches removes the pointless registrations. The pmem driver also does this, but a patch has already been sent for that driver.
Note that I am not in a position to test these beyond simple compile testing.
Thanks,
NeilBrown
---
NeilBrown (4):
nvdimm/blk: don't allocate unused major device number nvdimm/btt: don't allocate unused major device number
memstick: don't allocate unused major for ms_block
NVMe: don't allocate unused nvme_major

Quoted 5 lines

 drivers/memstick/core/ms_block.c |   17 ++---------------
 drivers/nvdimm/blk.c             |   18 +-----------------
 drivers/nvdimm/btt.c             |   19 ++-----------------
 drivers/nvme/host/core.c         |   16 +---------------
 4 files changed, 6 insertions(+), 64 deletions(-)

There are several other drivers that allocate a major, but then use it for some small number of minors (1 for null_blk.c and 16 for virtio_blk.c). They both have GENHD_FL_EXT_DEVT set, so I think what happens is that after we exhaust the allocated minors they hop over to having BLOCK_EXT_MAJOR as a major and a dynamically assigned minor.

It seems like these could easily be converted in the same way so they'd use BLOCK_EXT_MAJOR for their major and have a bunch of dynamically assigned minors.

Does this break something I'm not seeing?

Yay for this series, by the way. :)

NeilBrown

author

2016-03-15 22:15 UTC #11 lore raw

Quoted 30 lines · Ross Zwisler

Quoted 23 lines · NeilBrown
When alloc_disk(0) is used, the ->major number is ignored and irrelevant. Yet several drivers register a major number anyway.
This series of patches removes the pointless registrations. The pmem driver also does this, but a patch has already been sent for that driver.
Note that I am not in a position to test these beyond simple compile testing.
Thanks,
NeilBrown
---
NeilBrown (4):
nvdimm/blk: don't allocate unused major device number nvdimm/btt: don't allocate unused major device number
memstick: don't allocate unused major for ms_block
NVMe: don't allocate unused nvme_major
Quoted 5 lines
 drivers/memstick/core/ms_block.c |   17 ++---------------
 drivers/nvdimm/blk.c             |   18 +-----------------
 drivers/nvdimm/btt.c             |   19 ++-----------------
 drivers/nvme/host/core.c         |   16 +---------------
 4 files changed, 6 insertions(+), 64 deletions(-)

Quoted 5 lines

There are several other drivers that allocate a major, but then use it for some small number of minors (1 for null_blk.c and 16 for virtio_blk.c). They both have GENHD_FL_EXT_DEVT set, so I think what happens is that after we exhaust the allocated minors they hop over to having BLOCK_EXT_MAJOR as a major and a dynamically assigned minor.

null_blk looks like it would be safe to convert - it is just used for testing. Jens Axboe would probably know for sure.

virtio_blk is a much older and there may will be code which has some sort of expectations about minor numbers. I think it would not be worth the risks to change it.

Quoted 8 lines

It seems like these could easily be converted in the same way so they'd use BLOCK_EXT_MAJOR for their major and have a bunch of dynamically assigned minors.
Does this break something I'm not seeing?
Yay for this series, by the way. :)

Thanks... two are in -next now (thank Dan) - I might poke the other two in a week or two if nothing happens.

Thanks,
NeilBrown

Jens Axboe

2016-03-16 00:20 UTC #12 lore raw

Quoted 38 lines · NeilBrown

Quoted 30 lines · Ross Zwisler
Quoted 23 lines · NeilBrown
When alloc_disk(0) is used, the ->major number is ignored and irrelevant. Yet several drivers register a major number anyway.
This series of patches removes the pointless registrations. The pmem driver also does this, but a patch has already been sent for that driver.
Note that I am not in a position to test these beyond simple compile testing.
Thanks,
NeilBrown
---
NeilBrown (4):
nvdimm/blk: don't allocate unused major device number nvdimm/btt: don't allocate unused major device number
memstick: don't allocate unused major for ms_block
NVMe: don't allocate unused nvme_major
Quoted 5 lines
  drivers/memstick/core/ms_block.c |   17 ++---------------
  drivers/nvdimm/blk.c             |   18 +-----------------
  drivers/nvdimm/btt.c             |   19 ++-----------------
  drivers/nvme/host/core.c         |   16 +---------------
  4 files changed, 6 insertions(+), 64 deletions(-)
Quoted 5 lines
There are several other drivers that allocate a major, but then use it for some small number of minors (1 for null_blk.c and 16 for virtio_blk.c). They both have GENHD_FL_EXT_DEVT set, so I think what happens is that after we exhaust the allocated minors they hop over to having BLOCK_EXT_MAJOR as a major and a dynamically assigned minor.

Quoted 6 lines

null_blk looks like it would be safe to convert - it is just used for testing. Jens Axboe would probably know for sure.
virtio_blk is a much older and there may will be code which has some sort of expectations about minor numbers. I think it would not be worth the risks to change it.

Agree on both - null_blk can be trivially converted, and I too would be worried about virt_blkio changes breaking existing assumptions.

Signature · 1 line

Jens Axboe