Please don't reply to lustre-devel. Instead, comment in Bugzilla by using the
following link:
https://bugzilla.lustre.org/show_bug.cgi?id=11013
What |Removed |Added
----------------------------------------------------------------------------
Attachment #9138|review?([EMAIL PROTECTED])|review?([EMAIL PROTECTED]
Flag| |m), review-
(From update of attachment 9138)
This DLD looks a lot closer to completion now. Number of issues have shrinked
dramatically.
I am adding Andreas to also take a look and give any comments he might have.
Comments:
lustre_hash_delitem_by_key calls lustre_hash_delitem, and both of them
calculate
hash value to find hash entry. this is probably suboptimal.
Also there is a problem with lustre_hash_delitem_by_key, it gets the list entry
and passes it to lustre_hash_delitem without any sort of lock. What protects
us from that entry being removed from the list by parallel thread?
Perhaps it needs to hold hash bucket's lock for the whole time
(And you need lockless versions of hash_del_item and getitem_in_bucket.
In fact, from what I see getitem_in_bucket should not take the bucket lock
by itself, it's always caller that needs to hold the lock, otherwise by
the time you return entry might be invalid already).
List entries refcounting:
I think you need to get object reference when adding to hash, and release it
after removing (probably you would need another method in the hash table for
this). Otherwise what protects us from situation of somebody freeing last
reference on the object without removing the object from hash?
In uuid_hashfn:
what is the point of checking if (key != NULL) and if (actual_hnode != NULL)?
You have verified by assertion above that this cannot happen.
And if it would happen - you use uninitialised variables which is also bad.
_______________________________________________
Lustre-devel mailing list
[email protected]
https://mail.clusterfs.com/mailman/listinfo/lustre-devel
