[PATCH v2 0/3] fixes for LightNVM

[PATCH v2 0/3] fixes for LightNVM

[Wenwei Tao]

Hi Matias
A couple of fixes based on 4.4rc2

Patch 1: the locking issue

move codes 'register with device with a supported manager'
into a funtion.
But I think iterate over a list to register nvm device
with a manger is not a good idea, since we can blocked
in manager's register_mgr function while holding nvm_lock.

Patch 2: handle targerts when underlying devices were removed

free dev when exited

call target type's exit function when target creation was aborted
due to nvm device exit.

Patch 3: change the interface between nvme and lightnvm

remove the unnecessary nvme_ns->type set to zero

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      | 197 ++++++++++++++++++++++++++-----------------
 drivers/nvme/host/lightnvm.c |  17 +++-
 drivers/nvme/host/nvme.h     |   1 +
 drivers/nvme/host/pci.c      |   7 +-
 include/linux/lightnvm.h     |   3 +
 5 files changed, 143 insertions(+), 82 deletions(-)

-- 
1.8.3.1

[PATCH v2 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 | 73 +++++++++++++++++++++++++------------------------
 1 file changed, 38 insertions(+), 35 deletions(-)

diff --git a/drivers/lightnvm/core.c b/drivers/lightnvm/core.c
index 5178645..071a3e4 100644
--- a/drivers/lightnvm/core.c
+++ b/drivers/lightnvm/core.c
@@ -123,6 +123,24 @@ void nvm_unregister_mgr(struct nvmm_type *mt)
 }
 EXPORT_SYMBOL(nvm_unregister_mgr);
 
+/* register with device with a supported manager */
+static int register_mgr(struct nvm_dev *dev)
+{
+	struct nvmm_type *mt;
+	int ret = 0;
+
+	list_for_each_entry(mt, &nvm_mgrs, list) {
+		ret = mt->register_mgr(dev);
+		if (ret > 0) {
+			dev->mt = mt;
+			break; /* successfully initialized */
+		}
+	}
+	if (!ret)
+		pr_info("nvm: no compatible nvm manager found.\n");
+	return ret;
+}
+
 static struct nvm_dev *nvm_find_nvm_dev(const char *name)
 {
 	struct nvm_dev *dev;
@@ -221,7 +239,6 @@ static void nvm_free(struct nvm_dev *dev)
 
 static int nvm_init(struct nvm_dev *dev)
 {
-	struct nvmm_type *mt;
 	int ret = -EINVAL;
 
 	if (!dev->q || !dev->ops)
@@ -251,22 +268,13 @@ static int nvm_init(struct nvm_dev *dev)
 		pr_err("nvm: could not initialize core structures.\n");
 		goto err;
 	}
-
-	/* register with device with a supported manager */
-	list_for_each_entry(mt, &nvm_mgrs, list) {
-		ret = mt->register_mgr(dev);
-		if (ret < 0)
-			goto err; /* initialization failed */
-		if (ret > 0) {
-			dev->mt = mt;
-			break; /* successfully initialized */
-		}
-	}
-
-	if (!ret) {
-		pr_info("nvm: no compatible manager found.\n");
+	down_write(&nvm_lock);
+	ret = register_mgr(dev);
+	up_write(&nvm_lock);
+	if (ret < 0)
+		goto err;
+	if (!ret)
 		return 0;
-	}
 
 	pr_info("nvm: registered %s [%u/%u/%u/%u/%u/%u]\n",
 			dev->name, dev->sec_per_pg, dev->nr_planes,
@@ -335,18 +343,18 @@ 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;
 	}
 
-	down_write(&nvm_lock);
 	list_del(&dev->devices);
 	up_write(&nvm_lock);
-
 	nvm_exit(dev);
 	kfree(dev);
 }
@@ -361,38 +369,30 @@ static int nvm_create_target(struct nvm_dev *dev,
 {
 	struct nvm_ioctl_create_simple *s = &create->conf.s;
 	struct request_queue *tqueue;
-	struct nvmm_type *mt;
 	struct gendisk *tdisk;
 	struct nvm_tgt_type *tt;
 	struct nvm_target *t;
 	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)
-				return ret; /* initialization failed */
-			if (ret > 0) {
-				dev->mt = mt;
-				break; /* successfully initialized */
-			}
-		}
-
-		if (!ret) {
-			pr_info("nvm: no compatible nvm manager found.\n");
-			return -ENODEV;
+		ret = register_mgr(dev);
+		if (!ret)
+			ret = -ENODEV;
+		if (ret < 0) {
+			up_write(&nvm_lock);
+			return ret;
 		}
 	}
 
 	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");
