From 9b0a32f80263cf528e56f8facc786e211cc0668c Mon Sep 17 00:00:00 2001 From: Burton Li Date: Mon, 16 Aug 2021 07:30:57 -0700 Subject: [PATCH] Support dynamic sector size in alignment validation for Windows. (#8613) Summary: - Use dynamic section size when calling IsSectorAligned() - Support relative path for GetSectorSize(). - Move buffer and sector alignment check to assert for better retail performance. - Typo fixes. Pull Request resolved: https://github.com/facebook/rocksdb/pull/8613 Reviewed By: ajkr Differential Revision: D30136082 Pulled By: mrambacher fbshipit-source-id: e8cb849befdcae4fea99de5ed5dd6565e612425f --- port/win/env_win.cc | 31 ++++++++++++++--------- port/win/io_win.cc | 62 ++++++++++++++++++++++----------------------- port/win/io_win.h | 10 +++++--- 3 files changed, 56 insertions(+), 47 deletions(-) diff --git a/port/win/env_win.cc b/port/win/env_win.cc index 1cab4cbe9..93e04d389 100644 --- a/port/win/env_win.cc +++ b/port/win/env_win.cc @@ -347,8 +347,7 @@ IOStatus WinFileSystem::NewRandomAccessFile( fileGuard.release(); } } else { - result->reset(new WinRandomAccessFile( - fname, hFile, std::max(GetSectorSize(fname), page_size_), options)); + result->reset(new WinRandomAccessFile(fname, hFile, page_size_, options)); fileGuard.release(); } return s; @@ -433,9 +432,8 @@ IOStatus WinFileSystem::OpenWritableFile( } else { // Here we want the buffer allocation to be aligned by the SSD page size // and to be a multiple of it - result->reset(new WinWritableFile( - fname, hFile, std::max(GetSectorSize(fname), GetPageSize()), - c_BufferCapacity, local_options)); + result->reset(new WinWritableFile(fname, hFile, GetPageSize(), + c_BufferCapacity, local_options)); } return s; } @@ -487,8 +485,7 @@ IOStatus WinFileSystem::NewRandomRWFile(const std::string& fname, } UniqueCloseHandlePtr fileGuard(hFile, CloseHandleFunc); - result->reset(new WinRandomRWFile( - fname, hFile, std::max(GetSectorSize(fname), GetPageSize()), options)); + result->reset(new WinRandomRWFile(fname, hFile, GetPageSize(), options)); fileGuard.release(); return s; @@ -1183,13 +1180,23 @@ bool WinFileSystem::DirExists(const std::string& dname) { size_t WinFileSystem::GetSectorSize(const std::string& fname) { size_t sector_size = kSectorSize; - if (RX_PathIsRelative(RX_FN(fname).c_str())) { - return sector_size; - } - // obtain device handle char devicename[7] = "\\\\.\\"; - int erresult = strncat_s(devicename, sizeof(devicename), fname.c_str(), 2); + int erresult = 0; + if (RX_PathIsRelative(RX_FN(fname).c_str())) { + RX_FILESTRING rx_current_dir; + rx_current_dir.resize(MAX_PATH); + DWORD len = RX_GetCurrentDirectory(MAX_PATH, &rx_current_dir[0]); + if (len == 0) { + return sector_size; + } + rx_current_dir.resize(len); + std::string current_dir = FN_TO_RX(rx_current_dir); + erresult = + strncat_s(devicename, sizeof(devicename), current_dir.c_str(), 2); + } else { + erresult = strncat_s(devicename, sizeof(devicename), fname.c_str(), 2); + } if (erresult) { assert(false); diff --git a/port/win/io_win.cc b/port/win/io_win.cc index 1e662c06d..41a6b8381 100644 --- a/port/win/io_win.cc +++ b/port/win/io_win.cc @@ -11,6 +11,7 @@ #include "port/win/io_win.h" +#include "env_win.h" #include "monitoring/iostats_context_imp.h" #include "test_util/sync_point.h" #include "util/aligned_buffer.h" @@ -30,10 +31,6 @@ inline bool IsPowerOfTwo(const size_t alignment) { return ((alignment) & (alignment - 1)) == 0; } -inline bool IsSectorAligned(const size_t off) { - return (off & (kSectorSize - 1)) == 0; -} - inline bool IsAligned(size_t alignment, const void* ptr) { return ((uintptr_t(ptr)) & (alignment - 1)) == 0; } @@ -186,6 +183,17 @@ size_t GetUniqueIdFromFile(HANDLE /*hFile*/, char* /*id*/, return 0; } +WinFileData::WinFileData(const std::string& filename, HANDLE hFile, + bool direct_io) + : filename_(filename), + hFile_(hFile), + use_direct_io_(direct_io), + sector_size_(WinFileSystem::GetSectorSize(filename)) {} + +bool WinFileData::IsSectorAligned(const size_t off) const { + return (off & (sector_size_ - 1)) == 0; +} + //////////////////////////////////////////////////////////////////////////////////////////////////// // WinMmapReadableFile @@ -624,10 +632,8 @@ IOStatus WinSequentialFile::PositionedRead(uint64_t offset, size_t n, return IOStatus::NotSupported("This function is only used for direct_io"); } - if (!IsSectorAligned(static_cast(offset)) || !IsSectorAligned(n)) { - return IOStatus::InvalidArgument( - "WinSequentialFile::PositionedRead: offset is not properly aligned"); - } + assert(IsSectorAligned(static_cast(offset))); + assert(IsSectorAligned(static_cast(n))); size_t bytes_read = 0; // out param IOStatus s = PositionedReadInternal(scratch, static_cast(n), offset, @@ -671,7 +677,8 @@ inline IOStatus WinRandomAccessImpl::PositionedReadInternal( inline WinRandomAccessImpl::WinRandomAccessImpl(WinFileData* file_base, size_t alignment, const FileOptions& options) - : file_base_(file_base), alignment_(alignment) { + : file_base_(file_base), + alignment_(std::max(alignment, file_base->GetSectorSize())) { assert(!options.use_mmap_reads); } @@ -680,12 +687,8 @@ inline IOStatus WinRandomAccessImpl::ReadImpl(uint64_t offset, size_t n, char* scratch) const { // Check buffer alignment if (file_base_->use_direct_io()) { - if (!IsSectorAligned(static_cast(offset)) || - !IsAligned(alignment_, scratch)) { - return IOStatus::InvalidArgument( - "WinRandomAccessImpl::ReadImpl: offset or scratch is not properly " - "aligned"); - } + assert(file_base_->IsSectorAligned(static_cast(offset))); + assert(IsAligned(alignment_, scratch)); } if (n == 0) { @@ -741,7 +744,7 @@ inline IOStatus WinWritableImpl::PreallocateInternal(uint64_t spaceToReserve) { inline WinWritableImpl::WinWritableImpl(WinFileData* file_data, size_t alignment) : file_data_(file_data), - alignment_(alignment), + alignment_(std::max(alignment, file_data->GetSectorSize())), next_write_offset_(0), reservedsize_(0) { // Query current position in case ReopenWritableFile is called @@ -774,14 +777,10 @@ inline IOStatus WinWritableImpl::AppendImpl(const Slice& data) { if (file_data_->use_direct_io()) { // With no offset specified we are appending // to the end of the file - assert(IsSectorAligned(next_write_offset_)); - if (!IsSectorAligned(data.size()) || - !IsAligned(static_cast(GetAlignement()), data.data())) { - s = IOStatus::InvalidArgument( - "WriteData must be page aligned, size must be sector aligned"); - } else { - s = pwrite(file_data_, data, next_write_offset_, bytes_written); - } + assert(file_data_->IsSectorAligned(next_write_offset_)); + assert(file_data_->IsSectorAligned(data.size())); + assert(IsAligned(static_cast(GetAlignment()), data.data())); + s = pwrite(file_data_, data, next_write_offset_, bytes_written); } else { DWORD bytesWritten = 0; if (!WriteFile(file_data_->GetFileHandle(), data.data(), @@ -812,12 +811,9 @@ inline IOStatus WinWritableImpl::AppendImpl(const Slice& data) { inline IOStatus WinWritableImpl::PositionedAppendImpl(const Slice& data, uint64_t offset) { if (file_data_->use_direct_io()) { - if (!IsSectorAligned(static_cast(offset)) || - !IsSectorAligned(data.size()) || - !IsAligned(static_cast(GetAlignement()), data.data())) { - return IOStatus::InvalidArgument( - "Data and offset must be page aligned, size must be sector aligned"); - } + assert(file_data_->IsSectorAligned(static_cast(offset))); + assert(file_data_->IsSectorAligned(data.size())); + assert(IsAligned(static_cast(GetAlignment()), data.data())); } size_t bytes_written = 0; @@ -929,7 +925,7 @@ bool WinWritableFile::use_direct_io() const { } size_t WinWritableFile::GetRequiredBufferAlignment() const { - return static_cast(GetAlignement()); + return static_cast(GetAlignment()); } IOStatus WinWritableFile::Append(const Slice& data, @@ -1003,7 +999,9 @@ bool WinRandomRWFile::use_direct_io() const { } size_t WinRandomRWFile::GetRequiredBufferAlignment() const { - return static_cast(GetAlignement()); + assert(WinRandomAccessImpl::GetAlignment() == + WinWritableImpl::GetAlignment()); + return static_cast(WinRandomAccessImpl::GetAlignment()); } IOStatus WinRandomRWFile::Write(uint64_t offset, const Slice& data, diff --git a/port/win/io_win.h b/port/win/io_win.h index 4119f5add..fd6606b32 100644 --- a/port/win/io_win.h +++ b/port/win/io_win.h @@ -67,12 +67,12 @@ class WinFileData { // will need to be aligned (not sure there is a guarantee that the buffer // passed in is aligned). const bool use_direct_io_; + const size_t sector_size_; public: // We want this class be usable both for inheritance (prive // or protected) and for containment so __ctor and __dtor public - WinFileData(const std::string& filename, HANDLE hFile, bool direct_io) - : filename_(filename), hFile_(hFile), use_direct_io_(direct_io) {} + WinFileData(const std::string& filename, HANDLE hFile, bool direct_io); virtual ~WinFileData() { this->CloseFile(); } @@ -93,6 +93,10 @@ class WinFileData { bool use_direct_io() const { return use_direct_io_; } + size_t GetSectorSize() const { return sector_size_; } + + bool IsSectorAligned(const size_t off) const; + WinFileData(const WinFileData&) = delete; WinFileData& operator=(const WinFileData&) = delete; }; @@ -321,7 +325,7 @@ class WinWritableImpl { ~WinWritableImpl() {} - uint64_t GetAlignement() const { return alignment_; } + uint64_t GetAlignment() const { return alignment_; } IOStatus AppendImpl(const Slice& data);