diff mbox

libnd: stable(ish) block device names

Message ID 20150430221144.26376.30479.stgit@dwillia2-desk3.amr.corp.intel.com
State Superseded
Headers show

Commit Message

Dan Williams April 30, 2015, 10:15 p.m. UTC
>From Toshi:

    We have seen a case that the region#->pmem# binding becomes
    inconsistent across a reboot when there are 8 NVDIMM cards
    (reported by Robert Elliott).  This leads user to access a
    wrong device.

While it is a bug for userspace to depend on stable device names from
boot-to-boot, we should try not to give random results in the case where
the configuration has not changed.  Now that PMEM only ever attaches to
'region' devices emitted by libnd, we can simply carry the region-id all
the way down to the pmem device name.

Since BLK-regions allow multiple namespaces per-region we need to carry
the <region-id>.<namespace-index> in the block device name if we want
stable(ish) device names.

Cc: Christoph Hellwig <hch@lst.de>
Cc: Ross Zwisler <ross.zwisler@linux.intel.com>
Reported-by: Toshi Kani <toshi.kani@hp.com>
Reported-by: Robert Elliott <Elliott@hp.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---

Will fold this into the v3 posting.  v3 will be a rebase on top of the
upcoming ACPICA NFIT enabling.

 drivers/block/nd/blk.c  |   14 ++------------
 drivers/block/nd/pmem.c |   21 +++++----------------
 2 files changed, 7 insertions(+), 28 deletions(-)

Comments

Dan Williams April 30, 2015, 10:29 p.m. UTC | #1
[ adding Linda, Toshi, and Robert ]