@@ -475,8 +475,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;
@@ -535,7 +536,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

[PATCH v2 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  | 128 +++++++++++++++++++++++++++++++----------------
 include/linux/lightnvm.h |   3 ++
 2 files changed, 87 insertions(+), 44 deletions(-)

diff --git a/drivers/lightnvm/core.c b/drivers/lightnvm/core.c
index 071a3e4..984db2f 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);
 
@@ -228,15 +239,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);
-}
-
 static int nvm_init(struct nvm_dev *dev)
 {
 	int ret = -EINVAL;
@@ -273,6 +275,7 @@ static int nvm_init(struct nvm_dev *dev)
 	up_write(&nvm_lock);
 	if (ret < 0)
 		goto err;
+	kref_init(&dev->kref);
 	if (!ret)
 		return 0;
 
@@ -286,12 +289,48 @@ 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);
+	kfree(dev);
 	pr_info("nvm: successfully unloaded\n");
 }
 
@@ -354,9 +393,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);
-	kfree(dev);
+	kref_put(&dev->kref, nvm_exit);
 }
 EXPORT_SYMBOL(nvm_unregister);
 
@@ -376,6 +416,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) {
 		ret = register_mgr(dev);
 		if (!ret)
@@ -438,10 +482,18 @@ 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);
+	if (tt->exit)
+		tt->exit(tdisk->private_data);
 err_init:
 	put_disk(tdisk);
 err_queue:
@@ -451,62 +503,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);
-	blk_cleanup_queue(q);
-
-	if (tt->exit)
-		tt->exit(tdisk->private_data);
-
-	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 *t, *n;
 	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 3db5552..6a365a2 100644
--- a/include/linux/lightnvm.h
+++ b/include/linux/lightnvm.h
@@ -189,6 +189,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;
@@ -203,6 +204,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;
 
 	unsigned int		max_phys_sect;
 };
@@ -238,6 +240,7 @@ struct nvm_dev {
 
 	/* Media manager */
 	struct nvmm_type *mt;
+	struct kref kref;
 	void *mp;
 
 	/* Device information */
-- 
1.8.3.1

[PATCH v2 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      |  7 +++----
 3 files changed, 20 insertions(+), 5 deletions(-)

diff --git a/drivers/nvme/host/lightnvm.c b/drivers/nvme/host/lightnvm.c
index 9202d1a..742875e 100644
--- a/drivers/nvme/host/lightnvm.c
+++ b/drivers/nvme/host/lightnvm.c
@@ -542,6 +542,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,
 
@@ -557,13 +565,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 f3b53af..5f0f934 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -1967,13 +1967,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);
@@ -2540,6 +2537,8 @@ 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);
 	if (kill)
 		blk_set_queue_dying(ns->queue);
 	if (ns->disk->flags & GENHD_FL_UP)
-- 
1.8.3.1

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

[Matias Bjørling]

