Lustre devel - Dec 2006 - [Bug 10902] IOR returns ERROR: Identifier removed

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



Hi

I still think this doesn''t answer the question I asked.

Vitaly Fertman wrote:
> Hi Peter
>
> On Thursday 14 December 2006 16:21, Peter Braam wrote:
>   
>> Hi
>>
>> We are promising this deliverable to Cray soon, so I mark the
discussion
>> urgent.
>>
>> The DLD is good, but I have a question at the HLD level.
>>
>> Without list heads for each skip list in the resource, I believe you 
>> will often be walking enormous amounts of locks still to find the 
>> beginning of the skip list.  Please explain why this is not the case OR
>> introduce list heads in the resource.  If you have a good answer,
please
>> implement.
>>     
>
> First of all, you need to know what lock modes you conflict with.
> Then you check every group of locks if it conflicts to your lock modes.
> If not, you skip the whole group. This is done as the following:
> 1) Locks are kept in the granted list and are grouped by modes.
> 2) the first lock of every group has the mode skip list initialized.
>   
> 3) you have a look at the first lock in the list and if its mode does
> not conflict, you skip all the group.
> 4) you do (3) for the first lock of every group as well.
>   
Hmm - there are two things that are not explained here:

1.  Clearly a simple algorithm can give you a list of lock modes that you
conflict with.  For lock enqueues you now need to check the list and send
blocking callbacks.

Key question (asked several times now, no answer yet!!!): how is the first
element of a group that conflicts with the enqueue found without  walking  over
all locks until that list head is "found"? 

I assume that the locks of a particular mode are in a list called a skiplist,
but how to find the beginning of that list isn''t well documented, and
should be
done preferably by embedding a list head for that skip list in the resource OR
by having pointers point to the first element.  Neither are stated or documented
in the DLD.

2. When other lock changes occur, such as a cancellation or conversion,
different scan needs to happen to send completion callbacks. Please explain.

- Peter -
> When you see the current group conflicts by mode you need to
> check every lock of this group if it conflicts by policy. However,
> if you know it does not, you would like to skip all the locks with
> the same policy. This is done as the following:
> 1) Locks are kept in the granted list, grouped by modes and
> sub-grouped by policy.
> 2) the first lock of every sub-group has the policy skip list initialized.
> 3) you have a look at the first lock in the sub-group and if its policy
> does not conflict, you skip all the sub-group with the same policy.
>
>

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 #9136|review?(vitaly@clusterfs.com|review-
               Flag|)                           |


(From update of attachment 9136)
some defects are found and to be fixed.

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 #9136 is|0                           |1
           obsolete|                            |


Created an attachment (id=9178)
Please don''t reply to lustre-devel. Instead, comment in Bugzilla by
using the following link:
 --> (https://bugzilla.lustre.org/attachment.cgi?id=9178&action=view)
DLD for plain/inodebits locks improvement (revision 4)

fix according to Vitaly''s suggestion & review result.

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 #9178|                            |review?(vitaly@clusterfs.com
               Flag|                            |)

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 #9178|review?(vitaly@clusterfs.com|review-
               Flag|)                           |


(From update of attachment 9178)
I see only spelling mistakes fixed. nothing else.

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 #9178 is|0                           |1
           obsolete|                            |


