From a1052417b78bc18898161c4ed44d79de4a1a2f23 Mon Sep 17 00:00:00 2001 From: Andreas Dilger Date: Fri, 7 Jun 2013 16:31:51 -0600 Subject: [PATCH 1/1] LU-2901 ldlm: fix resource/fid check, use DLDLMRES In ll_md_blocking_ast() the FID/resource comparison is incorrectly checking for fid_ver() stored in res_id.name[2] instead of name[1] changed since http://review.whamcloud.com/2271 (commit 4f91d5161d00) landed. This does not impact current clients, since name[2] and fid_ver() are always zero, but it could cause problems in the future. In ldlm_cli_enqueue_fini() use ldlm_res_eq() instead of comparing each of the resource fields separately. Use DLDLMRES/PLDLMRES when printing resource names everywhere. Signed-off-by: Andreas Dilger Signed-off-by: Lai Siyao Change-Id: I7b45d382d2ee87c1df813ebdd9c7f21029500c1e Reviewed-on: http://review.whamcloud.com/6592 Tested-by: Hudson Tested-by: Maloo Reviewed-by: Johann Lombardi --- lustre/ldlm/ldlm_request.c | 58 ++++++++++++++-------------------- lustre/ldlm/ldlm_resource.c | 37 +++++++++------------- lustre/liblustre/namei.c | 14 +++------ lustre/llite/namei.c | 11 +++---- lustre/mdc/mdc_locks.c | 15 +++------ lustre/mdt/mdt_handler.c | 76 +++++++++++++++++++++------------------------ lustre/mgc/mgc_request.c | 14 ++++----- 7 files changed, 95 insertions(+), 130 deletions(-) diff --git a/lustre/ldlm/ldlm_request.c b/lustre/ldlm/ldlm_request.c index 7d9ab66..95b2f63 100644 --- a/lustre/ldlm/ldlm_request.c +++ b/lustre/ldlm/ldlm_request.c @@ -624,25 +624,19 @@ int ldlm_cli_enqueue_fini(struct obd_export *exp, struct ptlrpc_request *req, lock->l_req_mode = newmode; } - if (memcmp(reply->lock_desc.l_resource.lr_name.name, - lock->l_resource->lr_name.name, - sizeof(struct ldlm_res_id))) { - CDEBUG(D_INFO, "remote intent success, locking " - "(%ld,%ld,%ld) instead of " - "(%ld,%ld,%ld)\n", - (long)reply->lock_desc.l_resource.lr_name.name[0], - (long)reply->lock_desc.l_resource.lr_name.name[1], - (long)reply->lock_desc.l_resource.lr_name.name[2], - (long)lock->l_resource->lr_name.name[0], - (long)lock->l_resource->lr_name.name[1], - (long)lock->l_resource->lr_name.name[2]); - - rc = ldlm_lock_change_resource(ns, lock, - &reply->lock_desc.l_resource.lr_name); - if (rc || lock->l_resource == NULL) - GOTO(cleanup, rc = -ENOMEM); - LDLM_DEBUG(lock, "client-side enqueue, new resource"); - } + if (!ldlm_res_eq(&reply->lock_desc.l_resource.lr_name, + &lock->l_resource->lr_name)) { + CDEBUG(D_INFO, "remote intent success, locking "DLDLMRES + " instead of "DLDLMRES"\n", + PLDLMRES(&reply->lock_desc.l_resource), + PLDLMRES(lock->l_resource)); + + rc = ldlm_lock_change_resource(ns, lock, + &reply->lock_desc.l_resource.lr_name); + if (rc || lock->l_resource == NULL) + GOTO(cleanup, rc = -ENOMEM); + LDLM_DEBUG(lock, "client-side enqueue, new resource"); + } if (with_policy) if (!(type == LDLM_IBITS && !(exp_connect_flags(exp) & OBD_CONNECT_IBITS))) @@ -1946,7 +1940,8 @@ int ldlm_cli_cancel_unused_resource(struct ldlm_namespace *ns, 0, flags | LCF_BL_AST, opaque); rc = ldlm_cli_cancel_list(&cancels, count, NULL, flags); if (rc != ELDLM_OK) - CERROR("ldlm_cli_cancel_unused_resource: %d\n", rc); + CERROR("canceling unused lock "DLDLMRES": rc = %d\n", + PLDLMRES(res), rc); LDLM_RESOURCE_DELREF(res); ldlm_resource_putref(res); @@ -1960,21 +1955,16 @@ struct ldlm_cli_cancel_arg { }; static int ldlm_cli_hash_cancel_unused(cfs_hash_t *hs, cfs_hash_bd_t *bd, - cfs_hlist_node_t *hnode, void *arg) + cfs_hlist_node_t *hnode, void *arg) { - struct ldlm_resource *res = cfs_hash_object(hs, hnode); - struct ldlm_cli_cancel_arg *lc = arg; - int rc; - - rc = ldlm_cli_cancel_unused_resource(ldlm_res_to_ns(res), &res->lr_name, - NULL, LCK_MINMODE, - lc->lc_flags, lc->lc_opaque); - if (rc != 0) { - CERROR("ldlm_cli_cancel_unused ("LPU64"): %d\n", - res->lr_name.name[0], rc); - } - /* must return 0 for hash iteration */ - return 0; + struct ldlm_resource *res = cfs_hash_object(hs, hnode); + struct ldlm_cli_cancel_arg *lc = arg; + + ldlm_cli_cancel_unused_resource(ldlm_res_to_ns(res), &res->lr_name, + NULL, LCK_MINMODE, lc->lc_flags, + lc->lc_opaque); + /* must return 0 for hash iteration */ + return 0; } /** diff --git a/lustre/ldlm/ldlm_resource.c b/lustre/ldlm/ldlm_resource.c index 0b74780..b86c66d 100644 --- a/lustre/ldlm/ldlm_resource.c +++ b/lustre/ldlm/ldlm_resource.c @@ -798,26 +798,19 @@ static int ldlm_resource_clean(cfs_hash_t *hs, cfs_hash_bd_t *bd, } static int ldlm_resource_complain(cfs_hash_t *hs, cfs_hash_bd_t *bd, - cfs_hlist_node_t *hnode, void *arg) + cfs_hlist_node_t *hnode, void *arg) { - struct ldlm_resource *res = cfs_hash_object(hs, hnode); + struct ldlm_resource *res = cfs_hash_object(hs, hnode); lock_res(res); - CERROR("Namespace %s resource refcount nonzero " - "(%d) after lock cleanup; forcing " - "cleanup.\n", - ldlm_ns_name(ldlm_res_to_ns(res)), - cfs_atomic_read(&res->lr_refcount) - 1); - - CERROR("Resource: %p ("LPU64"/"LPU64"/"LPU64"/" - LPU64") (rc: %d)\n", res, - res->lr_name.name[0], res->lr_name.name[1], - res->lr_name.name[2], res->lr_name.name[3], - cfs_atomic_read(&res->lr_refcount) - 1); + CERROR("%s: namespace resource "DLDLMRES" (%p) refcount nonzero " + "(%d) after lock cleanup; forcing cleanup.\n", + ldlm_ns_name(ldlm_res_to_ns(res)), PLDLMRES(res), res, + cfs_atomic_read(&res->lr_refcount) - 1); ldlm_resource_dump(D_ERROR, res); unlock_res(res); - return 0; + return 0; } /** @@ -1443,18 +1436,16 @@ EXPORT_SYMBOL(ldlm_namespace_dump); */ void ldlm_resource_dump(int level, struct ldlm_resource *res) { - struct ldlm_lock *lock; - unsigned int granted = 0; + struct ldlm_lock *lock; + unsigned int granted = 0; - CLASSERT(RES_NAME_SIZE == 4); + CLASSERT(RES_NAME_SIZE == 4); - if (!((libcfs_debug | D_ERROR) & level)) - return; + if (!((libcfs_debug | D_ERROR) & level)) + return; - CDEBUG(level, "--- Resource: %p ("LPU64"/"LPU64"/"LPU64"/"LPU64 - ") (rc: %d)\n", res, res->lr_name.name[0], res->lr_name.name[1], - res->lr_name.name[2], res->lr_name.name[3], - cfs_atomic_read(&res->lr_refcount)); + CDEBUG(level, "--- Resource: "DLDLMRES" (%p) refcount = %d\n", + PLDLMRES(res), res, cfs_atomic_read(&res->lr_refcount)); if (!cfs_list_empty(&res->lr_granted)) { CDEBUG(level, "Granted locks (in reverse order):\n"); diff --git a/lustre/liblustre/namei.c b/lustre/liblustre/namei.c index 209cc92..0e72637 100644 --- a/lustre/liblustre/namei.c +++ b/lustre/liblustre/namei.c @@ -50,6 +50,7 @@ #include #include "llite_lib.h" +#include void ll_intent_drop_lock(struct lookup_intent *it) { @@ -148,15 +149,10 @@ int llu_md_blocking_ast(struct ldlm_lock *lock, if (bits & MDS_INODELOCK_UPDATE) lli->lli_flags &= ~LLIF_MDS_SIZE_LOCK; - fid = &lli->lli_fid; - if (lock->l_resource->lr_name.name[0] != fid_seq(fid) || - lock->l_resource->lr_name.name[1] != fid_oid(fid) || - lock->l_resource->lr_name.name[2] != fid_ver(fid)) { - LDLM_ERROR(lock,"data mismatch with ino %llu/%llu/%llu", - (long long)fid_seq(fid), - (long long)fid_oid(fid), - (long long)fid_ver(fid)); - } + fid = &lli->lli_fid; + if (!fid_res_name_eq(fid, &lock->l_resource->lr_name)) + LDLM_ERROR(lock, "data mismatch with object " + DFID" (%p)", PFID(fid), inode); if (S_ISDIR(st->st_mode) && (bits & MDS_INODELOCK_UPDATE)) { CDEBUG(D_INODE, "invalidating inode %llu\n", diff --git a/lustre/llite/namei.c b/lustre/llite/namei.c index 64855b1..81bf255 100644 --- a/lustre/llite/namei.c +++ b/lustre/llite/namei.c @@ -236,13 +236,10 @@ int ll_md_blocking_ast(struct ldlm_lock *lock, struct ldlm_lock_desc *desc, if (bits & MDS_INODELOCK_OPEN) ll_have_md_lock(inode, &bits, mode); - fid = ll_inode2fid(inode); - if (lock->l_resource->lr_name.name[0] != fid_seq(fid) || - lock->l_resource->lr_name.name[1] != fid_oid(fid) || - lock->l_resource->lr_name.name[2] != fid_ver(fid)) { - LDLM_ERROR(lock, "data mismatch with object " - DFID" (%p)", PFID(fid), inode); - } + fid = ll_inode2fid(inode); + if (!fid_res_name_eq(fid, &lock->l_resource->lr_name)) + LDLM_ERROR(lock, "data mismatch with object " + DFID" (%p)", PFID(fid), inode); if (bits & MDS_INODELOCK_OPEN) { int flags = 0; diff --git a/lustre/mdc/mdc_locks.c b/lustre/mdc/mdc_locks.c index 560bdf9..d6e155a 100644 --- a/lustre/mdc/mdc_locks.c +++ b/lustre/mdc/mdc_locks.c @@ -989,16 +989,11 @@ static int mdc_finish_intent_lock(struct obd_export *exp, ldlm_policy_data_t policy = lock->l_policy_data; LDLM_DEBUG(lock, "matching against this"); - LASSERTF(fid_res_name_eq(&mdt_body->fid1, - &lock->l_resource->lr_name), - "Lock res_id: %lu/%lu/%lu, fid: %lu/%lu/%lu.\n", - (unsigned long)lock->l_resource->lr_name.name[0], - (unsigned long)lock->l_resource->lr_name.name[1], - (unsigned long)lock->l_resource->lr_name.name[2], - (unsigned long)fid_seq(&mdt_body->fid1), - (unsigned long)fid_oid(&mdt_body->fid1), - (unsigned long)fid_ver(&mdt_body->fid1)); - LDLM_LOCK_PUT(lock); + LASSERTF(fid_res_name_eq(&mdt_body->fid1, + &lock->l_resource->lr_name), + "Lock res_id: "DLDLMRES", fid: "DFID"\n", + PLDLMRES(lock->l_resource), PFID(&mdt_body->fid1)); + LDLM_LOCK_PUT(lock); memcpy(&old_lock, lockh, sizeof(*lockh)); if (ldlm_lock_match(NULL, LDLM_FL_BLOCK_GRANTED, NULL, diff --git a/lustre/mdt/mdt_handler.c b/lustre/mdt/mdt_handler.c index 276ddfd..3c558ae 100644 --- a/lustre/mdt/mdt_handler.c +++ b/lustre/mdt/mdt_handler.c @@ -1329,35 +1329,33 @@ static int mdt_getattr_name_lock(struct mdt_thread_info *info, */ child = mdt_object_find(info->mti_env, info->mti_mdt, child_fid); - if (unlikely(IS_ERR(child))) - GOTO(out_parent, rc = PTR_ERR(child)); - if (is_resent) { - /* Do not take lock for resent case. */ - lock = ldlm_handle2lock(&lhc->mlh_reg_lh); - LASSERTF(lock != NULL, "Invalid lock handle "LPX64"\n", - lhc->mlh_reg_lh.cookie); - - res_id = &lock->l_resource->lr_name; - if (!fid_res_name_eq(mdt_object_fid(child), - &lock->l_resource->lr_name)) { - LASSERTF(fid_res_name_eq(mdt_object_fid(parent), - &lock->l_resource->lr_name), - "Lock res_id: %lu/%lu/%lu, Fid: "DFID".\n", - (unsigned long)res_id->name[0], - (unsigned long)res_id->name[1], - (unsigned long)res_id->name[2], - PFID(mdt_object_fid(parent))); - CWARN("Although resent, but still not get child lock" - "parent:"DFID" child:"DFID"\n", - PFID(mdt_object_fid(parent)), - PFID(mdt_object_fid(child))); - lustre_msg_clear_flags(req->rq_reqmsg, MSG_RESENT); - LDLM_LOCK_PUT(lock); - GOTO(relock, 0); - } - LDLM_LOCK_PUT(lock); - rc = 0; - } else { + if (unlikely(IS_ERR(child))) + GOTO(out_parent, rc = PTR_ERR(child)); + if (is_resent) { + /* Do not take lock for resent case. */ + lock = ldlm_handle2lock(&lhc->mlh_reg_lh); + LASSERTF(lock != NULL, "Invalid lock handle "LPX64"\n", + lhc->mlh_reg_lh.cookie); + + res_id = &lock->l_resource->lr_name; + if (!fid_res_name_eq(mdt_object_fid(child), + &lock->l_resource->lr_name)) { + LASSERTF(fid_res_name_eq(mdt_object_fid(parent), + &lock->l_resource->lr_name), + "Lock res_id: "DLDLMRES", fid: "DFID"\n", + PLDLMRES(lock->l_resource), + PFID(mdt_object_fid(parent))); + CWARN("Although resent, but still not get child lock" + "parent:"DFID" child:"DFID"\n", + PFID(mdt_object_fid(parent)), + PFID(mdt_object_fid(child))); + lustre_msg_clear_flags(req->rq_reqmsg, MSG_RESENT); + LDLM_LOCK_PUT(lock); + GOTO(relock, 0); + } + LDLM_LOCK_PUT(lock); + rc = 0; + } else { bool try_layout = false; relock: @@ -1439,17 +1437,15 @@ relock: rc = mdt_getattr_internal(info, child, ma_need); if (unlikely(rc != 0)) { mdt_object_unlock(info, child, lhc, 1); - } else if (lock) { - /* Debugging code. */ - res_id = &lock->l_resource->lr_name; - LDLM_DEBUG(lock, "Returning lock to client"); - LASSERTF(fid_res_name_eq(mdt_object_fid(child), - &lock->l_resource->lr_name), - "Lock res_id: %lu/%lu/%lu, Fid: "DFID".\n", - (unsigned long)res_id->name[0], - (unsigned long)res_id->name[1], - (unsigned long)res_id->name[2], - PFID(mdt_object_fid(child))); + } else if (lock) { + /* Debugging code. */ + res_id = &lock->l_resource->lr_name; + LDLM_DEBUG(lock, "Returning lock to client"); + LASSERTF(fid_res_name_eq(mdt_object_fid(child), + &lock->l_resource->lr_name), + "Lock res_id: "DLDLMRES", fid: "DFID"\n", + PLDLMRES(lock->l_resource), + PFID(mdt_object_fid(child))); if (mdt_object_exists(child) && !mdt_object_remote(child)) mdt_pack_size2body(info, child); } diff --git a/lustre/mgc/mgc_request.c b/lustre/mgc/mgc_request.c index e024b9d..cd77628 100644 --- a/lustre/mgc/mgc_request.c +++ b/lustre/mgc/mgc_request.c @@ -813,14 +813,14 @@ static int mgc_blocking_ast(struct ldlm_lock *lock, struct ldlm_lock_desc *desc, LDLM_DEBUG(lock, "MGC blocking CB"); ldlm_lock2handle(lock, &lockh); rc = ldlm_cli_cancel(&lockh, LCF_ASYNC); - break; - case LDLM_CB_CANCELING: - /* We've given up the lock, prepare ourselves to update. */ - LDLM_DEBUG(lock, "MGC cancel CB"); + break; + case LDLM_CB_CANCELING: + /* We've given up the lock, prepare ourselves to update. */ + LDLM_DEBUG(lock, "MGC cancel CB"); - CDEBUG(D_MGC, "Lock res "LPX64" (%.8s)\n", - lock->l_resource->lr_name.name[0], - (char *)&lock->l_resource->lr_name.name[0]); + CDEBUG(D_MGC, "Lock res "DLDLMRES" (%.8s)\n", + PLDLMRES(lock->l_resource), + (char *)&lock->l_resource->lr_name.name[0]); if (!cld) { CDEBUG(D_INFO, "missing data, won't requeue\n"); -- 1.8.3.1