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 <andreas.dilger@intel.com>
Signed-off-by: Lai Siyao <lai.siyao@intel.com>
Change-Id: I7b45d382d2ee87c1df813ebdd9c7f21029500c1e
Reviewed-on: http://review.whamcloud.com/6592
Tested-by: Hudson
Tested-by: Maloo <whamcloud.maloo@gmail.com>
Reviewed-by: Johann Lombardi <johann.lombardi@intel.com>
Reviewed-on: http://review.whamcloud.com/7316
Reviewed-by: Oleg Drokin <oleg.drokin@intel.com>

diff --git a/lustre/ldlm/ldlm_request.c b/lustre/ldlm/ldlm_request.c
index 476c90c..9cbdcad 100644 (file)
--- 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)))
@@ -1947,7 +1941,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);
@@ -1961,21 +1956,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 43fb2d1..a74e4aa 100644 (file)
--- a/lustre/ldlm/ldlm_resource.c
+++ b/lustre/ldlm/ldlm_resource.c
@@ -793,26 +793,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;
 }
 
 /**
@@ -1410,18 +1403,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 7d91f64..a3d6d6e 100644 (file)
--- a/lustre/liblustre/namei.c
+++ b/lustre/liblustre/namei.c
@@ -50,6 +50,7 @@
 #include <sys/queue.h>
 
 #include "llite_lib.h"
+#include <lustre_fid.h>
 
 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 1249af6..ea9afaa 100644 (file)
--- 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 74112fb..22c0b02 100644 (file)
--- a/lustre/mdc/mdc_locks.c
+++ b/lustre/mdc/mdc_locks.c
@@ -973,16 +973,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 6dab6dd..12b1b2d 100644 (file)
--- a/lustre/mdt/mdt_handler.c
+++ b/lustre/mdt/mdt_handler.c
@@ -1316,35 +1316,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:
@@ -1426,17 +1424,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 83bd75e..65cac64 100644 (file)
--- a/lustre/mgc/mgc_request.c
+++ b/lustre/mgc/mgc_request.c
@@ -816,14 +816,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");