DAFS: Fix demand-salvages of attached volumes

Currently, when an error is encountered for an attached volume, we
call VRequestSalvage_r, which makes the volume go into the
VOL_STATE_SALVAGING state. This state implies that the volume is
offline, however, which is not necessarily the case if we're calling
VRequestSalvage_r from, for example, VAllocVnode_r or VUpdateVolume_r.

So now, make a new state called VOL_STATE_SALVAGE_REQ to indicate when
a salvage has been requested but the volume is not offline yet (and
thus is not yet ready to give to the salvager). If VCheckSalvage finds
a volume in this state, it offlines the volume first. The FSSYNC
VOL_OFF handler now checks for this state, and if we're giving the
volume to the salvager, we wait for the volume to exit that state.

VRequestSalvage_r also gains a new flag, VOL_SALVAGE_NO_OFFLINE. This
is to ensure that the existing salvaging code paths for unattached
volumes does not change (for when VRequesetSalvage_r is called from
attach2). If this flag is passed, we do what we used to do, which is
just salvage the volume without offlining it.

Reviewed-on: http://gerrit.openafs.org/2329
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Tested-by: Derrick Brashear <shadow@dementia.org>
Reviewed-by: Derrick Brashear <shadow@dementia.org>
(cherry picked from commit 0aa7fac246ce044c757530ebe96d3a05c2f33894)
Change-Id: I76db861c862789801d74578c4965a2bc41c2047b
Reviewed-on: http://gerrit.openafs.org/3223

diff --git a/src/vol/fssync-debug.c b/src/vol/fssync-debug.c
index 15e08a973d6d38bbffe4bdee5fc996bc3a68c3c1..a4776debb8c6bf8a486ce79b2e97024d994b4632 100644 (file)
--- a/src/vol/fssync-debug.c
+++ b/src/vol/fssync-debug.c
@@ -555,6 +555,7 @@ vol_state_to_string(VolState state)
        ENUMCASE(VOL_STATE_VNODE_RELEASE);
        ENUMCASE(VOL_STATE_VLRU_ADD);
        ENUMCASE(VOL_STATE_DELETED);
+       ENUMCASE(VOL_STATE_SALVAGE_REQ);
        ENUMCASE(VOL_STATE_FREED);
     default:
        return "**UNKNOWN**";

diff --git a/src/vol/fssync-server.c b/src/vol/fssync-server.c
index 5db43fa33821f95a9987be35c54473f04011e922..9ed8be83bff85bada99a82de6f0328a992280088 100644 (file)
--- a/src/vol/fssync-server.c
+++ b/src/vol/fssync-server.c
@@ -915,14 +915,22 @@ FSYNC_com_VolOff(FSSYNC_VolOp_command * vcom, SYNC_response * res)
            if (vp->salvage.requested && !vp->salvage.scheduled) {
                vp->salvage.scheduled = 1;
            }
+
+           /* If the volume is in VOL_STATE_SALVAGE_REQ, we need to wait
+            * for the vol to go offline before we can give it away. Also
+            * make sure we don't come out with vp in an excl state. */
+           while (V_attachState(vp) == VOL_STATE_SALVAGE_REQ ||
+                  VIsExclusiveState(V_attachState(vp))) {
+
+               VOL_CV_WAIT(&V_attachCV(vp));
+           }
+
        case debugUtility:
            break;
 
        case volumeUtility:
        case volumeServer:
