Libguestfs - Jun 2023 - [PATCH libnbd] ocaml: Release runtime lock around call to nbd_close

OCaml 5 is stricter than earlier versions about correct locking.

We must release the OCaml runtime lock when calling nbd_close since it
may do some long-running operations and we want to allow concurrent
threads to run.

However specifically if there are callbacks (eg. a debug callback)
then we would end up trying to re-acquire the lock in the callback,
resulting in a crash:

  (gdb) bt
  #0  __pthread_kill_implementation (threadid=<optimized out>,
      signo=signo at entry=6, no_tid=no_tid at entry=0) at pthread_kill.c:44
  #1  0x00007f47ffc8f773 in __pthread_kill_internal (signo=6,
      threadid=<optimized out>) at pthread_kill.c:78
  #2  0x00007f47ffc3e71e in __GI_raise (sig=sig at entry=6)
      at ../sysdeps/posix/raise.c:26
  #3  0x00007f47ffc2687f in __GI_abort () at abort.c:79
  #4  0x0000560c9eb62779 in caml_fatal_error ()
  #5  0x0000560c9eb63238 in caml_plat_fatal_error ()
  #6  0x0000560c9eb4ea77 in caml_acquire_domain_lock ()
  #7  0x0000560c9eb65cdc in caml_leave_blocking_section ()
  #8  0x0000560c9eaf8a87 in debug_wrapper (user_data=0x560ca0af2670,
      context=0x7f47fff8ca60 "nbd_close", msg=0x560ca0af28b0
"closing handle")
      at ../nbd-c.c:187
  #9  0x00007f47fff7072f in nbd_internal_debug (h=h at entry=0x560ca0b57db0,
      context=0x7f47fff8ca60 "nbd_close", context at entry=0x0,
      fs=fs at entry=0x7f47fff8ca6a "closing handle")
      at /home/rjones/d/libnbd/lib/debug.c:90
  #10 0x00007f47fff73f23 in nbd_close (h=0x560ca0b57db0)
      at /home/rjones/d/libnbd/lib/handle.c:127
  #11 0x0000560c9eae8dbe in nbd_internal_ocaml_handle_finalize (
      hv=<optimized out>) at ../handle.c:39
  #12 nbd_internal_ocaml_nbd_close (hv=<optimized out>) at ../handle.c:62
  #13 <signal handler called>
  #14 0x0000560c9eae84dc in camlTest_140_explicit_close__entry () at NBD.ml:148
  #15 0x0000560c9eae5c5b in caml_program ()
  #16 <signal handler called>
  #17 0x0000560c9eb6cb77 in caml_startup_common ()
  #18 0x0000560c9eb6cbef in caml_main ()
  #19 0x0000560c9eae5910 in main ()
---
 ocaml/handle.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/ocaml/handle.c b/ocaml/handle.c
index fb45b30102..b3e5a0fc33 100644
--- a/ocaml/handle.c
+++ b/ocaml/handle.c
@@ -26,6 +26,7 @@
 #include <caml/fail.h>
 #include <caml/memory.h>
 #include <caml/mlvalues.h>
+#include <caml/threads.h>
 
 #include <libnbd.h>
 
@@ -36,7 +37,9 @@ nbd_internal_ocaml_handle_finalize (value hv)
 {
   struct nbd_handle *h = NBD_val (hv);
 
+  caml_enter_blocking_section ();
   nbd_close (h);
+  caml_leave_blocking_section ();
 }
 
 value
-- 
2.41.0

On 6/21/23 23:54, Richard W.M. Jones wrote:
> OCaml 5 is stricter than earlier versions about correct locking.
> 
> We must release the OCaml runtime lock when calling nbd_close since it
> may do some long-running operations and we want to allow concurrent
> threads to run.
> 
> However specifically if there are callbacks (eg. a debug callback)
> then we would end up trying to re-acquire the lock in the callback,
> resulting in a crash:
Drop the word "However"?

Paragraph #3 appears to support paragraph #2, not to oppose it.

Acked-by: Laszlo Ersek <lersek at redhat.com>

Laszlo
> 
>   (gdb) bt
>   #0  __pthread_kill_implementation (threadid=<optimized out>,
>       signo=signo at entry=6, no_tid=no_tid at entry=0) at
pthread_kill.c:44
>   #1  0x00007f47ffc8f773 in __pthread_kill_internal (signo=6,
>       threadid=<optimized out>) at pthread_kill.c:78
>   #2  0x00007f47ffc3e71e in __GI_raise (sig=sig at entry=6)
>       at ../sysdeps/posix/raise.c:26
>   #3  0x00007f47ffc2687f in __GI_abort () at abort.c:79
>   #4  0x0000560c9eb62779 in caml_fatal_error ()
>   #5  0x0000560c9eb63238 in caml_plat_fatal_error ()
>   #6  0x0000560c9eb4ea77 in caml_acquire_domain_lock ()
>   #7  0x0000560c9eb65cdc in caml_leave_blocking_section ()
>   #8  0x0000560c9eaf8a87 in debug_wrapper (user_data=0x560ca0af2670,
>       context=0x7f47fff8ca60 "nbd_close", msg=0x560ca0af28b0
"closing handle")
>       at ../nbd-c.c:187
>   #9  0x00007f47fff7072f in nbd_internal_debug (h=h at
entry=0x560ca0b57db0,
>       context=0x7f47fff8ca60 "nbd_close", context at entry=0x0,
>       fs=fs at entry=0x7f47fff8ca6a "closing handle")
>       at /home/rjones/d/libnbd/lib/debug.c:90
>   #10 0x00007f47fff73f23 in nbd_close (h=0x560ca0b57db0)
>       at /home/rjones/d/libnbd/lib/handle.c:127
>   #11 0x0000560c9eae8dbe in nbd_internal_ocaml_handle_finalize (
>       hv=<optimized out>) at ../handle.c:39
>   #12 nbd_internal_ocaml_nbd_close (hv=<optimized out>) at
../handle.c:62
>   #13 <signal handler called>
>   #14 0x0000560c9eae84dc in camlTest_140_explicit_close__entry () at
NBD.ml:148
>   #15 0x0000560c9eae5c5b in caml_program ()
>   #16 <signal handler called>
>   #17 0x0000560c9eb6cb77 in caml_startup_common ()
>   #18 0x0000560c9eb6cbef in caml_main ()
>   #19 0x0000560c9eae5910 in main ()
> ---
>  ocaml/handle.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/ocaml/handle.c b/ocaml/handle.c
> index fb45b30102..b3e5a0fc33 100644
> --- a/ocaml/handle.c
> +++ b/ocaml/handle.c
> @@ -26,6 +26,7 @@
>  #include <caml/fail.h>
>  #include <caml/memory.h>
>  #include <caml/mlvalues.h>
> +#include <caml/threads.h>
>  
>  #include <libnbd.h>
>  
> @@ -36,7 +37,9 @@ nbd_internal_ocaml_handle_finalize (value hv)
>  {
>    struct nbd_handle *h = NBD_val (hv);
>  
> +  caml_enter_blocking_section ();
>    nbd_close (h);
> +  caml_leave_blocking_section ();
>  }
>  
>  value