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.

Reviewed-on: http://gerrit.openafs.org/10123
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Derrick Brashear <shadow@your-file-system.com>
(cherry picked from commit 7f15a1bbb34fa6f0d52800880f31be367d77a64f)

Change-Id: I56904bc5989ffe346af9213584dee2ef5ce190ff
Reviewed-on: http://gerrit.openafs.org/10167
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Mark Vitale <mvitale@sinenomine.net>
Reviewed-by: Andrew Deason <adeason@sinenomine.net>
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
Reviewed-by: Derrick Brashear <shadow@your-file-system.com>
Reviewed-by: Stephan Wiesand <stephan.wiesand@desy.de>

diff --git a/src/tsalvaged/Makefile.in b/src/tsalvaged/Makefile.in
index 35c9213fa6ba58382b8c85ec3fad6fe0bb6c3eec..0f5dcf8cd2d94adf39d1dd2d38c697e529c9c29b 100644 (file)
--- a/src/tsalvaged/Makefile.in
+++ b/src/tsalvaged/Makefile.in
@@ -18,7 +18,7 @@ INSTALL_SCRIPT = @INSTALL_SCRIPT@
 CC=${MT_CC}
 CFLAGS=${COMMON_CFLAGS} -I.. -DNINTERFACE ${MT_CFLAGS} -DRXDEBUG -DFSSYNC_BUILD_CLIENT \
        -DSALVSYNC_BUILD_SERVER -DSALVSYNC_BUILD_CLIENT -DAFS_DEMAND_ATTACH_FS
-SCFLAGS=${COMMON_CFLAGS} -I.. -DNINTERFACE ${XCFLAGS} ${ARCHFLAGS} -DRXDEBUG -DFSSYNC_BUILD_CLIENT -DAFS_DEMAND_ATTACH_UTIL
+SCFLAGS=${COMMON_CFLAGS} -I.. -DNINTERFACE ${MT_CFLAGS} ${XCFLAGS} ${ARCHFLAGS} -DRXDEBUG -DFSSYNC_BUILD_CLIENT -DAFS_DEMAND_ATTACH_FS
 
 CCRULE=${CC} ${CFLAGS} -c $?
 SCCRULE=${CC} ${SCFLAGS} -c $? -o $@
@@ -210,7 +210,7 @@ salvageserver: ${OBJECTS} ${LIBS}
        ${CC} ${LDFLAGS} -o salvageserver ${OBJECTS} ${LIBS} ${MT_LIBS} ${XLIBS}
 
 dasalvager: ${SOBJECTS} ${SLIBS}
-       ${CC} ${LDFLAGS} -o dasalvager ${SOBJECTS} ${SLIBS} ${XLIBS}
+       ${CC} ${LDFLAGS} -o dasalvager ${SOBJECTS} ${SLIBS} ${MT_LIBS} ${XLIBS}
 
 dafssync-debug: ${FSSDEBUG_OBJS} ${LIBS}
        ${CC} ${LDFLAGS} -o dafssync-debug ${FSSDEBUG_OBJS} ${LIBS} ${MT_LIBS} ${XLIBS}

diff --git a/src/vol/partition.h b/src/vol/partition.h
index ed38d9f84ce004669fc308fcea1330ee46fc02a5..37112a12a3f7ad260b1267b8b4349f6c9fbb8b65 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 reversed. That is, "name"
@@ -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 3902c2de6cbcfc947a41092f9a62629c6c3f03cd..e49747ce79504ff4a3b00558403d61335420bef7 100644 (file)
--- a/src/vol/salvager.c
+++ b/src/vol/salvager.c
@@ -334,7 +334,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'.";
@@ -524,7 +524,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 ceca9b52a47603fa63943b9f4a31d4c03ad027db..e653f1700751b672ffebde337d762232bcd78c32 100644 (file)
--- a/src/vol/vol-salvage.c
+++ b/src/vol/vol-salvage.c
@@ -323,9 +323,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
@@ -754,13 +754,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;
@@ -779,11 +779,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;
 
@@ -794,11 +794,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;
@@ -960,10 +960,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.
@@ -1672,7 +1672,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. */
@@ -1681,7 +1681,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) {
@@ -4233,7 +4233,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
@@ -4350,7 +4350,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.
  *
@@ -4436,7 +4436,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)
@@ -4477,13 +4477,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");
@@ -4601,13 +4601,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 6468d6dae6085f374cdad6ad066a86de9b6f6725..dc41928276d8f2667c4073c7e1cd771aaf089194 100644 (file)
--- a/src/vol/volume.h
+++ b/src/vol/volume.h
@@ -892,13 +892,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 5610d549ee4b0779552c8ef8faf0410cd596e4d4..342288217111644184e412942d75e0132f15b863 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
 
@@ -85,7 +85,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) {
@@ -95,7 +95,7 @@ VRequiresPartLock(void)
     default:
         return 0;
     }
-#endif /* AFS_DEMAND_ATTACH_FS || AFS_DEMAND_ATTACH_UTIL */
+#endif /* AFS_DEMAND_ATTACH_FS */
 }
 
 /**
@@ -166,7 +166,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.
  *
@@ -285,9 +285,6 @@ VVolLockType(int mode, int writeable)
        }
     }
 }
-#endif /* AFS_DEMAND_ATTACH_FS || AFS_DEMAND_ATTACH_UTIL */
-
-#ifdef AFS_DEMAND_ATTACH_FS
 
 /**
  * tells caller whether or not the volume is effectively salvaging.