Fix sync driver support in vmbackup plugin.

The cause of the hangs seen while investigating the bug is that the Legato
sync driver doesn't like even when people are doing read-only operations
like stat(2); so it would block the calling thread, which just happened to
be the Tools service's main thread, and things would go to hell.

The fix is to add a flag to the shared app context that lets code knows
when it's safe to access the disk. The two instances of the hang I was
able to capture in the kernel debugger (the log file check task and the
guest info gathering task) now check this flag and just bail out if it's
set. This is the "quick & less intrusive" version of the fix, which will
be modified once it hits vmcore-main to be more generic and more in line
with the Tools Core API.

This *may not* fix all instances of such hangs. I've actually been able
to hang a VM while performing a quiesced snapshot with this fix, but the
frequency is much, much, much lower (it happened once in a few hundred
tries). I haven't yet been able to catch this hang in a debugger. But,
code-wise, we're at least on par with K/L (in fact, better, since K/L
does not have the guest info workaround).

A flag was added that allows disabling the sync driver without having to
uninstall it, in case of emergency. The driver is enabled by default.

Also fix the sync driver support in the vmbackup plugin. The propagation
of the sync driver handle through the different operations was busted, so
querying the handle status was not working as expected. Also make sure
resources are properly freed and errors are correctly handled.

Signed-off-by: Marcelo Vanzin <mvanzin@vmware.com>

diff --git a/open-vm-tools/lib/include/vmware/tools/plugin.h b/open-vm-tools/lib/include/vmware/tools/plugin.h
index cf8e319aa37239e86c262e90a0dbbf3ced654f67..11f3c04e74e149cbeb28d5e0c282236a931cc056 100644 (file)
--- a/open-vm-tools/lib/include/vmware/tools/plugin.h
+++ b/open-vm-tools/lib/include/vmware/tools/plugin.h
@@ -216,6 +216,11 @@ typedef struct ToolsAppCtx {
     * register and emit their own signals using this object.
     */
    gpointer          serviceObj;
+   /**
+    * If this flag is TRUE, it's not safe to access the disk. Code that tries
+    * to access the disk should avoid running in that case.
+    */
+   gboolean          disksFrozen;
 } ToolsAppCtx;
 
 #if defined(G_PLATFORM_WIN32)

diff --git a/open-vm-tools/services/plugins/guestInfo/guestInfoServer.c b/open-vm-tools/services/plugins/guestInfo/guestInfoServer.c
index e402b1dd5c2489464a41c06aa8c24ee65212374c..92ad1ef5c126f58aa82b22f39e59ac39b5048de8 100644 (file)
--- a/open-vm-tools/services/plugins/guestInfo/guestInfoServer.c
+++ b/open-vm-tools/services/plugins/guestInfo/guestInfoServer.c
@@ -360,6 +360,12 @@ GuestInfoGather(gpointer data)
 
    g_debug("Entered guest info gather.\n");
 
