vol: Do not give back not-checked-out vols

VAttachVolumeByName_r has logic to give back a volume over FSSYNC if
we checked out a volume but failed to attach it for whatever reason.
However, the logic used for determining if the volume was checked out
or not is a bit inaccurate (even moreso than the comments imply),
potentially causing us to VOL_ON volumes that don't exist at all.

Instead of trying to guess based on various conditions whether or not
we checked out the volume, keep track of a variable that is only set
when we actually checkout the volume from the fileserver. Then only
give back the volume if it is set.

Change-Id: I03197eca3e1a31a4b9566552eb9032fdc7cc5909
Reviewed-on: http://gerrit.openafs.org/3274
Tested-by: Andrew Deason <adeason@sinenomine.net>
Reviewed-by: Derrick Brashear <shadow@dementia.org>
Tested-by: Derrick Brashear <shadow@dementia.org>

diff --git a/src/vol/volume.c b/src/vol/volume.c
index 6686bf45e925572e398d837d410e71d525bb0cf0..d1d6f9fc956593386455debdc2023087b513a13d 100644 (file)
--- a/src/vol/volume.c
+++ b/src/vol/volume.c
@@ -175,7 +175,7 @@ extern void *calloc(), *realloc();
 /* Forward declarations */
 static Volume *attach2(Error * ec, VolId volumeId, char *path,
                       struct DiskPartition64 *partp, Volume * vp,
-                      int isbusy, int mode);
+                      int isbusy, int mode, int *acheckedOut);
 static void ReallyFreeVolume(Volume * vp);
 #ifdef AFS_DEMAND_ATTACH_FS
 static void FreeVolume(Volume * vp);
@@ -2239,6 +2239,7 @@ VAttachVolumeByName_r(Error * ec, char *partition, char *name, int mode)
     char path[64];
     int isbusy = 0;
     VolId volumeId;
+    int checkedOut;
 #ifdef AFS_DEMAND_ATTACH_FS
     VolumeStats stats_save;
     Volume *svp = NULL;
@@ -2403,7 +2404,7 @@ VAttachVolumeByName_r(Error * ec, char *partition, char *name, int mode)
 
     /* attach2 is entered without any locks, and returns
      * with vol_glock_mutex held */
-    vp = attach2(ec, volumeId, path, partp, vp, isbusy, mode);
+    vp = attach2(ec, volumeId, path, partp, vp, isbusy, mode, &checkedOut);
 
     if (VCanUseFSSYNC() && vp) {
 #ifdef AFS_DEMAND_ATTACH_FS
@@ -2431,20 +2432,11 @@ VAttachVolumeByName_r(Error * ec, char *partition, char *name, int mode)
            vp->needsPutBack = 1;
 #endif /* !AFS_DEMAND_ATTACH_FS */
     }
-    /* OK, there's a problem here, but one that I don't know how to
-     * fix right now, and that I don't think should arise often.
-     * Basically, we should only put back this volume to the server if
-     * it was given to us by the server, but since we don't have a vp,
-     * we can't run the VolumeWriteable function to find out as we do
-     * above when computing vp->needsPutBack.  So we send it back, but
-     * there's a path in VAttachVolume on the server which may abort
-     * if this volume doesn't have a header.  Should be pretty rare
-     * for all of that to happen, but if it does, probably the right
-     * fix is for the server to allow the return of readonly volumes
-     * that it doesn't think are really checked out. */
 #ifdef FSSYNC_BUILD_CLIENT
