From 0badac9d0b002d5e4399e59aa7bde28a10021f1f Mon Sep 17 00:00:00 2001 From: Andriy Skulysh Date: Fri, 27 Feb 2015 14:15:15 +0200 Subject: [PATCH] LU-6304 ldlm: crash on umount in cleanup_resource cfs_hash_for_each_relax() assumes that cfs_hash_put_locked() doesn't release bd lock, but it isn't true for ldlm_res_hop_put_locked(). Add recfcount on next hnode in cfs_hash_for_each_relax() and remove ldlm_res_hop_put_locked() Change-Id: I275c0d29737119d084205e604c336a42e489bfe7 Xyratex-bug-id: MRP-2352 Signed-off-by: Andriy Skulysh Reviewed-by: Vitaly Fertman Reviewed-by: Alexander Boyko Tested-by: Alexander Lezhoev Reviewed-on: http://review.whamcloud.com/13908 Tested-by: Jenkins Tested-by: Maloo Reviewed-by: Oleg Drokin --- libcfs/libcfs/hash.c | 55 ++++++++++++++++++++++++++++----------------- lustre/ldlm/ldlm_internal.h | 1 - lustre/ldlm/ldlm_resource.c | 46 ------------------------------------- 3 files changed, 35 insertions(+), 67 deletions(-) diff --git a/libcfs/libcfs/hash.c b/libcfs/libcfs/hash.c index c3d662f..9c37ee1 100644 --- a/libcfs/libcfs/hash.c +++ b/libcfs/libcfs/hash.c @@ -1038,7 +1038,7 @@ cfs_hash_create(char *name, unsigned cur_bits, unsigned max_bits, LASSERT(ops->hs_object); LASSERT(ops->hs_keycmp); LASSERT(ops->hs_get != NULL); - LASSERT(ops->hs_put_locked != NULL); + LASSERT(ops->hs_put != NULL || ops->hs_put_locked != NULL); if ((flags & CFS_HASH_REHASH) != 0) flags |= CFS_HASH_COUNTER; /* must have counter */ @@ -1589,18 +1589,19 @@ cfs_hash_for_each_relax(struct cfs_hash *hs, cfs_hash_for_each_cb_t func, void *data, int start) { struct hlist_node *hnode; - struct hlist_node *tmp; + struct hlist_node *next = NULL; struct cfs_hash_bd bd; __u32 version; int count = 0; int stop_on_change; + int has_put_locked; int rc = 0; int i, end = -1; ENTRY; stop_on_change = cfs_hash_with_rehash_key(hs) || - !cfs_hash_with_no_itemref(hs) || - hs->hs_ops->hs_put_locked == NULL; + !cfs_hash_with_no_itemref(hs); + has_put_locked = hs->hs_ops->hs_put_locked != NULL; cfs_hash_lock(hs, 0); again: LASSERT(!cfs_hash_is_rehashing(hs)); @@ -1617,38 +1618,52 @@ again: version = cfs_hash_bd_version_get(&bd); cfs_hash_bd_for_each_hlist(hs, &bd, hhead) { - for (hnode = hhead->first; hnode != NULL;) { - cfs_hash_bucket_validate(hs, &bd, hnode); - cfs_hash_get(hs, hnode); + hnode = hhead->first; + if (hnode == NULL) + continue; + cfs_hash_get(hs, hnode); + for (; hnode != NULL; hnode = next) { + cfs_hash_bucket_validate(hs, &bd, hnode); + next = hnode->next; + if (next != NULL) + cfs_hash_get(hs, next); cfs_hash_bd_unlock(hs, &bd, 0); cfs_hash_unlock(hs, 0); rc = func(hs, &bd, hnode, data); - if (stop_on_change) + if (stop_on_change || !has_put_locked) cfs_hash_put(hs, hnode); + cond_resched(); count++; cfs_hash_lock(hs, 0); cfs_hash_bd_lock(hs, &bd, 0); - if (!stop_on_change) { - tmp = hnode->next; - cfs_hash_put_locked(hs, hnode); - hnode = tmp; - } else { /* bucket changed? */ - if (version != - cfs_hash_bd_version_get(&bd)) - break; - /* safe to continue because no change */ - hnode = hnode->next; - } + if (stop_on_change) { + if (version != + cfs_hash_bd_version_get(&bd)) + rc = -EINTR; + } else if (has_put_locked) { + cfs_hash_put_locked(hs, hnode); + } if (rc) /* callback wants to break iteration */ break; } - if (rc) /* callback wants to break iteration */ + if (next != NULL) { + if (has_put_locked) { + cfs_hash_put_locked(hs, next); + next = NULL; + } break; + } else if (rc != 0) { + break; + } } cfs_hash_bd_unlock(hs, &bd, 0); + if (next != NULL && !has_put_locked) { + cfs_hash_put(hs, next); + next = NULL; + } if (rc) /* callback wants to break iteration */ break; } diff --git a/lustre/ldlm/ldlm_internal.h b/lustre/ldlm/ldlm_internal.h index c997425..3107b24 100644 --- a/lustre/ldlm/ldlm_internal.h +++ b/lustre/ldlm/ldlm_internal.h @@ -123,7 +123,6 @@ extern struct kmem_cache *ldlm_resource_slab; extern struct kmem_cache *ldlm_lock_slab; extern struct kmem_cache *ldlm_interval_tree_slab; -int ldlm_resource_putref_locked(struct ldlm_resource *res); void ldlm_resource_insert_lock_after(struct ldlm_lock *original, struct ldlm_lock *new); diff --git a/lustre/ldlm/ldlm_resource.c b/lustre/ldlm/ldlm_resource.c index 7ed21e3..d8d6db2 100644 --- a/lustre/ldlm/ldlm_resource.c +++ b/lustre/ldlm/ldlm_resource.c @@ -571,16 +571,6 @@ ldlm_res_hop_get_locked(struct cfs_hash *hs, struct hlist_node *hnode) ldlm_resource_getref(res); } -static void -ldlm_res_hop_put_locked(struct cfs_hash *hs, struct hlist_node *hnode) -{ - struct ldlm_resource *res; - - res = hlist_entry(hnode, struct ldlm_resource, lr_hash); - /* cfs_hash_for_each_nolock is the only chance we call it */ - ldlm_resource_putref_locked(res); -} - static void ldlm_res_hop_put(struct cfs_hash *hs, struct hlist_node *hnode) { struct ldlm_resource *res; @@ -596,7 +586,6 @@ static struct cfs_hash_ops ldlm_ns_hash_ops = { .hs_keycpy = NULL, .hs_object = ldlm_res_hop_object, .hs_get = ldlm_res_hop_get_locked, - .hs_put_locked = ldlm_res_hop_put_locked, .hs_put = ldlm_res_hop_put }; @@ -607,7 +596,6 @@ static struct cfs_hash_ops ldlm_ns_fid_hash_ops = { .hs_keycpy = NULL, .hs_object = ldlm_res_hop_object, .hs_get = ldlm_res_hop_get_locked, - .hs_put_locked = ldlm_res_hop_put_locked, .hs_put = ldlm_res_hop_put }; @@ -1326,40 +1314,6 @@ int ldlm_resource_putref(struct ldlm_resource *res) } EXPORT_SYMBOL(ldlm_resource_putref); -/* Returns 1 if the resource was freed, 0 if it remains. */ -int ldlm_resource_putref_locked(struct ldlm_resource *res) -{ - struct ldlm_namespace *ns = ldlm_res_to_ns(res); - - LASSERT_ATOMIC_GT_LT(&res->lr_refcount, 0, LI_POISON); - CDEBUG(D_INFO, "putref res: %p count: %d\n", - res, atomic_read(&res->lr_refcount) - 1); - - if (atomic_dec_and_test(&res->lr_refcount)) { - struct cfs_hash_bd bd; - - cfs_hash_bd_get(ldlm_res_to_ns(res)->ns_rs_hash, - &res->lr_name, &bd); - __ldlm_resource_putref_final(&bd, res); - cfs_hash_bd_unlock(ns->ns_rs_hash, &bd, 1); - /* NB: ns_rs_hash is created with CFS_HASH_NO_ITEMREF, - * so we should never be here while calling cfs_hash_del, - * cfs_hash_for_each_nolock is the only case we can get - * here, which is safe to release cfs_hash_bd_lock. - */ - if (ns->ns_lvbo && ns->ns_lvbo->lvbo_free) - ns->ns_lvbo->lvbo_free(res); - if (res->lr_itree != NULL) - OBD_SLAB_FREE(res->lr_itree, ldlm_interval_tree_slab, - sizeof(*res->lr_itree) * LCK_MODE_NUM); - OBD_SLAB_FREE(res, ldlm_resource_slab, sizeof *res); - - cfs_hash_bd_lock(ns->ns_rs_hash, &bd, 1); - return 1; - } - return 0; -} - /** * Add a lock into a given resource into specified lock list. */ -- 1.8.3.1