DAFS: Remove AFS_DEMAND_ATTACH_UTIL

Currently we have two DAFS-related preprocessor defines in the
codebase: AFS_DEMAND_ATTACH_FS and AFS_DEMAND_ATTACH_UTIL. DAFS_FS is
the symbol for enabling DAFS code, and turns on demand attachment and
all of the related complicated volume handling; it requires pthreads.
DAFS_UTIL is supposed to be used for utilities interacting with DAFS,
but do not have pthreads and so cannot build the relevant threads for
e.g. the VLRU, so they don't support demand attachment and a lot of
more advanced volume handling techniques.

Having both of these exist is confusing. For example, currently in
partition.c we only initialize dp->volLockFile for DAFS_FS, even
though the structure exists if _either_ DAFS_FS or DAFS_UTIL is
defined. This means when only DAFS_UTIL is defined, volLockFile will
exist in the partition structure, but will be uninitialized!

Amongst other possible issues, this means right now that DAFS_UTIL
users (dasalvager is the only one right now) will try to use an
uninitialized volLockFile whenever they try to use a volume that needs
locking. Since the partition struct is usually initialized to all
zeroes, this means we'll try to issue a lock request for FD 0,
whatever FD 0 is. If FD 0 is not open, we'll fail with EBADF and bail
out. But if FD 0 is open to some random file, the lock will probably
succeed, and we'll proceed without actually locking the volume lock
file. While the fssync volume checkout mechanism still works, the
on-disk locking mechanism protects against race conditions the fssync
volume checkout mechanism cannot protect against, and so handling
volumes in this way is not safe.

This is just one example; there are other issues with the partition
headerLockFile and probably may other things; most instances of
DAFS_FS really should be enabled for DAFS_UTIL as well.

So, instead of trying to account for and fix all of these problems
individually, get rid of AFS_DEMAND_ATTACH_UTIL, and just use
AFS_DEMAND_ATTACH_FS. This means that all relevant code must be
pthreaded, but since the only relevant code is for the dasalvager, we
can just make dasalvager pthreaded. Salvaging does not make use of any
threads or LWPs, so this should not have any side-effects.

Thanks to Ralf Brunckhorst for reporting the issue where we encounter
EBADF when FD 0 is not open, leading to the discovery of this.

Change-Id: I3848eb877f26b9d65833d5ce0e03f5cf7ba28de4
Reviewed-on: http://gerrit.openafs.org/10123
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Derrick Brashear <shadow@your-file-system.com>

diff --git a/src/tsalvaged/Makefile.in b/src/tsalvaged/Makefile.in
index 169255a..5e8265d 100644 (file)
--- a/src/tsalvaged/Makefile.in
+++ b/src/tsalvaged/Makefile.in
@@ -18,7 +18,7 @@ MODULE_CFLAGS = -DRXDEBUG -DFSSYNC_BUILD_CLIENT \
                -DAFS_DEMAND_ATTACH_FS
 
 SCFLAGS=$(COMMON_CFLAGS) -I.. -DRXDEBUG -DFSSYNC_BUILD_CLIENT \
-       -DAFS_DEMAND_ATTACH_UTIL $(PTH_CFLAGS)
+       -DAFS_DEMAND_ATTACH_FS $(PTH_CFLAGS)
 
 SCCRULE=$(RUN_CC) ${MT_CC} ${SCFLAGS} -c $? -o $@
 

diff --git a/src/vol/partition.h b/src/vol/partition.h
index 6be1c29..5fcecb0 100644 (file)
--- a/src/vol/partition.h
+++ b/src/vol/partition.h
@@ -33,7 +33,7 @@
 #endif
 
 #include "lock.h"
-#if defined(AFS_DEMAND_ATTACH_FS) || defined(AFS_DEMAND_ATTACH_UTIL)
+#ifdef AFS_DEMAND_ATTACH_FS
 # include <pthread.h>
 #endif
 