On Thu, Apr 30, 2015 at 3:15 PM, Dan Williams <dan.j.williams@intel.com> wrote:
> >From Toshi:
>
>     We have seen a case that the region#->pmem# binding becomes
>     inconsistent across a reboot when there are 8 NVDIMM cards
>     (reported by Robert Elliott).  This leads user to access a
>     wrong device.
>
> While it is a bug for userspace to depend on stable device names from
> boot-to-boot, we should try not to give random results in the case where
> the configuration has not changed.  Now that PMEM only ever attaches to
> 'region' devices emitted by libnd, we can simply carry the region-id all
> the way down to the pmem device name.
>
> Since BLK-regions allow multiple namespaces per-region we need to carry
> the <region-id>.<namespace-index> in the block device name if we want
> stable(ish) device names.
>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Ross Zwisler <ross.zwisler@linux.intel.com>
> Reported-by: Toshi Kani <toshi.kani@hp.com>
> Reported-by: Robert Elliott <Elliott@hp.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>
> Will fold this into the v3 posting.  v3 will be a rebase on top of the
> upcoming ACPICA NFIT enabling.
>
>  drivers/block/nd/blk.c  |   14 ++------------
>  drivers/block/nd/pmem.c |   21 +++++----------------
>  2 files changed, 7 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/block/nd/blk.c b/drivers/block/nd/blk.c
> index 8536ee8b2009..9a2b93bdeccc 100644
> --- a/drivers/block/nd/blk.c
> +++ b/drivers/block/nd/blk.c
> @@ -28,11 +28,9 @@ struct nd_blk_device {
>         struct nd_blk_region *ndbr;
>         struct nd_io ndio;
>         size_t disk_size;
> -       int id;
>  };
>
>  static int nd_blk_major;
> -static DEFINE_IDA(nd_blk_ida);
>
>  static resource_size_t to_dev_offset(struct nd_namespace_blk *nsblk,
>                                 resource_size_t ns_offset, unsigned int len)
> @@ -139,6 +137,7 @@ static const struct block_device_operations nd_blk_fops = {
>  static int nd_blk_probe(struct device *dev)
>  {
>         struct nd_namespace_blk *nsblk = to_nd_namespace_blk(dev);
> +       struct nd_region *nd_region = to_nd_region(dev->parent);
>         struct nd_blk_device *blk_dev;
>         resource_size_t disk_size;
>         struct gendisk *disk;
> @@ -152,12 +151,6 @@ static int nd_blk_probe(struct device *dev)
>         if (!blk_dev)
>                 return -ENOMEM;
>
> -       blk_dev->id = ida_simple_get(&nd_blk_ida, 0, 0, GFP_KERNEL);
> -       if (blk_dev->id < 0) {
> -               err = blk_dev->id;
> -               goto err_ida;
> -       }
> -
>         blk_dev->disk_size      = disk_size;
>
>         blk_dev->queue = blk_alloc_queue(GFP_KERNEL);
> @@ -186,7 +179,7 @@ static int nd_blk_probe(struct device *dev)
>         disk->private_data      = blk_dev;
>         disk->queue             = blk_dev->queue;
>         disk->flags             = GENHD_FL_EXT_DEVT;
> -       sprintf(disk->disk_name, "ndblk%d", blk_dev->id);
> +       sprintf(disk->disk_name, "ndblk%d.%d", nd_region->id, nsblk->id);
>         set_capacity(disk, disk_size >> SECTOR_SHIFT);
>
>         nd_bus_lock(dev);
> @@ -202,8 +195,6 @@ static int nd_blk_probe(struct device *dev)
>   err_alloc_disk:
>         blk_cleanup_queue(blk_dev->queue);
>   err_alloc_queue:
> -       ida_simple_remove(&nd_blk_ida, blk_dev->id);
> - err_ida:
>         kfree(blk_dev);
>         return err;
>  }
> @@ -219,7 +210,6 @@ static int nd_blk_remove(struct device *dev)
>         del_gendisk(blk_dev->disk);
>         put_disk(blk_dev->disk);
>         blk_cleanup_queue(blk_dev->queue);
> -       ida_simple_remove(&nd_blk_ida, blk_dev->id);
>         kfree(blk_dev);
>
>         return 0;
> diff --git a/drivers/block/nd/pmem.c b/drivers/block/nd/pmem.c
> index 900dad61a6b9..5814c8aed481 100644
> --- a/drivers/block/nd/pmem.c
> +++ b/drivers/block/nd/pmem.c
> @@ -37,11 +37,9 @@ struct pmem_device {
>         phys_addr_t             phys_addr;
>         void                    *virt_addr;
>         size_t                  size;
> -       int                     id;
>  };
>
>  static int pmem_major;
> -static DEFINE_IDA(pmem_ida);
>
>  static void pmem_do_bvec(struct pmem_device *pmem, struct page *page,
>                         unsigned int len, unsigned int off, int rw,
> @@ -142,7 +140,7 @@ static const struct block_device_operations pmem_fops = {
>         .direct_access =        pmem_direct_access,
>  };
>
> -static struct pmem_device *pmem_alloc(struct device *dev, struct resource *res)
> +static struct pmem_device *pmem_alloc(struct device *dev, struct resource *res, int id)
>  {
>         struct pmem_device *pmem;
>         struct gendisk *disk;
> @@ -153,19 +151,13 @@ static struct pmem_device *pmem_alloc(struct device *dev, struct resource *res)
>         if (!pmem)
>                 goto out;
>
> -       pmem->id = ida_simple_get(&pmem_ida, 0, 0, GFP_KERNEL);
> -       if (pmem->id < 0) {
> -               err = pmem->id;
> -               goto out_free_dev;
> -       }
> -
>         pmem->phys_addr = res->start;
>         pmem->size = resource_size(res);
>
>         err = -EINVAL;
>         if (!request_mem_region(pmem->phys_addr, pmem->size, "pmem")) {
>                 dev_warn(dev, "could not reserve region [0x%pa:0x%zx]\n", &pmem->phys_addr, pmem->size);
> -               goto out_free_ida;
> +               goto out_free_dev;
>         }
>
>         /*
> @@ -190,12 +182,12 @@ static struct pmem_device *pmem_alloc(struct device *dev, struct resource *res)
>                 goto out_free_queue;
>
>         disk->major             = pmem_major;
> -       disk->first_minor       = PMEM_MINORS * pmem->id;
> +       disk->first_minor       = PMEM_MINORS * id;
>         disk->fops              = &pmem_fops;
>         disk->private_data      = pmem;
>         disk->queue             = pmem->pmem_queue;
>         disk->flags             = GENHD_FL_EXT_DEVT;
> -       sprintf(disk->disk_name, "pmem%d", pmem->id);
> +       sprintf(disk->disk_name, "pmem%d", id);
>         disk->driverfs_dev = dev;
>         set_capacity(disk, pmem->size >> 9);
>         pmem->pmem_disk = disk;
> @@ -208,8 +200,6 @@ out_unmap:
>         iounmap(pmem->virt_addr);
>  out_release_region:
>         release_mem_region(pmem->phys_addr, pmem->size);
> -out_free_ida:
> -       ida_simple_remove(&pmem_ida, pmem->id);
>  out_free_dev:
>         kfree(pmem);
>  out:
> @@ -223,7 +213,6 @@ static void pmem_free(struct pmem_device *pmem)
>         blk_cleanup_queue(pmem->pmem_queue);
>         iounmap(pmem->virt_addr);
>         release_mem_region(pmem->phys_addr, pmem->size);
> -       ida_simple_remove(&pmem_ida, pmem->id);
>         kfree(pmem);
>  }
>
> @@ -250,7 +239,7 @@ static int nd_pmem_probe(struct device *dev)
>                 }
>         }
>
> -       pmem = pmem_alloc(dev, &nsio->res);
> +       pmem = pmem_alloc(dev, &nsio->res, nd_region->id);
>         if (IS_ERR(pmem))
>                 return PTR_ERR(pmem);
>
>
> _______________________________________________
> Linux-nvdimm mailing list
> Linux-nvdimm@lists.01.org
> https://lists.01.org/mailman/listinfo/linux-nvdimm
diff mbox

Patch

diff --git a/drivers/block/nd/blk.c b/drivers/block/nd/blk.c
index 8536ee8b2009..9a2b93bdeccc 100644
--- a/drivers/block/nd/blk.c
+++ b/drivers/block/nd/blk.c
@@ -28,11 +28,9 @@  struct nd_blk_device {
 	struct nd_blk_region *ndbr;
 	struct nd_io ndio;
 	size_t disk_size;
-	int id;
 };
 
 static int nd_blk_major;
-static DEFINE_IDA(nd_blk_ida);
 
 static resource_size_t to_dev_offset(struct nd_namespace_blk *nsblk,
 				resource_size_t ns_offset, unsigned int len)
@@ -139,6 +137,7 @@  static const struct block_device_operations nd_blk_fops = {
 static int nd_blk_probe(struct device *dev)
 {
 	struct nd_namespace_blk *nsblk = to_nd_namespace_blk(dev);
+	struct nd_region *nd_region = to_nd_region(dev->parent);
 	struct nd_blk_device *blk_dev;
 	resource_size_t disk_size;
 	struct gendisk *disk;
@@ -152,12 +151,6 @@  static int nd_blk_probe(struct device *dev)
 	if (!blk_dev)
 		return -ENOMEM;
 
-	blk_dev->id = ida_simple_get(&nd_blk_ida, 0, 0, GFP_KERNEL);
-	if (blk_dev->id < 0) {
-		err = blk_dev->id;
-		goto err_ida;
-	}
-
 	blk_dev->disk_size	= disk_size;
 
 	blk_dev->queue = blk_alloc_queue(GFP_KERNEL);
@@ -186,7 +179,7 @@  static int nd_blk_probe(struct device *dev)
 	disk->private_data	= blk_dev;
 	disk->queue		= blk_dev->queue;
 	disk->flags		= GENHD_FL_EXT_DEVT;
-	sprintf(disk->disk_name, "ndblk%d", blk_dev->id);
+	sprintf(disk->disk_name, "ndblk%d.%d", nd_region->id, nsblk->id);
 	set_capacity(disk, disk_size >> SECTOR_SHIFT);
 
 	nd_bus_lock(dev);
@@ -202,8 +195,6 @@  static int nd_blk_probe(struct device *dev)
  err_alloc_disk:
 	blk_cleanup_queue(blk_dev->queue);
  err_alloc_queue:
-	ida_simple_remove(&nd_blk_ida, blk_dev->id);
- err_ida:
 	kfree(blk_dev);
 	return err;
 }
@@ -219,7 +210,6 @@  static int nd_blk_remove(struct device *dev)
 	del_gendisk(blk_dev->disk);
 	put_disk(blk_dev->disk);
 	blk_cleanup_queue(blk_dev->queue);
-	ida_simple_remove(&nd_blk_ida, blk_dev->id);
 	kfree(blk_dev);
 
 	return 0;
diff --git a/drivers/block/nd/pmem.c b/drivers/block/nd/pmem.c
index 900dad61a6b9..5814c8aed481 100644
--- a/drivers/block/nd/pmem.c
+++ b/drivers/block/nd/pmem.c
@@ -37,11 +37,9 @@  struct pmem_device {
 	phys_addr_t		phys_addr;
 	void			*virt_addr;
 	size_t			size;
-	int			id;
 };
 
 static int pmem_major;
-static DEFINE_IDA(pmem_ida);
 
 static void pmem_do_bvec(struct pmem_device *pmem, struct page *page,
 			unsigned int len, unsigned int off, int rw,
@@ -142,7 +140,7 @@  static const struct block_device_operations pmem_fops = {
 	.direct_access =	pmem_direct_access,
 };
 
-static struct pmem_device *pmem_alloc(struct device *dev, struct resource *res)
+static struct pmem_device *pmem_alloc(struct device *dev, struct resource *res, int id)
 {
 	struct pmem_device *pmem;
 	struct gendisk *disk;
@@ -153,19 +151,13 @@  static struct pmem_device *pmem_alloc(struct device *dev, struct resource *res)
 	if (!pmem)
 		goto out;
 
-	pmem->id = ida_simple_get(&pmem_ida, 0, 0, GFP_KERNEL);
-	if (pmem->id < 0) {
-		err = pmem->id;
-		goto out_free_dev;
-	}
-
 	pmem->phys_addr = res->start;
 	pmem->size = resource_size(res);
 
 	err = -EINVAL;
 	if (!request_mem_region(pmem->phys_addr, pmem->size, "pmem")) {
 		dev_warn(dev, "could not reserve region [0x%pa:0x%zx]\n", &pmem->phys_addr, pmem->size);
-		goto out_free_ida;
+		goto out_free_dev;
 	}
 
 	/*
@@ -190,12 +182,12 @@  static struct pmem_device *pmem_alloc(struct device *dev, struct resource *res)
 		goto out_free_queue;
 
 	disk->major		= pmem_major;
-	disk->first_minor	= PMEM_MINORS * pmem->id;
+	disk->first_minor	= PMEM_MINORS * id;
 	disk->fops		= &pmem_fops;
 	disk->private_data	= pmem;
 	disk->queue		= pmem->pmem_queue;
 	disk->flags		= GENHD_FL_EXT_DEVT;
-	sprintf(disk->disk_name, "pmem%d", pmem->id);
+	sprintf(disk->disk_name, "pmem%d", id);
 	disk->driverfs_dev = dev;
 	set_capacity(disk, pmem->size >> 9);
 	pmem->pmem_disk = disk;
@@ -208,8 +200,6 @@  out_unmap:
 	iounmap(pmem->virt_addr);
 out_release_region:
 	release_mem_region(pmem->phys_addr, pmem->size);
-out_free_ida:
-	ida_simple_remove(&pmem_ida, pmem->id);
 out_free_dev:
 	kfree(pmem);
 out:
@@ -223,7 +213,6 @@  static void pmem_free(struct pmem_device *pmem)
 	blk_cleanup_queue(pmem->pmem_queue);
 	iounmap(pmem->virt_addr);
 	release_mem_region(pmem->phys_addr, pmem->size);
-	ida_simple_remove(&pmem_ida, pmem->id);
 	kfree(pmem);
 }
 
@@ -250,7 +239,7 @@  static int nd_pmem_probe(struct device *dev)
 		}
 	}
 
-	pmem = pmem_alloc(dev, &nsio->res);
+	pmem = pmem_alloc(dev, &nsio->res, nd_region->id);
 	if (IS_ERR(pmem))
 		return PTR_ERR(pmem);