Created an attachment (id=9201)
Please don''t reply to lustre-devel. Instead, comment in Bugzilla by
using the following link:
 --> (https://bugzilla.lustre.org/attachment.cgi?id=9201&action=view)
DLD for plain/inodebits locks improvement (revision 5)

new DLD
+ handle of inserting an inodebits lock at the head of a mode group when it
belongs to the mode group while no same policy group is found in the mode
group.
+ other fixes.

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 #9201 is|0                           |1
           obsolete|                            |


Created an attachment (id=9206)
Please don''t reply to lustre-devel. Instead, comment in Bugzilla by
using the following link:
 --> (https://bugzilla.lustre.org/attachment.cgi?id=9206&action=view)
DLD for plain/inodebits locks improvement (revision 6)

* get rid of goto labels
* fix some descriptions
* change macro with too generic names
* fix some psudocode bugs

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



Created an attachment (id=9222)
Please don''t reply to lustre-devel. Instead, comment in Bugzilla by
using the following link:
 --> (https://bugzilla.lustre.org/attachment.cgi?id=9222&action=view)
code

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?(green@clusterfs.com)|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.

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



Created an attachment (id=9231)
Please don''t reply to lustre-devel. Instead, comment in Bugzilla by
using the following link:
 --> (https://bugzilla.lustre.org/attachment.cgi?id=9231&action=view)
Quick enhancement to current lock matching code

This patch is a simple speedup of lock comparison code.
While nowhere as sophisticated as bug 11300 or skiplists code here (developed
for b1_5 currently), this code reduces n^2/2 comparisons required to grant N
identical compatible locks to only N comparisons (if there was nothing
granted).
It achieves such a task by assuming that if we have same mode lock already
granted with policy that is a superset of requested lock policy, then there are

no conflicting locks to requested lock in a granted queue and so we can return
success immediatelly.

Situations it should help with:
many clients do ls on a same dir/file. (read on a same file)
Situations that it has no effect on:
After above many clients ls on same dir/file, touch dir/file1 (no easy
equivalent for extent locks)

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



(In reply to comment #90)
> (From update of attachment 9222)
> >Index: include/lustre_dlm.h
> >==================================================================
> >+        struct list_head     *l_res_listhead;   // inserted list
> 
> This l_res_listhead is not in DLD, and the purpose is totally unclean.
> > ldlm_inodebits_compat_queue(struct list_head *queue, struct ldlm_lock
*req,
> >                             struct list_head *work_list)
> > {
> >+
> >+        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?
Just found this issue during code phase, so it''s not in DLD. The issue
is that
this code is called not only for granted list, it will be called as well for
waiting and converting list, in this way, it will be determined fast whether the
lock to be checked is or not in the list. (For current code, it will travers all
locks on the list to check whether the lock is already in the list, while in
to-be-optimized granted list, this way is impossible because we need jump over
locks of the compatible requst mode/same bits from time to time).
> 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.
Thank you for the hint. But in this situation here, we don''t know what
the list is.
> 
> >         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. 
IMO it is replaced by the code above, the code above makes the judgement faster.
> Also if you think some code is unneeded, just remove it.
ok
> 
> >@@ -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.
>       
because in the current code of ldlm_lock_convert(lock, newmode, flag), it will
firstly unlink the lock from the granted list, on ther server, after some policy
processing, it will call ldlm_resource_add_lock() to add it back. That means,
there are possible two ways for a lock to be added in the granted list, one
through ldlm_grant_lock(), the other is through ldlm_resource_add_lock() as
ldlm_lock_convert() does. So in "ldlm_resource_add_lock(res, head,
lock)" where
"head" here sometimes is not 1 of the 3 list_heads of a resource.

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



(From update of attachment 9231)
I think it is good to have this but I''d like to retain the original
plan also.

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



(In reply to comment #95)
> I think it is good to have this but I''d like to retain the
original plan also.
Sure. I do not propose to drop original plan at all, it is just a low-hanging
fruit that we can get immediatelly.

Bobi, Huangwei: Please make sure you have a similar chek in your code/design (I
think current Bobi''s code will iterate through all skiplists, for
example).

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 is|0                           |1
           obsolete|                            |


Created an attachment (id=9242)
Please don''t reply to lustre-devel. Instead, comment in Bugzilla by
using the following link:
 --> (https://bugzilla.lustre.org/attachment.cgi?id=9242&action=view)
code for PLAIN/IBITS granted list improvement (revision 2)

* based on Oleg''s code review result
+ Oleg''s enhancement code (Attachment #9231)

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 #9242|review?(green@clusterfs.com)|review-
               Flag|                            |


(From update of attachment 9242)
> 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;
>         int compat = 1;
>+        int granted_list = -1;
Minor comment, probably easiest of all would be to not assign anything to
granted list and have this construction instead here:
if (!list_empty(queue)) {
lock = list_entry(queue->next, ...);
granted_list = (lock->reqmode == lock->grantedmode) ? 1:0;
}
(hm, this will probably generate a warning about possible uninitialised usage
of granted_list, so you
might initially init it to 0, or have an else part that will do RETURN(0)
immediatelly).
This way we can save one comparison for every loop iteration.

Same in plain_compat function.

Also may be there was some sort of miscommunication between us. I did not ask
you to remove ldlm_lock_convert() changes.
What you need to do there is when you remove lock from granted queue, you need
to remember position where it was, and on a branch where the lock is reinserted
to granted queue again (and only there), you need to insert the lock back at
that position (one of your functions allows this). Of course you only need to
do this for plain and inodebits locks only.
And please think up a plan on how this specific codepath can be tested.

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 #9242 is|0                           |1
           obsolete|                            |


Created an attachment (id=9248)
Please don''t reply to lustre-devel. Instead, comment in Bugzilla by
using the following link:
 --> (https://bugzilla.lustre.org/attachment.cgi?id=9248&action=view)
code for PLAIN/IBITS granted list improvement (revision 3)

* follow Oleg''s suggestion. (condition judget ment out of loop, handle
ldlm_lock_convert)

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 #9248|review?(vitaly@clusterfs.com|review-
               Flag|)                           |


(From update of attachment 9248)
1. Oleg, your optimization above does not look very helpful.
Once a lock with a compatible mode occurs, we skip all the
locks of this mode by mode skip list -- locks are grouped by
mode. As we do not walk through locks with compatible modes,
your optimization works for the very first lock in every compatible
mode group only. I do not think we need an optimization that
works for 1 lock per mode group of locks.

2. we should probably check in search_grant_lock() if @lock is
not plain nor inodebits type, and LBUG if so.

3. when we call ldlm_granted_list_add_lock(), we do not call 
ldlm_resource_add_lock() anymore. Thus ldlm_granted_list_add_lock()
should print the same debug info as ldlm_resource_add_lock() does.
It also should handle destroyed locks in the same way.

4. ldlm_lock_convert()
if the lock is in the middle of the mode and policy groups,
@mark_lock will be NULL and the lock will be inserted later
to the end of the granted list in ldlm_granted_list_add_lock(),
what is wrong.

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 #9248 is|0                           |1
           obsolete|                            |


Created an attachment (id=9273)
Please don''t reply to lustre-devel. Instead, comment in Bugzilla by
using the following link:
 --> (https://bugzilla.lustre.org/attachment.cgi?id=9273&action=view)
code for PLAIN/IBITS granted list improvement (revision 4)
> 1. Oleg, your optimization above does not look very helpful.
> Once a lock with a compatible mode occurs, we skip all the
> locks of this mode by mode skip list -- locks are grouped by
> mode. As we do not walk through locks with compatible modes,
> your optimization works for the very first lock in every compatible
> mode group only. I do not think we need an optimization that
> works for 1 lock per mode group of locks.
well, i didn''t change it in this patch, need Oleg''s feedback
of your opinion.
> 2. we should probably check in search_grant_lock() if @lock is
> not plain nor inodebits type, and LBUG if so.
included in this patch.
> 3. when we call ldlm_granted_list_add_lock(), we do not call 
> ldlm_resource_add_lock() anymore. Thus ldlm_granted_list_add_lock()
> should print the same debug info as ldlm_resource_add_lock() does.
> It also should handle destroyed locks in the same way.
included in this patch.
> 4. ldlm_lock_convert()
> if the lock is in the middle of the mode and policy groups,
> @mark_lock will be NULL and the lock will be inserted later
> to the end of the granted list in ldlm_granted_list_add_lock(),
> what is wrong.
when lock is in the middle of the mode/policy group, code in lines(1552-1554)
of
ldlm_lock.c will be hit

1548	if ((join & LDLM_MODE_JOIN_LEFT) ||
1549			(join & LDLM_POLICY_JOIN_LEFT))
1550			    mark_lock = list_entry(lock->l_res_link.prev,
1551						      struct ldlm_lock,
l_res_link);
1552		   else if (lock->l_res_link.next != &res->lr_granted)
1553			   mark_lock = list_entry(lock->l_res_link.next,
1554						   struct ldlm_lock,
l_res_link);

so the mark_lock will not be NULL in this case.

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



(In reply to comment #106)
> > 1. Oleg, your optimization above does not look very helpful.
> > Once a lock with a compatible mode occurs, we skip all the
> > locks of this mode by mode skip list -- locks are grouped by
> > mode. As we do not walk through locks with compatible modes,
> > your optimization works for the very first lock in every compatible
> > mode group only. I do not think we need an optimization that
> > works for 1 lock per mode group of locks.
> well, i didn''t change it in this patch, need Oleg''s
feedback of your opinion.
This is in fact quite right suggestion. I somehow thought that we iterate
through every group, but since we skips all compatible mode locks, this makes no
sense to adopt of course.

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



(From update of attachment 9273)
please remove that recently added code with my optimization. it was really
meant for b1_4 without this patch.

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 #9273 is|0                           |1
           obsolete|                            |


Created an attachment (id=9306)
Please don''t reply to lustre-devel. Instead, comment in Bugzilla by
using the following link:
 --> (https://bugzilla.lustre.org/attachment.cgi?id=9306&action=view)
code for PLAIN/IBITS granted list improvement (revision 5)

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



Comment Tags: Progress Update

passed the acceptance-small.sh test.