-    if (VCanUseFSSYNC() && vp == NULL &&
-       mode != V_SECRETLY && mode != V_PEEK) {
+    /* Only give back the vol to the fileserver if we checked it out; attach2
+     * will set checkedOut only if we successfully checked it out from the
+     * fileserver. */
+    if (VCanUseFSSYNC() && vp == NULL && checkedOut) {
 
 #ifdef AFS_DEMAND_ATTACH_FS
         /* If we couldn't attach but we scheduled a salvage, we already
@@ -2532,6 +2524,7 @@ VAttachVolumeByVp_r(Error * ec, Volume * vp, int mode)
     VolId volumeId;
     Volume * nvp = NULL;
     VolumeStats stats_save;
+    int checkedOut;
     *ec = 0;
 
     /* volume utility should never call AttachByVp */
@@ -2599,7 +2592,7 @@ VAttachVolumeByVp_r(Error * ec, Volume * vp, int mode)
      *
      * NOTE: attach2 is entered without any locks, and returns
      * with vol_glock_mutex held */
-    vp = attach2(ec, volumeId, path, partp, vp, isbusy, mode);
+    vp = attach2(ec, volumeId, path, partp, vp, isbusy, mode, &checkedOut);
 
     /*
      * the event that an error was encountered, or
@@ -2722,6 +2715,9 @@ VUnlockVolume(Volume *vp)
  *                    we don't try to lock the vol, or check it out from
  *                    FSSYNC or anything like that; 0 otherwise, for 'normal'
  *                    operation
+ * @param[out] acheckedOut   If we successfully checked-out the volume from
+ *                           the fileserver (if we needed to), this is set
+ *                           to 1, otherwise it is untouched.
  *
  * @note As part of DAFS volume attachment, the volume header may be either
  *       read- or write-locked to ensure mutual exclusion of certain volume
@@ -2734,7 +2730,7 @@ VUnlockVolume(Volume *vp)
  */
 static void
 attach_volume_header(Error *ec, Volume *vp, struct DiskPartition64 *partp,
-                     int mode, int peek)
+                     int mode, int peek, int *acheckedOut)
 {
     struct VolumeDiskHeader diskHeader;
     struct VolumeHeader header;
@@ -2796,6 +2792,7 @@ attach_volume_header(Error *ec, Volume *vp, struct DiskPartition64 *partp,
             }
            goto done;
        }
+       *acheckedOut = 1;
     }
 #endif
 
@@ -2961,7 +2958,7 @@ attach_volume_header(Error *ec, Volume *vp, struct DiskPartition64 *partp,
 #ifdef AFS_DEMAND_ATTACH_FS
 static void
 attach_check_vop(Error *ec, VolumeId volid, struct DiskPartition64 *partp,
-                 Volume *vp)
+                 Volume *vp, int *acheckedOut)
 {
     *ec = 0;
 
@@ -2986,7 +2983,7 @@ attach_check_vop(Error *ec, VolumeId volid, struct DiskPartition64 *partp,
                /* attach header with peek=1 to avoid checking out the volume
                 * or locking it; we just want the header info, we're not
                 * messing with the volume itself at all */
-               attach_volume_header(ec, vp, partp, V_PEEK, 1);
+               attach_volume_header(ec, vp, partp, V_PEEK, 1, acheckedOut);
                if (*ec) {
                    return;
                }
@@ -3058,6 +3055,9 @@ attach_check_vop(Error *ec, VolumeId volid, struct DiskPartition64 *partp,
  *                     otherwise. (see VVolOpSetVBusy_r)
  * @param[in] mode     attachment mode such as V_VOLUPD, V_DUMP, etc (see
  *                     volume.h)
+ * @param[out] acheckedOut   If we successfully checked-out the volume from
+ *                           the fileserver (if we needed to), this is set
+ *                           to 1, otherwise it is 0.
  *
  * @return pointer to the semi-attached volume pointer
  *  @retval NULL an error occurred (check value of *ec)
@@ -3069,7 +3069,7 @@ attach_check_vop(Error *ec, VolumeId volid, struct DiskPartition64 *partp,
  */
 static Volume *
 attach2(Error * ec, VolId volumeId, char *path, struct DiskPartition64 *partp,
-        Volume * vp, int isbusy, int mode)
+        Volume * vp, int isbusy, int mode, int *acheckedOut)
 {
     /* have we read in the header successfully? */
     int read_header = 0;
@@ -3091,13 +3091,15 @@ attach2(Error * ec, VolId volumeId, char *path, struct DiskPartition64 *partp,
     vp->diskDataHandle = NULL;
     vp->linkHandle = NULL;
 
+    *acheckedOut = 0;
+
 #ifdef AFS_DEMAND_ATTACH_FS
-    attach_check_vop(ec, volumeId, partp, vp);
+    attach_check_vop(ec, volumeId, partp, vp, acheckedOut);
     if (!*ec) {
-       attach_volume_header(ec, vp, partp, mode, 0);
+       attach_volume_header(ec, vp, partp, mode, 0, acheckedOut);
     }
 #else
-    attach_volume_header(ec, vp, partp, mode, 0);
+    attach_volume_header(ec, vp, partp, mode, 0, acheckedOut);
 #endif /* !AFS_DEMAND_ATTACH_FS */
 
     if (*ec == VNOVOL) {