@@ -63,12 +63,12 @@ struct VLockFile {
     char *path;             /**< path to the lock file */
     int refcount;           /**< how many locks we have on the file */
 
-#if defined(AFS_PTHREAD_ENV) || defined(AFS_DEMAND_ATTACH_UTIL)
+#ifdef AFS_PTHREAD_ENV
     pthread_mutex_t mutex;  /**< lock for the VLockFile struct */
-#endif /* AFS_PTHREAD_ENV || AFS_DEMAND_ATTACH_UTIL */
+#endif /* AFS_PTHREAD_ENV */
 };
 
-#if defined(AFS_DEMAND_ATTACH_FS) || defined(AFS_DEMAND_ATTACH_UTIL)
+#ifdef AFS_DEMAND_ATTACH_FS
 /*
  * flag bits for 'flags' in struct VDiskLock.
  */
@@ -90,7 +90,7 @@ struct VDiskLock {
 
     unsigned int flags;         /**< see above for flag bits */
 };
-#endif /* AFS_DEMAND_ATTACH_FS || AFS_DEMAND_ATTACH_UTIL */
+#endif /* AFS_DEMAND_ATTACH_FS */
 
 
 /* For NT, the roles of "name" and "devName" are no longer reversed. 
@@ -131,7 +131,7 @@ struct DiskPartition64 {
                                 * from the superblock */
     int flags;
     afs_int64 f_files;         /* total number of files in this partition */