On 11/27/2015 05:09 AM, 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 | 73 +++++++++++++++++++++++++------------------------
>   1 file changed, 38 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/lightnvm/core.c b/drivers/lightnvm/core.c
> index 5178645..071a3e4 100644
> --- a/drivers/lightnvm/core.c
> +++ b/drivers/lightnvm/core.c
> @@ -123,6 +123,24 @@ void nvm_unregister_mgr(struct nvmm_type *mt)
>   }
>   EXPORT_SYMBOL(nvm_unregister_mgr);
>
> +/* register with device with a supported manager */
> +static int register_mgr(struct nvm_dev *dev)
> +{
> +	struct nvmm_type *mt;
> +	int ret = 0;
> +
> +	list_for_each_entry(mt, &nvm_mgrs, list) {
> +		ret = mt->register_mgr(dev);
> +		if (ret > 0) {
> +			dev->mt = mt;
> +			break; /* successfully initialized */
> +		}
> +	}
> +	if (!ret)
> +		pr_info("nvm: no compatible nvm manager found.\n");
> +	return ret;
> +}
> +
>   static struct nvm_dev *nvm_find_nvm_dev(const char *name)
>   {
>   	struct nvm_dev *dev;
> @@ -221,7 +239,6 @@ static void nvm_free(struct nvm_dev *dev)
>
>   static int nvm_init(struct nvm_dev *dev)
>   {
> -	struct nvmm_type *mt;
>   	int ret = -EINVAL;
>
>   	if (!dev->q || !dev->ops)
> @@ -251,22 +268,13 @@ static int nvm_init(struct nvm_dev *dev)
>   		pr_err("nvm: could not initialize core structures.\n");
>   		goto err;
>   	}
> -
> -	/* register with device with a supported manager */
> -	list_for_each_entry(mt, &nvm_mgrs, list) {
> -		ret = mt->register_mgr(dev);
> -		if (ret < 0)
> -			goto err; /* initialization failed */
> -		if (ret > 0) {
> -			dev->mt = mt;
> -			break; /* successfully initialized */
> -		}
> -	}
> -
> -	if (!ret) {
> -		pr_info("nvm: no compatible manager found.\n");
> +	down_write(&nvm_lock);
> +	ret = register_mgr(dev);
> +	up_write(&nvm_lock);
> +	if (ret < 0)
> +		goto err;
> +	if (!ret)
>   		return 0;
> -	}
>
>   	pr_info("nvm: registered %s [%u/%u/%u/%u/%u/%u]\n",
>   			dev->name, dev->sec_per_pg, dev->nr_planes,
> @@ -335,18 +343,18 @@ 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;
>   	}
>
> -	down_write(&nvm_lock);
>   	list_del(&dev->devices);
>   	up_write(&nvm_lock);
> -
>   	nvm_exit(dev);
>   	kfree(dev);
>   }
> @@ -361,38 +369,30 @@ static int nvm_create_target(struct nvm_dev *dev,
>   {
>   	struct nvm_ioctl_create_simple *s = &create->conf.s;
>   	struct request_queue *tqueue;
> -	struct nvmm_type *mt;
>   	struct gendisk *tdisk;
>   	struct nvm_tgt_type *tt;
>   	struct nvm_target *t;
>   	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)
> -				return ret; /* initialization failed */
> -			if (ret > 0) {
> -				dev->mt = mt;
> -				break; /* successfully initialized */
> -			}
> -		}
> -
> -		if (!ret) {
> -			pr_info("nvm: no compatible nvm manager found.\n");
> -			return -ENODEV;
> +		ret = register_mgr(dev);
> +		if (!ret)
> +			ret = -ENODEV;
> +		if (ret < 0) {
> +			up_write(&nvm_lock);
> +			return ret;
>   		}
>   	}
>
>   	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");
> @@ -475,8 +475,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;
> @@ -535,7 +536,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 Wei. I applied the previous version of the patch.

You can see the patches I have queued for upstream in the for-next at 
https://github.com/OpenChannelSSD/linux/commits/for-next

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

[Wenwei Tao]

Hi Matias
Previous version patch miss acquire lock before find supported manager
in nvm_init().
And find supported manager code is duplicated in nvm_init() and
nvm_create_target(), we may move it into a function to make the code
more clean.
And find supported manager may blocked while holding the lock, this is
not good.

