Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754555AbbKYOSs (ORCPT ); Wed, 25 Nov 2015 09:18:48 -0500 Received: from mail-ig0-f180.google.com ([209.85.213.180]:33344 "EHLO mail-ig0-f180.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752322AbbKYOSr convert rfc822-to-8bit (ORCPT ); Wed, 25 Nov 2015 09:18:47 -0500 MIME-Version: 1.0 In-Reply-To: References: <1448378697-2532-1-git-send-email-ww.tao0320@gmail.com> <5654AE2D.2080802@lightnvm.io> <56559D5C.6090004@lightnvm.io> Date: Wed, 25 Nov 2015 22:18:45 +0800 Message-ID: Subject: Re: [PATCH] lightnvm: missing nvm_lock acquire From: Wenwei Tao To: =?UTF-8?Q?Matias_Bj=C3=B8rling?= Cc: linux-kernel@vger.kernel.org, linux-block@vger.kernel.org Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3171 Lines: 95 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 : > 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 : >> 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 >> Signed-off-by: Matias Bjørling >> --- >> 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-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/