-#if defined(AFS_DEMAND_ATTACH_FS) || defined(AFS_DEMAND_ATTACH_UTIL)
+#ifdef AFS_DEMAND_ATTACH_FS
     struct {
        struct rx_queue head;   /* list of volumes on this partition (VByPList) */
        afs_uint32 len;         /* length of volume list */
@@ -142,7 +142,7 @@ struct DiskPartition64 {
     struct VDiskLock headerLock; /* lock for the collective headers on the partition */
 
     struct VLockFile volLockFile; /* lock file for individual volume locks */
-#endif /* AFS_DEMAND_ATTACH_FS || AFS_DEMAND_ATTACH_UTIL */
+#endif /* AFS_DEMAND_ATTACH_FS */
 };
 
 struct DiskPartitionStats64 {

diff --git a/src/vol/salvager.c b/src/vol/salvager.c
index 6d70361..1a5e92b 100644 (file)
--- a/src/vol/salvager.c
+++ b/src/vol/salvager.c
@@ -319,7 +319,7 @@ handleit(struct cmd_syndesc *as, void *arock)
      */
     if (seenvol) {
        char *msg = NULL;
-#if defined(AFS_DEMAND_ATTACH_FS) || defined(AFS_DEMAND_ATTACH_UTIL)
+#ifdef AFS_DEMAND_ATTACH_FS
        if (!AskDAFS()) {
            msg =
                "The DAFS dasalvager cannot be run with a non-DAFS fileserver.  Please use 'salvager'.";
@@ -509,7 +509,7 @@ main(int argc, char **argv)
 #ifdef FAST_RESTART
     cmd_AddParm(ts, "-DontSalvage", CMD_FLAG, CMD_OPTIONAL,
                "Don't salvage. This my be set in BosConfig to let the fileserver restart immediately after a crash. Bad volumes will be taken offline");
-#elif defined(AFS_DEMAND_ATTACH_FS) || defined(AFS_DEMAND_ATTACH_UTIL)
+#elif defined(AFS_DEMAND_ATTACH_FS)
     cmd_Seek(ts, 20); /* skip DontSalvage */
     cmd_AddParm(ts, "-forceDAFS", CMD_FLAG, CMD_OPTIONAL,
                "For Demand Attach Fileserver, permit a manual volume salvage outside of the salvageserver");

diff --git a/src/vol/vol-salvage.c b/src/vol/vol-salvage.c
index 15b4789..7854696 100644 (file)
--- a/src/vol/vol-salvage.c
+++ b/src/vol/vol-salvage.c
@@ -287,9 +287,9 @@ static int AskVolumeSummary(struct SalvInfo *salvinfo,
 static void MaybeAskOnline(struct SalvInfo *salvinfo, VolumeId volumeId);
 static void AskError(struct SalvInfo *salvinfo, VolumeId volumeId);
 
-#if defined(AFS_DEMAND_ATTACH_FS) || defined(AFS_DEMAND_ATTACH_UTIL)
+#ifdef AFS_DEMAND_ATTACH_FS
 static int LockVolume(struct SalvInfo *salvinfo, VolumeId volumeId);
-#endif /* AFS_DEMAND_ATTACH_FS || AFS_DEMAND_ATTACH_UTIL */
+#endif /* AFS_DEMAND_ATTACH_FS */
 
 /* Uniquifier stored in the Inode */
 static Unique
@@ -716,13 +716,13 @@ SalvageFileSys1(struct DiskPartition64 *partP, VolumeId singleVolumeNumber)
        Abort("Raced too many times with fileserver restarts while trying to "
              "checkout/lock volumes; Aborted\n");
     }
-#if defined(AFS_DEMAND_ATTACH_FS) || defined(AFS_DEMAND_ATTACH_UTIL)
+#ifdef AFS_DEMAND_ATTACH_FS
     if (tries > 1) {
        /* unlock all previous volume locks, since we're about to lock them
         * again */
        VLockFileReinit(&partP->volLockFile);
     }
-#endif /* AFS_DEMAND_ATTACH_FS || AFS_DEMAND_ATTACH_UTIL */
+#endif /* AFS_DEMAND_ATTACH_FS */
 
     salvinfo->fileSysPartition = partP;
     salvinfo->fileSysDevice = salvinfo->fileSysPartition->device;
@@ -741,11 +741,11 @@ SalvageFileSys1(struct DiskPartition64 *partP, VolumeId singleVolumeNumber)
 #endif
 
     if (singleVolumeNumber) {
-#if !(defined(AFS_DEMAND_ATTACH_FS) || defined(AFS_DEMAND_ATTACH_UTIL))
+#ifndef AFS_DEMAND_ATTACH_FS
        /* only non-DAFS locks the partition when salvaging a single volume;
         * DAFS will lock the individual volumes in the VG */
        VLockPartition(partP->name);
-#endif /* !(AFS_DEMAND_ATTACH_FS || AFS_DEMAND_ATTACH_UTIL) */
+#endif /* !AFS_DEMAND_ATTACH_FS */
 
        ForceSalvage = 1;
 
@@ -756,11 +756,11 @@ SalvageFileSys1(struct DiskPartition64 *partP, VolumeId singleVolumeNumber)
 
        salvinfo->useFSYNC = 1;
        AskOffline(salvinfo, singleVolumeNumber);
-#if defined(AFS_DEMAND_ATTACH_FS) || defined(AFS_DEMAND_ATTACH_UTIL)
+#ifdef AFS_DEMAND_ATTACH_FS
        if (LockVolume(salvinfo, singleVolumeNumber)) {
            goto retry;
        }
-#endif /* AFS_DEMAND_ATTACH_FS || AFS_DEMAND_ATTACH_UTIL */
+#endif /* AFS_DEMAND_ATTACH_FS */
 
     } else {
        salvinfo->useFSYNC = 0;
@@ -922,10 +922,10 @@ SalvageFileSys1(struct DiskPartition64 *partP, VolumeId singleVolumeNumber)
 
     if (!Testing && singleVolumeNumber) {
        int foundSVN = 0;
-#if defined(AFS_DEMAND_ATTACH_FS) || defined(AFS_DEMAND_ATTACH_UTIL)
+#ifdef AFS_DEMAND_ATTACH_FS
        /* unlock vol headers so the fs can attach them when we AskOnline */
        VLockFileReinit(&salvinfo->fileSysPartition->volLockFile);
-#endif /* AFS_DEMAND_ATTACH_FS || AFS_DEMAND_ATTACH_UTIL */
+#endif /* AFS_DEMAND_ATTACH_FS */
 
        /* Step through the volumeSummary list and set all volumes on-line.
         * Most volumes were taken off-line in GetVolumeSummary.
@@ -1635,7 +1635,7 @@ RecordHeader(struct DiskPartition64 *dp, const char *name,
 
                AskOffline(salvinfo, summary.header.id);
 
-#if defined(AFS_DEMAND_ATTACH_FS) || defined(AFS_DEMAND_ATTACH_UTIL)
+#ifdef AFS_DEMAND_ATTACH_FS
                if (!badname) {
                    /* don't lock the volume if the header is bad, since we're
                     * about to delete it anyway. */
@@ -1644,7 +1644,7 @@ RecordHeader(struct DiskPartition64 *dp, const char *name,
                        return -1;
                    }
                }
-#endif /* AFS_DEMAND_ATTACH_FS || AFS_DEMAND_ATTACH_UTIL */
+#endif /* AFS_DEMAND_ATTACH_FS */
            }
        }
        if (badname) {
@@ -4190,7 +4190,7 @@ SalvageVolume(struct SalvInfo *salvinfo, struct InodeSummary *rwIsp, IHandle_t *
        }
 #endif /* FSSYNC_BUILD_CLIENT */
 
-#if defined(AFS_DEMAND_ATTACH_FS) || defined(AFS_DEMAND_ATTACH_UTIL)
+#ifdef AFS_DEMAND_ATTACH_FS
        if (!salvinfo->useFSYNC) {
            /* A volume's contents have changed, but the fileserver will not
             * break callbacks on the volume until it tries to load the vol
@@ -4307,7 +4307,7 @@ MaybeZapVolume(struct SalvInfo *salvinfo, struct InodeSummary *isp,
     }
 }
 
-#if defined(AFS_DEMAND_ATTACH_FS) || defined(AFS_DEMAND_ATTACH_UTIL)
+#ifdef AFS_DEMAND_ATTACH_FS
 /**
  * Locks a volume on disk for salvaging.
  *
@@ -4394,7 +4394,7 @@ LockVolume(struct SalvInfo *salvinfo, VolumeId volumeId)
 
     return 0;
 }
-#endif /* AFS_DEMAND_ATTACH_FS || AFS_DEMAND_ATTACH_UTIL */
+#endif /* AFS_DEMAND_ATTACH_FS */
 
 static void
 AskError(struct SalvInfo *salvinfo, VolumeId volumeId)
@@ -4435,13 +4435,13 @@ AskOffline(struct SalvInfo *salvinfo, VolumeId volumeId)
            Log("AskOffline:  fssync protocol mismatch (bad command word '%d'); salvage aborting.\n",
                FSYNC_VOL_OFF);
            if (AskDAFS()) {
-#if defined(AFS_DEMAND_ATTACH_FS) || defined(AFS_DEMAND_ATTACH_UTIL)
+#ifdef AFS_DEMAND_ATTACH_FS
                Log("AskOffline:  please make sure dafileserver, davolserver, salvageserver and dasalvager binaries are same version.\n");
 #else
                Log("AskOffline:  fileserver is DAFS but we are not.\n");
 #endif
            } else {
-#if defined(AFS_DEMAND_ATTACH_FS) || defined(AFS_DEMAND_ATTACH_UTIL)
+#ifdef AFS_DEMAND_ATTACH_FS
                Log("AskOffline:  fileserver is not DAFS but we are.\n");
 #else
                Log("AskOffline:  please make sure fileserver, volserver and salvager binaries are same version.\n");
@@ -4559,13 +4559,13 @@ AskDelete(struct SalvInfo *salvinfo, VolumeId volumeId)
            Log("AskOnline:  fssync protocol mismatch (bad command word '%d')\n",
                FSYNC_VOL_DONE);
            if (AskDAFS()) {
-#if defined(AFS_DEMAND_ATTACH_FS) || defined(AFS_DEMAND_ATTACH_UTIL)
+#ifdef AFS_DEMAND_ATTACH_FS
                Log("AskOnline:  please make sure dafileserver, davolserver, salvageserver and dasalvager binaries are same version.\n");
 #else
                Log("AskOnline:  fileserver is DAFS but we are not.\n");
 #endif
            } else {
-#if defined(AFS_DEMAND_ATTACH_FS) || defined(AFS_DEMAND_ATTACH_UTIL)
+#ifdef AFS_DEMAND_ATTACH_FS
                Log("AskOnline:  fileserver is not DAFS but we are.\n");
 #else
                Log("AskOnline:  please make sure fileserver, volserver and salvager binaries are same version.\n");

diff --git a/src/vol/volume.h b/src/vol/volume.h
index 9290082..5e7d71b 100644 (file)
--- a/src/vol/volume.h
+++ b/src/vol/volume.h
@@ -891,13 +891,13 @@ extern int VChildProcReconnectFS_r(void);
 extern void VOfflineForVolOp_r(Error *ec, Volume *vp, char *message);
 #endif /* AFS_DEMAND_ATTACH_FS */
 
-#if defined(AFS_DEMAND_ATTACH_FS) || defined(AFS_DEMAND_ATTACH_UTIL)
+#ifdef AFS_DEMAND_ATTACH_FS
 struct VDiskLock;
 extern void VDiskLockInit(struct VDiskLock *dl, struct VLockFile *lf,
                           afs_uint32 offset);
 extern int VGetDiskLock(struct VDiskLock *dl, int locktype, int nonblock);
 extern void VReleaseDiskLock(struct VDiskLock *dl, int locktype);
-#endif /* AFS_DEMAND_ATTACH_FS || AFS_DEMAND_ATTACH_UTIL */
+#endif /* AFS_DEMAND_ATTACH_FS */
 extern int VVolOpLeaveOnline_r(Volume * vp, FSSYNC_VolOp_info * vopinfo);
 extern int VVolOpLeaveOnlineNoHeader_r(Volume * vp, FSSYNC_VolOp_info * vopinfo);
 extern int VVolOpSetVBusy_r(Volume * vp, FSSYNC_VolOp_info * vopinfo);

diff --git a/src/vol/volume_inline.h b/src/vol/volume_inline.h
index 2af2976..0d4a6a7 100644 (file)
--- a/src/vol/volume_inline.h
+++ b/src/vol/volume_inline.h
@@ -13,7 +13,7 @@
 #include "volume.h"
 #include "partition.h"
 
-#if defined(AFS_DEMAND_ATTACH_FS) || defined(AFS_DEMAND_ATTACH_UTIL)
+#ifdef AFS_DEMAND_ATTACH_FS
 # include "lock.h"
 #endif
 
@@ -88,7 +88,7 @@ VIsSalvager(ProgramType type)
 static_inline int
 VRequiresPartLock(void)
 {
-#if defined(AFS_DEMAND_ATTACH_FS) || defined(AFS_DEMAND_ATTACH_UTIL)
+#ifdef AFS_DEMAND_ATTACH_FS
     return 0;
 #else
     switch (programType) {
@@ -98,7 +98,7 @@ VRequiresPartLock(void)
     default:
         return 0;
     }
-#endif /* AFS_DEMAND_ATTACH_FS || AFS_DEMAND_ATTACH_UTIL */
+#endif /* AFS_DEMAND_ATTACH_FS */
 }
 
 /**
@@ -169,7 +169,7 @@ VShouldCheckInUse(int mode)
     return 0;
 }
 
-#if defined(AFS_DEMAND_ATTACH_FS) || defined(AFS_DEMAND_ATTACH_UTIL)
+#ifdef AFS_DEMAND_ATTACH_FS
 /**
  * acquire a non-blocking disk lock for a particular volume id.
  *
@@ -288,9 +288,6 @@ VVolLockType(int mode, int writable)
        }
     }
 }
-#endif /* AFS_DEMAND_ATTACH_FS || AFS_DEMAND_ATTACH_UTIL */
-
-#ifdef AFS_DEMAND_ATTACH_FS
 
 /**
  * tells caller whether or not the volume is effectively salvaging.