2015-11-27 16:31 GMT+08:00 Matias Bjørling <m@bjorling.me>:
> On 11/27/2015 05:09 AM, 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 | 73
>> +++++++++++++++++++++++++------------------------
>>   1 file changed, 38 insertions(+), 35 deletions(-)
>>
>> diff --git a/drivers/lightnvm/core.c b/drivers/lightnvm/core.c
>> index 5178645..071a3e4 100644
>> --- a/drivers/lightnvm/core.c
>> +++ b/drivers/lightnvm/core.c
>> @@ -123,6 +123,24 @@ void nvm_unregister_mgr(struct nvmm_type *mt)
>>   }
>>   EXPORT_SYMBOL(nvm_unregister_mgr);
>>
>> +/* register with device with a supported manager */
>> +static int register_mgr(struct nvm_dev *dev)
>> +{
>> +       struct nvmm_type *mt;
>> +       int ret = 0;
>> +
>> +       list_for_each_entry(mt, &nvm_mgrs, list) {
>> +               ret = mt->register_mgr(dev);
>> +               if (ret > 0) {
>> +                       dev->mt = mt;
>> +                       break; /* successfully initialized */
>> +               }
>> +       }
>> +       if (!ret)
>> +               pr_info("nvm: no compatible nvm manager found.\n");
>> +       return ret;
>> +}
>> +
>>   static struct nvm_dev *nvm_find_nvm_dev(const char *name)
>>   {
>>         struct nvm_dev *dev;
>> @@ -221,7 +239,6 @@ static void nvm_free(struct nvm_dev *dev)
>>
>>   static int nvm_init(struct nvm_dev *dev)
>>   {
>> -       struct nvmm_type *mt;
>>         int ret = -EINVAL;
>>
>>         if (!dev->q || !dev->ops)
>> @@ -251,22 +268,13 @@ static int nvm_init(struct nvm_dev *dev)
>>                 pr_err("nvm: could not initialize core structures.\n");
>>                 goto err;
>>         }
>> -
>> -       /* register with device with a supported manager */
>> -       list_for_each_entry(mt, &nvm_mgrs, list) {
>> -               ret = mt->register_mgr(dev);
>> -               if (ret < 0)
>> -                       goto err; /* initialization failed */
>> -               if (ret > 0) {
>> -                       dev->mt = mt;
>> -                       break; /* successfully initialized */
>> -               }
>> -       }
>> -
>> -       if (!ret) {
>> -               pr_info("nvm: no compatible manager found.\n");
>> +       down_write(&nvm_lock);
>> +       ret = register_mgr(dev);
>> +       up_write(&nvm_lock);
>> +       if (ret < 0)
>> +               goto err;
>> +       if (!ret)
>>                 return 0;
>> -       }
>>
>>         pr_info("nvm: registered %s [%u/%u/%u/%u/%u/%u]\n",
>>                         dev->name, dev->sec_per_pg, dev->nr_planes,
>> @@ -335,18 +343,18 @@ 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;
>>         }
>>
>> -       down_write(&nvm_lock);
>>         list_del(&dev->devices);
>>         up_write(&nvm_lock);
>> -
>>         nvm_exit(dev);
>>         kfree(dev);
>>   }
>> @@ -361,38 +369,30 @@ static int nvm_create_target(struct nvm_dev *dev,
>>   {
>>         struct nvm_ioctl_create_simple *s = &create->conf.s;
>>         struct request_queue *tqueue;
>> -       struct nvmm_type *mt;
>>         struct gendisk *tdisk;
>>         struct nvm_tgt_type *tt;
>>         struct nvm_target *t;
>>         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)
>> -                               return ret; /* initialization failed */
>> -                       if (ret > 0) {
>> -                               dev->mt = mt;
>> -                               break; /* successfully initialized */
>> -                       }
>> -               }
>> -
>> -               if (!ret) {
>> -                       pr_info("nvm: no compatible nvm manager
>> found.\n");
>> -                       return -ENODEV;
>> +               ret = register_mgr(dev);
>> +               if (!ret)
>> +                       ret = -ENODEV;
>> +               if (ret < 0) {
>> +                       up_write(&nvm_lock);
>> +                       return ret;
>>                 }
>>         }
>>
>>         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");
>> @@ -475,8 +475,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;
>> @@ -535,7 +536,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 Wei. I applied the previous version of the patch.
>
> You can see the patches I have queued for upstream in the for-next at
> https://github.com/OpenChannelSSD/linux/commits/for-next

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

[Matias Bjørling]

On 11/27/2015 01:04 PM, Wenwei Tao wrote:
> Hi Matias
> Previous version patch miss acquire lock before find supported manager
> in nvm_init().
> And find supported manager code is duplicated in nvm_init() and
> nvm_create_target(), we may move it into a function to make the code
> more clean.
> And find supported manager may blocked while holding the lock, this is
> not good.
>

Ok, thanks. I'll pick this one up instead then.

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

[Wenwei Tao]

Hi Matiasl
Just to let you know this patch and previous version both cannot
address 'register device with supported manager may blocked while
holding the lock' issue.

2015-11-28 3:41 GMT+08:00 Matias Bjørling <m@bjorling.me>:
> On 11/27/2015 01:04 PM, Wenwei Tao wrote:
>>
>> Hi Matias
>> Previous version patch miss acquire lock before find supported manager
>> in nvm_init().
>> And find supported manager code is duplicated in nvm_init() and
>> nvm_create_target(), we may move it into a function to make the code
>> more clean.
>> And find supported manager may blocked while holding the lock, this is
>> not good.
>>
>
> Ok, thanks. I'll pick this one up instead then.
>