From patchwork Mon Mar 4 06:31:38 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: NeilBrown X-Patchwork-Id: 10837377 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id 8C4921399 for ; Mon, 4 Mar 2019 06:35:53 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 7761C289BC for ; Mon, 4 Mar 2019 06:35:53 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 6B56429D79; Mon, 4 Mar 2019 06:35:53 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-2.9 required=2.0 tests=BAYES_00,MAILING_LIST_MULTI, RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.1 Received: from pdx1-mailman02.dreamhost.com (pdx1-mailman02.dreamhost.com [64.90.62.194]) (using TLSv1.2 with cipher DHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id D61D6289BC for ; Mon, 4 Mar 2019 06:35:52 +0000 (UTC) Received: from pdx1-mailman02.dreamhost.com (localhost [IPv6:::1]) by pdx1-mailman02.dreamhost.com (Postfix) with ESMTP id 90DE468299C; Sun, 3 Mar 2019 22:35:52 -0800 (PST) X-Original-To: lustre-devel@lists.lustre.org Delivered-To: lustre-devel-lustre.org@pdx1-mailman02.dreamhost.com Received: from mx1.suse.de (mx2.suse.de [195.135.220.15]) by pdx1-mailman02.dreamhost.com (Postfix) with ESMTP id D6187682921 for ; Sun, 3 Mar 2019 22:35:50 -0800 (PST) X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay2.suse.de (unknown [195.135.220.254]) by mx1.suse.de (Postfix) with ESMTP id 12402AC9C; Mon, 4 Mar 2019 06:35:50 +0000 (UTC) From: NeilBrown To: Andreas Dilger , James Simmons , Oleg Drokin Date: Mon, 04 Mar 2019 17:31:38 +1100 Message-ID: <155168109895.31333.12559034505115708060.stgit@noble.brown> In-Reply-To: <155168107971.31333.14345309795939467246.stgit@noble.brown> References: <155168107971.31333.14345309795939467246.stgit@noble.brown> User-Agent: StGit/0.17.1-dirty MIME-Version: 1.0 Subject: [lustre-devel] [PATCH 24/28] lustre: ldlm: discard l_lock from struct ldlm_lock. X-BeenThere: lustre-devel@lists.lustre.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: "For discussing Lustre software development." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Lustre Development List Errors-To: lustre-devel-bounces@lists.lustre.org Sender: "lustre-devel" X-Virus-Scanned: ClamAV using ClamSMTP This spinlock (l_lock) is only used to stablise the l_resource pointer while taking a spinlock on the resource. This is not necessary - it is sufficient to take the resource spinlock, and then check if l_resource has changed or not. If it hasn't then it cannot change until the resource spinlock is dropped. We must ensure this is safe even if the resource is freed before lock_res_and_lock() managed to get the lock. To do this we mark the slab as SLAB_TYPESAFE_BY_RCU and initialise the lock in an init_once() function, but not on every allocate (and specifically don't zero the whole srtuct on each allocation). This means that if we find a resource after taking the RCU read lock, then it is always safe to take and then drop the spinlock. After taking the spinlock, we can check if it is more generally safe to use. Discarding l_lock shrinks 'struct ldlm_lock' which helps save memory. Signed-off-by: NeilBrown --- drivers/staging/lustre/lustre/include/lustre_dlm.h | 5 ---- drivers/staging/lustre/lustre/ldlm/l_lock.c | 21 +++++++++------- drivers/staging/lustre/lustre/ldlm/ldlm_lock.c | 26 ++++++++++---------- drivers/staging/lustre/lustre/ldlm/ldlm_lockd.c | 20 +++++++++++++++ drivers/staging/lustre/lustre/ldlm/ldlm_resource.c | 9 ++++--- 5 files changed, 49 insertions(+), 32 deletions(-) diff --git a/drivers/staging/lustre/lustre/include/lustre_dlm.h b/drivers/staging/lustre/lustre/include/lustre_dlm.h index fd9b0f870c93..341766123e67 100644 --- a/drivers/staging/lustre/lustre/include/lustre_dlm.h +++ b/drivers/staging/lustre/lustre/include/lustre_dlm.h @@ -590,11 +590,6 @@ struct ldlm_lock { * Must be first in the structure. */ struct portals_handle l_handle; - /** - * Internal spinlock protects l_resource. We should hold this lock - * first before taking res_lock. - */ - spinlock_t l_lock; /** * Pointer to actual resource this lock is in. * ldlm_lock_change_resource() can change this. diff --git a/drivers/staging/lustre/lustre/ldlm/l_lock.c b/drivers/staging/lustre/lustre/ldlm/l_lock.c index 296259aa51e6..315eb5099bdb 100644 --- a/drivers/staging/lustre/lustre/ldlm/l_lock.c +++ b/drivers/staging/lustre/lustre/ldlm/l_lock.c @@ -45,15 +45,20 @@ * being an atomic operation. */ struct ldlm_resource *lock_res_and_lock(struct ldlm_lock *lock) - __acquires(&lock->l_lock) __acquires(&lock->l_resource->lr_lock) { - spin_lock(&lock->l_lock); - - lock_res(lock->l_resource); - - ldlm_set_res_locked(lock); - return lock->l_resource; + struct ldlm_resource *res; + rcu_read_lock(); + while (1) { + res = rcu_dereference(lock->l_resource); + lock_res(res); + if (res == lock->l_resource) { + ldlm_set_res_locked(lock); + rcu_read_unlock(); + return res; + } + unlock_res(res); + } } EXPORT_SYMBOL(lock_res_and_lock); @@ -62,12 +67,10 @@ EXPORT_SYMBOL(lock_res_and_lock); */ void unlock_res_and_lock(struct ldlm_lock *lock) __releases(&lock->l_resource->lr_lock) - __releases(&lock->l_lock) { /* on server-side resource of lock doesn't change */ ldlm_clear_res_locked(lock); unlock_res(lock->l_resource); - spin_unlock(&lock->l_lock); } EXPORT_SYMBOL(unlock_res_and_lock); diff --git a/drivers/staging/lustre/lustre/ldlm/ldlm_lock.c b/drivers/staging/lustre/lustre/ldlm/ldlm_lock.c index 5ac77238e5f2..e62dad14d822 100644 --- a/drivers/staging/lustre/lustre/ldlm/ldlm_lock.c +++ b/drivers/staging/lustre/lustre/ldlm/ldlm_lock.c @@ -383,7 +383,6 @@ static struct ldlm_lock *ldlm_lock_new(struct ldlm_resource *resource) if (!lock) return NULL; - spin_lock_init(&lock->l_lock); lock->l_resource = resource; lu_ref_add(&resource->lr_reference, "lock", lock); @@ -452,12 +451,13 @@ int ldlm_lock_change_resource(struct ldlm_namespace *ns, struct ldlm_lock *lock, lu_ref_add(&newres->lr_reference, "lock", lock); /* - * To flip the lock from the old to the new resource, lock, oldres and - * newres have to be locked. Resource spin-locks are nested within - * lock->l_lock, and are taken in the memory address order to avoid - * dead-locks. + * To flip the lock from the old to the new resource, oldres + * and newres have to be locked. Resource spin-locks are taken + * in the memory address order to avoid dead-locks. + * As this is the only circumstance where ->l_resource + * can change, and this cannot race with itself, it is safe + * to access lock->l_resource without being careful about locking. */ - spin_lock(&lock->l_lock); oldres = lock->l_resource; if (oldres < newres) { lock_res(oldres); @@ -468,9 +468,9 @@ int ldlm_lock_change_resource(struct ldlm_namespace *ns, struct ldlm_lock *lock, } LASSERT(memcmp(new_resid, &oldres->lr_name, sizeof(oldres->lr_name)) != 0); - lock->l_resource = newres; + rcu_assign_pointer(lock->l_resource, newres); unlock_res(oldres); - unlock_res_and_lock(lock); + unlock_res(newres); /* ...and the flowers are still standing! */ lu_ref_del(&oldres->lr_reference, "lock", lock); @@ -1964,11 +1964,11 @@ void _ldlm_lock_debug(struct ldlm_lock *lock, va_list args; struct va_format vaf; - if (spin_trylock(&lock->l_lock)) { - if (lock->l_resource) - resource = ldlm_resource_getref(lock->l_resource); - spin_unlock(&lock->l_lock); - } + rcu_read_lock(); + resource = rcu_dereference(lock->l_resource); + if (resource && !atomic_inc_not_zero(&resource->lr_refcount)) + resource = NULL; + rcu_read_unlock(); va_start(args, fmt); vaf.fmt = fmt; diff --git a/drivers/staging/lustre/lustre/ldlm/ldlm_lockd.c b/drivers/staging/lustre/lustre/ldlm/ldlm_lockd.c index 589b89d5885a..a9ca9a5e49be 100644 --- a/drivers/staging/lustre/lustre/ldlm/ldlm_lockd.c +++ b/drivers/staging/lustre/lustre/ldlm/ldlm_lockd.c @@ -1097,6 +1097,23 @@ static int ldlm_cleanup(void) return 0; } +void ldlm_resource_init_once(void *p) +{ + /* + * It is import to initialise the spinlock only once, + * as ldlm_lock_change_resource() could try to lock + * the resource *after* it has been freed and possibly + * reused. SLAB_TYPESAFE_BY_RCU ensures the memory won't + * be freed while the lock is being taken, but we need to + * ensure that it doesn't get reinitialized either. + */ + struct ldlm_resource *res = p; + + memset(res, 0, sizeof(*res)); + mutex_init(&res->lr_lvb_mutex); + spin_lock_init(&res->lr_lock); +} + int ldlm_init(void) { mutex_init(&ldlm_ref_mutex); @@ -1104,7 +1121,8 @@ int ldlm_init(void) mutex_init(ldlm_namespace_lock(LDLM_NAMESPACE_CLIENT)); ldlm_resource_slab = kmem_cache_create("ldlm_resources", sizeof(struct ldlm_resource), 0, - SLAB_HWCACHE_ALIGN, NULL); + SLAB_TYPESAFE_BY_RCU | + SLAB_HWCACHE_ALIGN, ldlm_resource_init_once); if (!ldlm_resource_slab) return -ENOMEM; diff --git a/drivers/staging/lustre/lustre/ldlm/ldlm_resource.c b/drivers/staging/lustre/lustre/ldlm/ldlm_resource.c index 45b2e97f7e1c..d79f70d17220 100644 --- a/drivers/staging/lustre/lustre/ldlm/ldlm_resource.c +++ b/drivers/staging/lustre/lustre/ldlm/ldlm_resource.c @@ -994,12 +994,14 @@ static struct ldlm_resource *ldlm_resource_new(enum ldlm_type ldlm_type) { struct ldlm_resource *res; - res = kmem_cache_zalloc(ldlm_resource_slab, GFP_NOFS); + res = kmem_cache_alloc(ldlm_resource_slab, GFP_NOFS); if (!res) return NULL; INIT_LIST_HEAD(&res->lr_granted); INIT_LIST_HEAD(&res->lr_waiting); + res->lr_lvb_inode = NULL; + res->lr_lvb_len = 0; if (ldlm_type == LDLM_EXTENT) { int idx; @@ -1017,16 +1019,15 @@ static struct ldlm_resource *ldlm_resource_new(enum ldlm_type ldlm_type) res->lr_itree[idx].lit_mode = 1 << idx; res->lr_itree[idx].lit_root = RB_ROOT_CACHED; } - } + } else + res->lr_itree = NULL; atomic_set(&res->lr_refcount, 1); - spin_lock_init(&res->lr_lock); lu_ref_init(&res->lr_reference); /* The creator of the resource must unlock the mutex after LVB * initialization. */ - mutex_init(&res->lr_lvb_mutex); mutex_lock(&res->lr_lvb_mutex); return res;