Re: [PATCH] lightnvm: missing nvm_lock acquire

Wenwei Tao
2015-11-25T13:11:42Z

Hi Matias

Place the lock inside nvm_find_nvm_dev can make code cleaner, but
sometime may lack of flexibility, cause redundant lock and unlock,
just my opinion. Place the lock inside the function is okay with me,
just make sure do the proper up_write(&nvm_lock) in
nvm_create_target(), it has a lot 'return'.

I just submit a patch set covering the lock issue and the target issue
mentioned before, you can have a look before you apply this patch.
Thanks.

2015-11-25 19:37 GMT+08:00 Matias Bjørling <mb@lightnvm.io>:
> On 11/25/2015 09:57 AM, Wenwei Tao wrote:
>>
>> Hi Matias
>> I think list_for_each_entry_safe(pos, n, head, member) cannot avoid
>> race condition
>> the item point by ‘n’ can be deleted and freed in the same time we
>> operate on 'pos'
>> so lock is still necessary.
>>
> I've made the patch like this, ok?
>
> To avoid race conditions, traverse dev, media manager,
> and target 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>
> Signed-off-by: Matias Bjørling <m@bjorling.me>
> ---
> drivers/lightnvm/core.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/lightnvm/core.c b/drivers/lightnvm/core.c
> index ea6dba5..9f901f6 100644
> --- a/drivers/lightnvm/core.c
> +++ b/drivers/lightnvm/core.c
> @@ -127,9 +127,11 @@ static struct nvm_dev *nvm_find_nvm_dev(const char
> *name)
> {
> struct nvm_dev *dev;
>
> + down_write(&nvm_lock);
> list_for_each_entry(dev, &nvm_devices, devices)
> if (!strcmp(name, dev->name))
> return dev;
> + up_write(&nvm_lock);
>
> return NULL;
> }
> @@ -388,13 +390,14 @@ static int nvm_create_target(struct nvm_dev *dev,
> }
> }
>
> + down_write(&nvm_lock);
>
> 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");
> --
> 2.1.4
>
>