diff mbox series

PR28339: debuginfod groom/scan concurrency fix

Message ID 20210914152058.GG19490@redhat.com
State Committed
Headers show
Series PR28339: debuginfod groom/scan concurrency fix | expand

Commit Message

Frank Ch. Eigler Sept. 14, 2021, 3:20 p.m. UTC
commit ce695bedf073127883bbbaab528dd1f2b0e955f1 (HEAD -> master)
Author: Frank Ch. Eigler <fche@redhat.com>
Date:   Tue Sep 14 08:15:23 2021 -0400

    PR28339: debuginfod: fix groom/scan race condition on just-emptied queue
    
    debuginfod's scan and groom operations (thread_main_scanner,
    thread_main_fts_source_paths) are intended to be mutually exclusive,
    as a substitute for more complicated sql transaction batching.  (This
    is because scanning / grooming involves inserting or deleting data
    from multiple related tables.)
    
    The workq class that governs this in debuginfod.cxx has a problem: if
    the workq just becomes empty, its sole entry pulled by a scanner
    thread in response to a wait_front(), an 'idler' groomer thread is
    ALSO permitted to run, because there is no indication as to when the
    scanner thread operation finishes, only when it starts.
    
    Extending the workq with a counter ("fronters") to track any active
    scanning activity (even if the workq is empty) lets us block idlers
    groomers a little longer.
    
    Signed-off-by: Frank Ch. Eigler <fche@redhat.com>

Comments

Mark Wielaard Sept. 14, 2021, 3:25 p.m. UTC | #1
Hi Frank,

On Tue, 2021-09-14 at 11:20 -0400, Frank Ch. Eigler via Elfutils-devel wrote:
> commit ce695bedf073127883bbbaab528dd1f2b0e955f1 (HEAD -> master)
> Author: Frank Ch. Eigler <fche@redhat.com>
> Date:   Tue Sep 14 08:15:23 2021 -0400
> 
>     PR28339: debuginfod: fix groom/scan race condition on just-emptied queue
>     
>     debuginfod's scan and groom operations (thread_main_scanner,
>     thread_main_fts_source_paths) are intended to be mutually exclusive,
>     as a substitute for more complicated sql transaction batching.  (This
>     is because scanning / grooming involves inserting or deleting data
>     from multiple related tables.)
>     
>     The workq class that governs this in debuginfod.cxx has a problem: if
>     the workq just becomes empty, its sole entry pulled by a scanner
>     thread in response to a wait_front(), an 'idler' groomer thread is
>     ALSO permitted to run, because there is no indication as to when the
>     scanner thread operation finishes, only when it starts.
>     
>     Extending the workq with a counter ("fronters") to track any active
>     scanning activity (even if the workq is empty) lets us block idlers
>     groomers a little longer.
>     
>     Signed-off-by: Frank Ch. Eigler <fche@redhat.com>

Thanks so much for finding (and fixing!) this.
It survived 3850+ cycles on my local machine. So I am hopeful this will
also work on the buildbots. Please push.

Cheers,

Mark
Magne Hov Nov. 11, 2021, 12:48 p.m. UTC | #2
Hi Frank,

On Tue, Sep 14 2021, Frank Ch. Eigler via Elfutils-devel wrote:

> commit ce695bedf073127883bbbaab528dd1f2b0e955f1 (HEAD -> master)
> Author: Frank Ch. Eigler <fche@redhat.com>
> Date:   Tue Sep 14 08:15:23 2021 -0400
>
>     PR28339: debuginfod: fix groom/scan race condition on just-emptied queue
>     
>     debuginfod's scan and groom operations (thread_main_scanner,
>     thread_main_fts_source_paths) are intended to be mutually exclusive,
>     as a substitute for more complicated sql transaction batching.  (This
>     is because scanning / grooming involves inserting or deleting data
>     from multiple related tables.)
>     
>     The workq class that governs this in debuginfod.cxx has a problem: if
>     the workq just becomes empty, its sole entry pulled by a scanner
>     thread in response to a wait_front(), an 'idler' groomer thread is
>     ALSO permitted to run, because there is no indication as to when the
>     scanner thread operation finishes, only when it starts.
>     
>     Extending the workq with a counter ("fronters") to track any active
>     scanning activity (even if the workq is empty) lets us block idlers
>     groomers a little longer.
>     
>     Signed-off-by: Frank Ch. Eigler <fche@redhat.com>

Thanks a lot for this fix. We've had an issue in automated testing where
`nuke orphan buildids` is executed after `rpm-buildid-intern` has run
but *before* `rpm-de-insert` has run. The result is missing BUILDIDS for
the archive, and rescans of the archive fail to re-add them because the
archive is registered as already having been scanned.

I think this fix will resovle this.
diff mbox series

Patch

diff --git a/debuginfod/ChangeLog b/debuginfod/ChangeLog
index c54598239c82..07ddad3019d9 100644
--- a/debuginfod/ChangeLog
+++ b/debuginfod/ChangeLog
@@ -1,3 +1,11 @@ 
+2021-09-14  Frank Ch. Eigler <fche@redhat.com>
+
+	PRPR28339
+	* debuginfod.cxx (waitq::fronters): New field.
+	(waitq::wait_idle): Respect it.
+	(waitq::done_front): New function.
+	(thread_main_scanner): Call it to match wait_front().
+
 2021-08-28  Mark Wielaard  <mjw@redhat.com>
 
 	* debuginfod.cxx (parse_opt): Turn the -d arg ":memory:" into
diff --git a/debuginfod/debuginfod.cxx b/debuginfod/debuginfod.cxx
index 3269f657cc8d..6387a27ff820 100644
--- a/debuginfod/debuginfod.cxx
+++ b/debuginfod/debuginfod.cxx
@@ -663,10 +663,11 @@  class workq
   mutex mtx;
   condition_variable cv;
   bool dead;
-  unsigned idlers;
+  unsigned idlers;   // number of threads busy with wait_idle / done_idle
+  unsigned fronters; // number of threads busy with wait_front / done_front
 
 public:
-  workq() { dead = false; idlers = 0; }
+  workq() { dead = false; idlers = 0; fronters = 0; }
   ~workq() {}
 
   void push_back(const Payload& p)
@@ -690,10 +691,11 @@  class workq
     unique_lock<mutex> lock(mtx);
     q.clear();
     set_metric("thread_work_pending","role","scan", q.size());
+    // NB: there may still be some live fronters
     cv.notify_all(); // maybe wake up waiting idlers
   }
 
-  // block this scanner thread until there is work to do and no active
+  // block this scanner thread until there is work to do and no active idler
   bool wait_front (Payload& p)
   {
     unique_lock<mutex> lock(mtx);
@@ -705,19 +707,29 @@  class workq
       {
         p = * q.begin();
         q.erase (q.begin());
+        fronters ++; // prevent idlers from starting awhile, even if empty q
         set_metric("thread_work_pending","role","scan", q.size());
-        if (q.size() == 0)
-          cv.notify_all(); // maybe wake up waiting idlers
+        // NB: don't wake up idlers yet!  The consumer is busy
+        // processing this element until it calls done_front().
         return true;
       }
   }
 
+  // notify waitq that scanner thread is done with that last item
+  void done_front ()
+  {
+    unique_lock<mutex> lock(mtx);
+    fronters --;
+    if (q.size() == 0 && fronters == 0)
+      cv.notify_all(); // maybe wake up waiting idlers
+  }
+  
   // block this idler thread until there is no work to do
   void wait_idle ()
   {
     unique_lock<mutex> lock(mtx);
     cv.notify_all(); // maybe wake up waiting scanners
-    while (!dead && (q.size() != 0))
+    while (!dead && ((q.size() != 0) || fronters > 0))
       cv.wait(lock);
     idlers ++;
   }
@@ -3145,6 +3157,8 @@  thread_main_scanner (void* arg)
           e.report(cerr);
         }
 
+      scanq.done_front(); // let idlers run
+      
       if (fts_cached || fts_executable || fts_debuginfo || fts_sourcefiles || fts_sref || fts_sdef)
         {} // NB: not just if a successful scan - we might have encountered -ENOSPC & failed
       (void) statfs_free_enough_p(db_path, "database"); // report sqlite filesystem size