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

diff --git a/port/win/env_win.cc b/port/win/env_win.cc
index 1cab4cbe93e7c7467a6c6acbb7755087dfda6370..93e04d389bee7c4451c5ad056a293e7bedd6a10f 100644 (file)
--- 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 1e662c06d91b48645f508c3d265c26a269ede927..41a6b8381d7be036f514bc0394f0a04ce31bba26 100644 (file)
--- 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<size_t>(offset)) || !IsSectorAligned(n)) {
-    return IOStatus::InvalidArgument(
-        "WinSequentialFile::PositionedRead: offset is not properly aligned");
-  }
+  assert(IsSectorAligned(static_cast<size_t>(offset)));
+  assert(IsSectorAligned(static_cast<size_t>(n)));
 
   size_t bytes_read = 0;  // out param
   IOStatus s = PositionedReadInternal(scratch, static_cast<size_t>(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<size_t>(offset)) ||
-        !IsAligned(alignment_, scratch)) {
-      return IOStatus::InvalidArgument(
-          "WinRandomAccessImpl::ReadImpl: offset or scratch is not properly "
-          "aligned");
-    }
+    assert(file_base_->IsSectorAligned(static_cast<size_t>(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<size_t>(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<size_t>(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<size_t>(offset)) ||
-        !IsSectorAligned(data.size()) ||
-        !IsAligned(static_cast<size_t>(GetAlignement()), data.data())) {
-      return IOStatus::InvalidArgument(
-          "Data and offset must be page aligned, size must be sector aligned");
-    }
+    assert(file_data_->IsSectorAligned(static_cast<size_t>(offset)));
+    assert(file_data_->IsSectorAligned(data.size()));
+    assert(IsAligned(static_cast<size_t>(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<size_t>(GetAlignement());
+  return static_cast<size_t>(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<size_t>(GetAlignement());
+  assert(WinRandomAccessImpl::GetAlignment() ==
+         WinWritableImpl::GetAlignment());
+  return static_cast<size_t>(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 4119f5add2ccb2b9f194cbbdc8ad87459de96813..fd6606b32972aa31d756a372396834f87e8071ee 100644 (file)
--- 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);