+   /* See bug 529653. */
+   if (ctx->disksFrozen) {
+      g_debug("Skipping guest info gathering while disks are frozen.");
+      return TRUE;
+   }
+
    /* Send tools version. */
    if (!GuestInfoUpdateVmdb(ctx, INFO_BUILD_NUMBER, BUILD_NUMBER)) {
       /*

diff --git a/open-vm-tools/services/plugins/vmbackup/stateMachine.c b/open-vm-tools/services/plugins/vmbackup/stateMachine.c
index bca1819f4d35e6e6515c69f8232d05d6e867f7d7..6560c61664e86809204ed9427ac4c3de66b6e9df 100644 (file)
--- a/open-vm-tools/services/plugins/vmbackup/stateMachine.c
+++ b/open-vm-tools/services/plugins/vmbackup/stateMachine.c
@@ -281,6 +281,7 @@ VmBackupOnError(void)
       /* Next state is "sync error". */
       gBackupState->pollPeriod = 1000;
       gBackupState->machineState = VMBACKUP_MSTATE_SYNC_ERROR;
+      gBackupState->ctx->disksFrozen = FALSE;
       break;
 
    case VMBACKUP_MSTATE_SCRIPT_THAW:
@@ -445,6 +446,7 @@ VmBackupAsyncCallback(void *clientData)
 
    case VMBACKUP_MSTATE_SYNC_THAW:
       /* Next state is "script thaw". */
+      gBackupState->ctx->disksFrozen = FALSE;
       if (!VmBackupStartScripts(VMBACKUP_SCRIPT_THAW)) {
          VmBackupOnError();
       }
@@ -492,7 +494,9 @@ static Bool
 VmBackupEnableSync(void)
 {
    g_debug("*** %s\n", __FUNCTION__);
+   gBackupState->ctx->disksFrozen = TRUE;
    if (!gSyncProvider->start(gBackupState, gSyncProvider->clientData)) {
+      gBackupState->ctx->disksFrozen = FALSE;
       VmBackup_SendEvent(VMBACKUP_EVENT_REQUESTOR_ERROR,
                          VMBACKUP_SYNC_ERROR,
                          "Error when enabling the sync provider.");

diff --git a/open-vm-tools/services/plugins/vmbackup/syncDriverOps.c b/open-vm-tools/services/plugins/vmbackup/syncDriverOps.c
index 2e1d1823b15a73e6957d3d9355ebb98d8dd07ce1..bea774bd3320789f19d8d6ec905765a8ca847f9b 100644 (file)
--- a/open-vm-tools/services/plugins/vmbackup/syncDriverOps.c
+++ b/open-vm-tools/services/plugins/vmbackup/syncDriverOps.c
@@ -39,20 +39,50 @@ typedef struct VmBackupDriverOp {
    const char *volumes;
    Bool freeze;
    Bool canceled;
-   SyncDriverHandle syncHandle;
+   SyncDriverHandle *syncHandle;
+   gboolean syncEnabled;
 } VmBackupDriverOp;
 
 
+/*
+ *-----------------------------------------------------------------------------
+ *
+ * VmBackupDriverThaw --
+ *
+ *    Thaws the frozen filesystems, and cleans up internal state kept by the
+ *    code.
+ *
+ * Results:
+ *    Whether thawing was successful.
+ *
+ * Side effects:
+ *    None.
+ *
+ *-----------------------------------------------------------------------------
+ */
+
+static Bool
+VmBackupDriverThaw(VmBackupDriverOp *op)
+{
+   Bool success = SyncDriver_Thaw(*op->syncHandle);
+   SyncDriver_CloseHandle(op->syncHandle);
+   return success;
+}
+
+
 /*
  *-----------------------------------------------------------------------------
  *
  *  VmBackupDriverOpQuery --
  *
- *    Checks the status of the app that is enabling or disabling the
+ *    Checks the status of the operation that is enabling or disabling the
  *    sync driver.
  *
  * Result
- *    None.
+ *    VMBACKUP_STATUS_PENDING:   still working.
+ *    VMBACKUP_STATUS_FINISHED:  done.
+ *    VMBACKUP_STATUS_CANCELED:  cancel request fulfilled.
+ *    VMBACKUP_STATUS_ERROR:     oops.
  *
  * Side effects:
  *    None.
@@ -66,26 +96,33 @@ VmBackupDriverOpQuery(VmBackupOp *_op) // IN
    VmBackupDriverOp *op = (VmBackupDriverOp *) _op;
    VmBackupOpStatus ret;
 
-   SyncDriverStatus st;
-
-   st = SyncDriver_QueryStatus(op->syncHandle, 0);
-   g_debug("SyncDriver status: %d\n", st);
-
-   switch(st) {
-   case SYNCDRIVER_BUSY:
-      ret = VMBACKUP_STATUS_PENDING;
-      break;
+   if (!op->syncEnabled) {
+      return VMBACKUP_STATUS_FINISHED;
+   }
 
-   case SYNCDRIVER_IDLE:
-      if (op->freeze && op->canceled) {
-         SyncDriver_Thaw(op->syncHandle);
+   if (op->freeze) {
+      SyncDriverStatus st = SyncDriver_QueryStatus(*op->syncHandle, 0);
+
+      g_debug("SyncDriver status: %d\n", st);
+      switch(st) {
+      case SYNCDRIVER_BUSY:
+         ret = VMBACKUP_STATUS_PENDING;
+         break;
+
+      case SYNCDRIVER_IDLE:
+         if (op->canceled) {
+            VmBackupDriverThaw(op);
+         }
+         ret = (op->canceled) ? VMBACKUP_STATUS_CANCELED : VMBACKUP_STATUS_FINISHED;
+         break;
+
+      default:
+         VmBackupDriverThaw(op);
+         ret = VMBACKUP_STATUS_ERROR;
+         break;
       }
-      ret = (op->canceled) ? VMBACKUP_STATUS_CANCELED : VMBACKUP_STATUS_FINISHED;
-      break;
-
-   default:
-      ret = VMBACKUP_STATUS_ERROR;
-      break;
+   } else {
+      ret = VMBACKUP_STATUS_FINISHED;
    }
 
    return ret;
@@ -112,9 +149,7 @@ static void
 VmBackupDriverOpRelease(VmBackupOp *_op)  // IN
 {
    VmBackupDriverOp *op = (VmBackupDriverOp *) _op;
-   if (!op->freeze) {
-      SyncDriver_CloseHandle(&op->syncHandle);
-   }
+   free(op->syncHandle);
    free(op);
 }
 
@@ -165,12 +200,18 @@ VmBackupDriverOpCancel(VmBackupOp *_op)   // IN
  */
 
 static VmBackupDriverOp *
-VmBackupNewDriverOp(Bool freeze,             // IN
-                    SyncDriverHandle handle, // IN
-                    const char *volumes)     // IN
+VmBackupNewDriverOp(VmBackupState *state,       // IN
+                    Bool freeze,                // IN
+                    SyncDriverHandle *handle,   // IN
+                    const char *volumes)        // IN
 {
+   GError *err = NULL;
    VmBackupDriverOp *op = NULL;
 
+   g_return_val_if_fail((handle == NULL || *handle == SYNCDRIVER_INVALID_HANDLE) ||
+                        !freeze,
+                        NULL);
+
    op = Util_SafeMalloc(sizeof *op);
    memset(op, 0, sizeof *op);
 
@@ -180,16 +221,31 @@ VmBackupNewDriverOp(Bool freeze,             // IN
    op->freeze = freeze;
    op->volumes = volumes;
 
-   if (freeze && !SyncDriver_Freeze(op->volumes, &handle)) {
-      g_warning("Error freezing filesystems.\n");
-      free(op);
-      op = NULL;
-   } else if (!freeze && !SyncDriver_Thaw(handle)) {
-      g_warning("Error thawing filesystems.\n");
-      free(op);
-      op = NULL;
-   } else {
-      op->syncHandle = handle;
+   op->syncHandle = Util_SafeMalloc(sizeof *op->syncHandle);
+   *op->syncHandle = (handle != NULL) ? *handle : SYNCDRIVER_INVALID_HANDLE;
+   op->syncEnabled = g_key_file_get_boolean(state->ctx->config,
+                                            "vmbackup",
+                                            "enableSyncDriver",
+                                            &err);
+
+   /* Enable the driver by default. */
+   if (err != NULL) {
+      g_clear_error(&err);
+      op->syncEnabled = TRUE;
+   }
+
+   if (op->syncEnabled) {
+      Bool success;
+      if (freeze) {
+         success = SyncDriver_Freeze(op->volumes, op->syncHandle);
+      } else {
+         success = VmBackupDriverThaw(op);
+      }
+      if (!success) {
+         g_warning("Error %s filesystems.", freeze ? "freezing" : "thawing");
+         free(op);
+         op = NULL;
+      }
    }
    return op;
 }
@@ -216,8 +272,13 @@ VmBackupNewDriverOp(Bool freeze,             // IN
 static Bool
 VmBackupSyncDriverReadyForSnapshot(VmBackupState *state)
 {
+   SyncDriverHandle *handle = state->clientData;
+
    g_debug("*** %s\n", __FUNCTION__);
-   return VmBackup_SendEvent(VMBACKUP_EVENT_SNAPSHOT_COMMIT, 0, "");
+   if (handle != NULL && *handle != SYNCDRIVER_INVALID_HANDLE) {
+      return VmBackup_SendEvent(VMBACKUP_EVENT_SNAPSHOT_COMMIT, 0, "");
+   }
+   return TRUE;
 }
 
 
@@ -247,12 +308,10 @@ VmBackupSyncDriverStart(VmBackupState *state,
    VmBackupDriverOp *op;
 
    g_debug("*** %s\n", __FUNCTION__);
-   op = VmBackupNewDriverOp(TRUE,
-                            (SyncDriverHandle) state->clientData,
-                            state->volumes);
+   op = VmBackupNewDriverOp(state, TRUE, NULL, state->volumes);
 
    if (op != NULL) {
-      state->clientData = (intptr_t) op->syncHandle;
+      state->clientData = op->syncHandle;
    }
 
    return VmBackup_SetCurrentOp(state,
@@ -286,9 +345,8 @@ VmBackupSyncDriverSnapshotDone(VmBackupState *state,
 
    g_debug("*** %s\n", __FUNCTION__);
 
-   op = VmBackupNewDriverOp(FALSE,
-                            (SyncDriverHandle) state->clientData,
-                            NULL);
+   op = VmBackupNewDriverOp(state, FALSE, state->clientData, NULL);
+   state->clientData = NULL;
 
    return VmBackup_SetCurrentOp(state, (VmBackupOp *) op, NULL, __FUNCTION__);
 }

diff --git a/open-vm-tools/services/plugins/vmbackup/vmBackupInt.h b/open-vm-tools/services/plugins/vmbackup/vmBackupInt.h
index 24e7cdba0a8311a800df51fbbcab96507837dd79..a5150b21935f3a96703c41414f27277b0ac3c705 100644 (file)
--- a/open-vm-tools/services/plugins/vmbackup/vmBackupInt.h
+++ b/open-vm-tools/services/plugins/vmbackup/vmBackupInt.h
@@ -88,7 +88,7 @@ typedef struct VmBackupState {
    Bool (*callback)(struct VmBackupState *);
    Bool           forceRequeue;
    Bool           generateManifests;
-   intptr_t       clientData;
+   gpointer       clientData;
    void          *scripts;
    const char    *configDir;
    ssize_t        currentScript;

diff --git a/open-vm-tools/services/vmtoolsd/mainLoop.c b/open-vm-tools/services/vmtoolsd/mainLoop.c
index 0a039f6f41da2d5b2edf1b49194278f682f285e8..1f08aec7229d76198cdf4f6b5d17eef410b1c9c8 100644 (file)
--- a/open-vm-tools/services/vmtoolsd/mainLoop.c
+++ b/open-vm-tools/services/vmtoolsd/mainLoop.c
@@ -127,7 +127,10 @@ ToolsCoreInitializeDebug(ToolsServiceState *state)
 static gboolean
 ToolsCoreConfFileCb(gpointer clientData)
 {
-   ToolsCore_ReloadConfig(clientData, FALSE);
+   ToolsServiceState *state = clientData;
+   if (!state->ctx.disksFrozen) {
+      ToolsCore_ReloadConfig(state, FALSE);
+   }
    return TRUE;
 }