Please don't reply to lustre-devel. Instead, comment in Bugzilla by using the
following link:
https://bugzilla.lustre.org/show_bug.cgi?id=10902
What |Removed |Added
----------------------------------------------------------------------------
Attachment #9222|review?([EMAIL PROTECTED])|review-
Flag| |
(From update of attachment 9222)
>Index: include/lustre_dlm.h
>===================================================================
>RCS file: /cvsroot/cfs/lustre-core/include/Attic/lustre_dlm.h,v
>retrieving revision 1.1.58.11
>diff -u -p -u -p -r1.1.58.11 lustre_dlm.h
>--- include/lustre_dlm.h 18 Dec 2006 23:33:54 -0000 1.1.58.11
>+++ include/lustre_dlm.h 27 Dec 2006 01:49:14 -0000
>@@ -134,6 +134,12 @@ typedef enum {
> #define LDLM_CB_BLOCKING 1
> #define LDLM_CB_CANCELING 2
>
>+/* position flag of skip list pointers */
>+#define LDLM_SL_HEAD(skip_list) ((skip_list)->next != NULL)
>+#define LDLM_SL_TAIL(skip_list) ((skip_list)->prev != NULL)
>+#define LDLM_SL_EMPTY(skip_list) ((skip_list)->next == NULL && \
>+ (skip_list)->prev == NULL)
>+
> /* compatibility matrix */
> #define LCK_COMPAT_EX LCK_NL
> #define LCK_COMPAT_PW (LCK_COMPAT_EX | LCK_CR)
>@@ -266,6 +272,10 @@ struct ldlm_lock {
> /* protected by lr_lock */
> struct list_head l_res_link; // position in one of three res
> lists
>
>+ struct list_head l_sl_mode; // skip pointer for request
>mode
>+ struct list_head l_sl_policy; // skip pointer for inodebits
>+ struct list_head *l_res_listhead; // inserted list
This l_res_listhead is not in DLD, and the purpose is totally unclean.
>diff -u -p -u -p -r1.3.12.1.2.3 ldlm_inodebits.c
>--- ldlm/ldlm_inodebits.c 19 Jun 2006 10:29:21 -0000 1.3.12.1.2.3
>+++ ldlm/ldlm_inodebits.c 27 Dec 2006 01:49:15 -0000
>@@ -37,7 +37,7 @@ static int
> ldlm_inodebits_compat_queue(struct list_head *queue, struct ldlm_lock *req,
> struct list_head *work_list)
> {
>- struct list_head *tmp;
>+ struct list_head *tmp, *tmp_tail;
> struct ldlm_lock *lock;
> ldlm_mode_t req_mode = req->l_req_mode;
> __u64 req_bits = req->l_policy_data.l_inodebits.bits;
>@@ -47,27 +47,71 @@ ldlm_inodebits_compat_queue(struct list_
> LASSERT(req_bits); /* There is no sense in lock with no bits set,
> I think. Also such a lock would be compatible
> with any other bit lock */
>+
>+ if (req->l_res_listhead == queue)
>+ RETURN(compat);
I am not sure what do you want to achieve with this code, but what actuall
happens here is any time you reprocess the queue, you always return 1 here for
every lock on this queue.
Also nothing like this code is in DLD?
Note that this code runs before you enter the locks-iterating loop below and
compat is always 1.
If you want to find out if the lock is in granted queue or not, you can do this
by comparing
requested vs granted lock mode. If they equal, this lock is on granted queue,
if they differ then if granted lockmode is 0 lock is in waiting list, and if it
is not 0, then it is in converting list.
> list_for_each(tmp, queue) {
> lock = list_entry(tmp, struct ldlm_lock, l_res_link);
>
>+ /*
> if (req == lock)
> RETURN(compat);
>+ */
Why do you comment out this code?
It is not replaced by that code above. Also if you think some code is unneeded,
just remove it.
> /* locks are compatible, bits don't matter */
>- if (lockmode_compat(lock->l_req_mode, req_mode))
>- continue;
>-
>- /* if bits don't overlap skip it */
>- if (!(lock->l_policy_data.l_inodebits.bits & req_bits))
>+ if (lockmode_compat(lock->l_req_mode, req_mode)) {
>+ /* jump to next mode group */
>+ if (LDLM_SL_HEAD(&lock->l_sl_mode))
>+ tmp = &list_entry(lock->l_sl_mode.next,
>+ struct ldlm_lock,
>+ l_sl_mode)->l_res_link;
> continue;
>+ }
>
>- if (!work_list)
>- RETURN(0);
>-
>- compat = 0;
>- if (lock->l_blocking_ast)
>- ldlm_add_ast_work_item(lock, req, work_list);
>- }
>+ tmp_tail = tmp;
>+ if (LDLM_SL_HEAD(&lock->l_sl_mode))
>+ tmp_tail = &list_entry(lock->l_sl_mode.next,
>+ struct ldlm_lock,
>+ l_sl_mode)->l_res_link;
>+ for (;;) {
>+ /* locks with bits overlapped are conflicting locks */
>+ if (lock->l_policy_data.l_inodebits.bits & req_bits) {
>+ /* conflicting policy */
>+ if (!work_list)
>+ RETURN(0);
>+
>+ compat = 0;
>+ if (lock->l_blocking_ast)
>+ ldlm_add_ast_work_item(lock, req,
>+ work_list);
>+ /* add all members of the policy group */
>+ if (LDLM_SL_HEAD(&lock->l_sl_policy)) {
>+ do {
>+ tmp = lock->l_res_link.next;
>+ lock = list_entry(tmp,
>+ struct ldlm_lock,
>+ l_res_link);
>+ if (lock->l_blocking_ast)
>+
>ldlm_add_ast_work_item(
>+ lock,
>+ req,
>+
>work_list);
>+ } while
>(!LDLM_SL_TAIL(&lock->l_sl_policy));
>+ }
>+ } else if (LDLM_SL_HEAD(&lock->l_sl_policy)) {
>+ /* jump to next policy group */
>+ tmp = &list_entry(lock->l_sl_policy.next,
>+ struct ldlm_lock,
>+ l_sl_policy)->l_res_link;
>+ }
>+ if (tmp == tmp_tail)
>+ break;
>+ else
>+ tmp = tmp->next;
>+ lock = list_entry(tmp, struct ldlm_lock, l_res_link);
>+ } /* for locks in a mode group */
>+ } /* for each lock in the granted queue */
Not sure what do you mean with this last comment. So fat the code above will
run for any queue.
>===================================================================
>RCS file: /cvsroot/cfs/lustre-core/ldlm/ldlm_plain.c,v
>retrieving revision 1.4.40.1.42.1.32.3
>diff -u -p -u -p -r1.4.40.1.42.1.32.3 ldlm_plain.c
>--- ldlm/ldlm_plain.c 25 Oct 2006 00:19:49 -0000 1.4.40.1.42.1.32.3
>+++ ldlm/ldlm_plain.c 27 Dec 2006 01:49:15 -0000
>@@ -48,14 +48,25 @@ ldlm_plain_compat_queue(struct list_head
>
> lockmode_verify(req_mode);
>
>+ if (req->l_res_listhead == queue)
>+ RETURN(compat);
>+
Same comments as above. for whole function.
>diff -u -p -u -p -r1.72.2.8.14.10.24.8 ldlm_resource.c
>--- ldlm/ldlm_resource.c 17 Nov 2006 17:34:27 -0000
>1.72.2.8.14.10.24.8
>+++ ldlm/ldlm_resource.c 27 Dec 2006 01:49:16 -0000
>@@ -703,6 +708,148 @@ int ldlm_resource_putref_locked(struct l
> RETURN(rc);
> }
>
>+/*
>+ * search_granted_lock
>+ *
>+ * Description:
>+ * Finds a position to insert the new lock.
>+ * Parameters:
>+ * queue [input]: the granted list where search acts on;
>+ * req [input]: the lock whose position to be located;
>+ * lockp [output]: the position where the lock should be inserted
>before, or
>+ * NULL indicating @req should be appended to @queue.
>+ * Return Values:
>+ * Bit-masks combination of following values indicating in which way the
>+ * lock need to be inserted.
>+ * - LDLM_JOIN_NONE: noting about skip list needs to be fixed;
>+ * - LDLM_MODE_JOIN_RIGHT: @req needs join right becoming the head of a
>+ * mode group;
>+ * - LDLM_POLICY_JOIN_RIGHT: @req needs join right becoming the head of
>+ * a policy group.
>+ * NOTE: called by
>+ * - ldlm_grant_lock_with_skiplist
>+ */
>+static int search_granted_lock(struct list_head *queue,
>+ struct ldlm_lock *req,
>+ struct ldlm_lock **lockp)
>+{
>+ struct list_head *tmp, *tmp_tail;
>+ struct ldlm_lock *lock, *mode_head_lock;
>+ int rc = LDLM_JOIN_NONE;
>+ ENTRY;
>+
>+ list_for_each(tmp, queue) {
>+ lock = list_entry(tmp, struct ldlm_lock, l_res_link);
>+
>+ if (lock->l_req_mode != req->l_req_mode) {
>+ if (LDLM_SL_HEAD(&lock->l_sl_mode))
>+ tmp = &list_entry(lock->l_sl_mode.next,
>+ struct ldlm_lock,
>+ l_sl_mode)->l_res_link;
>+ continue;
>+ }
>+
>+ /* found the same mode group */
>+ if (lock->l_resource->lr_type == LDLM_PLAIN) {
>+ *lockp = lock;
>+ rc = LDLM_MODE_JOIN_RIGHT;
>+ goto out;
GOTO(out, rc);
>+ }
>+
>+ if (lock->l_resource->lr_type == LDLM_IBITS) {
>+ tmp_tail = tmp;
>+ if (LDLM_SL_HEAD(&lock->l_sl_mode))
>+ tmp_tail = &list_entry(lock->l_sl_mode.next,
>+ struct ldlm_lock,
>+ l_sl_mode)->l_res_link;
>+ mode_head_lock = lock;
>+ for (;;) {
>+ if (lock->l_policy_data.l_inodebits.bits ==
>+ req->l_policy_data.l_inodebits.bits) {
>+ /* matched policy lock is found */
>+ *lockp = lock;
>+ rc |= LDLM_POLICY_JOIN_RIGHT;
>+
>+ /* if the policy group head is also a
>+ * mode group head or a single mode
>+ * group lock */
>+ if (LDLM_SL_HEAD(&lock->l_sl_mode) ||
>+ (tmp == tmp_tail &&
>+ LDLM_SL_EMPTY(&lock->l_sl_mode)))
>+ rc |= LDLM_MODE_JOIN_RIGHT;
>+ goto out;
GOTO(out, rc);
>+ }
>+
>+ if (LDLM_SL_HEAD(&lock->l_sl_policy))
>+ tmp =
>&list_entry(lock->l_sl_policy.next,
>+ struct ldlm_lock,
>+
>l_sl_policy)->l_res_link;
>+
>+ if (tmp == tmp_tail)
>+ break;
>+ else
>+ tmp = tmp->next;
>+ lock = list_entry(tmp, struct ldlm_lock,
>+ l_res_link);
>+ } /* for all locks in the matched mode group */
>+
>+ /* no matched policy group is found, insert before
>+ * the mode group head lock */
>+ *lockp = mode_head_lock;
>+ rc = LDLM_MODE_JOIN_RIGHT;
>+ goto out;
this should be GOTO(out, rc);
>+ } /* for inodebits locks */
>+ }
>+
>+ /* no matched mode group is found, append to the end */
>+ *lockp = NULL;
>+ rc = LDLM_JOIN_NONE;
>+out:
>+ EXIT;
EXIT should be before out label.
>+ return rc;
>@@ -719,7 +866,16 @@ void ldlm_resource_add_lock(struct ldlm_
>
> LASSERT(list_empty(&lock->l_res_link));
>
>- list_add_tail(&lock->l_res_link, head);
>+ if (head == &res->lr_granted || head == &res->lr_converting ||
>+ head == &res->lr_waiting)
>+ lock->l_res_listhead = head;
Why do you need this?
(also the if condition covers all possible HEADs, I think.)
>+ if (head == &res->lr_granted &&
>+ res->lr_namespace &&
>+ res->lr_namespace->ns_client == LDLM_NAMESPACE_SERVER &&
>+ (res->lr_type == LDLM_PLAIN || res->lr_type == LDLM_IBITS))
>+ ldlm_grant_lock_with_skiplist(lock);
>+ else
>+ list_add_tail(&lock->l_res_link, head);
> }
Hm, I think this code should be added to ldlm_grant_lock(), as per DLD, then no
'if' is needed there too, of course.
_______________________________________________
Lustre-devel mailing list
[email protected]
https://mail.clusterfs.com/mailman/listinfo/lustre-devel
