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
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 <[email protected]>
---
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
block creations of new targets, remove exiting targets when
underlying device was gone.
Signed-off-by: Wenwei Tao <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
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 <[email protected]>:
> 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 <[email protected]>
>> ---
>> 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
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.
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 <[email protected]>:
> 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.
>