From ed1632c65243a3f20c5be73121a8bb8067d41f89 Mon Sep 17 00:00:00 2001 From: Emily Brooks Date: Mon, 21 Nov 2022 16:54:57 -0500 Subject: [PATCH] DiskWriteDriver: Remove use of atomic buffer --- disk/diskwritedriver.cpp | 32 ++++--------- disk/neomemorydiskdriver.cpp | 7 +-- disk/nulldiskdriver.cpp | 2 +- include/icsneo/disk/diskwritedriver.h | 26 +---------- include/icsneo/disk/neomemorydiskdriver.h | 2 +- include/icsneo/disk/nulldiskdriver.h | 2 +- test/diskdrivertest.h | 14 +----- test/diskdriverwritetest.cpp | 56 +---------------------- 8 files changed, 16 insertions(+), 125 deletions(-) diff --git a/disk/diskwritedriver.cpp b/disk/diskwritedriver.cpp index b1763c0..e49fc56 100644 --- a/disk/diskwritedriver.cpp +++ b/disk/diskwritedriver.cpp @@ -4,9 +4,6 @@ using namespace icsneo; using namespace icsneo::Disk; -const uint64_t WriteDriver::RetryAtomic = std::numeric_limits::max(); -const APIEvent::Severity WriteDriver::NonatomicSeverity = APIEvent::Severity::EventInfo; - std::optional WriteDriver::writeLogicalDisk(Communication& com, device_eventhandler_t report, ReadDriver& readDriver, uint64_t pos, const uint8_t* from, uint64_t amount, std::chrono::milliseconds timeout) { if(amount == 0) @@ -19,11 +16,7 @@ std::optional WriteDriver::writeLogicalDisk(Communication& com, device // Write from here if we need to read-modify-write a block // That would be the case either if we don't want some at the // beginning or end of the block. - std::vector alignedWriteBuffer; - - // Read to here, ideally this can be sent back to the device to - // ensure an operation is atomic - std::vector atomicBuffer(idealBlockSize); + std::vector alignedWriteBuffer(idealBlockSize); pos += vsaOffset; const uint64_t startBlock = pos / idealBlockSize; @@ -54,31 +47,24 @@ std::optional WriteDriver::writeLogicalDisk(Communication& com, device t = APIEvent::Type::EOFReached; report(t, s); }; - auto bytesTransferred = readDriver.readLogicalDisk(com, reportFromRead, currentBlock * idealBlockSize, - atomicBuffer.data(), idealBlockSize, timeout); - timeout -= std::chrono::duration_cast(std::chrono::high_resolution_clock::now() - start); - - if(bytesTransferred != idealBlockSize) - break; // readLogicalDisk reports its own errors const bool useAlignedWriteBuffer = (posWithinCurrentBlock != 0 || curAmt != idealBlockSize); if(useAlignedWriteBuffer) { - alignedWriteBuffer = atomicBuffer; + auto read = readDriver.readLogicalDisk(com, reportFromRead, currentBlock * idealBlockSize, + alignedWriteBuffer.data(), idealBlockSize, timeout); + timeout -= std::chrono::duration_cast(std::chrono::high_resolution_clock::now() - start); + + if(read != idealBlockSize) + break; // readLogicalDisk reports its own errors + memcpy(alignedWriteBuffer.data() + posWithinCurrentBlock, from + fromOffset, curAmt); } start = std::chrono::high_resolution_clock::now(); - bytesTransferred = writeLogicalDiskAligned(com, report, currentBlock * idealBlockSize, atomicBuffer.data(), + auto bytesTransferred = writeLogicalDiskAligned(com, report, currentBlock * idealBlockSize, useAlignedWriteBuffer ? alignedWriteBuffer.data() : (from + fromOffset), idealBlockSize, timeout); timeout -= std::chrono::duration_cast(std::chrono::high_resolution_clock::now() - start); - if(bytesTransferred == RetryAtomic) { - // The user may want to log these events in order to see how many atomic misses they are getting - report(APIEvent::Type::AtomicOperationRetried, APIEvent::Severity::EventInfo); - readDriver.invalidateCache(currentBlock * idealBlockSize, idealBlockSize); - continue; - } - if(!bytesTransferred.has_value() || *bytesTransferred < curAmt) { if(timeout < std::chrono::milliseconds::zero()) report(APIEvent::Type::Timeout, APIEvent::Severity::Error); diff --git a/disk/neomemorydiskdriver.cpp b/disk/neomemorydiskdriver.cpp index 7ea5e06..b7f6a86 100644 --- a/disk/neomemorydiskdriver.cpp +++ b/disk/neomemorydiskdriver.cpp @@ -44,7 +44,7 @@ std::optional NeoMemoryDiskDriver::readLogicalDiskAligned(Communicatio } std::optional NeoMemoryDiskDriver::writeLogicalDiskAligned(Communication& com, device_eventhandler_t report, - uint64_t pos, const uint8_t* atomicBuf, const uint8_t* from, uint64_t amount, std::chrono::milliseconds timeout) { + uint64_t pos, const uint8_t* from, uint64_t amount, std::chrono::milliseconds timeout) { static std::shared_ptr NeoMemoryDone = std::make_shared(Network::NetID::NeoMemoryWriteDone); @@ -54,11 +54,6 @@ std::optional NeoMemoryDiskDriver::writeLogicalDiskAligned(Communicati if(amount != SectorSize) return std::nullopt; - // Requesting an atomic operation, but neoMemory does not support it - // Continue on anyway but warn the caller - if(atomicBuf != nullptr) - report(APIEvent::Type::AtomicOperationCompletedNonatomically, NonatomicSeverity); - const uint64_t currentSector = pos / SectorSize; auto msg = com.waitForMessageSync([¤tSector, &com, from, amount] { std::vector command = { diff --git a/disk/nulldiskdriver.cpp b/disk/nulldiskdriver.cpp index d3342b7..eccdd87 100644 --- a/disk/nulldiskdriver.cpp +++ b/disk/nulldiskdriver.cpp @@ -22,7 +22,7 @@ std::optional NullDriver::writeLogicalDisk(Communication&, device_even } std::optional NullDriver::writeLogicalDiskAligned(Communication&, device_eventhandler_t report, - uint64_t, const uint8_t*, const uint8_t*, uint64_t, std::chrono::milliseconds) { + uint64_t, const uint8_t*, uint64_t, std::chrono::milliseconds) { report(APIEvent::Type::DiskNotSupported, APIEvent::Severity::Error); return std::nullopt; } \ No newline at end of file diff --git a/include/icsneo/disk/diskwritedriver.h b/include/icsneo/disk/diskwritedriver.h index cb9f26c..93e6d47 100644 --- a/include/icsneo/disk/diskwritedriver.h +++ b/include/icsneo/disk/diskwritedriver.h @@ -23,38 +23,14 @@ public: uint64_t pos, const uint8_t* from, uint64_t amount, std::chrono::milliseconds timeout = DefaultTimeout); protected: - /** - * Flag returned from writeLogicalDiskAligned when the - * operation failed to be performed atomically and can - * be retried after rereading. - */ - static const uint64_t RetryAtomic; - - /** - * The severity to report with when an atomic operation - * is requested that the driver is unable to attempt. - */ - static const APIEvent::Severity NonatomicSeverity; - /** * Perform a write which the driver can do in one shot. * * The `pos` requested must be sector-aligned, and the `amount` must be * within the block size bounds provided by the driver. - * - * If `atomicBuf` is provided, it will be used to ensure that the disk - * data changes from `atomicBuf` to `from` without trampling any reads - * that may have happened while modifying the data. - * - * The flag `RetryAtomic` is returned if the operation was attempted - * atomically but failed. - * - * If the driver does not support atomic operations, but `atomicBuf` - * is non-null, an APIEvent::AtomicOperationCompletedNonatomically - * should be reported with `NonatomicSeverity`. */ virtual std::optional writeLogicalDiskAligned(Communication& com, device_eventhandler_t report, - uint64_t pos, const uint8_t* atomicBuf, const uint8_t* from, uint64_t amount, std::chrono::milliseconds timeout) = 0; + uint64_t pos, const uint8_t* from, uint64_t amount, std::chrono::milliseconds timeout) = 0; }; } // namespace Disk diff --git a/include/icsneo/disk/neomemorydiskdriver.h b/include/icsneo/disk/neomemorydiskdriver.h index 95250cc..4fa7bfc 100644 --- a/include/icsneo/disk/neomemorydiskdriver.h +++ b/include/icsneo/disk/neomemorydiskdriver.h @@ -34,7 +34,7 @@ private: uint64_t pos, uint8_t* into, uint64_t amount, std::chrono::milliseconds timeout) override; std::optional writeLogicalDiskAligned(Communication& com, device_eventhandler_t report, - uint64_t pos, const uint8_t* atomicBuf, const uint8_t* from, uint64_t amount, std::chrono::milliseconds timeout) override; + uint64_t pos, const uint8_t* from, uint64_t amount, std::chrono::milliseconds timeout) override; }; } // namespace Disk diff --git a/include/icsneo/disk/nulldiskdriver.h b/include/icsneo/disk/nulldiskdriver.h index 666114f..1d40ca6 100644 --- a/include/icsneo/disk/nulldiskdriver.h +++ b/include/icsneo/disk/nulldiskdriver.h @@ -34,7 +34,7 @@ private: std::optional readLogicalDiskAligned(Communication& com, device_eventhandler_t report, uint64_t pos, uint8_t* into, uint64_t amount, std::chrono::milliseconds timeout) override; std::optional writeLogicalDiskAligned(Communication& com, device_eventhandler_t report, - uint64_t pos, const uint8_t* atomicBuf, const uint8_t* from, uint64_t amount, std::chrono::milliseconds timeout) override; + uint64_t pos, const uint8_t* from, uint64_t amount, std::chrono::milliseconds timeout) override; }; } // namespace Disk diff --git a/test/diskdrivertest.h b/test/diskdrivertest.h index e0af4e0..946b208 100644 --- a/test/diskdrivertest.h +++ b/test/diskdrivertest.h @@ -40,7 +40,7 @@ public: } std::optional writeLogicalDiskAligned(Communication&, device_eventhandler_t report, uint64_t pos, - const uint8_t* atomicBuf, const uint8_t* from, uint64_t amount, std::chrono::milliseconds) override { + const uint8_t* from, uint64_t amount, std::chrono::milliseconds) override { writeCalls++; EXPECT_EQ(pos % getBlockSizeBounds().first, 0); // Ensure the alignment rules are respected @@ -52,16 +52,6 @@ public: std::optional writeAmount = std::min(amount, mockDisk.size() - pos); if(writeAmount > 0u) { - if(atomicBuf) { - if(supportsAtomic) { - atomicityChecks++; - if(memcmp(mockDisk.data() + pos, atomicBuf, static_cast(*writeAmount))) - return RetryAtomic; // Atomic check failed - } else { - report(APIEvent::Type::AtomicOperationCompletedNonatomically, NonatomicSeverity); - } - } - memcpy(mockDisk.data() + pos, from, static_cast(*writeAmount)); } return writeAmount; @@ -70,8 +60,6 @@ public: std::array mockDisk; size_t readCalls = 0; size_t writeCalls = 0; - size_t atomicityChecks = 0; - bool supportsAtomic = true; // Ability to simulate a driver that doesn't support atomic writes std::function afterReadHook; private: diff --git a/test/diskdriverwritetest.cpp b/test/diskdriverwritetest.cpp index 1da8435..a4b1fca 100644 --- a/test/diskdriverwritetest.cpp +++ b/test/diskdriverwritetest.cpp @@ -7,7 +7,6 @@ TEST_F(DiskDriverTest, Write) { EXPECT_STREQ(reinterpret_cast(driver->mockDisk.data()), TEST_OVERWRITE_STRING); EXPECT_EQ(driver->mockDisk[sizeof(TEST_OVERWRITE_STRING) + 1], TEST_STRING[sizeof(TEST_OVERWRITE_STRING) + 1]); EXPECT_EQ(driver->mockDisk[126], 126u); - EXPECT_EQ(driver->atomicityChecks, 1u); EXPECT_EQ(driver->readCalls, 1u); EXPECT_EQ(driver->writeCalls, 1u); } @@ -18,25 +17,10 @@ TEST_F(DiskDriverTest, WriteZero) { EXPECT_TRUE(amountWritten.has_value()); EXPECT_EQ(amountWritten, 0u); EXPECT_EQ(driver->mockDisk[0], TEST_STRING[0]); - EXPECT_EQ(driver->atomicityChecks, 0u); EXPECT_EQ(driver->readCalls, 0u); EXPECT_EQ(driver->writeCalls, 0u); } -TEST_F(DiskDriverTest, WriteNoAtomicityCheck) { - driver->supportsAtomic = false; - expectedErrors.push({ APIEvent::Type::AtomicOperationCompletedNonatomically, APIEvent::Severity::EventInfo }); - const auto amountWritten = writeLogicalDisk(0u, reinterpret_cast(TEST_OVERWRITE_STRING), sizeof(TEST_OVERWRITE_STRING)); - EXPECT_TRUE(amountWritten.has_value()); - EXPECT_EQ(amountWritten, sizeof(TEST_OVERWRITE_STRING)); - EXPECT_STREQ(reinterpret_cast(driver->mockDisk.data()), TEST_OVERWRITE_STRING); - EXPECT_EQ(driver->mockDisk[sizeof(TEST_OVERWRITE_STRING) + 1], TEST_STRING[sizeof(TEST_OVERWRITE_STRING) + 1]); - EXPECT_EQ(driver->mockDisk[126], 126u); - EXPECT_EQ(driver->atomicityChecks, 0u); - EXPECT_EQ(driver->readCalls, 1u); - EXPECT_EQ(driver->writeCalls, 1u); -} - TEST_F(DiskDriverTest, WriteUnaligned) { const auto amountWritten = writeLogicalDisk(3, reinterpret_cast(TEST_OVERWRITE_STRING), sizeof(TEST_OVERWRITE_STRING)); EXPECT_TRUE(amountWritten.has_value()); @@ -44,7 +28,6 @@ TEST_F(DiskDriverTest, WriteUnaligned) { EXPECT_EQ(driver->mockDisk[0], TEST_STRING[0]); EXPECT_EQ(driver->mockDisk[5], TEST_OVERWRITE_STRING[2]); EXPECT_EQ(driver->mockDisk[110], 110u); - EXPECT_EQ(driver->atomicityChecks, 1u); EXPECT_EQ(driver->readCalls, 1u); EXPECT_EQ(driver->writeCalls, 1u); } @@ -58,46 +41,10 @@ TEST_F(DiskDriverTest, WriteUnalignedLong) { EXPECT_EQ(amountWritten, buf.size()); EXPECT_EQ(driver->mockDisk[0], TEST_STRING[0]); EXPECT_EQ(driver->mockDisk[330], ((buf.size() - 30) + 20) & 0xFF); - EXPECT_EQ(driver->atomicityChecks, 3u); - EXPECT_EQ(driver->readCalls, 3u); + EXPECT_EQ(driver->readCalls, 2u); EXPECT_EQ(driver->writeCalls, 3u); } -TEST_F(DiskDriverTest, WriteUnalignedLongAtomicityFailures) { - std::array buf; - for(size_t i = 0; i < buf.size(); i++) - buf[i] = static_cast((buf.size() - i) + 20); - for(int i = 0; i < 4; i++) - expectedErrors.push({ APIEvent::Type::AtomicOperationRetried, APIEvent::Severity::EventInfo }); - - int i = 0; - driver->afterReadHook = [&i, this]() { - switch(i) { - case 0: driver->mockDisk[295] = uint8_t(0xCD); break; - case 1: break; // We don't mess with this one so the first block can be written - case 2: driver->mockDisk[600] = uint8_t(0xDC); break; - case 3: driver->mockDisk[602] = uint8_t(0xDC); break; - case 4: break; // We don't mess with this one so the second block can be written - case 5: driver->mockDisk[777] = uint8_t(0x22); break; - case 6: break; // We don't mess with this one so the third block can be written - } - i++; - }; - - const auto amountWritten = writeLogicalDisk(300, buf.data(), buf.size()); - EXPECT_TRUE(amountWritten.has_value()); - EXPECT_EQ(amountWritten, buf.size()); - EXPECT_EQ(driver->mockDisk[0], TEST_STRING[0]); - EXPECT_EQ(driver->mockDisk[295], 0xCDu); // If the atomic worked correctly this write won't have gotten trampled - // Our writes happen after both of these, so they overwrite the 0xDC values - EXPECT_EQ(driver->mockDisk[600], ((buf.size() - 300) + 20) & 0xFF); - EXPECT_EQ(driver->mockDisk[602], ((buf.size() - 302) + 20) & 0xFF); - - EXPECT_EQ(driver->atomicityChecks, 7u); - EXPECT_EQ(driver->readCalls, 7u); - EXPECT_EQ(driver->writeCalls, 7u); -} - TEST_F(DiskDriverTest, WritePastEnd) { expectedErrors.push({ APIEvent::Type::EOFReached, APIEvent::Severity::Error }); const auto amountWritten = writeLogicalDisk(1020, reinterpret_cast(TEST_OVERWRITE_STRING), sizeof(TEST_OVERWRITE_STRING)); @@ -114,7 +61,6 @@ TEST_F(DiskDriverTest, WriteBadStartingPos) { expectedErrors.push({ APIEvent::Type::ParameterOutOfRange, APIEvent::Severity::Error }); const auto amountWritten = writeLogicalDisk(2000, reinterpret_cast(TEST_OVERWRITE_STRING), sizeof(TEST_OVERWRITE_STRING)); EXPECT_FALSE(amountWritten.has_value()); - EXPECT_EQ(driver->atomicityChecks, 0u); EXPECT_EQ(driver->readCalls, 1u); EXPECT_EQ(driver->writeCalls, 0u); // We never even attempt the write } \ No newline at end of file