-            if (V_attachState(vp) == VOL_STATE_SALVAGING ||
-               vp->salvage.requested) {
-
+            if (VIsSalvaging(vp)) {
                 Log("denying offline request for volume %lu; volume is in salvaging state\n",
                    afs_printable_uint32_lu(vp->hashid));
                 res->hdr.reason = FSYNC_SALVAGE;

diff --git a/src/vol/volume.c b/src/vol/volume.c
index 552f8eee841f1769b7d46f5774585a1a1ea65597..8ebc10284044b451341be9797295442b01b55eb2 100644 (file)
--- a/src/vol/volume.c
+++ b/src/vol/volume.c
@@ -3182,7 +3182,8 @@ attach2(Error * ec, VolId volumeId, char *path, struct DiskPartition64 *partp,
        if (!VCanScheduleSalvage()) {
            Log("VAttachVolume: Error attaching volume %s; volume needs salvage; error=%u\n", path, *ec);
        }
-       VRequestSalvage_r(ec, vp, SALVSYNC_ERROR, VOL_SALVAGE_INVALIDATE_HEADER);
+       VRequestSalvage_r(ec, vp, SALVSYNC_ERROR, VOL_SALVAGE_INVALIDATE_HEADER |
+                                                 VOL_SALVAGE_NO_OFFLINE);
        vp->nUsers = 0;
 
        goto locked_error;
@@ -3205,7 +3206,8 @@ attach2(Error * ec, VolId volumeId, char *path, struct DiskPartition64 *partp,
        if (!VCanScheduleSalvage()) {
            Log("VAttachVolume: volume salvage flag is ON for %s; volume needs salvage\n", path);
        }
-       VRequestSalvage_r(ec, vp, SALVSYNC_NEEDED, VOL_SALVAGE_INVALIDATE_HEADER);
+       VRequestSalvage_r(ec, vp, SALVSYNC_NEEDED, VOL_SALVAGE_INVALIDATE_HEADER |
+                                                  VOL_SALVAGE_NO_OFFLINE);
        vp->nUsers = 0;
 
 #else /* AFS_DEMAND_ATTACH_FS */
@@ -3227,7 +3229,8 @@ attach2(Error * ec, VolId volumeId, char *path, struct DiskPartition64 *partp,
        if (!VCanScheduleSalvage()) {
            Log("VAttachVolume: volume %s needs to be salvaged; not attached.\n", path);
        }
-       VRequestSalvage_r(ec, vp, SALVSYNC_NEEDED, VOL_SALVAGE_INVALIDATE_HEADER);
+       VRequestSalvage_r(ec, vp, SALVSYNC_NEEDED, VOL_SALVAGE_INVALIDATE_HEADER |
+                                                  VOL_SALVAGE_NO_OFFLINE);
        vp->nUsers = 0;
 
 #else /* AFS_DEMAND_ATTACH_FS */
@@ -3249,7 +3252,8 @@ attach2(Error * ec, VolId volumeId, char *path, struct DiskPartition64 *partp,
 
 #if defined(AFS_DEMAND_ATTACH_FS)
        /* schedule a salvage so the volume goes away on disk */
-       VRequestSalvage_r(ec, vp, SALVSYNC_ERROR, VOL_SALVAGE_INVALIDATE_HEADER);
+       VRequestSalvage_r(ec, vp, SALVSYNC_ERROR, VOL_SALVAGE_INVALIDATE_HEADER |
+                                                 VOL_SALVAGE_NO_OFFLINE);
        VChangeState_r(vp, VOL_STATE_ERROR);
        vp->nUsers = 0;
 #endif /* AFS_DEMAND_ATTACH_FS */
@@ -3267,7 +3271,8 @@ attach2(Error * ec, VolId volumeId, char *path, struct DiskPartition64 *partp,
            VGetBitmap_r(ec, vp, i);
            if (*ec) {
 #ifdef AFS_DEMAND_ATTACH_FS
-               VRequestSalvage_r(ec, vp, SALVSYNC_ERROR, VOL_SALVAGE_INVALIDATE_HEADER);
+               VRequestSalvage_r(ec, vp, SALVSYNC_ERROR, VOL_SALVAGE_INVALIDATE_HEADER |
+                                                         VOL_SALVAGE_NO_OFFLINE);
                vp->nUsers = 0;
 #endif /* AFS_DEMAND_ATTACH_FS */
                Log("VAttachVolume: error getting bitmap for volume (%s)\n",
@@ -3315,7 +3320,8 @@ attach2(Error * ec, VolId volumeId, char *path, struct DiskPartition64 *partp,
                "%lu; needs salvage\n", (int)*ec,
                afs_printable_uint32_lu(V_id(vp)));
 #ifdef AFS_DEMAND_ATTACH_FS
-           VRequestSalvage_r(ec, vp, SALVSYNC_ERROR, VOL_SALVAGE_INVALIDATE_HEADER);
+           VRequestSalvage_r(ec, vp, SALVSYNC_ERROR, VOL_SALVAGE_INVALIDATE_HEADER |
+                                                     VOL_SALVAGE_NO_OFFLINE);
            vp->nUsers = 0;
 #else /* !AFS_DEMAND_ATTACH_FS */
            *ec = VSALVAGE;
@@ -3720,6 +3726,7 @@ GetVolume(Error * ec, Error * client_ec, VolId volumeId, Volume * hint, int nowa
         *   - PREATTACHED
         *   - ATTACHED
         *   - SALVAGING
+        *   - SALVAGE_REQ
         */
 
        if (vp->salvage.requested) {
@@ -3762,8 +3769,7 @@ GetVolume(Error * ec, Error * client_ec, VolId volumeId, Volume * hint, int nowa
            }
        }
 
-       if ((V_attachState(vp) == VOL_STATE_SALVAGING) ||
-           (*ec == VSALVAGING)) {
+       if (VIsSalvaging(vp) || (*ec == VSALVAGING)) {
            if (client_ec) {
                /* see CheckVnode() in afsfileprocs.c for an explanation
                 * of this error code logic */
@@ -4874,6 +4880,62 @@ VVolOpSetVBusy_r(Volume * vp, FSSYNC_VolOp_info * vopinfo)
 /* online salvager routines                        */
 /***************************************************/
 #if defined(AFS_DEMAND_ATTACH_FS)
+
+/**
+ * offline a volume to let it be salvaged.
+ *
+ * @param[in] vp  Volume to offline
+ *
+ * @return whether we offlined the volume successfully
+ *  @retval 0 volume was not offlined
+ *  @retval 1 volume is now offline
+ *
+ * @note This is similar to VCheckOffline, but slightly different. We do not
+ *       deal with vp->goingOffline, and we try to avoid touching the volume
+ *       header except just to set needsSalvaged
+ *
+ * @pre VOL_LOCK held
+ * @pre vp->nUsers == 0
+ * @pre V_attachState(vp) == VOL_STATE_SALVAGE_REQ
+ */
+static int
+VOfflineForSalvage_r(struct Volume *vp)
+{
+    Error error;
+
+    VCreateReservation_r(vp);
+    VWaitExclusiveState_r(vp);
+
+    if (vp->nUsers || V_attachState(vp) == VOL_STATE_SALVAGING) {
+       /* Someone's using the volume, or someone got to scheduling the salvage
+        * before us. I don't think either of these should be possible, as we
+        * should gain no new heavyweight references while we're trying to
+        * salvage, but just to be sure... */
+       VCancelReservation_r(vp);
+       return 0;
+    }
+
+    VChangeState_r(vp, VOL_STATE_OFFLINING);
+
+    VLRU_Delete_r(vp);
+    if (vp->header) {
+       V_needsSalvaged(vp) = 1;
+       /* ignore error; updating needsSalvaged is just best effort */
+       VUpdateVolume_r(&error, vp, VOL_UPDATE_NOFORCEOFF);
+    }
+    VCloseVolumeHandles_r(vp);
+
+    FreeVolumeHeader(vp);
+
+    /* volume has been effectively offlined; we can mark it in the SALVAGING
+     * state now, which lets FSSYNC give it away */
+    VChangeState_r(vp, VOL_STATE_SALVAGING);
+
+    VCancelReservation_r(vp);
+
+    return 1;
+}
+
 /**
  * check whether a salvage needs to be performed on this volume.
  *
@@ -4899,12 +4961,31 @@ VCheckSalvage(Volume * vp)
 {
     int ret = 0;
 #if defined(SALVSYNC_BUILD_CLIENT) || defined(FSSYNC_BUILD_CLIENT)
-    if (vp->nUsers || vp->nWaiters)
+    if (vp->nUsers)
+       return ret;
+    if (!vp->salvage.requested) {
+       return ret;
+    }
+
+    /* prevent recursion; some of the code below creates and removes
+     * lightweight refs, which can call VCheckSalvage */
+    if (vp->salvage.scheduling) {
        return ret;
+    }
+    vp->salvage.scheduling = 1;
+
+    if (V_attachState(vp) == VOL_STATE_SALVAGE_REQ) {
+       if (!VOfflineForSalvage_r(vp)) {
+           vp->salvage.scheduling = 0;
+           return ret;
+       }
+    }
+
     if (vp->salvage.requested) {
        VScheduleSalvage_r(vp);
        ret = 1;
     }
+    vp->salvage.scheduling = 0;
 #endif /* SALVSYNC_BUILD_CLIENT || FSSYNC_BUILD_CLIENT */
     return ret;
 }
@@ -4974,7 +5055,24 @@ VRequestSalvage_r(Error * ec, Volume * vp, int reason, int flags)
         * fear of a salvage already running for this volume. */
 
        if (vp->stats.salvages < SALVAGE_COUNT_MAX) {
-           VChangeState_r(vp, VOL_STATE_SALVAGING);
+
+           /* if we don't need to offline the volume, we can go directly
+            * to SALVAGING. SALVAGING says the volume is offline and is
+            * either salvaging or ready to be handed to the salvager.
+            * SALVAGE_REQ says that we want to salvage the volume, but we
+            * are waiting for it to go offline first. */
+           if (flags & VOL_SALVAGE_NO_OFFLINE) {
+               VChangeState_r(vp, VOL_STATE_SALVAGING);
+           } else {
+               VChangeState_r(vp, VOL_STATE_SALVAGE_REQ);
+               if (vp->nUsers == 0) {
+                   /* normally VOfflineForSalvage_r would be called from
+                    * PutVolume et al when nUsers reaches 0, but if
+                    * it's already 0, just do it ourselves, since PutVolume
+                    * isn't going to get called */
+                   VOfflineForSalvage_r(vp);
+               }
+           }
            *ec = VSALVAGING;
        } else {
            Log("VRequestSalvage: volume %u online salvaged too many times; forced offline.\n", vp->hashid);
@@ -5162,6 +5260,13 @@ VScheduleSalvage_r(Volume * vp)
        return 1;
     }
 
+    if (vp->salvage.scheduled) {
+       return ret;
+    }
+
+    VCreateReservation_r(vp);
+    VWaitExclusiveState_r(vp);
+
     /*
      * XXX the scheduling process should really be done asynchronously
      *     to avoid fssync deadlocks
@@ -5171,9 +5276,6 @@ VScheduleSalvage_r(Volume * vp)
         *
         * set the volume to an exclusive state and drop the lock
         * around the SALVSYNC call
-        *
-        * note that we do NOT acquire a reservation here -- doing so
-        * could result in unbounded recursion
         */
        strlcpy(partName, VPartitionPath(vp->partition), sizeof(partName));
        state_save = VChangeState_r(vp, VOL_STATE_SALVSYNC_REQ);
@@ -5216,6 +5318,7 @@ VScheduleSalvage_r(Volume * vp)
            }
        }
     }
+    VCancelReservation_r(vp);
     return ret;
 }
 #endif /* SALVSYNC_BUILD_CLIENT || FSSYNC_BUILD_CLIENT */

diff --git a/src/vol/volume.h b/src/vol/volume.h
index fe70dbdd97b82c82180e88fdcce0d7d008f3b136..998bb86aaa505537e8f24c6f2b01dd3842209c2a 100644 (file)
--- a/src/vol/volume.h
+++ b/src/vol/volume.h
@@ -171,9 +171,12 @@ typedef enum {
     VOL_STATE_VNODE_RELEASE     = 18,   /**< volume is busy releasing vnodes */
     VOL_STATE_VLRU_ADD          = 19,   /**< volume is busy being added to a VLRU queue */
     VOL_STATE_DELETED           = 20,   /**< volume has been deleted by the volserver */
+    VOL_STATE_SALVAGE_REQ       = 21,   /**< volume has been requested to be salvaged,
+                                         *   but is waiting for other users to go away
+                                         *   so it can be offlined */
     /* please add new states directly above this line */
-    VOL_STATE_FREED             = 21,   /**< debugging aid */
-    VOL_STATE_COUNT             = 22    /**< total number of valid states */
+    VOL_STATE_FREED             = 22,   /**< debugging aid */
+    VOL_STATE_COUNT             = 23    /**< total number of valid states */
 } VolState;
 
 /**
@@ -603,7 +606,11 @@ typedef struct VolumeOnlineSalvage {
     int reason;                 /**< reason for requesting online salvage */
     byte requested;             /**< flag specifying that salvage should be scheduled */
     byte scheduled;             /**< flag specifying whether online salvage scheduled */
-    byte reserved[2];           /**< padding */
+    byte scheduling;            /**< if nonzero, this volume has entered
+                                 *   VCheckSalvage(), so if we recurse into
+                                 *   VCheckSalvage() with this set, exit immediately
+                                 *   to avoid recursing forever */
+    byte reserved[1];           /**< padding */
 } VolumeOnlineSalvage;
 
 /**
@@ -977,6 +984,8 @@ extern int VWalkVolumeHeaders(struct DiskPartition64 *dp, const char *partpath,
 
 /* VRequestSalvage_r flags */
 #define VOL_SALVAGE_INVALIDATE_HEADER 0x1 /* for demand attach fs, invalidate volume header cache */
+#define VOL_SALVAGE_NO_OFFLINE        0x2 /* we do not need to wait to offline the volume; it has
+                                           * not been fully attached */
 
 
 #if    defined(NEARINODE_HINT)

diff --git a/src/vol/volume_inline.h b/src/vol/volume_inline.h
index 1c717aeb85c6e6c390888569adcd7a34726f56b4..2d8e4987bc47619f539769c1eda7c3df504503ac 100644 (file)
--- a/src/vol/volume_inline.h
+++ b/src/vol/volume_inline.h
@@ -239,6 +239,37 @@ VVolLockType(int mode, int writeable)
     }
 }
 
+/**
+ * tells caller whether or not the volume is effectively salvaging.
+ *
+ * @param vp  volume pointer
+ *
+ * @return whether volume is salvaging or not
+ *  @retval 0 no, volume is not salvaging
+ *  @retval 1 yes, volume is salvaging
+ *
+ * @note The volume may not actually be getting salvaged at the moment if
+ *       this returns 1, but may have just been requested or scheduled to be
+ *       salvaged. Callers should treat these cases as pretty much the same
+ *       anyway, since we should not touch a volume that is busy salvaging or
+ *       waiting to be salvaged.
+ */
+static_inline int
+VIsSalvaging(struct Volume *vp)
+{
+    /* these tests are a bit redundant, but to be safe... */
+    switch(V_attachState(vp)) {
+    case VOL_STATE_SALVAGING:
+    case VOL_STATE_SALVAGE_REQ:
+       return 1;
+    default:
+       if (vp->salvage.requested || vp->salvage.scheduled) {
+           return 1;
+       }
+    }
+    return 0;
+}
+
 /**
  * tells caller whether or not the current state requires
  * exclusive access without holding glock.
@@ -291,6 +322,7 @@ VIsErrorState(VolState state)
     switch (state) {
     case VOL_STATE_ERROR:
     case VOL_STATE_SALVAGING:
+    case VOL_STATE_SALVAGE_REQ:
        return 1;
     default:
        return 0;