diff options
author | Henning Baldersheim <balder@yahoo-inc.com> | 2023-08-15 17:22:39 +0200 |
---|---|---|
committer | GitHub <noreply@github.com> | 2023-08-15 17:22:39 +0200 |
commit | 2ae55dbfddc73cfff8619b1735f52895afe1be9e (patch) | |
tree | c43e911c2a75e108b9d905001fc5a93507379130 | |
parent | e72ccbe7d79e47235414909720097c648476eab5 (diff) | |
parent | f15cba5580e897378fe46adfbfd4ee2776740cd4 (diff) |
Merge pull request #28055 from vespa-engine/balder/gc-unused-functionality
Balder/gc unused functionality
-rw-r--r-- | document/src/tests/serialization/vespadocumentserializer_test.cpp | 2 | ||||
-rw-r--r-- | vespalib/src/tests/io/fileutil/fileutiltest.cpp | 42 | ||||
-rw-r--r-- | vespalib/src/vespa/vespalib/io/fileutil.cpp | 208 | ||||
-rw-r--r-- | vespalib/src/vespa/vespalib/io/fileutil.h | 58 |
4 files changed, 46 insertions, 264 deletions
diff --git a/document/src/tests/serialization/vespadocumentserializer_test.cpp b/document/src/tests/serialization/vespadocumentserializer_test.cpp index 1839005d720..03878f43e4b 100644 --- a/document/src/tests/serialization/vespadocumentserializer_test.cpp +++ b/document/src/tests/serialization/vespadocumentserializer_test.cpp @@ -686,7 +686,7 @@ void deserializeAndCheck(const string &file_name, FieldValueT &value, const string &field_name) { File file(file_name); file.open(File::READONLY); - vector<char> content(file.stat()._size); + vector<char> content(file.getFileSize()); size_t r = file.read(&content[0], content.size(), 0); ASSERT_EQUAL(content.size(), r); diff --git a/vespalib/src/tests/io/fileutil/fileutiltest.cpp b/vespalib/src/tests/io/fileutil/fileutiltest.cpp index 5d1d1a14475..93803c1fe9e 100644 --- a/vespalib/src/tests/io/fileutil/fileutiltest.cpp +++ b/vespalib/src/tests/io/fileutil/fileutiltest.cpp @@ -102,15 +102,6 @@ TEST("require that vespalib::File::open works") ASSERT_TRUE(fileExists("mydir/myfile")); f.unlink(); } - // Opening with direct IO support works. - { - File f("mydir/myfile"); - f.open(File::CREATE | File::DIRECTIO, false); - ASSERT_TRUE(fileExists("mydir/myfile")); - if (!f.isOpenWithDirectIO()) { - std::cerr << "This platform does not support direct IO\n"; - } - } // Opening plain file works { File f("myfile"); @@ -126,16 +117,6 @@ TEST("require that vespalib::File::open works") //std::cerr << e.what() << "\n"; EXPECT_EQUAL(IoException::ILLEGAL_PATH, e.getType()); } - // Test opening already open file - { - std::unique_ptr<File> f(new File("myfile")); - f->open(File::CREATE, false); - f->closeFileWhenDestructed(false); - File f2(f->getFileDescriptor(), "myfile"); - f.reset(); - ASSERT_TRUE(f2.isOpen()); - f2.write(" ", 1, 0); - } // Test reopening file in same object { File f("myfile"); @@ -161,29 +142,6 @@ TEST("require that vespalib::File::isOpen works") ASSERT_TRUE(!f.isOpen()); } -TEST("require that vespalib::File::stat works") -{ - std::filesystem::remove(std::filesystem::path("myfile")); - std::filesystem::remove_all(std::filesystem::path("mydir")); - EXPECT_EQUAL(false, fileExists("myfile")); - EXPECT_EQUAL(false, fileExists("mydir")); - std::filesystem::create_directory(std::filesystem::path("mydir")); - File f("myfile"); - f.open(File::CREATE, false); - f.write("foobar", 6, 0); - - FileInfo info = f.stat(); - EXPECT_EQUAL(6, info._size); - EXPECT_EQUAL(true, info._plainfile); - EXPECT_EQUAL(false, info._directory); - - EXPECT_EQUAL(6, f.getFileSize()); - f.close(); - - EXPECT_EQUAL(true, fileExists("myfile")); - EXPECT_EQUAL(true, fileExists("mydir")); -} - TEST("require that vespalib::File::resize works") { std::filesystem::remove(std::filesystem::path("myfile")); diff --git a/vespalib/src/vespa/vespalib/io/fileutil.cpp b/vespalib/src/vespa/vespalib/io/fileutil.cpp index 0a70b343b13..cb478f0f225 100644 --- a/vespalib/src/vespa/vespalib/io/fileutil.cpp +++ b/vespalib/src/vespa/vespalib/io/fileutil.cpp @@ -5,7 +5,6 @@ #include <vespa/vespalib/stllike/asciistream.h> #include <vespa/vespalib/util/size_literals.h> #include <vespa/vespalib/util/stringfmt.h> -#include <ostream> #include <cassert> #include <filesystem> #include <dirent.h> @@ -16,6 +15,8 @@ #include <vespa/log/log.h> LOG_SETUP(".vespalib.io.fileutil"); +namespace fs = std::filesystem; + namespace vespalib { namespace { @@ -27,7 +28,6 @@ processStat(struct stat& filestats, bool result, stringref path) { resval = std::make_unique<FileInfo>(); resval->_plainfile = S_ISREG(filestats.st_mode); resval->_directory = S_ISDIR(filestats.st_mode); - resval->_symlink = S_ISLNK(filestats.st_mode); resval->_size = filestats.st_size; } else if (errno != ENOENT) { asciistream ost; @@ -52,79 +52,14 @@ safeStrerror(int errnum) } -bool -FileInfo::operator==(const FileInfo& fi) const -{ - return (_size == fi._size && _plainfile == fi._plainfile - && _directory == fi._directory); -} - -std::ostream& -operator<<(std::ostream& out, const FileInfo& info) -{ - out << "FileInfo(size: " << info._size; - if (info._plainfile) out << ", plain file"; - if (info._directory) out << ", directory"; - out << ")"; - return out; -} - File::File(stringref filename) : _fd(-1), - _flags(0), - _filename(filename), - _close(true), - _fileReads(0), - _fileWrites(0) -{ -} - -File::File(int fileDescriptor, stringref filename) - : _fd(fileDescriptor), - _flags(0), - _filename(filename), - _close(true), - _fileReads(0), - _fileWrites(0) -{ -} + _filename(filename) +{ } File::~File() { - if (_close && _fd != -1) close(); -} - -File::File(File& f) - : _fd(f._fd), - _flags(f._flags), - _filename(f._filename), - _close(f._close), - _fileReads(f._fileReads), - _fileWrites(f._fileWrites) -{ - f._fd = -1; - f._flags = 0; - f._close = true; - f._fileReads = 0; - f._fileWrites = 0; -} - -File& -File::operator=(File& f) -{ - if (_close && _fd != -1) close(); - _fd = f._fd; - _flags = f._flags; - _filename = f._filename; - _close = f._close; - _fileReads = f._fileReads; - _fileWrites = f._fileWrites; - f._fd = -1; - f._flags = 0; - f._close = true; - f._fileReads = 0; - f._fileWrites = 0; - return *this; + if (_fd != -1) close(); } namespace { @@ -137,9 +72,8 @@ int openAndCreateDirsIfMissing(const string & filename, int flags, bool createDi auto pos = filename.rfind('/'); if (pos != string::npos) { string path(filename.substr(0, pos)); - std::filesystem::create_directories(std::filesystem::path(path)); - LOG(spam, "open(%s, %d): Retrying open after creating parent " - "directories.", filename.c_str(), flags); + fs::create_directories(fs::path(path)); + LOG(spam, "open(%s, %d): Retrying open after creating parent directories.", filename.c_str(), flags); fd = ::open(filename.c_str(), flags, 0644); } } @@ -151,56 +85,28 @@ void File::open(int flags, bool autoCreateDirectories) { if ((flags & File::READONLY) != 0) { if ((flags & File::CREATE) != 0) { - throw IllegalArgumentException( - "Cannot use READONLY and CREATE options at the same time", - VESPA_STRLOC); + throw IllegalArgumentException("Cannot use READONLY and CREATE options at the same time", VESPA_STRLOC); } if ((flags & File::TRUNC) != 0) { - throw IllegalArgumentException( - "Cannot use READONLY and TRUNC options at the same time", - VESPA_STRLOC); + throw IllegalArgumentException("Cannot use READONLY and TRUNC options at the same time", VESPA_STRLOC); } if (autoCreateDirectories) { - throw IllegalArgumentException( - "No point in auto-creating directories on read only access", - VESPA_STRLOC); + throw IllegalArgumentException("No point in auto-creating directories on read only access", VESPA_STRLOC); } } int openflags = ((flags & File::READONLY) != 0 ? O_RDONLY : O_RDWR) | ((flags & File::CREATE) != 0 ? O_CREAT : 0) -#ifdef __linux__ - | ((flags & File::DIRECTIO) != 0 ? O_DIRECT : 0) -#endif | ((flags & File::TRUNC) != 0 ? O_TRUNC: 0); int fd = openAndCreateDirsIfMissing(_filename, openflags, autoCreateDirectories); -#ifdef __linux__ - if (fd < 0 && ((flags & File::DIRECTIO) != 0)) { - openflags = (openflags ^ O_DIRECT); - flags = (flags ^ DIRECTIO); - LOG(debug, "open(%s, %d): Retrying without direct IO due to failure " - "opening with errno(%d): %s", - _filename.c_str(), flags, errno, safeStrerror(errno).c_str()); - fd = openAndCreateDirsIfMissing(_filename, openflags, autoCreateDirectories); - } -#endif if (fd < 0) { asciistream ost; - ost << "open(" << _filename << ", 0x" - << hex << flags << dec << "): Failed, errno(" << errno - << "): " << safeStrerror(errno); + ost << "open(" << _filename << ", 0x" << hex << flags << dec + << "): Failed, errno(" << errno << "): " << safeStrerror(errno); throw IoException(ost.str(), IoException::getErrorType(errno), VESPA_STRLOC); } - _flags = flags; - if (_close && _fd != -1) close(); + if (_fd != -1) close(); _fd = fd; - LOG(debug, "open(%s, %d). File opened with file descriptor %d.", - _filename.c_str(), flags, fd); -} - -void -File::closeFileWhenDestructed(bool closeOnDestruct) -{ - _close = closeOnDestruct; + LOG(debug, "open(%s, %d). File opened with file descriptor %d.", _filename.c_str(), flags, fd); } FileInfo @@ -212,13 +118,11 @@ File::stat() const result = processStat(filestats, fstat(_fd, &filestats) == 0, _filename); assert(result.get()); // The file must exist in a file instance } else { - result = processStat(filestats, - ::stat(_filename.c_str(), &filestats) == 0, - _filename); + result = processStat(filestats, ::stat(_filename.c_str(), &filestats) == 0, _filename); // If the file does not exist yet, act like it does. It will // probably be created when opened. - if (result.get() == 0) { - result.reset(new FileInfo()); + if ( ! result) { + result = std::make_unique<FileInfo>(); result->_size = 0; result->_directory = false; result->_plainfile = true; @@ -232,69 +136,32 @@ File::resize(off_t size) { if (ftruncate(_fd, size) != 0) { asciistream ost; - ost << "resize(" << _filename << ", " << size << "): Failed, errno(" - << errno << "): " << safeStrerror(errno); + ost << "resize(" << _filename << ", " << size << "): Failed, errno(" << errno << "): " << safeStrerror(errno); throw IoException(ost.str(), IoException::getErrorType(errno), VESPA_STRLOC); } - LOG(debug, "resize(%s): Resized to %" PRIu64 " bytes.", - _filename.c_str(), size); -} - -void -File::verifyDirectIO(uint64_t buf, size_t bufsize, off_t offset) const -{ - if (offset % 512 != 0) { - LOG(error, - "Access to file %s failed because offset %" PRIu64 " wasn't 512-byte " - "aligned. Buffer memory address was %" PRIx64 ", length %zu", - _filename.c_str(), static_cast<uint64_t>(offset), buf, bufsize); - assert(false); - } - if (buf % 512 != 0) { - LOG(error, - "Access to file %s failed because buffer memory address %" PRIx64 " " - "wasn't 512-byte aligned. Offset was %" PRIu64 ", length %zu", - _filename.c_str(), buf, static_cast<uint64_t>(offset), bufsize); - assert(false); - } - if (bufsize % 512 != 0) { - LOG(error, - "Access to file %s failed because buffer size %zu wasn't 512-byte " - "aligned. Buffer memory address was %" PRIx64 ", offset %" PRIu64, - _filename.c_str(), bufsize, buf, static_cast<uint64_t>(offset)); - assert(false); - } + LOG(debug, "resize(%s): Resized to %" PRIu64 " bytes.", _filename.c_str(), size); } off_t File::write(const void *buf, size_t bufsize, off_t offset) { - ++_fileWrites; size_t left = bufsize; - LOG(debug, "write(%s): Writing %zu bytes at offset %" PRIu64 ".", - _filename.c_str(), bufsize, offset); - - if (_flags & DIRECTIO) { - verifyDirectIO((uint64_t)buf, bufsize, offset); - } + LOG(debug, "write(%s): Writing %zu bytes at offset %" PRIu64 ".", _filename.c_str(), bufsize, offset); while (left > 0) { ssize_t written = ::pwrite(_fd, buf, left, offset); if (written > 0) { - LOG(spam, "write(%s): Wrote %zd bytes at offset %" PRIu64 ".", - _filename.c_str(), written, offset); + LOG(spam, "write(%s): Wrote %zd bytes at offset %" PRIu64 ".", _filename.c_str(), written, offset); left -= written; buf = ((const char*) buf) + written; offset += written; } else if (written == 0) { - LOG(spam, "write(%s): Wrote %zd bytes at offset %" PRIu64 ".", - _filename.c_str(), written, offset); + LOG(spam, "write(%s): Wrote %zd bytes at offset %" PRIu64 ".", _filename.c_str(), written, offset); assert(false); // Can this happen? } else if (errno != EINTR && errno != EAGAIN) { asciistream ost; - ost << "write(" << _fd << ", " << buf - << ", " << left << ", " << offset << "), Failed, errno(" - << errno << "): " << safeStrerror(errno); + ost << "write(" << _fd << ", " << buf << ", " << left << ", " << offset + << "), Failed, errno(" << errno << "): " << safeStrerror(errno); throw IoException(ost.str(), IoException::getErrorType(errno), VESPA_STRLOC); } } @@ -304,37 +171,23 @@ File::write(const void *buf, size_t bufsize, off_t offset) size_t File::read(void *buf, size_t bufsize, off_t offset) const { - ++_fileReads; size_t remaining = bufsize; - LOG(debug, "read(%s): Reading %zu bytes from offset %" PRIu64 ".", - _filename.c_str(), bufsize, offset); - - if (_flags & DIRECTIO) { - verifyDirectIO((uint64_t)buf, bufsize, offset); - } + LOG(debug, "read(%s): Reading %zu bytes from offset %" PRIu64 ".", _filename.c_str(), bufsize, offset); while (remaining > 0) { ssize_t bytesread = ::pread(_fd, buf, remaining, offset); if (bytesread > 0) { - LOG(spam, "read(%s): Read %zd bytes from offset %" PRIu64 ".", - _filename.c_str(), bytesread, offset); + LOG(spam, "read(%s): Read %zd bytes from offset %" PRIu64 ".", _filename.c_str(), bytesread, offset); remaining -= bytesread; buf = ((char*) buf) + bytesread; offset += bytesread; - if (((_flags & DIRECTIO) != 0) && ((bytesread % 512) != 0) && (offset == getFileSize())) { - LOG(spam, "read(%s): Found EOF. Directio read to unaligned file end at offset %" PRIu64 ".", - _filename.c_str(), offset); - break; - } } else if (bytesread == 0) { // EOF - LOG(spam, "read(%s): Found EOF. Zero bytes read from offset %" PRIu64 ".", - _filename.c_str(), offset); + LOG(spam, "read(%s): Found EOF. Zero bytes read from offset %" PRIu64 ".", _filename.c_str(), offset); break; } else if (errno != EINTR && errno != EAGAIN) { asciistream ost; ost << "read(" << _fd << ", " << buf << ", " << remaining << ", " - << offset << "): Failed, errno(" << errno << "): " - << safeStrerror(errno); + << offset << "): Failed, errno(" << errno << "): " << safeStrerror(errno); throw IoException(ost.str(), IoException::getErrorType(errno), VESPA_STRLOC); } } @@ -419,7 +272,7 @@ bool File::unlink() { close(); - return std::filesystem::remove(std::filesystem::path(_filename)); + return fs::remove(fs::path(_filename)); } DirectoryList @@ -487,8 +340,7 @@ getOpenErrorString(const int osError, stringref filename) { asciistream os; string dirName(dirname(filename)); - os << "error=" << osError << "(\"" << - getErrorString(osError) << "\") fileStat"; + os << "error=" << osError << "(\"" << getErrorString(osError) << "\") fileStat"; addStat(os, filename); os << " dirStat"; addStat(os, dirName); diff --git a/vespalib/src/vespa/vespalib/io/fileutil.h b/vespalib/src/vespa/vespalib/io/fileutil.h index 29e9bd66918..148317a7edf 100644 --- a/vespalib/src/vespa/vespalib/io/fileutil.h +++ b/vespalib/src/vespa/vespalib/io/fileutil.h @@ -43,14 +43,10 @@ struct FileInfo { bool _plainfile; bool _directory; - bool _symlink; off_t _size; - bool operator==(const FileInfo&) const; }; -std::ostream& operator<<(std::ostream&, const FileInfo&); - /** * @brief A File instance is used to access a single open file. * @@ -62,66 +58,43 @@ std::ostream& operator<<(std::ostream&, const FileInfo&); class File { private: int _fd; - int _flags; string _filename; - bool _close; - mutable int _fileReads; // Tracks number of file reads done on this file - mutable int _fileWrites; // Tracks number of file writes done in this file + void sync(); /** - * Verify that direct I/O alignment preconditions hold. Triggers assertion - * failure on violations. + * Get information about the current file. If file is opened, file descriptor + * will be used for stat. If file is not open, and the file does not exist + * yet, you will get fileinfo describing an empty file. */ - void verifyDirectIO(uint64_t buf, size_t bufsize, off_t offset) const; - - /** Copying a file instance, moves any open file descriptor. */ - File(File& f); - File& operator=(File& f); + FileInfo stat() const; public: using UP = std::unique_ptr<File>; /** * If failing to open file using direct IO it will retry using cached IO. */ - enum Flag { READONLY = 1, CREATE = 2, DIRECTIO = 4, TRUNC = 8 }; + enum Flag { READONLY = 1, CREATE = 2, TRUNC = 8 }; /** Create a file instance, without opening the file. */ File(stringref filename); - /** Create a file instance of an already open file. */ - File(int fileDescriptor, stringref filename); - /** Closes the file if not instructed to do otherwise. */ - virtual ~File(); + ~File(); const string& getFilename() const { return _filename; } - virtual void open(int flags, bool autoCreateDirectories = false); + void open(int flags, bool autoCreateDirectories = false); bool isOpen() const { return (_fd != -1); } - bool isOpenWithDirectIO() const { return ((_flags & DIRECTIO) != 0); } - /** - * Whether or not file should be closed when this instance is destructed. - * By default it will be closed. - */ - void closeFileWhenDestructed(bool close); - - virtual int getFileDescriptor() const { return _fd; } - - /** - * Get information about the current file. If file is opened, file descriptor - * will be used for stat. If file is not open, and the file does not exist - * yet, you will get fileinfo describing an empty file. - */ - virtual FileInfo stat() const; + int getFileDescriptor() const { return _fd; } /** * Get the filesize of a file, specified by a file descriptor. * * @throw IoException If we failed to stat the file. */ - virtual off_t getFileSize() const { return stat()._size; } + off_t getFileSize() const { return stat()._size; } /** * Resize the currently open file to a given size, @@ -131,7 +104,7 @@ public: * @param size new size of file * @throw IoException If we failed to resize the file. */ - virtual void resize(off_t size); + void resize(off_t size); /** * Writes data to file. @@ -145,7 +118,7 @@ public: * @throw IoException If we failed to write to the file. * @return Always return bufsize. */ - virtual off_t write(const void *buf, size_t bufsize, off_t offset); + off_t write(const void *buf, size_t bufsize, off_t offset); /** * Read characters from a file. @@ -160,7 +133,7 @@ public: * @return The number of bytes actually read. If less than * bufsize, this indicates that EOF was reached. */ - virtual size_t read(void *buf, size_t bufsize, off_t offset) const; + size_t read(void *buf, size_t bufsize, off_t offset) const; /** * Read the file into a string. @@ -193,9 +166,8 @@ public: */ static void sync(stringref path); - virtual void sync(); - virtual bool close(); - virtual bool unlink(); + bool close(); + bool unlink(); }; /** |