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 <vol>', 'vos examine
<vol>', '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 <volume>' will still fail with:

  Could not lock VLDB entry for volume <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 <volume>'.  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 <buildbot@rampaginggeek.com>
Reviewed-by: Andrew Deason <adeason@sinenomine.net>
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
Reviewed-by: Cheyenne Wills <cwills@sinenomine.net>
Reviewed-by: Benjamin Kaduk <kaduk@mit.edu>

diff --git a/src/volser/vsprocs.c b/src/volser/vsprocs.c
index 12961e3..97ce6a0 100644 (file)
--- 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 (file)
--- 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) {