From 735fa5fb090ee0efc2161597a3974f6fa45126f6 Mon Sep 17 00:00:00 2001 From: Mark Vitale Date: Thu, 30 Jan 2020 14:04:05 -0500 Subject: [PATCH] dir: distinguish logical and physical errors on reads The directory package (src/dir) salvage routines DirOK and DirSalvage check a global variable 'DErrno' to distinguish logical errors (e.g. short read) from physical errors (e.g. EIO). However, since the original IBM import, this logic has not worked correctly because there is no longer any code that sets the value of DErrno - its value is always zero. Instead, modify all implementations of ReallyRead to optionally return the errno for low-level IO errors. Also, create a new userspace-only variant - DReadWithErrno() - of the src/dir/buffer.c version of DRead (the version called by DirOK and DirSalvage, and the only caller of ReallyRead) to return the ReallyRead errno upon request. Also create an analogous variant of afs_dir_GetBlobs, afs_dir_GetBlobsWithErrno(). Finally, convert DirOK and DirSalvage to use the new variants and replace DErrno with equivalent logic. Remove all other references to DErrno. Change-Id: I3de182ce49c1682572142da594af5dc2c00ede74 Reviewed-on: https://gerrit.openafs.org/13798 Reviewed-by: Andrew Deason Tested-by: BuildBot Reviewed-by: Michael Meffie Reviewed-by: Benjamin Kaduk --- src/WINNT/afsd/cm_dir.c | 2 -- src/afs/afs_buffer.c | 29 +++++++++++++++++++++++-- src/dir/buffer.c | 44 ++++++++++++++++++++++++++++++-------- src/dir/dir.c | 29 +++++++++++++++++++------ src/dir/dir.h | 3 +++ src/dir/salvage.c | 57 +++++++++++++++++++++++-------------------------- src/dir/test/dtest.c | 46 +++++++++++++++++++++++++++++++-------- src/viced/physio.c | 42 ++++++++++++++++++++++++++---------- src/vol/physio.c | 39 +++++++++++++++++++++++++-------- src/volser/physio.c | 38 +++++++++++++++++++++++++-------- 10 files changed, 241 insertions(+), 88 deletions(-) diff --git a/src/WINNT/afsd/cm_dir.c b/src/WINNT/afsd/cm_dir.c index 7430bd4..521ec20 100644 --- a/src/WINNT/afsd/cm_dir.c +++ b/src/WINNT/afsd/cm_dir.c @@ -24,8 +24,6 @@ #include -afs_int32 DErrno; - afs_uint32 dir_lookup_hits = 0; afs_uint32 dir_lookup_misses = 0; afs_uint32 dir_create_entry = 0; diff --git a/src/afs/afs_buffer.c b/src/afs/afs_buffer.c index 99af18e..e9b05d7 100644 --- a/src/afs/afs_buffer.c +++ b/src/afs/afs_buffer.c @@ -153,12 +153,16 @@ DInit(int abuffers) * \param[in] adc pointer to directory dcache * \param[in] page number of the desired directory page * \param[out] entry buffer to return requested page + * \param[out] physerr (optional) pointer to return errno, if any * * \retval 0 success - * \retval non-zero invalid directory or internal IO error + * \retval non-zero invalid directory or internal IO error; + * if physerr is supplied by caller, it will be set: + * 0 logical error + * errno physical error */ int -DRead(struct dcache *adc, int page, struct DirBuffer *entry) +DReadWithErrno(struct dcache *adc, int page, struct DirBuffer *entry, int *physerr) { /* Read a page from the disk. */ struct buffer *tb, *tb2; @@ -167,6 +171,9 @@ DRead(struct dcache *adc, int page, struct DirBuffer *entry) AFS_STATCNT(DRead); + if (physerr != NULL) + *physerr = 0; + memset(entry, 0, sizeof(struct DirBuffer)); if (adc->f.chunk == 0 && adc->f.chunkBytes == 0) { @@ -260,6 +267,8 @@ DRead(struct dcache *adc, int page, struct DirBuffer *entry) AFS_BUFFER_PAGESIZE); afs_CFileClose(tfile); if (code < AFS_BUFFER_PAGESIZE) { + if (code < 0 && physerr != NULL) + *physerr = -code; code = EIO; goto error; } @@ -278,6 +287,22 @@ DRead(struct dcache *adc, int page, struct DirBuffer *entry) return code; } +/*! + * Read and return the requested directory page. + * + * \param[in] adc pointer to directory dcache + * \param[in] page number of the desired directory page + * \param[out] entry buffer to return requested page + * + * \retval 0 success + * \retval non-zero invalid directory or internal IO error; + */ +int +DRead(struct dcache *adc, int page, struct DirBuffer *entry) +{ + return DReadWithErrno(adc, page, entry, NULL); +} + static void FixupBucket(struct buffer *ap) { diff --git a/src/dir/buffer.c b/src/dir/buffer.c index 1a8da8f..d522d92 100644 --- a/src/dir/buffer.c +++ b/src/dir/buffer.c @@ -83,12 +83,12 @@ struct buffer *newslot(dir_file_t dir, afs_int32 apage, * * extern void FidZero(DirHandle *); * extern int FidEq(DirHandle *a, DirHandle *b); - * extern int ReallyRead(DirHandle *a, int block, char *data); + * extern int ReallyRead(DirHandle *a, int block, char *data, int *physerr); */ extern void FidZero(dir_file_t); extern int FidEq(dir_file_t, dir_file_t); -extern int ReallyRead(dir_file_t, int block, char *data); +extern int ReallyRead(dir_file_t, int block, char *data, int *physerr); extern int ReallyWrite(dir_file_t, int block, char *data); extern void FidZap(dir_file_t); extern int FidVolEq(dir_file_t, afs_int32 vid); @@ -148,17 +148,26 @@ DInit(int abuffers) /** * read a page out of a directory object. * - * @param[in] fid directory object fid - * @param[in] page page in hash table to be read + * @param[in] fid directory object fid + * @param[in] page page in hash table to be read + * @param[out] entry buffer to be filled w/ page contents + * @param[out] physerr (optional) pointer to return possible errno * - * @return pointer to requested page in directory cache - * @retval NULL read failed + * @retval 0 success + * @retval EIO logical directory error or physical IO error; + * if *physerr was supplied, it will be set to 0 + * for logical errors, or the errno for physical + * errors. */ int -DRead(dir_file_t fid, int page, struct DirBuffer *entry) +DReadWithErrno(dir_file_t fid, int page, struct DirBuffer *entry, int *physerr) { /* Read a page from the disk. */ struct buffer *tb, *tb2, **bufhead; + int code; + + if (physerr != NULL) + *physerr = 0; memset(entry, 0, sizeof(struct DirBuffer)); @@ -228,11 +237,12 @@ DRead(dir_file_t fid, int page, struct DirBuffer *entry) ObtainWriteLock(&tb->lock); tb->lockers++; ReleaseWriteLock(&afs_bufferLock); - if (ReallyRead(bufferDir(tb), tb->page, tb->data)) { + code = ReallyRead(bufferDir(tb), tb->page, tb->data, physerr); + if (code != 0) { tb->lockers--; FidZap(bufferDir(tb)); /* disaster */ ReleaseWriteLock(&tb->lock); - return EIO; + return code; } /* Note that findslot sets the page field in the buffer equal to * what it is searching for. @@ -243,6 +253,22 @@ DRead(dir_file_t fid, int page, struct DirBuffer *entry) return 0; } +/** + * read a page out of a directory object. + * + * @param[in] fid directory object fid + * @param[in] page page in hash table to be read + * @param[out] entry buffer to be filled w/ page contents + * + * @retval 0 success + * @retval EIO logical directory error or physical IO error + */ +int +DRead(dir_file_t fid, int page, struct DirBuffer *entry) +{ + return DReadWithErrno(fid, page, entry, NULL); +} + static int FixupBucket(struct buffer *ap) diff --git a/src/dir/dir.c b/src/dir/dir.c index 8aa336d..eaacd32 100644 --- a/src/dir/dir.c +++ b/src/dir/dir.c @@ -70,8 +70,6 @@ extern int DNew(struct dcache *adc, int page, struct DirBuffer *); # include "dir.h" #endif /* KERNEL */ -afs_int32 DErrno; - /* Local static prototypes */ static int FindBlobs(dir_file_t, int); static void AddPage(dir_file_t, int); @@ -506,11 +504,14 @@ afs_dir_IsEmpty(dir_file_t dir) /* Return a pointer to an entry, given its number. Also return the maximum * size of the entry, which is determined by its position within the directory * page. + * + * If physerr is supplied by caller, it will be set to: + * 0 for logical errors + * errno for physical errors */ - static int GetBlobWithLimit(dir_file_t dir, afs_int32 blobno, - struct DirBuffer *buffer, afs_size_t *maxlen) + struct DirBuffer *buffer, afs_size_t *maxlen, int *physerr) { afs_size_t pos; int code; @@ -518,7 +519,7 @@ GetBlobWithLimit(dir_file_t dir, afs_int32 blobno, *maxlen = 0; memset(buffer, 0, sizeof(struct DirBuffer)); - code = DRead(dir, blobno >> LEPP, buffer); + code = DReadWithErrno(dir, blobno >> LEPP, buffer, physerr); if (code) return code; @@ -531,12 +532,26 @@ GetBlobWithLimit(dir_file_t dir, afs_int32 blobno, return 0; } +/* + * Given an entry's number, return a pointer to that entry. + * If physerr is supplied by caller, it will be set to: + * 0 for logical errors + * errno for physical errors + */ +int +afs_dir_GetBlobWithErrno(dir_file_t dir, afs_int32 blobno, struct DirBuffer *buffer, + int *physerr) +{ + afs_size_t maxlen = 0; + return GetBlobWithLimit(dir, blobno, buffer, &maxlen, physerr); +} + /* Given an entries number, return a pointer to that entry */ int afs_dir_GetBlob(dir_file_t dir, afs_int32 blobno, struct DirBuffer *buffer) { afs_size_t maxlen = 0; - return GetBlobWithLimit(dir, blobno, buffer, &maxlen); + return GetBlobWithLimit(dir, blobno, buffer, &maxlen, NULL); } /* Return an entry, having verified that the name held within the entry @@ -554,7 +569,7 @@ afs_dir_GetVerifiedBlob(dir_file_t file, afs_int32 blobno, int code; char *cp; - code = GetBlobWithLimit(file, blobno, &buffer, &maxlen); + code = GetBlobWithLimit(file, blobno, &buffer, &maxlen, NULL); if (code) return code; diff --git a/src/dir/dir.h b/src/dir/dir.h index f5c8eef..3948689 100644 --- a/src/dir/dir.h +++ b/src/dir/dir.h @@ -104,6 +104,8 @@ extern int afs_dir_EnumerateDir(dir_file_t dir, extern int afs_dir_IsEmpty(dir_file_t dir); extern int afs_dir_GetBlob(dir_file_t dir, afs_int32 blobno, struct DirBuffer *); +extern int afs_dir_GetBlobWithErrno(dir_file_t dir, afs_int32 blobno, + struct DirBuffer *, int *physerr); extern int afs_dir_GetVerifiedBlob(dir_file_t dir, afs_int32 blobno, struct DirBuffer *); extern int afs_dir_DirHash(char *string); @@ -119,6 +121,7 @@ extern int afs_dir_ChangeFid(dir_file_t dir, char *entry, extern void DInit(int abuffers); extern int DRead(dir_file_t fid, int page, struct DirBuffer *); +extern int DReadWithErrno(dir_file_t fid, int page, struct DirBuffer *, int *physerr); extern int DFlush(void); extern int DFlushVolume(afs_int32); extern int DNew(dir_file_t fid, int page, struct DirBuffer *); diff --git a/src/dir/salvage.c b/src/dir/salvage.c index 3fb195c..a720f95 100644 --- a/src/dir/salvage.c +++ b/src/dir/salvage.c @@ -31,8 +31,6 @@ extern void Log(const char *format, ...) #define MAXENAME 256 -extern afs_int32 DErrno; - /* figure out how many pages in use in a directory, given ptr to its (locked) * header */ static int @@ -82,18 +80,19 @@ DirOK(void *file) afs_int32 entcount, maxents; unsigned short ne; int code; + int physerr; eaSize = BIGMAXPAGES * EPP / 8; /* Read the directory header */ - code = DRead(file,0, &headerbuf); + code = DReadWithErrno(file, 0, &headerbuf, &physerr); if (code) { - /* if DErrno is 0, then we know that the read worked, but was short, + /* if physerr is 0, then we know that the read worked, but was short, * and the damage is permanent. Otherwise, we got an I/O or programming * error. Claim the dir is OK, but log something. */ - if (DErrno != 0) { - printf("Could not read first page in directory (%d)\n", DErrno); + if (physerr != 0) { + printf("Could not read first page in directory (%d)\n", physerr); Die("dirok1"); AFS_UNREACHED(return 1); } @@ -171,12 +170,12 @@ DirOK(void *file) */ for (i = 0; i < usedPages; i++) { /* Read the page header */ - code = DRead(file, i, &pagebuf); + code = DReadWithErrno(file, i, &pagebuf, &physerr); if (code) { DRelease(&headerbuf, 0); - if (DErrno != 0) { + if (physerr != 0) { /* couldn't read page, but not because it wasn't there permanently */ - printf("Failed to read dir page %d (errno %d)\n", i, DErrno); + printf("Failed to read dir page %d (errno %d)\n", i, physerr); Die("dirok2"); AFS_UNREACHED(return 1); } @@ -261,15 +260,14 @@ DirOK(void *file) } /* Read the directory entry */ - DErrno = 0; - code = afs_dir_GetBlob(file, entry, &entrybuf); + code = afs_dir_GetBlobWithErrno(file, entry, &entrybuf, &physerr); if (code) { - if (DErrno != 0) { + if (physerr != 0) { /* something went wrong reading the page, but it wasn't * really something wrong with the dir that we can fix. */ printf("Could not get dir blob %d (errno %d)\n", entry, - DErrno); + physerr); DRelease(&headerbuf, 0); Die("dirok3"); } @@ -380,17 +378,18 @@ DirOK(void *file) * Note that if this matches, alloMap has already been checked against it. */ for (i = 0; i < usedPages; i++) { - code = DRead(file, i, &pagebuf); + code = DReadWithErrno(file, i, &pagebuf, &physerr); if (code) { printf ("Failed on second attempt to read dir page %d (errno %d)\n", - i, DErrno); + i, physerr); DRelease(&headerbuf, 0); - /* if DErrno is 0, then the dir is really bad, and we return dir *not* OK. - * otherwise, we want to return true (1), meaning the dir isn't known - * to be bad (we can't tell, since I/Os are failing. + /* if physerr is 0, then the dir is really bad, and we return dir + * *not* OK. Otherwise, we Die instead of returning true (1), + * becauase the dir isn't known to be bad (we can't tell, since + * I/Os are failing). */ - if (DErrno != 0) + if (physerr != 0) Die("dirok4"); else return 0; /* dir is really shorter */ @@ -443,6 +442,7 @@ DirSalvage(void *fromFile, void *toFile, afs_int32 vn, afs_int32 vu, struct DirHeader *dhp; struct DirEntry *ep; int entry; + int physerr; memset(dot, 0, sizeof(dot)); memset(dotdot, 0, sizeof(dotdot)); @@ -453,17 +453,15 @@ DirSalvage(void *fromFile, void *toFile, afs_int32 vn, afs_int32 vu, afs_dir_MakeDir(toFile, dot, dotdot); /* Returns no error code. */ - /* Find out how many pages are valid, using stupid heuristic since DRead - * never returns null. - */ - code = DRead(fromFile, 0, &headerbuf); + /* Find out how many pages are valid. */ + code = DReadWithErrno(fromFile, 0, &headerbuf, &physerr); if (code) { printf("Failed to read first page of fromDir!\n"); - /* if DErrno != 0, then our call failed and we should let our + /* if physerr != 0, then our call failed and we should let our * caller know that there's something wrong with the new dir. If not, * then we return here anyway, with an empty, but at least good, directory. */ - return DErrno; + return physerr; } dhp = (struct DirHeader *)headerbuf.data; @@ -481,15 +479,14 @@ DirSalvage(void *fromFile, void *toFile, afs_int32 vn, afs_int32 vu, break; } - DErrno = 0; - code = afs_dir_GetBlob(fromFile, entry, &entrybuf); + code = afs_dir_GetBlobWithErrno(fromFile, entry, &entrybuf, &physerr); if (code) { - if (DErrno) { + if (physerr != 0) { printf ("can't continue down hash chain (entry %d, errno %d)\n", - entry, DErrno); + entry, physerr); DRelease(&headerbuf, 0); - return DErrno; + return physerr; } printf ("Warning: bogus hash chain encountered, switching to next.\n"); diff --git a/src/dir/test/dtest.c b/src/dir/test/dtest.c index 17b58f5..dd58e8f 100644 --- a/src/dir/test/dtest.c +++ b/src/dir/test/dtest.c @@ -191,18 +191,46 @@ CreateDir(char *name, dirhandle *dir) } } +/* + * Read the specified page from a directory object + * + * \parm[in] dir handle to the directory object + * \parm[in] block requested page from the directory object + * \parm[out] data buffer for the returned page + * \parm[out] physerr (optional) pointer to errno if physical error + * + * \retval 0 success + * \retval EIO physical or logical error; + * if physerr is supplied by caller, it will be set to: + * 0 for logical errors + * errno for physical errors + */ int -ReallyRead(dirhandle *dir, int block, char *data) +ReallyRead(dirhandle *dir, int block, char *data, int *physerr) { - int code; - if (lseek(dir->fd, block * PAGESIZE, 0) == -1) - return errno; + int code = 0; + + if (lseek(dir->fd, block * PAGESIZE, 0) == -1) { + code = EIO; + goto done; + } code = read(dir->fd, data, PAGESIZE); - if (code < 0) - return errno; - if (code != PAGESIZE) - return EIO; - return 0; + if (code < 0) { + code = EIO; + goto done; + } + if (code != PAGESIZE) { + errno = 0; /* logical error - short read */ + code = EIO; + goto done; + } + + errno = 0; + + done: + if (physerr != NULL) + *physerr = errno; + return code; } int diff --git a/src/viced/physio.c b/src/viced/physio.c index 345c23d..4f0dfe2 100644 --- a/src/viced/physio.c +++ b/src/viced/physio.c @@ -39,43 +39,63 @@ afs_int32 lpErrno, lpCount; -/* returns 0 on success, errno on failure */ +/* + * Read the specified page from a directory object + * + * \parm[in] file handle to the directory object + * \parm[in] block requested page from the directory object + * \parm[out] data buffer for the returned page + * \parm[out] physerr (optional) pointer to errno if physical error + * + * \retval 0 success + * \retval EIO physical or logical error; + * if physerr is supplied by caller, it will be set to: + * 0 for logical errors + * errno for physical errors + */ int -ReallyRead(DirHandle * file, int block, char *data) +ReallyRead(DirHandle *file, int block, char *data, int *physerr) { - int code; + int saverr = 0; + int code = 0; ssize_t rdlen; FdHandle_t *fdP; afs_ino_str_t stmp; fdP = IH_OPEN(file->dirh_handle); if (fdP == NULL) { - code = errno; + saverr = errno; + code = EIO; ViceLog(0, ("ReallyRead(): open failed device %X inode %s (volume=%" AFS_VOLID_FMT ") errno %d\n", file->dirh_handle->ih_dev, PrintInode(stmp, file->dirh_handle-> ih_ino), - afs_printable_VolumeId_lu(file->dirh_handle->ih_vid), code)); - return code; + afs_printable_VolumeId_lu(file->dirh_handle->ih_vid), saverr)); + goto done; } rdlen = FDH_PREAD(fdP, data, PAGESIZE, ((afs_foff_t)block) * PAGESIZE); if (rdlen != PAGESIZE) { if (rdlen < 0) - code = errno; + saverr = errno; else - code = EIO; + saverr = 0; /* logical error: short read */ + code = EIO; ViceLog(0, ("ReallyRead(): read failed device %X inode %s (volume=%" AFS_VOLID_FMT ") errno %d\n", file->dirh_handle->ih_dev, PrintInode(stmp, file->dirh_handle-> ih_ino), - afs_printable_VolumeId_lu(file->dirh_handle->ih_vid), code)); + afs_printable_VolumeId_lu(file->dirh_handle->ih_vid), saverr)); FDH_REALLYCLOSE(fdP); - return code; + goto done; } FDH_CLOSE(fdP); - return 0; + + done: + if (physerr != NULL) + *physerr = saverr; + return code; } diff --git a/src/vol/physio.c b/src/vol/physio.c index 170c9c7..37afe78 100644 --- a/src/vol/physio.c +++ b/src/vol/physio.c @@ -32,31 +32,52 @@ #include #include "vol_internal.h" -/* returns 0 on success, errno on failure */ +/* + * Read the specified page from a directory object + * + * \parm[in] file handle to the directory object + * \parm[in] block requested page from the directory object + * \parm[out] data buffer for the returned page + * \parm[out] physerr (optional) pointer to errno if physical error + * + * \retval 0 success + * \retval EIO physical or logical error; + * if physerr is supplied by caller, it will be set to: + * 0 for logical errors + * errno for physical errors + */ int -ReallyRead(DirHandle * file, int block, char *data) +ReallyRead(DirHandle *file, int block, char *data, int *physerr) { FdHandle_t *fdP; - int code; + int code = 0; + int saverr = 0; ssize_t nBytes; + errno = 0; fdP = IH_OPEN(file->dirh_handle); if (fdP == NULL) { - code = errno; - return code; + saverr = errno; + code = EIO; + goto done; } nBytes = FDH_PREAD(fdP, data, (afs_fsize_t) AFS_PAGESIZE, ((afs_foff_t)block) * AFS_PAGESIZE); if (nBytes != AFS_PAGESIZE) { if (nBytes < 0) - code = errno; + saverr = errno; else - code = EIO; + saverr = 0; /* logical error: short read */ + code = EIO; FDH_REALLYCLOSE(fdP); - return code; + goto done; } FDH_CLOSE(fdP); - return 0; + + done: + if (physerr != NULL) + *physerr = saverr; + return code; } /* returns 0 on success, errno on failure */ diff --git a/src/volser/physio.c b/src/volser/physio.c index 3562cf8..a1418ad 100644 --- a/src/volser/physio.c +++ b/src/volser/physio.c @@ -22,30 +22,50 @@ #include "vol.h" #include "physio.h" -/* returns 0 on success, errno on failure */ +/* + * Read the specified page from a directory object + * + * \parm[in] file handle to the directory object + * \parm[in] block requested page from the directory object + * \parm[out] data buffer for the returned page + * \parm[out] physerr (optional) pointer to errno if physical error + * + * \retval 0 success + * \retval EIO physical or logical error; + * if physerr is supplied by caller, it will be set to: + * 0 for logical errors + * errno for physical errors + */ int -ReallyRead(DirHandle * file, int block, char *data) +ReallyRead(DirHandle *file, int block, char *data, int *physerr) { FdHandle_t *fdP; - int code; + int code = 0; + int saverr = 0; ssize_t nBytes; errno = 0; fdP = IH_OPEN(file->dirh_handle); if (fdP == NULL) { - code = errno; - return code; + saverr = errno; + code = EIO; + goto done;; } nBytes = FDH_PREAD(fdP, data, AFS_PAGESIZE, ((afs_foff_t)block) * AFS_PAGESIZE); if (nBytes != AFS_PAGESIZE) { if (nBytes < 0) - code = errno; + saverr = errno; else - code = EIO; + saverr = 0; /* logical error: short read */ + code = EIO; FDH_REALLYCLOSE(fdP); - return code; + goto done; } FDH_CLOSE(fdP); - return 0; + + done: + if (physerr != NULL) + *physerr = saverr; + return code; } /* returns 0 on success, errno on failure */ -- 1.9.4