<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=us-ascii">
</head>
<body>
<br>
Neil,<br>
<br>
So the semantics and behavior for a single bit bit lock and a mutex are the same?  All the scheduler stuff, optimistic spin, etc?  It may not matter here (probably not) but it makes me curious...<br>
<br>
- Patrick<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> lustre-devel <lustre-devel-bounces@lists.lustre.org> on behalf of NeilBrown <neilb@suse.com><br>
<b>Sent:</b> Sunday, December 9, 2018 6:46:16 PM<br>
<b>To:</b> James Simmons; Oleg Drokin; Andreas Dilger<br>
<b>Cc:</b> Lustre Development List<br>
<b>Subject:</b> [lustre-devel] [PATCH 3/4] lustre: use bit-locking in echo_client.</font>
<div> </div>
</div>
<div class="BodyFragment"><font size="2"><span style="font-size:11pt;">
<div class="PlainText">The ep_lock used by echo client causes lockdep to complain.<br>
Multiple locks of the same class are taken concurrently which<br>
appear to lockdep to be prone to deadlocking, and can fill up<br>
lockdep's fixed size stack for locks.<br>
<br>
Ass ep_lock is taken on multiple pages always in ascending page order,<br>
deadlocks don't happen, so this is a false-positive.<br>
<br>
The function of the ep_lock is the same as thats for page_lock(),<br>
which is implemented as a bit-lock using wait_on_bit().  lockdep<br>
cannot see these locks, and doesn't really need to.<br>
<br>
So convert ep_lock to a simple bit-lock using wait_on_bit for<br>
waiting.  This provides similar functionality, matches how page_lock()<br>
works, and avoids lockdep problems.<br>
<br>
Signed-off-by: NeilBrown <neilb@suse.com><br>
---<br>
 .../staging/lustre/lustre/obdecho/echo_client.c    |   29 +++++++++++++-------<br>
 1 file changed, 19 insertions(+), 10 deletions(-)<br>
<br>
diff --git a/drivers/staging/lustre/lustre/obdecho/echo_client.c b/drivers/staging/lustre/lustre/obdecho/echo_client.c<br>
index 1ddb4a6dd8f3..887df7ce6b5c 100644<br>
--- a/drivers/staging/lustre/lustre/obdecho/echo_client.c<br>
+++ b/drivers/staging/lustre/lustre/obdecho/echo_client.c<br>
@@ -78,7 +78,7 @@ struct echo_object_conf {<br>
 <br>
 struct echo_page {<br>
         struct cl_page_slice   ep_cl;<br>
-       struct mutex            ep_lock;<br>
+       unsigned long           ep_lock;<br>
 };<br>
 <br>
 struct echo_lock {<br>
@@ -217,10 +217,13 @@ static int echo_page_own(const struct lu_env *env,<br>
 {<br>
         struct echo_page *ep = cl2echo_page(slice);<br>
 <br>
-       if (!nonblock)<br>
-               mutex_lock(&ep->ep_lock);<br>
-       else if (!mutex_trylock(&ep->ep_lock))<br>
-               return -EAGAIN;<br>
+       if (nonblock) {<br>
+               if (test_and_set_bit(0, &ep->ep_lock))<br>
+                       return -EAGAIN;<br>
+       } else {<br>
+               while (test_and_set_bit(0, &ep->ep_lock))<br>
+                       wait_on_bit(&ep->ep_lock, 0, TASK_UNINTERRUPTIBLE);<br>
+       }<br>
         return 0;<br>
 }<br>
 <br>
@@ -230,8 +233,8 @@ static void echo_page_disown(const struct lu_env *env,<br>
 {<br>
         struct echo_page *ep = cl2echo_page(slice);<br>
 <br>
-       LASSERT(mutex_is_locked(&ep->ep_lock));<br>
-       mutex_unlock(&ep->ep_lock);<br>
+       LASSERT(test_bit(0, &ep->ep_lock));<br>
+       clear_and_wake_up_bit(0, &ep->ep_lock);<br>
 }<br>
 <br>
 static void echo_page_discard(const struct lu_env *env,<br>
@@ -244,7 +247,7 @@ static void echo_page_discard(const struct lu_env *env,<br>
 static int echo_page_is_vmlocked(const struct lu_env *env,<br>
                                  const struct cl_page_slice *slice)<br>
 {<br>
-       if (mutex_is_locked(&cl2echo_page(slice)->ep_lock))<br>
+       if (test_bit(0, &cl2echo_page(slice)->ep_lock))<br>
                 return -EBUSY;<br>
         return -ENODATA;<br>
 }<br>
@@ -279,7 +282,7 @@ static int echo_page_print(const struct lu_env *env,<br>
         struct echo_page *ep = cl2echo_page(slice);<br>
 <br>
         (*printer)(env, cookie, LUSTRE_ECHO_CLIENT_NAME "-page@%p %d vm@%p\n",<br>
-                  ep, mutex_is_locked(&ep->ep_lock),<br>
+                  ep, test_bit(0, &ep->ep_lock),<br>
                    slice->cpl_page->cp_vmpage);<br>
         return 0;<br>
 }<br>
@@ -339,7 +342,13 @@ static int echo_page_init(const struct lu_env *env, struct cl_object *obj,<br>
         struct echo_object *eco = cl2echo_obj(obj);<br>
 <br>
         get_page(page->cp_vmpage);<br>
-       mutex_init(&ep->ep_lock);<br>
+       /*<br>
+        * ep_lock is similar to the lock_page() lock, and<br>
+        * cannot usefully be monitored by lockdep.<br>
+        * So just a bit in an "unsigned long" and use the<br>
+        * wait_on_bit() interface to wait for the bit to be clera.<br>
+        */<br>
+       ep->ep_lock = 0;<br>
         cl_page_slice_add(page, &ep->ep_cl, obj, index, &echo_page_ops);<br>
         atomic_inc(&eco->eo_npages);<br>
         return 0;<br>
<br>
<br>
_______________________________________________<br>
lustre-devel mailing list<br>
lustre-devel@lists.lustre.org<br>
<a href="http://lists.lustre.org/listinfo.cgi/lustre-devel-lustre.org">http://lists.lustre.org/listinfo.cgi/lustre-devel-lustre.org</a><br>
</div>
</span></font></div>
</body>
</html>