[PATCH 0/3] fixes fo LightNVM

[PATCH 0/3] fixes fo LightNVM

[Wenwei Tao]

Hi

A couple of fixes.

Patch 1 add the missing lock aquire
Patch 2-3 handle the targets when corresponding underlying devices were removed


Wenwei Tao (3):
  lightnvm: missing nvm_lock acquire
  lightnvm: handle targets when corresponding nvm device exit
  nvme: change the interface between nvme and lightnvm

 drivers/lightnvm/core.c      | 140 ++++++++++++++++++++++++++++---------------
 drivers/nvme/host/lightnvm.c |  17 +++++-
 drivers/nvme/host/nvme.h     |   1 +
 drivers/nvme/host/pci.c      |  10 ++--
 include/linux/lightnvm.h     |   3 +
 5 files changed, 118 insertions(+), 53 deletions(-)

-- 
1.8.3.1

[PATCH 1/3] lightnvm: missing nvm_lock acquire

[Wenwei Tao]

To avoid race conditions, traverse dev, media manager,
and targeet lists and also register, unregister entries
to/from them, should be always under the nvm_lock control.

Signed-off-by: Wenwei Tao <ww.tao0320@gmail.com>
---
 drivers/lightnvm/core.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/drivers/lightnvm/core.c b/drivers/lightnvm/core.c
index f659e60..39aec3a 100644
--- a/drivers/lightnvm/core.c
+++ b/drivers/lightnvm/core.c
@@ -333,19 +333,19 @@ EXPORT_SYMBOL(nvm_register);
 
 void nvm_unregister(char *disk_name)
 {
+	down_write(&nvm_lock);
 	struct nvm_dev *dev = nvm_find_nvm_dev(disk_name);
 
 	if (!dev) {
 		pr_err("nvm: could not find device %s to unregister\n",
 								disk_name);
+		up_write(&nvm_lock);
 		return;
 	}
 
-	nvm_exit(dev);
-
-	down_write(&nvm_lock);
 	list_del(&dev->devices);
 	up_write(&nvm_lock);
+	nvm_exit(dev);
 }
 EXPORT_SYMBOL(nvm_unregister);
 
