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.
_______________________________________________
Lustre-devel mailing list
[email protected]
https://mail.clusterfs.com/mailman/listinfo/lustre-devel
