Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add locking around Consumer/Producer operations #42

Merged
merged 2 commits into from Sep 14, 2015

Conversation

simonjbeaumont
Copy link
Contributor

No description provided.

Currently there is a race condition between these operations with the caching
of the ring states. If Consumer.advance starts and calls set_consumer to
advance the consumer pointer on it's sector it will then block. Then another
thread may call suspend which will call set_consumer to set the suspend bit
which will also block on IO. In some cases the write to the suspend bit will be
clobbered by the write to advance the pointer.

This can be seen by applying the below patch to the advance code which causes
the new test to consistently fail.

diff --git a/lib/ring.ml b/lib/ring.ml
index cc80121..c2edc6b 100644
--- a/lib/ring.ml
+++ b/lib/ring.ml
@@ -519,6 +519,9 @@ module Consumer = struct
         let open C in
         let sector = alloc t.info.B.sector_size in
         let consumer' = { t.consumer with consumer = consumer } in
+        let open Lwt in
+        Lwt_unix.sleep 0.1 >>= fun () ->
+        let open C in
         set_consumer ~queue:t.queue ~client:t.client t.disk sector consumer' >>= fun () ->
         t.consumer <- consumer';
         return (`Ok ())

Signed-off-by: Si Beaumont <simon.beaumont@citrix.com>
Add locking around all entry points to the Ring, Consumer and Producer modules.

Both Consumer.pop and Consumer.fold make use of an internal pop and so the lock
is taken around the internal function in this instance.

Signed-off-by: Si Beaumont <simon.beaumont@citrix.com>
jonludlam pushed a commit that referenced this pull request Sep 14, 2015
Add locking around Consumer/Producer operations
@jonludlam jonludlam merged commit c12880e into mirage:master Sep 14, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants