From: Kern Sibbald <kern@sibbald.com>
Date: Wed, 9 Dec 2009 21:47:08 +0000 (+0100)
Subject: Reorder lock_volumes and dlock in SD to avoid race conditions
X-Git-Tag: Release-5.0.0~160
X-Git-Url: https://git.sur5r.net/?a=commitdiff_plain;h=8d4c44a95f0959ad8b3e44506b573f422d27b781;p=bacula%2Fbacula

Reorder lock_volumes and dlock in SD to avoid race conditions
---

diff --git a/bacula/src/lib/mutex_list.h b/bacula/src/lib/mutex_list.h
index fce166ead5..5b3e09460d 100644
--- a/bacula/src/lib/mutex_list.h
+++ b/bacula/src/lib/mutex_list.h
@@ -34,9 +34,9 @@
  * race conditions and dead locks
  */
 
-#define PRIO_SD_DEV_ACQUIRE   4
-#define PRIO_SD_VOL_LIST      5
-#define PRIO_SD_DEV_ACCESS    10
-#define PRIO_SD_DEV_SPOOL     14
+#define PRIO_SD_DEV_ACQUIRE   4            /* dev.acquire_mutex */
+#define PRIO_SD_DEV_ACCESS    5            /* dev.m_mutex */
+#define PRIO_SD_VOL_LIST      10           /* vol_list_lock */
+#define PRIO_SD_DEV_SPOOL     14           /* dev.spool_mutex */
 
 #endif
diff --git a/bacula/src/lib/plugins.c b/bacula/src/lib/plugins.c
index 26028534d5..70ce223ffd 100644
--- a/bacula/src/lib/plugins.c
+++ b/bacula/src/lib/plugins.c
@@ -244,18 +244,17 @@ void dbg_plugin_add_hook(dbg_plugin_hook_t *fct)
 void dbg_print_plugin(FILE *fp)
 {
    Plugin *plugin;
-   fprintf(fp, "Attempt to dump plugins\n");
+   fprintf(fp, "Attempt to dump plugins. Hook count=%d\n", plugin_hook_count);
 
    if (!plugin_list) {
       return;
    }
-
    foreach_alist(plugin, plugin_list) {
       for(int i=0; i < dbg_plugin_hook_count; i++) {
          dbg_plugin_hook_t *fct = dbg_plugin_hooks[i];
          fprintf(fp, "Plugin %p name=\"%s\" disabled=%d\n",
                  plugin, plugin->file, plugin->disabled);
-         fct(plugin, fp);
+//       fct(plugin, fp);
       }
    }
 }
diff --git a/bacula/src/stored/acquire.c b/bacula/src/stored/acquire.c
index d1b41e20ad..4c9f627a1f 100644
--- a/bacula/src/stored/acquire.c
+++ b/bacula/src/stored/acquire.c
@@ -447,13 +447,13 @@ bool release_device(DCR *dcr)
    bool ok = true;
    char tbuf[100];
 
-   lock_volumes();
    dev->dlock();
    if (!dev->is_blocked()) {
       block_device(dev, BST_RELEASING);
    } else if (dev->blocked() == BST_DESPOOLING) {
       dev->set_blocked(BST_RELEASING);
    }
+   lock_volumes();
    Dmsg2(100, "release_device device %s is %s\n", dev->print_name(), dev->is_tape()?"tape":"disk");
 
    /* if device is reserved, job never started, so release the reserve here */
@@ -550,8 +550,8 @@ bool release_device(DCR *dcr)
    Dmsg2(100, "JobId=%u broadcast wait_device_release at %s\n", 
          (uint32_t)jcr->JobId, bstrftimes(tbuf, sizeof(tbuf), (utime_t)time(NULL)));
    pthread_cond_broadcast(&wait_device_release);
-   dev->dunblock(true);
    unlock_volumes();
+   dev->dunblock(true);
    if (dcr->keep_dcr) {
       detach_dcr_from_dev(dcr);
    } else {
@@ -685,8 +685,8 @@ static void locked_detach_dcr_from_dev(DCR *dcr)
 
    /* Detach this dcr only if attached */
    if (dcr->attached_to_dev && dev) {
-      dev->dlock();
       dcr->unreserve_device();
+      dev->dlock();
       dcr->dev->attached_dcrs->remove(dcr);  /* detach dcr from device */
 //    remove_dcr_from_dcrs(dcr);      /* remove dcr from jcr list */
       dev->dunlock();
diff --git a/bacula/src/stored/mount.c b/bacula/src/stored/mount.c
index 462b9b5b4f..4c022d54df 100644
--- a/bacula/src/stored/mount.c
+++ b/bacula/src/stored/mount.c
@@ -64,6 +64,9 @@ enum {
  * This routine returns a 0 only if it is REALLY
  *  impossible to get the requested Volume.
  *
+ * This routine is entered with the device blocked, but not
+ *   locked.
+ *
  */
 bool DCR::mount_next_write_volume()
 {
@@ -138,6 +141,7 @@ mount_next_vol:
     * and move the tape to the end of data.
     *
     */
+   unlock_volumes();
    if (autoload_device(dcr, true/*writing*/, NULL) > 0) {
       autochanger = true;
       ask = false;
@@ -147,6 +151,7 @@ mount_next_vol:
       ask = retry >= 2;
       do_find = true;           /* do find_a_volume if we retry */
    }
+   lock_volumes();
    Dmsg1(150, "autoload_dev returns %d\n", autochanger);
    /*
     * If we autochanged to correct Volume or (we have not just
diff --git a/bacula/src/stored/reserve.c b/bacula/src/stored/reserve.c
index 677d5ebdea..8119f84af7 100644
--- a/bacula/src/stored/reserve.c
+++ b/bacula/src/stored/reserve.c
@@ -145,6 +145,7 @@ void DCR::clear_reserved()
  */
 void DCR::unreserve_device()
 {
+   dev->dlock();
    lock_volumes();
    if (is_reserved()) {
       clear_reserved();
@@ -162,6 +163,7 @@ void DCR::unreserve_device()
       }
    }
    unlock_volumes();
+   dev->dunlock();
 }
 
 /*