green@clusterfs.com
2006-Dec-18 15:56 UTC
[Lustre-devel] [Bug 11013] implement hash table for client UUID/NID export lookups
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?(green@clusterfs.com)|review?(adilger@clusterfs.co
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.
Eric Barton
2006-Dec-18 16:29 UTC
[Lustre-devel] [Bug 11013] implement hash table for client UUID/NIDexport lookups
Oleg,
Is 11394 relevent?
Cheers,
Eric
Oleg Drokin
2006-Dec-19 05:58 UTC
[Lustre-devel] [Bug 11013] implement hash table for client UUID/NIDexport lookups
Hello! On Mon, Dec 18, 2006 at 11:29:44PM -0000, Eric Barton wrote:> Is 11394 relevent?No, I do not think so. Bye, Oleg
yzy@clusterfs.com
2006-Dec-27 02:28 UTC
[Lustre-devel] [Bug 11013] implement hash table for client UUID/NID export lookups
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 Created an attachment (id=9224) Please don''t reply to lustre-devel. Instead, comment in Bugzilla by using the following link: --> (https://bugzilla.lustre.org/attachment.cgi?id=9224&action=view) code the inital code for the bug.
yzy@clusterfs.com
2006-Dec-27 02:32 UTC
[Lustre-devel] [Bug 11013] implement hash table for client UUID/NID export lookups
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 #9224| |review?(bobijam@clusterfs.co
Flag| |m)
(From update of attachment 9224)
Please review the code.
thanks
yzy@clusterfs.com
2006-Dec-27 02:33 UTC
[Lustre-devel] [Bug 11013] implement hash table for client UUID/NID export lookups
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 #9224| |review?(jxiong@clusterfs.com
Flag| |)
(From update of attachment 9224)
Please review the code.
thanks
yzy@clusterfs.com
2006-Dec-30 20:25 UTC
[Lustre-devel] [Bug 11013] implement hash table for client UUID/NID export lookups
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 #9224 is|0 |1
obsolete| |
Created an attachment (id=9249)
Please don''t reply to lustre-devel. Instead, comment in Bugzilla by
using the following link:
--> (https://bugzilla.lustre.org/attachment.cgi?id=9249&action=view)
new code
imporve some minor bug per bobijam''s review result.
yzy@clusterfs.com
2006-Dec-30 20:28 UTC
[Lustre-devel] [Bug 11013] implement hash table for client UUID/NID export lookups
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 #9249| |review?(bobijam@clusterfs.co
Flag| |m)
(From update of attachment 9249)
please reivew the new patch.
thanks
yzy@clusterfs.com
2007-Jan-10 23:48 UTC
[Lustre-devel] [Bug 11013] implement hash table for client UUID/NID export lookups
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
----------------------------------------------------------------------------
Comment|(From update of attachment |(From update of attachment
|9224) |9224)
|>diff -Nupr |>diff -Nupr
|lustre.orig/lustre/include/c|lustre.orig/lustre/include/c
|lass_hash.h |lass_hash.h
|lustre/lustre/include/class_|lustre/lustre/include/class_
|hash.h |hash.h
|>--- |>---
|lustre.orig/lustre/include/c|lustre.orig/lustre/include/c
|lass_hash.h 1970-01-01 |lass_hash.h 1970-01-01
|08:00:00.000000000 +0800 |08:00:00.000000000 +0800
|>+++ |>+++
|lustre/lustre/include/class_|
|hash.h 2006-12-27 |
Comment|17:38:14.000000000 +0800 |lustre/lustre/include/class_
|>@@ -0,0 +1 |hash.h 2006-12-27
| |17:38:14.000000000 +0800
| |>@@ -0,0 +1
Comment|130 @@ |130 @@
|>+/* -*- mode: c; c-basic- |>+/* -*- mode: c;
c-basic-
|offset: 8; indent-tabs-mode:|offset: 8; indent-tabs-mode:
|nil; -*- |nil; -*-
|>+ * |>+ *
|vim:expandtab:shiftwidth=8:t|vim:expandtab:shiftwidth=8:t
|abstop=8: |abstop=8:
|>+ */ |>+ */
|>+ |>+
|>+#ifndef __CLASS_HASH_H |>+#ifndef __CLASS_HASH_H
|>+#define __CLASS_HASH_H |>+#define __CLASS_HASH_H
|>+ |>+
|>+#include <lustre_lib.h> |>+#include
<lustre_lib.h>
|>+ |>+
|>+#ifndef __KERNEL__ |>+#ifndef __KERNEL__
|>+#define |>+#define
Comment|hlist_add_head_rcu |hlist_add_head_rcu
|hlist_add_head |hlist_add_head
|>+#define hlist_for_each_rcu|>+#define
hlist_for_each_rcu
|hlist_for_each |hlist_for_each
|in Linux kernel 2.6.18, it |in Linux kernel 2.6.18, it
|seems that there is no more |seems that there is no more
|hlist_for_each_rcu |hlist_for_each_rcu
Comment|only |only
|has hlist_for_each_entry_rcu|has hlist_for_each_entry_rcu
|>+#define rcu_read_lock() |>+#define rcu_read_lock()
|>+#define rcu_read_unlock() |>+#define
rcu_read_unlock()
|>+#endif |>+#endif
|>+ |>+
|<snip>... |<snip>...
|>diff -Nupr |>diff -Nupr
|lustre.orig/lustre/include/o|lustre.orig/lustre/include/o
|bd.h |bd.h
|lustre/lustre/include/obd.h |lustre/lustre/include/obd.h
|>--- |>---
|lustre.orig/lustre/include/o|lustre.orig/lustre/include/o
|bd.h 2006-12-19 |bd.h 2006-12-19
Comment|08:28:43.000000000 +0800 |08:28:43.000000000 +0800
|>+++ |>+++
|lustre/lustre/include/obd.h |lustre/lustre/include/obd.h
|2006-12-27 |2006-12-27
|17:38:14.000000000 +0800 |17:38:14.000000000 +0800
|>@@ -31,6 +31 |>@@ -31,6 +31
Comment|7 @@ |7 @@
|> #include |> #include
|<lustre/lustre_idl.h>
|<lustre/lustre_idl.h>
|> #include <lustre_export.h>|> #include
<lustre_export.h>
|> #include <lustre_quota.h> |> #include
<lustre_quota.h>
|>+#include <class_hash.h> |>+#include
<class_hash.h>
|> |>
|> #define MAX_OBD_DEVICES |> #define MAX_OBD_DEVICES
|8192 |8192
|> |>
|>@@ -729,6 +730,10 @@ struct|>@@ -729,6 +730,10 @@
struct
|obd_device { |obd_device {
|> |>
|obd_fail:1 |obd_fail:1
Comment|/* cleanup with failover */ |/* cleanup with failover */
|> |>
|obd_async_recov:1; /* |obd_async_recov:1; /*
|allow asyncronous orphan |allow asyncronous orphan
|cleanup */ |cleanup */
|> atomic_t |> atomic_t
|obd_refcount; |obd_refcount;
|>+ /* uuid-export |>+ /* uuid-export
|hash body */ |hash body */
|>+ struct |>+ struct
|lustre_class_hash_body |lustre_class_hash_body
|*obd_uuid_hash_body; |*obd_uuid_hash_body;
|>+ |>+
Comment|/* uuid-export hash body */ |/* uuid-export hash body */
|wrong comment. |wrong comment.
|>+ struct |>+ struct
|lustre_class_hash_body |lustre_class_hash_body
|*obd_nid_hash_body; |*obd_nid_hash_body;
|> cfs_waitq_t |> cfs_waitq_t
|obd_refcount_waitq; |obd_refcount_waitq;
|> struct list_head |> struct
list_head
|obd_exports; |obd_exports;
|> int |> int
Comment|obd_num_exports; |obd_num_exports;
|<snip>... |<snip>...
|>diff -Nupr |>diff -Nupr
|lustre.orig/lustre/obdclass/|lustre.orig/lustre/obdclass/
|class_hash.c |class_hash.c
|lustre/lustre/obdclass/class|lustre/lustre/obdclass/class
|_hash.c |_hash.c
|>--- |>---
|lustre.orig/lustre/obdclass/|lustre.orig/lustre/obdclass/
|class_hash.c 1970-01-01 |class_hash.c 1970-01-01
|08:00:00.000000000 +0800 |08:00:00.000000000 +0800
|>+++ |>+++
Comment|lustre/lustre/obdclass/class|lustre/lustre/obdclass/class
|_hash.c 2006-12-27 |_hash.c 2006-12-27
|17:38:14.000000000 +0800 |17:38:14.000000000 +0800
|<snip>... |<snip>...
|>+ |>+
|>+/* |>+/*
|>+ * only allow unique @key |>+ * only allow unique
@key
|in hashtables, if the same |in hashtables, if the same
|@key has existed in |@key has existed in
|hashtables |hashtables
Comment|>+ * it will return with |>+ * it will return with
|fails. |fails.
|>+ */ |>+ */
|>+int |>+int
|lustre_hash_additem_unique(s|lustre_hash_additem_unique(s
|truct lustre_class_hash_body|truct lustre_class_hash_body
|*hash_body, |*hash_body,
|>+ |>+
|void *key |void *key
Comment|struct hlist_node |struct hlist_node
|*actual_hnode) |*actual_hnode)
|>+{ |>+{
|>+ int hashent; |>+ int hashent;
|>+ struct |>+ struct
|lustre_hash_bucket *bucket =|lustre_hash_bucket *bucket
|NULL; |NULL;
|>+ struct |>+ struct
|lustre_hash_operations *hop |lustre_hash_operations *hop
|= hash_body- |= hash_body-
|>lchb_hash_operations; |>lchb_hash_operations;
|>+ ENTRY; |>+ ENTRY;
|>+ |>+
|>+ hashent = |>+ hashent
Comment|hop- |hop-
|>lustre_hashfn(hash_body, |>lustre_hashfn(hash_body,
|key); |key);
|>+ |>+
|>+ if ( hashent == -1|>+ if ( hashent ==
-1
|) { |) {
|>+ |>+
|CERROR("Cannot get correct |CERROR("Cannot get
correct
|hash value , hashname = %s |hash value , hashname = %s
|\n" |\n"
Comment|>+ |>+
|hash_body->hashname); |hash_body->hashname);
|>+ RETURN(- |>+
RETURN(-
|EINVAL); |EINVAL);
|>+ } |>+ }
|>+ |>+
|>+ /* get the hash- |>+ /* get the
hash-
|bucket and lock it */ |bucket and lock it */
|>+ bucket = |>+ bucket
|&hash_body- |&hash_body-
|>lchb_hash_tables[hashent];
|>lchb_hash_tables[hashent];
|>+ |>+
Comment|spin_lock(&bucket- |spin_lock(&bucket-
|>lhb_lock); |>lhb_lock);
|>+ |>+
|>+ if ( |>+ if (
|(lustre_hash_getitem_in_buck|(lustre_hash_getitem_in_buck
|et_nolock(hash_body, |et_nolock(hash_body,
|hashent, key)) != NULL) { |hashent, key))
|>+ /* the |!= NULL) {
|added-item exist in |>+ /* the
|hashtables |added-item exist in
| |hashtables
Comment|so cannot add it again */ |so cannot add it again */
|>+ |>+
|spin_unlock(&bucket- |spin_unlock(&bucket-
|>lhb_lock); |>lhb_lock);
|>+ |>+
|>+ |>+
|CWARN("Object exist\n"); |CWARN("Object
exist\n");
|>+ RETURN(- |>+
RETURN(-
|EALREADY); |EALREADY);
|>+ } |>+ }
|>+ |>+
|>+ |>+
|hlist_add_head(actual_hnode |hlist_add_head(actual_hnode
Comment|&(bucket->lhb_head));
|&(bucket->lhb_head));
|should it be |should it be
|"hlist_add_head_rcu" ?
|"hlist_add_head_rcu" ?
|>+ |>+
|>+#ifdef LUSTRE_HASH_DEBUG |>+#ifdef
LUSTRE_HASH_DEBUG
|>+ /* hash distribute|>+ /* hash
distribute
|debug */ |debug */
|>+ hash_body- |>+ hash_body-
|>lchb_hash_tables[hashent]-
|>lchb_hash_tables[hashent]-
|>lhb_item_count++; |>lhb_item_count++;
|>+ CDEBUG(D_INFO |>+ CDEBUG(D_INFO
Comment|"hashname[%s] bucket[%d] has|"hashname[%s]
bucket[%d] has
|[%d] hashitem\n", |[%d] hashitem\n",
|>+ |>+
|hash_body->hashname, hashent|hash_body->hashname,
hashent
Comment|>+ |>+
|hash_body- |hash_body-
|>lchb_hash_tables[hashent]-
|>lchb_hash_tables[hashent]-
|>lhb_item_count); |>lhb_item_count);
|>+#endif |>+#endif
|>+ hop- |>+ hop-
|>lustre_hash_object_refcount|>lustre_hash_object_refcount
|_get(actual_hnode); |_get(actual_hnode);
|>+ |>+
|>+ |>+
|spin_unlock(&bucket- |spin_unlock(&bucket-
|>lhb_lock); |>lhb_lock);
|>+ |>+
|>+ |>+
Comment|RETURN(0); |RETURN(0);
|>+} |>+}
|>+EXPORT_SYMBOL(lustre_hash_|>+EXPORT_SYMBOL(lustre_hash_
|additem_unique); |additem_unique);
|>+ |>+
|>+/* |>+/*
|>+ * this version of |>+ * this version of
|additem, it allow multi same|additem, it allow multi same
|@key <key, value> in |@key <key, value>
in
|hashtables. * in this |hashtables. * in this
|additem version, we don''t |additem version, we
don''t
|need to check if exist same |need to check if exist same
|@key in hash |@key
|>+ * tables |in hash
| |>+ * tables
2007-1-11 Status Update : imporved code per code review result, do RCU object
free improve now.
yzy@clusterfs.com
2007-Jan-16 23:41 UTC
[Lustre-devel] [Bug 11013] implement hash table for client UUID/NID export lookups
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 Created an attachment (id=9352) Please don''t reply to lustre-devel. Instead, comment in Bugzilla by using the following link: --> (https://bugzilla.lustre.org/attachment.cgi?id=9352&action=view) new patch upload new patch per jxiong''s suggestion, doesn''t include RCU''s improving.
yzy@clusterfs.com
2007-Jan-18 02:06 UTC
[Lustre-devel] [Bug 11013] implement hash table for client UUID/NID export lookups
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 #9352 is|0 |1
obsolete| |
Created an attachment (id=9367)
Please don''t reply to lustre-devel. Instead, comment in Bugzilla by
using the following link:
--> (https://bugzilla.lustre.org/attachment.cgi?id=9367&action=view)
new patch
new patch
yzy@clusterfs.com
2007-Jan-18 02:06 UTC
[Lustre-devel] [Bug 11013] implement hash table for client UUID/NID export lookups
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 #9367| |review?(jxiong@clusterfs.com
Flag| |)
(From update of attachment 9367)
Please review the patch.
thanks
jxiong@clusterfs.com
2007-Jan-18 06:06 UTC
[Lustre-devel] [Bug 11013] implement hash table for client UUID/NID export lookups
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 #9367|review?(jxiong@clusterfs.com|review+
Flag|) |
(From update of attachment 9367)
looks well. please fix the above issue i just mentioned.