From: Mark Vitale Date: Mon, 20 Apr 2020 18:51:08 +0000 (-0400) Subject: vos: avoid 'half-locked' volume after interrupted 'vos rename' X-Git-Tag: openafs-devel-1_9_0~13 X-Git-Url: https://git.openafs.org/?p=openafs.git;a=commitdiff_plain;h=291bad659e26c21332abd2954ee8d49fccad90da;hp=21cd26cb0d0a37d9412c0285a3c73c693222fd8a vos: avoid 'half-locked' volume after interrupted 'vos rename' Reported symptoms: If a 'vos rename' is interrupted after it has locked the volume and replaced the VLDB entry, but before it has unlocked the volume, the volume will remain locked. However, the locked volume will NOT be listed as locked in any vos commands that display locked status (see below for details). Background: Most vos write operations lock the VLDB volume entry before proceeding, then release the volume lock when finished. This is accomplished via VL_SetLock and VL_ReleaseLock, respectively. VL_SetLock always sets these members in the VLDB volume entry: - flags is modified to set the required VLOP_* code bit as specified - LockAFSid is set to 0 (never implemented) - LockTimestamp is set to the current time VL_ReleaseLock always sets them as follows: - flags is cleared of any VLOP_* code bit - LockAFSid is set to 0 (never implemented) - LockTimestamp is set to 0 VL_ReplaceEntry(N) may also optionally clear each of these members: - flags operation bits may be explicitly cleared via LOCKREL_OPCODE - LockAFSid may be explicitly cleared via LOCKREL_AFSID - LockTimestamp may be explicitly cleared via LOCKREL_TIMESTAMP When all 3 options are specified, VL_ReplaceEntry also does the functional equivalent of a VL_ReleaseLock. Most vos operations use this method. However, when no lock release options are specified on VL_ReplaceEntry(N), the VLDB entry is simply replaced with the supplied entry. This includes whatever flags values are specified in the supplied entry; therefore, this amounts to an additional, implicit way to set or modify the flags. Root cause: 'vos rename' (UV_RenameVolume) is the only vos operation that does all of the following things: - accepts a replacement volume entry that was obtained before VL_SetLock (and thus does NOT have any lock flags set) - issues VL_SetLock (which sets the lock flag in the VLDB) - issues VL_ReplaceEntry(N) with the original unlocked entry, and with no lock release options (thus with explicit intent to leave the lock flag unchanged, but inadvertently doing an implicit clear of the lock flag in the VLDB) - (performs some additional volserver work) - issues VL_ReleaseLock to release the volume lock Therefore, if 'vos rename' is cancelled or killed before reaching the final VL_ReleaseLock step, the VLDB entry is left with the lock flags cleared but the LockTimestamp still set. As we will see below, this 'half-locked' state produces confusing results from other vos commands. Detection of locked state: The 'vos lock' command (and all other vos commands that issue VL_SetLock) use the lock timestamp to determine if a volume is locked. However, several other vos commands ('vos listvldb ', 'vos examine ', 'vos listvldb -locked') use the VLDB entry's lock flags (not the lock timestamp) to determine if the volume is locked. Therefore, if the lock flags have been cleared but the lock timestamp is still set, these commands fail to detect that the volume is still locked. Yet an administrator's 'vos lock ' will still fail with: Could not lock VLDB entry for volume VLDB: vldb entry is already locked This is the external manifestation of the 'half-locked' state. Workaround and fix: This scenario has a simple workaround: 'vos unlock '. However, to avoid this confusing outcome in the first place, modify the 'vos rename' logic so that the lock flags are no longer inadvertently cleared. Now, if the 'vos rename' is interrupted before the volume is unlocked, it will still appear locked in normal vos command output. Change-Id: I6cc16d20c4487de4e9a866c6f0c89d950efd2f7d Reviewed-on: https://gerrit.openafs.org/14157 Tested-by: BuildBot Reviewed-by: Andrew Deason Reviewed-by: Michael Meffie Reviewed-by: Cheyenne Wills Reviewed-by: Benjamin Kaduk --- diff --git a/src/volser/vsprocs.c b/src/volser/vsprocs.c index 12961e3..97ce6a0 100644 --- a/src/volser/vsprocs.c +++ b/src/volser/vsprocs.c @@ -6993,7 +6993,39 @@ UV_RenameVolume(struct nvldbentry *entry, char oldname[], char newname[]) goto rvfail; } islocked = 1; + + /* + * Match the flags we just set via SetLock, + * so we don't invalidate our compare below. + */ + entry->flags &= ~VLOP_ALLOPERS; + entry->flags |= VLOP_ADDSITE; + + /* + * Now get the entry again (under lock) and + * verify the volume hasn't otherwise changed. + */ + vcode = VLDB_GetEntryByID(entry->volumeId[RWVOL], RWVOL, &storeEntry); + if (vcode) { + fprintf(STDERR, + "Could not obtain the VLDB entry for the volume %u\n", + entry->volumeId[RWVOL]); + error = vcode; + goto rvfail; + } + /* Convert to net order to match entry, which was passed in net order. */ + MapHostToNetwork(&storeEntry); + if (memcmp(entry, &storeEntry, sizeof(*entry)) != 0) { + fprintf(STDERR, + "VLDB entry for volume %u has changed; " + "please reissue the command.\n", + entry->volumeId[RWVOL]); + error = VL_BADENTRY; /* an arbitrary choice, but closest to the truth */ + goto rvfail; + } + strncpy(entry->name, newname, VOLSER_OLDMAXVOLNAME); + /* Note that we are reusing storeEntry. */ MapNetworkToHost(entry, &storeEntry); vcode = VLDB_ReplaceEntry(entry->volumeId[RWVOL], RWVOL, &storeEntry, 0); if (vcode) { diff --git a/src/volser/vsutils.c b/src/volser/vsutils.c index eec694d..642d273 100644 --- a/src/volser/vsutils.c +++ b/src/volser/vsutils.c @@ -136,6 +136,7 @@ VLDB_GetEntryByID(afs_uint32 volid, afs_int32 voltype, struct nvldbentry *entryp ovlentry_to_nvlentry(&oentry, entryp); return code; } + memset(entryp, 0, sizeof(*entryp)); /* ensure padding is cleared */ code = ubik_VL_GetEntryByIDN(cstruct, 0, volid, voltype, entryp); if (newvlserver == vltype_unknown) { if (code == RXGEN_OPCODE) { @@ -161,6 +162,7 @@ VLDB_GetEntryByName(char *namep, struct nvldbentry *entryp) ovlentry_to_nvlentry(&oentry, entryp); return code; } + memset(entryp, 0, sizeof(*entryp)); /* ensure padding is cleared */ code = ubik_VL_GetEntryByNameN(cstruct, 0, namep, entryp); if (newvlserver == vltype_unknown) { if (code == RXGEN_OPCODE) {