<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=Windows-1252">
</head>
<body>
<br>
Thanks much, both of you. I’ll take a look.<br>
<br>
<hr style="display:inline-block;width:98%" tabindex="-1">
<div id="divRplyFwdMsg" dir="ltr"><font face="Calibri, sans-serif" style="font-size:11pt" color="#000000"><b>From:</b> James Simmons <jsimmons@infradead.org><br>
<b>Sent:</b> Wednesday, November 7, 2018 1:24:27 PM<br>
<b>To:</b> NeilBrown<br>
<b>Cc:</b> Patrick Farrell; Lustre Developement; James Simmons; sihara@whamcloud.com<br>
<b>Subject:</b> Re: Apparent bug in LU-8130 ptlrpc: convert conn_hash to rhashtable / Linux-commit: ac2370ac2bc5215daf78546cd8d925510065bb7f</font>
<div> </div>
</div>
<div class="BodyFragment"><font size="2"><span style="font-size:11pt;">
<div class="PlainText"><br>
> > Neil, James,<br>
> ><br>
> > Sent this earlier, but with nasty formatting that got it rejected. Whoops.<br>
> > <br>
> > It looks like the patch we landed as:<br>
> > LU-8130 ptlrpc: convert conn_hash to rhashtable<br>
> > <br>
> > Linux has a resizeable hashtable implementation in lib,<br>
> > so we should use that instead of having one in libcfs.<br>
> > <br>
> > This patch converts the ptlrpc conn_hash to use rhashtable.<br>
> > In the process we gain lockless lookup.<br>
> > <br>
> > As connections are never deleted until the hash table is destroyed,<br>
> > there is no need to count the reference in the hash table. There<br>
> > is also no need to enable automatic_shrinking.<br>
> > <br>
> > Linux-commit: ac2370ac2bc5215daf78546cd8d925510065bb7f<br>
> > <br>
> > Introduced a bug. Ihara-san opened something to track it here:<br>
> > <a href="https://jira.whamcloud.com/browse/LU-11624">https://jira.whamcloud.com/browse/LU-11624</a><br>
> > <br>
> > It’s a null pointer in nid_hash(); there are some more details at that link.<br>
> <br>
> That commit changed ptlrpc_connection_get() so that it can return NULL.<br>
> <br>
> This is actually one of the really annoying things about rhashtable. I<br>
> cannot convince the maintainers that sometimes you need guaranteed<br>
> success (which is quite possible with a hash table). They come from the<br>
> networking community were they drop packets on errors all the time - no<br>
> big deal.<br>
> <br>
> Anyway, all users of ptlrpc_connection_get() in the client code (in<br>
> drivers/staging) test the return value against NULL, so this was safe.<br>
> <br>
> In SFS lustre, the call in target_handle_connect() assigned the<br>
> result to export->exp_connection without checking against NULL.<br>
> That leads to the oops.<br>
> <br>
> ptlrpc_connection_get() will return NULL when<br>
> rhashtable_lookup_get_insert_fast() returns an error.<br>
> The errors are mostly weird internal implementation details leaking out<br>
> the API. A retry, possibly after a short delay, will usually succeed.<br>
> (see <a href="https://lwn.net/Articles/751974/">https://lwn.net/Articles/751974/</a>, section "Failure during insertion)<br>
> If this happens a lot, it might suggest that the hash function is poor.<br>
> <br>
> Ahhhh... this was built against a 3.10 kernel.<br>
> Prior to 4.6, hash_64() was not so much "poor" as "appalling".<br>
> See <a href="https://lwn.net/Articles/687494/">https://lwn.net/Articles/687494/</a><br>
> <br>
> For SFS we should avoid using hash_64() (at least before 4.6) and<br>
> instead use something like<br>
> hash_32((foo>>32) ^ hash_32(foo & 0xFFFFFFFF))<br>
<br>
The libcfs hash is not much better :-( <br>
<br>
I have a purposed fix. Let me know if its correct. Thank you.<br>
<br>
>From 32df018c1af8fdbc833c537aafeea081ab3d8d7e Mon Sep 17 00:00:00 2001<br>
From: James Simmons <uja.ornl@yahoo.com><br>
Date: Wed, 7 Nov 2018 14:06:37 -0500<br>
Subject: [PATCH] LU-11624 ptlrpc: handle no ptlrpc connection case<br>
<br>
With the move of ptlrpc connections hash to rhashtable it is now<br>
possible for ptlrpc_connection_get() to return NULL. The reason<br>
this can happen is due to conditions. One is that the hash has no<br>
more free slots which results in an E2BIG error. By default the<br>
maximum size for rhashtables is 2^31 so this is very unlikely<br>
going to happen. The second case returns a -ENOMEM which can<br>
happen when the bucket can be resized. For this case test the<br>
return value of rhashtable_lookup_get_insert_fast() and if it is<br>
-ENOMEM try again inserting the new connection into the hash<br>
table. Just as an extra level of protection against a failed<br>
connection attempt in target_handle_connect() exit out with<br>
-ENOTCONN.<br>
<br>
Change-Id: I0537564ba544ed06be42ba243606a884a1290f20<br>
Signed-off-by: James Simmons <uja.ornl@yahoo.com><br>
---<br>
lustre/ldlm/ldlm_lib.c | 3 +++<br>
lustre/ptlrpc/connection.c | 6 +++++-<br>
2 files changed, 8 insertions(+), 1 deletion(-)<br>
<br>
diff --git a/lustre/ldlm/ldlm_lib.c b/lustre/ldlm/ldlm_lib.c<br>
index de66aa3..463c6c6 100644<br>
--- a/lustre/ldlm/ldlm_lib.c<br>
+++ b/lustre/ldlm/ldlm_lib.c<br>
@@ -1349,6 +1349,9 @@ dont_check_exports:<br>
export->exp_connection = ptlrpc_connection_get(req->rq_peer,<br>
req->rq_self,<br>
&cluuid);<br>
+ if (!export->exp_connection)<br>
+ GOTO(out, rc = -ENOTCONN);<br>
+<br>
if (hlist_unhashed(&export->exp_nid_hash))<br>
cfs_hash_add(export->exp_obd->obd_nid_hash,<br>
&export->exp_connection->c_peer.nid,<br>
diff --git a/lustre/ptlrpc/connection.c b/lustre/ptlrpc/connection.c<br>
index 21a24a2..045b3ee 100644<br>
--- a/lustre/ptlrpc/connection.c<br>
+++ b/lustre/ptlrpc/connection.c<br>
@@ -103,13 +103,17 @@ ptlrpc_connection_get(struct lnet_process_id peer, lnet_nid_t self,<br>
* connection. The object which exists in the hash will be<br>
* returned,otherwise NULL is returned on success.<br>
*/<br>
+try_again:<br>
conn2 = rhashtable_lookup_get_insert_fast(&conn_hash, &conn->c_hash,<br>
conn_hash_params);<br>
if (conn2) {<br>
/* insertion failed */<br>
OBD_FREE_PTR(conn);<br>
- if (IS_ERR(conn2))<br>
+ if (IS_ERR(conn2)) {<br>
+ if (PTR_ERR(conn2) == -ENOMEM)<br>
+ goto try_again;<br>
return NULL;<br>
+ }<br>
conn = conn2;<br>
ptlrpc_connection_addref(conn);<br>
}<br>
-- <br>
1.8.3.1<br>
</div>
</span></font></div>
</body>
</html>