| Message ID | 56559D5C.6090004@lightnvm.io (mailing list archive) |
|---|---|
| State | Awaiting Upstream, archived |
| Delegated to: | Jens Axboe |
| Headers | show |
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 > > -- To unsubscribe from this list: send the line "unsubscribe linux-block" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Or we can add a new function
static struct nvm_dev *nvm_find_nvm_dev_locked(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;
}
use nvm_find_nvm_dev_locked() and nvm_find_nvm_dev() properly.
2015-11-25 21:11 GMT+08:00 Wenwei Tao <ww.tao0320@gmail.com>:
> 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
>>
>>
--
To unsubscribe from this list: send the line "unsubscribe linux-block" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/25/2015 03:18 PM, Wenwei Tao wrote: > Or we can add a new function > > static struct nvm_dev *nvm_find_nvm_dev_locked(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; > } > > use nvm_find_nvm_dev_locked() and nvm_find_nvm_dev() properly. > That could also work. Let's go with your original solution. I don't like it completely, but on the other hand. The other iterators build up to the locks in that way. So making a special case for the nvm_find_nvm_dev properly makes the code harder to understand. -- To unsubscribe from this list: send the line "unsubscribe linux-block" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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;