@@ -365,12 +365,15 @@ static int nvm_create_target(struct nvm_dev *dev,
 	void *targetdata;
 	int ret = 0;
 
+	down_write(&nvm_lock);
 	if (!dev->mt) {
 		/* register with device with a supported NVM manager */
 		list_for_each_entry(mt, &nvm_mgrs, list) {
 			ret = mt->register_mgr(dev);
-			if (ret < 0)
+			if (ret < 0) {
+				up_write(&nvm_lock);
 				return ret; /* initialization failed */
+			}
 			if (ret > 0) {
 				dev->mt = mt;
 				break; /* successfully initialized */
@@ -379,6 +382,7 @@ static int nvm_create_target(struct nvm_dev *dev,
 
 		if (!ret) {
 			pr_info("nvm: no compatible nvm manager found.\n");
+			up_write(&nvm_lock);
 			return -ENODEV;
 		}
 	}
@@ -386,10 +390,10 @@ static int nvm_create_target(struct nvm_dev *dev,
 	tt = nvm_find_target_type(create->tgttype);
 	if (!tt) {
 		pr_err("nvm: target type %s not found\n", create->tgttype);
+		up_write(&nvm_lock);
 		return -EINVAL;
 	}
 
-	down_write(&nvm_lock);
 	list_for_each_entry(t, &dev->online_targets, list) {
 		if (!strcmp(create->tgtname, t->disk->disk_name)) {
 			pr_err("nvm: target name already exists.\n");
@@ -472,8 +476,9 @@ static int __nvm_configure_create(struct nvm_ioctl_create *create)
 {
 	struct nvm_dev *dev;
 	struct nvm_ioctl_create_simple *s;
-
+	down_write(&nvm_lock);
 	dev = nvm_find_nvm_dev(create->dev);
+	up_write(&nvm_lock);
 	if (!dev) {
 		pr_err("nvm: device not found\n");
 		return -EINVAL;
@@ -532,7 +537,9 @@ static int nvm_configure_show(const char *val)
 		return -EINVAL;
 	}
 
+	down_write(&nvm_lock);
 	dev = nvm_find_nvm_dev(devname);
+	up_write(&nvm_lock);
 	if (!dev) {
 		pr_err("nvm: device not found\n");
 		return -EINVAL;
-- 
1.8.3.1

Re: [PATCH 1/3] lightnvm: missing nvm_lock acquire

[Matias Bjørling]

On 11/25/2015 01:42 PM, Wenwei Tao wrote:
> To avoid race conditions, traverse dev, media manager,
> and targeet lists and also register, unregister entries
> to/from them, should be always under the nvm_lock control.
>
> Signed-off-by: Wenwei Tao <ww.tao0320@gmail.com>
> ---
>   drivers/lightnvm/core.c | 19 +++++++++++++------
>   1 file changed, 13 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/lightnvm/core.c b/drivers/lightnvm/core.c
> index f659e60..39aec3a 100644
> --- a/drivers/lightnvm/core.c
> +++ b/drivers/lightnvm/core.c
> @@ -333,19 +333,19 @@ EXPORT_SYMBOL(nvm_register);
>
>   void nvm_unregister(char *disk_name)
>   {
> +	down_write(&nvm_lock);
>   	struct nvm_dev *dev = nvm_find_nvm_dev(disk_name);
>
>   	if (!dev) {
>   		pr_err("nvm: could not find device %s to unregister\n",
>   								disk_name);
> +		up_write(&nvm_lock);
>   		return;
>   	}
>
> -	nvm_exit(dev);
> -
> -	down_write(&nvm_lock);
>   	list_del(&dev->devices);
>   	up_write(&nvm_lock);
> +	nvm_exit(dev);
>   }
>   EXPORT_SYMBOL(nvm_unregister);
>
> @@ -365,12 +365,15 @@ static int nvm_create_target(struct nvm_dev *dev,
>   	void *targetdata;
>   	int ret = 0;
>
> +	down_write(&nvm_lock);
>   	if (!dev->mt) {
>   		/* register with device with a supported NVM manager */
>   		list_for_each_entry(mt, &nvm_mgrs, list) {
>   			ret = mt->register_mgr(dev);
> -			if (ret < 0)
> +			if (ret < 0) {
> +				up_write(&nvm_lock);
>   				return ret; /* initialization failed */
> +			}
>   			if (ret > 0) {
>   				dev->mt = mt;
>   				break; /* successfully initialized */
> @@ -379,6 +382,7 @@ static int nvm_create_target(struct nvm_dev *dev,
>
>   		if (!ret) {
>   			pr_info("nvm: no compatible nvm manager found.\n");
> +			up_write(&nvm_lock);
>   			return -ENODEV;
>   		}
>   	}
> @@ -386,10 +390,10 @@ static int nvm_create_target(struct nvm_dev *dev,
>   	tt = nvm_find_target_type(create->tgttype);
>   	if (!tt) {
>   		pr_err("nvm: target type %s not found\n", create->tgttype);
> +		up_write(&nvm_lock);
>   		return -EINVAL;
>   	}
>
> -	down_write(&nvm_lock);
>   	list_for_each_entry(t, &dev->online_targets, list) {
>   		if (!strcmp(create->tgtname, t->disk->disk_name)) {
>   			pr_err("nvm: target name already exists.\n");
> @@ -472,8 +476,9 @@ static int __nvm_configure_create(struct nvm_ioctl_create *create)
>   {
>   	struct nvm_dev *dev;
>   	struct nvm_ioctl_create_simple *s;
> -
> +	down_write(&nvm_lock);
>   	dev = nvm_find_nvm_dev(create->dev);
> +	up_write(&nvm_lock);
>   	if (!dev) {
>   		pr_err("nvm: device not found\n");
>   		return -EINVAL;
> @@ -532,7 +537,9 @@ static int nvm_configure_show(const char *val)
>   		return -EINVAL;
>   	}
>
> +	down_write(&nvm_lock);
>   	dev = nvm_find_nvm_dev(devname);
> +	up_write(&nvm_lock);
>   	if (!dev) {
>   		pr_err("nvm: device not found\n");
>   		return -EINVAL;
>
Thanks Tao, applied.

[PATCH 2/3] lightnvm: handle targets when corresponding nvm device exit

[Wenwei Tao]

block creations of new targets, remove exiting targets when
underlying device was gone.

Signed-off-by: Wenwei Tao <ww.tao0320@gmail.com>
---
 drivers/lightnvm/core.c  | 127 ++++++++++++++++++++++++++++++-----------------
 include/linux/lightnvm.h |   3 ++
 2 files changed, 85 insertions(+), 45 deletions(-)

diff --git a/drivers/lightnvm/core.c b/drivers/lightnvm/core.c
index 39aec3a..0b71dd2 100644
--- a/drivers/lightnvm/core.c
+++ b/drivers/lightnvm/core.c
@@ -33,6 +33,17 @@ static LIST_HEAD(nvm_targets);
 static LIST_HEAD(nvm_mgrs);
 static LIST_HEAD(nvm_devices);
 static DECLARE_RWSEM(nvm_lock);
+#define NVM_EXITING	1
+
+static inline int NvmExiting(struct nvm_dev *dev)
+{
+	return ((unsigned long)dev->ppalist_pool & NVM_EXITING) != 0;
+}
+
+static inline void *ppalist_pool(struct nvm_dev *dev)
+{
+	return (void *)((unsigned long)dev->ppalist_pool & ~NVM_EXITING);
+}
 
 static struct nvm_tgt_type *nvm_find_target_type(const char *name)
 {
@@ -74,7 +85,7 @@ EXPORT_SYMBOL(nvm_unregister_target);
 void *nvm_dev_dma_alloc(struct nvm_dev *dev, gfp_t mem_flags,
 							dma_addr_t *dma_handler)
 {
-	return dev->ops->dev_dma_alloc(dev->q, dev->ppalist_pool, mem_flags,
+	return dev->ops->dev_dma_alloc(dev->q, ppalist_pool(dev), mem_flags,
 								dma_handler);
 }
 EXPORT_SYMBOL(nvm_dev_dma_alloc);
@@ -82,7 +93,7 @@ EXPORT_SYMBOL(nvm_dev_dma_alloc);
 void nvm_dev_dma_free(struct nvm_dev *dev, void *ppa_list,
 							dma_addr_t dma_handler)
 {
-	dev->ops->dev_dma_free(dev->ppalist_pool, ppa_list, dma_handler);
+	dev->ops->dev_dma_free(ppalist_pool(dev), ppa_list, dma_handler);
 }
 EXPORT_SYMBOL(nvm_dev_dma_free);
 
@@ -206,17 +217,6 @@ static int nvm_core_init(struct nvm_dev *dev)
 	return 0;
 }
 
-static void nvm_free(struct nvm_dev *dev)
-{
-	if (!dev)
-		return;
-
-	if (dev->mt)
-		dev->mt->unregister_mgr(dev);
-
-	nvm_core_free(dev);
-}
-
 static int nvm_init(struct nvm_dev *dev)
 {
 	struct nvmm_type *mt;
@@ -262,6 +262,7 @@ static int nvm_init(struct nvm_dev *dev)
 		}
 	}
 
+	kref_init(&dev->kref);
 	if (!ret) {
 		pr_info("nvm: no compatible manager found.\n");
 		return 0;
@@ -278,11 +279,47 @@ err:
 	return ret;
 }
 
-static void nvm_exit(struct nvm_dev *dev)
+static void nvm_remove_target(struct nvm_target *t)
 {
-	if (dev->ppalist_pool)
-		dev->ops->destroy_dma_pool(dev->ppalist_pool);
-	nvm_free(dev);
+	struct nvm_tgt_type *tt = t->type;
+	struct gendisk *tdisk = t->disk;
+	struct request_queue *q = tdisk->queue;
+
+	lockdep_assert_held(&nvm_lock);
+
+	del_gendisk(tdisk);
+	if (tt->exit)
+		tt->exit(tdisk->private_data);
+
+	blk_cleanup_queue(q);
+
+	put_disk(tdisk);
+
+	list_del(&t->list);
+	kfree(t);
+}
+
+static inline void nvm_remove_targets(struct nvm_dev *dev)
+{
+	struct nvm_target *t, *n;
+
+	list_for_each_entry_safe(t, n, &dev->online_targets, list)
+		nvm_remove_target(t);
+}
+
+static void nvm_exit(struct kref *kref)
+{
+	struct nvm_dev *dev;
+
+	dev = container_of(kref, struct nvm_dev, kref);
+	if (ppalist_pool(dev))
+		dev->ops->destroy_dma_pool(ppalist_pool(dev));
+
+	if (dev->mt)
+		dev->mt->unregister_mgr(dev);
+
+	if (dev->ops->dev_remove)
+		dev->ops->dev_remove(dev->q);
 
 	pr_info("nvm: successfully unloaded\n");
 }
@@ -344,8 +381,10 @@ void nvm_unregister(char *disk_name)
 	}
 
 	list_del(&dev->devices);
+	nvm_remove_targets(dev);
+	dev->ppalist_pool += NVM_EXITING;
 	up_write(&nvm_lock);
-	nvm_exit(dev);
+	kref_put(&dev->kref, nvm_exit);
 }
 EXPORT_SYMBOL(nvm_unregister);
 
@@ -366,6 +405,10 @@ static int nvm_create_target(struct nvm_dev *dev,
 	int ret = 0;
 
 	down_write(&nvm_lock);
+	if (NvmExiting(dev)) {
+		up_write(&nvm_lock);
+		return -ENODEV;
+	}
 	if (!dev->mt) {
 		/* register with device with a supported NVM manager */
 		list_for_each_entry(mt, &nvm_mgrs, list) {
@@ -439,10 +482,16 @@ static int nvm_create_target(struct nvm_dev *dev,
 	t->disk = tdisk;
 
 	down_write(&nvm_lock);
+	if (NvmExiting(dev)) {
+		up_write(&nvm_lock);
+		goto err_nvm_exiting;
+	}
 	list_add_tail(&t->list, &dev->online_targets);
 	up_write(&nvm_lock);
 
 	return 0;
+err_nvm_exiting:
+	del_gendisk(tdisk);
 err_init:
 	put_disk(tdisk);
 err_queue:
@@ -452,62 +501,50 @@ err_t:
 	return -ENOMEM;
 }
 
-static void nvm_remove_target(struct nvm_target *t)
-{
-	struct nvm_tgt_type *tt = t->type;
-	struct gendisk *tdisk = t->disk;
-	struct request_queue *q = tdisk->queue;
-
-	lockdep_assert_held(&nvm_lock);
-
-	del_gendisk(tdisk);
-	if (tt->exit)
-		tt->exit(tdisk->private_data);
-
-	blk_cleanup_queue(q);
-
-	put_disk(tdisk);
-
-	list_del(&t->list);
-	kfree(t);
-}
-
 static int __nvm_configure_create(struct nvm_ioctl_create *create)
 {
 	struct nvm_dev *dev;
 	struct nvm_ioctl_create_simple *s;
+	int ret = -EINVAL;
+
 	down_write(&nvm_lock);
 	dev = nvm_find_nvm_dev(create->dev);
-	up_write(&nvm_lock);
 	if (!dev) {
 		pr_err("nvm: device not found\n");
-		return -EINVAL;
+		up_write(&nvm_lock);
+		goto out;
 	}
+	kref_get(&dev->kref);
+	up_write(&nvm_lock);
 
 	if (create->conf.type != NVM_CONFIG_TYPE_SIMPLE) {
 		pr_err("nvm: config type not valid\n");
-		return -EINVAL;
+		goto out;
 	}
 	s = &create->conf.s;
 
 	if (s->lun_begin > s->lun_end || s->lun_end > dev->nr_luns) {
 		pr_err("nvm: lun out of bound (%u:%u > %u)\n",
 			s->lun_begin, s->lun_end, dev->nr_luns);
-		return -EINVAL;
+		goto out;
 	}
 
-	return nvm_create_target(dev, create);
+	ret = nvm_create_target(dev, create);
+out:
+	if (dev)
+		kref_put(&dev->kref, nvm_exit);
+	return ret;
 }
 
 static int __nvm_configure_remove(struct nvm_ioctl_remove *remove)
 {
-	struct nvm_target *t = NULL;
+	struct nvm_target *n, *t = NULL;
 	struct nvm_dev *dev;
 	int ret = -1;
 
 	down_write(&nvm_lock);
 	list_for_each_entry(dev, &nvm_devices, devices)
-		list_for_each_entry(t, &dev->online_targets, list) {
+		list_for_each_entry_safe(t, n, &dev->online_targets, list) {
 			if (!strcmp(remove->tgtname, t->disk->disk_name)) {
 				nvm_remove_target(t);
 				ret = 0;
diff --git a/include/linux/lightnvm.h b/include/linux/lightnvm.h
index 69c9057..1b42305 100644
--- a/include/linux/lightnvm.h
+++ b/include/linux/lightnvm.h
@@ -205,6 +205,7 @@ typedef void (nvm_destroy_dma_pool_fn)(void *);
 typedef void *(nvm_dev_dma_alloc_fn)(struct request_queue *, void *, gfp_t,
 								dma_addr_t *);
 typedef void (nvm_dev_dma_free_fn)(void *, void*, dma_addr_t);
+typedef void (nvm_dev_remove_fn) (struct request_queue *);
 
 struct nvm_dev_ops {
 	nvm_id_fn		*identity;
@@ -219,6 +220,7 @@ struct nvm_dev_ops {
 	nvm_destroy_dma_pool_fn	*destroy_dma_pool;
 	nvm_dev_dma_alloc_fn	*dev_dma_alloc;
 	nvm_dev_dma_free_fn	*dev_dma_free;
+	nvm_dev_remove_fn	*dev_remove;
 
 	uint8_t			max_phys_sect;
 };
@@ -252,6 +254,7 @@ struct nvm_dev {
 
 	/* Media manager */
 	struct nvmm_type *mt;
+	struct kref kref;
 	void *mp;
 
 	/* Device information */
-- 
1.8.3.1

Re: [PATCH 2/3] lightnvm: handle targets when corresponding nvm device exit

[Matias Bjørling]

On 11/25/2015 01:42 PM, Wenwei Tao wrote:
> block creations of new targets, remove exiting targets when
> underlying device was gone.
>
> Signed-off-by: Wenwei Tao <ww.tao0320@gmail.com>
> ---
>   drivers/lightnvm/core.c  | 127 ++++++++++++++++++++++++++++++-----------------
>   include/linux/lightnvm.h |   3 ++
>   2 files changed, 85 insertions(+), 45 deletions(-)
>
> diff --git a/drivers/lightnvm/core.c b/drivers/lightnvm/core.c
> index 39aec3a..0b71dd2 100644
> --- a/drivers/lightnvm/core.c
> +++ b/drivers/lightnvm/core.c
> @@ -33,6 +33,17 @@ static LIST_HEAD(nvm_targets);
>   static LIST_HEAD(nvm_mgrs);
>   static LIST_HEAD(nvm_devices);
>   static DECLARE_RWSEM(nvm_lock);
> +#define NVM_EXITING	1
> +
> +static inline int NvmExiting(struct nvm_dev *dev)
> +{
> +	return ((unsigned long)dev->ppalist_pool & NVM_EXITING) != 0;
> +}
> +
> +static inline void *ppalist_pool(struct nvm_dev *dev)
> +{
> +	return (void *)((unsigned long)dev->ppalist_pool & ~NVM_EXITING);
> +}
>

I think I rather want to have a state variable (so we have a state 
machine for three state. Initializing, running, and stopping.

What was the reason you'll like to put it in the ppalist_pool?

ps. could you rebase it on top of the latest master. Then I'll review 
the rest of it.

>   static struct nvm_tgt_type *nvm_find_target_type(const char *name)
>   {
> @@ -74,7 +85,7 @@ EXPORT_SYMBOL(nvm_unregister_target);
>   void *nvm_dev_dma_alloc(struct nvm_dev *dev, gfp_t mem_flags,
>   							dma_addr_t *dma_handler)
>   {
> -	return dev->ops->dev_dma_alloc(dev->q, dev->ppalist_pool, mem_flags,
> +	return dev->ops->dev_dma_alloc(dev->q, ppalist_pool(dev), mem_flags,
>   								dma_handler);
>   }
>   EXPORT_SYMBOL(nvm_dev_dma_alloc);
> @@ -82,7 +93,7 @@ EXPORT_SYMBOL(nvm_dev_dma_alloc);
>   void nvm_dev_dma_free(struct nvm_dev *dev, void *ppa_list,
>   							dma_addr_t dma_handler)
>   {
> -	dev->ops->dev_dma_free(dev->ppalist_pool, ppa_list, dma_handler);
> +	dev->ops->dev_dma_free(ppalist_pool(dev), ppa_list, dma_handler);
>   }
>   EXPORT_SYMBOL(nvm_dev_dma_free);
>
> @@ -206,17 +217,6 @@ static int nvm_core_init(struct nvm_dev *dev)
>   	return 0;
>   }
>
> -static void nvm_free(struct nvm_dev *dev)
> -{
> -	if (!dev)
> -		return;
> -
> -	if (dev->mt)
> -		dev->mt->unregister_mgr(dev);
> -
> -	nvm_core_free(dev);
> -}
> -
>   static int nvm_init(struct nvm_dev *dev)
>   {
>   	struct nvmm_type *mt;
> @@ -262,6 +262,7 @@ static int nvm_init(struct nvm_dev *dev)
>   		}
>   	}
>
> +	kref_init(&dev->kref);
>   	if (!ret) {
>   		pr_info("nvm: no compatible manager found.\n");
>   		return 0;
> @@ -278,11 +279,47 @@ err:
>   	return ret;
>   }
>
> -static void nvm_exit(struct nvm_dev *dev)
> +static void nvm_remove_target(struct nvm_target *t)
>   {
> -	if (dev->ppalist_pool)
> -		dev->ops->destroy_dma_pool(dev->ppalist_pool);
> -	nvm_free(dev);
> +	struct nvm_tgt_type *tt = t->type;
> +	struct gendisk *tdisk = t->disk;
> +	struct request_queue *q = tdisk->queue;
> +
> +	lockdep_assert_held(&nvm_lock);
> +
> +	del_gendisk(tdisk);
> +	if (tt->exit)
> +		tt->exit(tdisk->private_data);
> +
> +	blk_cleanup_queue(q);
> +
> +	put_disk(tdisk);
> +
> +	list_del(&t->list);
> +	kfree(t);
> +}
> +
> +static inline void nvm_remove_targets(struct nvm_dev *dev)
> +{
> +	struct nvm_target *t, *n;
> +
> +	list_for_each_entry_safe(t, n, &dev->online_targets, list)
> +		nvm_remove_target(t);
> +}
> +
> +static void nvm_exit(struct kref *kref)
> +{
> +	struct nvm_dev *dev;
> +
> +	dev = container_of(kref, struct nvm_dev, kref);
> +	if (ppalist_pool(dev))
> +		dev->ops->destroy_dma_pool(ppalist_pool(dev));
> +
> +	if (dev->mt)
> +		dev->mt->unregister_mgr(dev);
> +
> +	if (dev->ops->dev_remove)
> +		dev->ops->dev_remove(dev->q);
>
>   	pr_info("nvm: successfully unloaded\n");
>   }
> @@ -344,8 +381,10 @@ void nvm_unregister(char *disk_name)
>   	}
>
>   	list_del(&dev->devices);
> +	nvm_remove_targets(dev);
> +	dev->ppalist_pool += NVM_EXITING;
>   	up_write(&nvm_lock);
> -	nvm_exit(dev);
> +	kref_put(&dev->kref, nvm_exit);
>   }
>   EXPORT_SYMBOL(nvm_unregister);
>
> @@ -366,6 +405,10 @@ static int nvm_create_target(struct nvm_dev *dev,
>   	int ret = 0;
>
>   	down_write(&nvm_lock);
> +	if (NvmExiting(dev)) {
> +		up_write(&nvm_lock);
> +		return -ENODEV;
> +	}
>   	if (!dev->mt) {
>   		/* register with device with a supported NVM manager */
>   		list_for_each_entry(mt, &nvm_mgrs, list) {
> @@ -439,10 +482,16 @@ static int nvm_create_target(struct nvm_dev *dev,
>   	t->disk = tdisk;
>
>   	down_write(&nvm_lock);
> +	if (NvmExiting(dev)) {
> +		up_write(&nvm_lock);
> +		goto err_nvm_exiting;
> +	}
>   	list_add_tail(&t->list, &dev->online_targets);
>   	up_write(&nvm_lock);
>
>   	return 0;
> +err_nvm_exiting:
> +	del_gendisk(tdisk);
>   err_init:
>   	put_disk(tdisk);
>   err_queue:
> @@ -452,62 +501,50 @@ err_t:
>   	return -ENOMEM;
>   }
>
> -static void nvm_remove_target(struct nvm_target *t)
> -{
> -	struct nvm_tgt_type *tt = t->type;
> -	struct gendisk *tdisk = t->disk;
> -	struct request_queue *q = tdisk->queue;
> -
> -	lockdep_assert_held(&nvm_lock);
> -
> -	del_gendisk(tdisk);
> -	if (tt->exit)
> -		tt->exit(tdisk->private_data);
> -
> -	blk_cleanup_queue(q);
> -
> -	put_disk(tdisk);
> -
> -	list_del(&t->list);
> -	kfree(t);
> -}
> -
>   static int __nvm_configure_create(struct nvm_ioctl_create *create)
>   {
>   	struct nvm_dev *dev;
>   	struct nvm_ioctl_create_simple *s;
> +	int ret = -EINVAL;
> +
>   	down_write(&nvm_lock);
>   	dev = nvm_find_nvm_dev(create->dev);
> -	up_write(&nvm_lock);
>   	if (!dev) {
>   		pr_err("nvm: device not found\n");
> -		return -EINVAL;
> +		up_write(&nvm_lock);
> +		goto out;
>   	}
> +	kref_get(&dev->kref);
> +	up_write(&nvm_lock);
>
>   	if (create->conf.type != NVM_CONFIG_TYPE_SIMPLE) {
>   		pr_err("nvm: config type not valid\n");
> -		return -EINVAL;
> +		goto out;
>   	}
>   	s = &create->conf.s;
>
>   	if (s->lun_begin > s->lun_end || s->lun_end > dev->nr_luns) {
>   		pr_err("nvm: lun out of bound (%u:%u > %u)\n",
>   			s->lun_begin, s->lun_end, dev->nr_luns);
> -		return -EINVAL;
> +		goto out;
>   	}
>
> -	return nvm_create_target(dev, create);
> +	ret = nvm_create_target(dev, create);
> +out:
> +	if (dev)
> +		kref_put(&dev->kref, nvm_exit);
> +	return ret;
>   }
>
>   static int __nvm_configure_remove(struct nvm_ioctl_remove *remove)
>   {
> -	struct nvm_target *t = NULL;
> +	struct nvm_target *n, *t = NULL;
>   	struct nvm_dev *dev;
>   	int ret = -1;
>
>   	down_write(&nvm_lock);
>   	list_for_each_entry(dev, &nvm_devices, devices)
> -		list_for_each_entry(t, &dev->online_targets, list) {
> +		list_for_each_entry_safe(t, n, &dev->online_targets, list) {
>   			if (!strcmp(remove->tgtname, t->disk->disk_name)) {
>   				nvm_remove_target(t);
>   				ret = 0;
> diff --git a/include/linux/lightnvm.h b/include/linux/lightnvm.h
> index 69c9057..1b42305 100644
> --- a/include/linux/lightnvm.h
> +++ b/include/linux/lightnvm.h
> @@ -205,6 +205,7 @@ typedef void (nvm_destroy_dma_pool_fn)(void *);
>   typedef void *(nvm_dev_dma_alloc_fn)(struct request_queue *, void *, gfp_t,
>   								dma_addr_t *);
>   typedef void (nvm_dev_dma_free_fn)(void *, void*, dma_addr_t);
> +typedef void (nvm_dev_remove_fn) (struct request_queue *);
>
>   struct nvm_dev_ops {
>   	nvm_id_fn		*identity;
> @@ -219,6 +220,7 @@ struct nvm_dev_ops {
>   	nvm_destroy_dma_pool_fn	*destroy_dma_pool;
>   	nvm_dev_dma_alloc_fn	*dev_dma_alloc;
>   	nvm_dev_dma_free_fn	*dev_dma_free;
> +	nvm_dev_remove_fn	*dev_remove;
>
>   	uint8_t			max_phys_sect;
>   };
> @@ -252,6 +254,7 @@ struct nvm_dev {
>
>   	/* Media manager */
>   	struct nvmm_type *mt;
> +	struct kref kref;
>   	void *mp;
>
>   	/* Device information */
>

Re: [PATCH 2/3] lightnvm: handle targets when corresponding nvm device exit

[Wenwei Tao]

The reason why I put it in the ppalist_pool is that when I wrote the
code I thought state we needed was only exiting/stopping, I didn't
want to add a variable for just a bit.

I will send you a patch base on the lasted master  4.4rc2.

2015-11-25 23:39 GMT+08:00 Matias Bjørling <m@bjorling.me>:
> On 11/25/2015 01:42 PM, Wenwei Tao wrote:
>>
>> block creations of new targets, remove exiting targets when
>> underlying device was gone.
>>
>> Signed-off-by: Wenwei Tao <ww.tao0320@gmail.com>
>> ---
>>   drivers/lightnvm/core.c  | 127
>> ++++++++++++++++++++++++++++++-----------------
>>   include/linux/lightnvm.h |   3 ++
>>   2 files changed, 85 insertions(+), 45 deletions(-)
>>
>> diff --git a/drivers/lightnvm/core.c b/drivers/lightnvm/core.c
>> index 39aec3a..0b71dd2 100644
>> --- a/drivers/lightnvm/core.c
>> +++ b/drivers/lightnvm/core.c
>> @@ -33,6 +33,17 @@ static LIST_HEAD(nvm_targets);
>>   static LIST_HEAD(nvm_mgrs);
>>   static LIST_HEAD(nvm_devices);
>>   static DECLARE_RWSEM(nvm_lock);
>> +#define NVM_EXITING    1
>> +
>> +static inline int NvmExiting(struct nvm_dev *dev)
>> +{
>> +       return ((unsigned long)dev->ppalist_pool & NVM_EXITING) != 0;
>> +}
>> +
>> +static inline void *ppalist_pool(struct nvm_dev *dev)
>> +{
>> +       return (void *)((unsigned long)dev->ppalist_pool & ~NVM_EXITING);
>> +}
>>
>
> I think I rather want to have a state variable (so we have a state machine
> for three state. Initializing, running, and stopping.
>
> What was the reason you'll like to put it in the ppalist_pool?
>
> ps. could you rebase it on top of the latest master. Then I'll review the
> rest of it.
>
>
>>   static struct nvm_tgt_type *nvm_find_target_type(const char *name)
>>   {
>> @@ -74,7 +85,7 @@ EXPORT_SYMBOL(nvm_unregister_target);
>>   void *nvm_dev_dma_alloc(struct nvm_dev *dev, gfp_t mem_flags,
>>                                                         dma_addr_t
>> *dma_handler)
>>   {
>> -       return dev->ops->dev_dma_alloc(dev->q, dev->ppalist_pool,
>> mem_flags,
>> +       return dev->ops->dev_dma_alloc(dev->q, ppalist_pool(dev),
>> mem_flags,
>>
>> dma_handler);
>>   }
>>   EXPORT_SYMBOL(nvm_dev_dma_alloc);
>> @@ -82,7 +93,7 @@ EXPORT_SYMBOL(nvm_dev_dma_alloc);
>>   void nvm_dev_dma_free(struct nvm_dev *dev, void *ppa_list,
>>                                                         dma_addr_t
>> dma_handler)
>>   {
>> -       dev->ops->dev_dma_free(dev->ppalist_pool, ppa_list, dma_handler);
>> +       dev->ops->dev_dma_free(ppalist_pool(dev), ppa_list, dma_handler);
>>   }
>>   EXPORT_SYMBOL(nvm_dev_dma_free);
>>
>> @@ -206,17 +217,6 @@ static int nvm_core_init(struct nvm_dev *dev)
>>         return 0;
>>   }
>>
>> -static void nvm_free(struct nvm_dev *dev)
>> -{
>> -       if (!dev)
>> -               return;
>> -
>> -       if (dev->mt)
>> -               dev->mt->unregister_mgr(dev);
>> -
>> -       nvm_core_free(dev);
>> -}
>> -
>>   static int nvm_init(struct nvm_dev *dev)
>>   {
>>         struct nvmm_type *mt;
>> @@ -262,6 +262,7 @@ static int nvm_init(struct nvm_dev *dev)
>>                 }
>>         }
>>
>> +       kref_init(&dev->kref);
>>         if (!ret) {
>>                 pr_info("nvm: no compatible manager found.\n");
>>                 return 0;
>> @@ -278,11 +279,47 @@ err:
>>         return ret;
>>   }
>>
>> -static void nvm_exit(struct nvm_dev *dev)
>> +static void nvm_remove_target(struct nvm_target *t)
>>   {
>> -       if (dev->ppalist_pool)
>> -               dev->ops->destroy_dma_pool(dev->ppalist_pool);
>> -       nvm_free(dev);
>> +       struct nvm_tgt_type *tt = t->type;
>> +       struct gendisk *tdisk = t->disk;
>> +       struct request_queue *q = tdisk->queue;
>> +
>> +       lockdep_assert_held(&nvm_lock);
>> +
>> +       del_gendisk(tdisk);
>> +       if (tt->exit)
>> +               tt->exit(tdisk->private_data);
>> +
>> +       blk_cleanup_queue(q);
>> +
>> +       put_disk(tdisk);
>> +
>> +       list_del(&t->list);
>> +       kfree(t);
>> +}
>> +
>> +static inline void nvm_remove_targets(struct nvm_dev *dev)
>> +{
>> +       struct nvm_target *t, *n;
>> +
>> +       list_for_each_entry_safe(t, n, &dev->online_targets, list)
>> +               nvm_remove_target(t);
>> +}
>> +
>> +static void nvm_exit(struct kref *kref)
>> +{
>> +       struct nvm_dev *dev;
>> +
>> +       dev = container_of(kref, struct nvm_dev, kref);
>> +       if (ppalist_pool(dev))
>> +               dev->ops->destroy_dma_pool(ppalist_pool(dev));
>> +
>> +       if (dev->mt)
>> +               dev->mt->unregister_mgr(dev);
>> +
>> +       if (dev->ops->dev_remove)
>> +               dev->ops->dev_remove(dev->q);
>>
>>         pr_info("nvm: successfully unloaded\n");
>>   }
>> @@ -344,8 +381,10 @@ void nvm_unregister(char *disk_name)
>>         }
>>
>>         list_del(&dev->devices);
>> +       nvm_remove_targets(dev);
>> +       dev->ppalist_pool += NVM_EXITING;
>>         up_write(&nvm_lock);
>> -       nvm_exit(dev);
>> +       kref_put(&dev->kref, nvm_exit);
>>   }
>>   EXPORT_SYMBOL(nvm_unregister);
>>
>> @@ -366,6 +405,10 @@ static int nvm_create_target(struct nvm_dev *dev,
>>         int ret = 0;
>>
>>         down_write(&nvm_lock);
>> +       if (NvmExiting(dev)) {
>> +               up_write(&nvm_lock);
>> +               return -ENODEV;
>> +       }
>>         if (!dev->mt) {
>>                 /* register with device with a supported NVM manager */
>>                 list_for_each_entry(mt, &nvm_mgrs, list) {
>> @@ -439,10 +482,16 @@ static int nvm_create_target(struct nvm_dev *dev,
>>         t->disk = tdisk;
>>
>>         down_write(&nvm_lock);
>> +       if (NvmExiting(dev)) {
>> +               up_write(&nvm_lock);
>> +               goto err_nvm_exiting;
>> +       }
>>         list_add_tail(&t->list, &dev->online_targets);
>>         up_write(&nvm_lock);
>>
>>         return 0;
>> +err_nvm_exiting:
>> +       del_gendisk(tdisk);
>>   err_init:
>>         put_disk(tdisk);
>>   err_queue:
>> @@ -452,62 +501,50 @@ err_t:
>>         return -ENOMEM;
>>   }
>>
>> -static void nvm_remove_target(struct nvm_target *t)
>> -{
>> -       struct nvm_tgt_type *tt = t->type;
>> -       struct gendisk *tdisk = t->disk;
>> -       struct request_queue *q = tdisk->queue;
>> -
>> -       lockdep_assert_held(&nvm_lock);
>> -
>> -       del_gendisk(tdisk);
>> -       if (tt->exit)
>> -               tt->exit(tdisk->private_data);
>> -
>> -       blk_cleanup_queue(q);
>> -
>> -       put_disk(tdisk);
>> -
>> -       list_del(&t->list);
>> -       kfree(t);
>> -}
>> -
>>   static int __nvm_configure_create(struct nvm_ioctl_create *create)
>>   {
>>         struct nvm_dev *dev;
>>         struct nvm_ioctl_create_simple *s;
>> +       int ret = -EINVAL;
>> +
>>         down_write(&nvm_lock);
>>         dev = nvm_find_nvm_dev(create->dev);
>> -       up_write(&nvm_lock);
>>         if (!dev) {
>>                 pr_err("nvm: device not found\n");
>> -               return -EINVAL;
>> +               up_write(&nvm_lock);
>> +               goto out;
>>         }
>> +       kref_get(&dev->kref);
>> +       up_write(&nvm_lock);
>>
>>         if (create->conf.type != NVM_CONFIG_TYPE_SIMPLE) {
>>                 pr_err("nvm: config type not valid\n");
>> -               return -EINVAL;
>> +               goto out;
>>         }
>>         s = &create->conf.s;
>>
>>         if (s->lun_begin > s->lun_end || s->lun_end > dev->nr_luns) {
>>                 pr_err("nvm: lun out of bound (%u:%u > %u)\n",
>>                         s->lun_begin, s->lun_end, dev->nr_luns);
>> -               return -EINVAL;
>> +               goto out;
>>         }
>>
>> -       return nvm_create_target(dev, create);
>> +       ret = nvm_create_target(dev, create);
>> +out:
>> +       if (dev)
>> +               kref_put(&dev->kref, nvm_exit);
>> +       return ret;
>>   }
>>
>>   static int __nvm_configure_remove(struct nvm_ioctl_remove *remove)
>>   {
>> -       struct nvm_target *t = NULL;
>> +       struct nvm_target *n, *t = NULL;
>>         struct nvm_dev *dev;
>>         int ret = -1;
>>
>>         down_write(&nvm_lock);
>>         list_for_each_entry(dev, &nvm_devices, devices)
>> -               list_for_each_entry(t, &dev->online_targets, list) {
>> +               list_for_each_entry_safe(t, n, &dev->online_targets, list)
>> {
>>                         if (!strcmp(remove->tgtname, t->disk->disk_name))
>> {
>>                                 nvm_remove_target(t);
>>                                 ret = 0;
>> diff --git a/include/linux/lightnvm.h b/include/linux/lightnvm.h
>> index 69c9057..1b42305 100644
>> --- a/include/linux/lightnvm.h
>> +++ b/include/linux/lightnvm.h
>> @@ -205,6 +205,7 @@ typedef void (nvm_destroy_dma_pool_fn)(void *);
>>   typedef void *(nvm_dev_dma_alloc_fn)(struct request_queue *, void *,
>> gfp_t,
>>                                                                 dma_addr_t
>> *);
>>   typedef void (nvm_dev_dma_free_fn)(void *, void*, dma_addr_t);
>> +typedef void (nvm_dev_remove_fn) (struct request_queue *);
>>
>>   struct nvm_dev_ops {
>>         nvm_id_fn               *identity;
>> @@ -219,6 +220,7 @@ struct nvm_dev_ops {
>>         nvm_destroy_dma_pool_fn *destroy_dma_pool;
>>         nvm_dev_dma_alloc_fn    *dev_dma_alloc;
>>         nvm_dev_dma_free_fn     *dev_dma_free;
>> +       nvm_dev_remove_fn       *dev_remove;
>>
>>         uint8_t                 max_phys_sect;
>>   };
>> @@ -252,6 +254,7 @@ struct nvm_dev {
>>
>>         /* Media manager */
>>         struct nvmm_type *mt;
>> +       struct kref kref;
>>         void *mp;
>>
>>         /* Device information */
>>
>

[PATCH 3/3] nvme: change the interface between nvme and lightnvm

[Wenwei Tao]

When nvme devices were removed, we need to handle the targets
build upon them properly: remove the existing targets, block
creations of new ones. To do this clean up job well, we
need to change the interface between nvme and lightnvm.

Signed-off-by: Wenwei Tao <ww.tao0320@gmail.com>
---
 drivers/nvme/host/lightnvm.c | 17 ++++++++++++++++-
 drivers/nvme/host/nvme.h     |  1 +
 drivers/nvme/host/pci.c      | 10 ++++++----
 3 files changed, 23 insertions(+), 5 deletions(-)

diff --git a/drivers/nvme/host/lightnvm.c b/drivers/nvme/host/lightnvm.c
index e0b7b95..3f4ffb7 100644
--- a/drivers/nvme/host/lightnvm.c
+++ b/drivers/nvme/host/lightnvm.c
@@ -468,6 +468,14 @@ static void nvme_nvm_dev_dma_free(void *pool, void *ppa_list,
 	dma_pool_free(pool, ppa_list, dma_handler);
 }
 
+static void nvme_nvm_dev_remove(struct request_queue *q)
+{
+	struct nvme_ns *ns = q->queuedata;
+
+	kref_put(&ns->kref, nvme_free_ns);
+
+}
+
 static struct nvm_dev_ops nvme_nvm_dev_ops = {
 	.identity		= nvme_nvm_identity,
 
@@ -482,13 +490,20 @@ static struct nvm_dev_ops nvme_nvm_dev_ops = {
 	.destroy_dma_pool	= nvme_nvm_destroy_dma_pool,
 	.dev_dma_alloc		= nvme_nvm_dev_dma_alloc,
 	.dev_dma_free		= nvme_nvm_dev_dma_free,
+	.dev_remove		= nvme_nvm_dev_remove,
 
 	.max_phys_sect		= 64,
 };
 
 int nvme_nvm_register(struct request_queue *q, char *disk_name)
 {
-	return nvm_register(q, disk_name, &nvme_nvm_dev_ops);
+	int ret;
+	struct nvme_ns *ns = q->queuedata;
+
+	ret = nvm_register(q, disk_name, &nvme_nvm_dev_ops);
+	if (!ret)
+		kref_get(&ns->kref);
+	return ret;
 }
 
 void nvme_nvm_unregister(struct request_queue *q, char *disk_name)
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index fdb4e5b..251ec9d 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -116,6 +116,7 @@ static inline u64 nvme_block_nr(struct nvme_ns *ns, sector_t sector)
 	return (sector >> (ns->lba_shift - 9));
 }
 
+void nvme_free_ns(struct kref *kref);
 int nvme_submit_sync_cmd(struct request_queue *q, struct nvme_command *cmd,
 		void *buf, unsigned bufflen);
 int __nvme_submit_sync_cmd(struct request_queue *q, struct nvme_command *cmd,
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 8187df2..f63223d 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -1960,13 +1960,10 @@ static int nvme_compat_ioctl(struct block_device *bdev, fmode_t mode,
 #endif
 
 static void nvme_free_dev(struct kref *kref);
-static void nvme_free_ns(struct kref *kref)
+void nvme_free_ns(struct kref *kref)
 {
 	struct nvme_ns *ns = container_of(kref, struct nvme_ns, kref);
 
-	if (ns->type == NVME_NS_LIGHTNVM)
-		nvme_nvm_unregister(ns->queue, ns->disk->disk_name);
-
 	spin_lock(&dev_list_lock);
 	ns->disk->private_data = NULL;
 	spin_unlock(&dev_list_lock);
@@ -2533,6 +2530,11 @@ static void nvme_ns_remove(struct nvme_ns *ns)
 {
 	bool kill = nvme_io_incapable(ns->dev) && !blk_queue_dying(ns->queue);
 
+	if (ns->type == NVME_NS_LIGHTNVM) {
+		nvme_nvm_unregister(ns->queue, ns->disk->disk_name);
+		ns->type = 0;
+	}
+
 	if (kill)
 		blk_set_queue_dying(ns->queue);
 	if (ns->disk->flags & GENHD_FL_UP)
-- 
1.8.3.1

Re: [PATCH 3/3] nvme: change the interface between nvme and lightnvm

[Matias Bjørling]

On 11/25/2015 01:42 PM, Wenwei Tao wrote:
> When nvme devices were removed, we need to handle the targets
> build upon them properly: remove the existing targets, block
> creations of new ones. To do this clean up job well, we
> need to change the interface between nvme and lightnvm.
>
> Signed-off-by: Wenwei Tao <ww.tao0320@gmail.com>
> ---
>   drivers/nvme/host/lightnvm.c | 17 ++++++++++++++++-
>   drivers/nvme/host/nvme.h     |  1 +
>   drivers/nvme/host/pci.c      | 10 ++++++----
>   3 files changed, 23 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/nvme/host/lightnvm.c b/drivers/nvme/host/lightnvm.c
> index e0b7b95..3f4ffb7 100644
> --- a/drivers/nvme/host/lightnvm.c
> +++ b/drivers/nvme/host/lightnvm.c
> @@ -468,6 +468,14 @@ static void nvme_nvm_dev_dma_free(void *pool, void *ppa_list,
>   	dma_pool_free(pool, ppa_list, dma_handler);
>   }
>
> +static void nvme_nvm_dev_remove(struct request_queue *q)
> +{
> +	struct nvme_ns *ns = q->queuedata;
> +
> +	kref_put(&ns->kref, nvme_free_ns);
> +
> +}
> +
>   static struct nvm_dev_ops nvme_nvm_dev_ops = {
>   	.identity		= nvme_nvm_identity,
>
> @@ -482,13 +490,20 @@ static struct nvm_dev_ops nvme_nvm_dev_ops = {
>   	.destroy_dma_pool	= nvme_nvm_destroy_dma_pool,
>   	.dev_dma_alloc		= nvme_nvm_dev_dma_alloc,
>   	.dev_dma_free		= nvme_nvm_dev_dma_free,
> +	.dev_remove		= nvme_nvm_dev_remove,
>
>   	.max_phys_sect		= 64,
>   };
>
>   int nvme_nvm_register(struct request_queue *q, char *disk_name)
>   {
> -	return nvm_register(q, disk_name, &nvme_nvm_dev_ops);
> +	int ret;
> +	struct nvme_ns *ns = q->queuedata;
> +
> +	ret = nvm_register(q, disk_name, &nvme_nvm_dev_ops);
> +	if (!ret)
> +		kref_get(&ns->kref);
> +	return ret;
>   }
>
>   void nvme_nvm_unregister(struct request_queue *q, char *disk_name)
> diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
> index fdb4e5b..251ec9d 100644
> --- a/drivers/nvme/host/nvme.h
> +++ b/drivers/nvme/host/nvme.h
> @@ -116,6 +116,7 @@ static inline u64 nvme_block_nr(struct nvme_ns *ns, sector_t sector)
>   	return (sector >> (ns->lba_shift - 9));
>   }
>
> +void nvme_free_ns(struct kref *kref);
>   int nvme_submit_sync_cmd(struct request_queue *q, struct nvme_command *cmd,
>   		void *buf, unsigned bufflen);
>   int __nvme_submit_sync_cmd(struct request_queue *q, struct nvme_command *cmd,
> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> index 8187df2..f63223d 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -1960,13 +1960,10 @@ static int nvme_compat_ioctl(struct block_device *bdev, fmode_t mode,
>   #endif
>
>   static void nvme_free_dev(struct kref *kref);
> -static void nvme_free_ns(struct kref *kref)
> +void nvme_free_ns(struct kref *kref)
>   {
>   	struct nvme_ns *ns = container_of(kref, struct nvme_ns, kref);
>
> -	if (ns->type == NVME_NS_LIGHTNVM)
> -		nvme_nvm_unregister(ns->queue, ns->disk->disk_name);
> -
>   	spin_lock(&dev_list_lock);
>   	ns->disk->private_data = NULL;
>   	spin_unlock(&dev_list_lock);
> @@ -2533,6 +2530,11 @@ static void nvme_ns_remove(struct nvme_ns *ns)
>   {
>   	bool kill = nvme_io_incapable(ns->dev) && !blk_queue_dying(ns->queue);
>
> +	if (ns->type == NVME_NS_LIGHTNVM) {
> +		nvme_nvm_unregister(ns->queue, ns->disk->disk_name);
> +		ns->type = 0;

Do we need to set it to zero, since we won't use it after its been removed?

> +	}
> +
>   	if (kill)
>   		blk_set_queue_dying(ns->queue);
>   	if (ns->disk->flags & GENHD_FL_UP)
>

Re: [PATCH 3/3] nvme: change the interface between nvme and lightnvm

[Wenwei Tao]

You are right. Reset it to zero is not necessary.

2015-11-25 23:13 GMT+08:00 Matias Bjørling <mb@lightnvm.io>:
> On 11/25/2015 01:42 PM, Wenwei Tao wrote:
>>
>> When nvme devices were removed, we need to handle the targets
>> build upon them properly: remove the existing targets, block
>> creations of new ones. To do this clean up job well, we
>> need to change the interface between nvme and lightnvm.
>>
>> Signed-off-by: Wenwei Tao <ww.tao0320@gmail.com>
>> ---
>>   drivers/nvme/host/lightnvm.c | 17 ++++++++++++++++-
>>   drivers/nvme/host/nvme.h     |  1 +
>>   drivers/nvme/host/pci.c      | 10 ++++++----
>>   3 files changed, 23 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/nvme/host/lightnvm.c b/drivers/nvme/host/lightnvm.c
>> index e0b7b95..3f4ffb7 100644
>> --- a/drivers/nvme/host/lightnvm.c
>> +++ b/drivers/nvme/host/lightnvm.c
>> @@ -468,6 +468,14 @@ static void nvme_nvm_dev_dma_free(void *pool, void
>> *ppa_list,
>>         dma_pool_free(pool, ppa_list, dma_handler);
>>   }
>>
>> +static void nvme_nvm_dev_remove(struct request_queue *q)
>> +{
>> +       struct nvme_ns *ns = q->queuedata;
>> +
>> +       kref_put(&ns->kref, nvme_free_ns);
>> +
>> +}
>> +
>>   static struct nvm_dev_ops nvme_nvm_dev_ops = {
>>         .identity               = nvme_nvm_identity,
>>
>> @@ -482,13 +490,20 @@ static struct nvm_dev_ops nvme_nvm_dev_ops = {
>>         .destroy_dma_pool       = nvme_nvm_destroy_dma_pool,
>>         .dev_dma_alloc          = nvme_nvm_dev_dma_alloc,
>>         .dev_dma_free           = nvme_nvm_dev_dma_free,
>> +       .dev_remove             = nvme_nvm_dev_remove,
>>
>>         .max_phys_sect          = 64,
>>   };
>>
>>   int nvme_nvm_register(struct request_queue *q, char *disk_name)
>>   {
>> -       return nvm_register(q, disk_name, &nvme_nvm_dev_ops);
>> +       int ret;
>> +       struct nvme_ns *ns = q->queuedata;
>> +
>> +       ret = nvm_register(q, disk_name, &nvme_nvm_dev_ops);
>> +       if (!ret)
>> +               kref_get(&ns->kref);
>> +       return ret;
>>   }
>>
>>   void nvme_nvm_unregister(struct request_queue *q, char *disk_name)
>> diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
>> index fdb4e5b..251ec9d 100644
>> --- a/drivers/nvme/host/nvme.h
>> +++ b/drivers/nvme/host/nvme.h
>> @@ -116,6 +116,7 @@ static inline u64 nvme_block_nr(struct nvme_ns *ns,
>> sector_t sector)
>>         return (sector >> (ns->lba_shift - 9));
>>   }
>>
>> +void nvme_free_ns(struct kref *kref);
>>   int nvme_submit_sync_cmd(struct request_queue *q, struct nvme_command
>> *cmd,
>>                 void *buf, unsigned bufflen);
>>   int __nvme_submit_sync_cmd(struct request_queue *q, struct nvme_command
>> *cmd,
>> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
>> index 8187df2..f63223d 100644
>> --- a/drivers/nvme/host/pci.c
>> +++ b/drivers/nvme/host/pci.c
>> @@ -1960,13 +1960,10 @@ static int nvme_compat_ioctl(struct block_device
>> *bdev, fmode_t mode,
>>   #endif
>>
>>   static void nvme_free_dev(struct kref *kref);
>> -static void nvme_free_ns(struct kref *kref)
>> +void nvme_free_ns(struct kref *kref)
>>   {
>>         struct nvme_ns *ns = container_of(kref, struct nvme_ns, kref);
>>
>> -       if (ns->type == NVME_NS_LIGHTNVM)
>> -               nvme_nvm_unregister(ns->queue, ns->disk->disk_name);
>> -
>>         spin_lock(&dev_list_lock);
>>         ns->disk->private_data = NULL;
>>         spin_unlock(&dev_list_lock);
>> @@ -2533,6 +2530,11 @@ static void nvme_ns_remove(struct nvme_ns *ns)
>>   {
>>         bool kill = nvme_io_incapable(ns->dev) &&
>> !blk_queue_dying(ns->queue);
>>
>> +       if (ns->type == NVME_NS_LIGHTNVM) {
>> +               nvme_nvm_unregister(ns->queue, ns->disk->disk_name);
>> +               ns->type = 0;
>
>
> Do we need to set it to zero, since we won't use it after its been removed?
>
>
>> +       }
>> +
>>         if (kill)
>>                 blk_set_queue_dying(ns->queue);
>>         if (ns->disk->flags & GENHD_FL_UP)
>>
>