DiskWriteDriver: Remove use of atomic buffer

disk/diskwritedriver.cpp

@@ -4,9 +4,6 @@
 using namespace icsneo;
 using namespace icsneo::Disk;
 
-const uint64_t WriteDriver::RetryAtomic = std::numeric_limits<uint64_t>::max();
-const APIEvent::Severity WriteDriver::NonatomicSeverity = APIEvent::Severity::EventInfo;
-
 std::optional<uint64_t> 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<uint64_t> 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<uint8_t> alignedWriteBuffer;
+	std::vector<uint8_t> alignedWriteBuffer(idealBlockSize);
-
-	// Read to here, ideally this can be sent back to the device to
-	// ensure an operation is atomic
-	std::vector<uint8_t> atomicBuffer(idealBlockSize);
 
 	pos += vsaOffset;
 	const uint64_t startBlock = pos / idealBlockSize;
@@ -54,31 +47,24 @@ std::optional<uint64_t> 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::milliseconds>(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::milliseconds>(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::milliseconds>(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);

disk/neomemorydiskdriver.cpp

@@ -44,7 +44,7 @@ std::optional<uint64_t> NeoMemoryDiskDriver::readLogicalDiskAligned(Communicatio
 }
 
 std::optional<uint64_t> 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<MessageFilter> NeoMemoryDone = std::make_shared<MessageFilter>(Network::NetID::NeoMemoryWriteDone);
 
@@ -54,11 +54,6 @@ std::optional<uint64_t> 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([&currentSector, &com, from, amount] {
 		std::vector<uint8_t> command = {

disk/nulldiskdriver.cpp

@@ -22,7 +22,7 @@ std::optional<uint64_t> NullDriver::writeLogicalDisk(Communication&, device_even
 }
 
 std::optional<uint64_t> 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;
 }

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<uint64_t> 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

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<uint64_t> 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

include/icsneo/disk/nulldiskdriver.h

@@ -34,7 +34,7 @@ private:
 	std::optional<uint64_t> readLogicalDiskAligned(Communication& com, device_eventhandler_t report,
 		uint64_t pos, uint8_t* into, uint64_t amount, std::chrono::milliseconds timeout) override;
 	std::optional<uint64_t> 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

test/diskdrivertest.h

@@ -40,7 +40,7 @@ public:
 	}
 
 	std::optional<uint64_t> 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<uint64_t> writeAmount = std::min(amount, mockDisk.size() - pos);
 		if(writeAmount > 0u) {
-			if(atomicBuf) {
-				if(supportsAtomic) {
-					atomicityChecks++;
-					if(memcmp(mockDisk.data() + pos, atomicBuf, static_cast<size_t>(*writeAmount)))
-						return RetryAtomic; // Atomic check failed
-				} else {
-					report(APIEvent::Type::AtomicOperationCompletedNonatomically, NonatomicSeverity);
-				}
-			}
-
 			memcpy(mockDisk.data() + pos, from, static_cast<size_t>(*writeAmount));
 		}
 		return writeAmount;
@@ -70,8 +60,6 @@ public:
 	std::array<uint8_t, 1024> 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<void(void)> afterReadHook;
 
 private:

test/diskdriverwritetest.cpp

@@ -7,7 +7,6 @@ TEST_F(DiskDriverTest, Write) {
 	EXPECT_STREQ(reinterpret_cast<char*>(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<const uint8_t*>(TEST_OVERWRITE_STRING), sizeof(TEST_OVERWRITE_STRING));
-	EXPECT_TRUE(amountWritten.has_value());
-	EXPECT_EQ(amountWritten, sizeof(TEST_OVERWRITE_STRING));
-	EXPECT_STREQ(reinterpret_cast<char*>(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<const uint8_t*>(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, 2u);
-	EXPECT_EQ(driver->readCalls, 3u);
 	EXPECT_EQ(driver->writeCalls, 3u);
 }
 
-TEST_F(DiskDriverTest, WriteUnalignedLongAtomicityFailures) {
-	std::array<uint8_t, 500> buf;
-	for(size_t i = 0; i < buf.size(); i++)
-		buf[i] = static_cast<uint8_t>((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<const uint8_t*>(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<const uint8_t*>(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
 }