From 7f15a1bbb34fa6f0d52800880f31be367d77a64f Mon Sep 17 00:00:00 2001 From: Andrew Deason Date: Thu, 1 Aug 2013 14:06:52 -0500 Subject: [PATCH] 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 Reviewed-by: Derrick Brashear --- src/tsalvaged/Makefile.in | 2 +- src/vol/partition.h | 14 +++++++------- src/vol/salvager.c | 4 ++-- src/vol/vol-salvage.c | 38 +++++++++++++++++++------------------- src/vol/volume.h | 4 ++-- src/vol/volume_inline.h | 11 ++++------- 6 files changed, 35 insertions(+), 38 deletions(-) diff --git a/src/tsalvaged/Makefile.in b/src/tsalvaged/Makefile.in index 169255a..5e8265d 100644 --- 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 --- 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 #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 --- 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 --- 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 --- 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 --- 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. -- 1.9.4