From: VMware, Inc <> Date: Mon, 22 Mar 2010 22:12:04 +0000 (-0700) Subject: Fix sync driver support in vmbackup plugin. X-Git-Tag: 2010.03.20-243334~22 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=a97ec76bc4dbe41faa82579f0ef022b22bff1e85;p=thirdparty%2Fopen-vm-tools.git 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 --- diff --git a/open-vm-tools/lib/include/vmware/tools/plugin.h b/open-vm-tools/lib/include/vmware/tools/plugin.h index cf8e319aa..11f3c04e7 100644 --- 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 e402b1dd5..92ad1ef5c 100644 --- 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 bca1819f4..6560c6166 100644 --- 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 2e1d1823b..bea774bd3 100644 --- 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 24e7cdba0..a5150b219 100644 --- 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 0a039f6f4..1f08aec72 100644 --- 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; }