Merge remote-tracking branch 'origin/release-6.2' into release-6.3

documentation/sphinx/source/release-notes/release-notes-620.rst

@@ -2,6 +2,11 @@
 Release Notes
 #############
 
+6.2.33
+======
+* Fix backup agent stall when writing to local filesystem with slow metadata operations. `(PR #4428) <https://github.com/apple/foundationdb/pull/4428>`_
+* Backup agent no longer uses 4k block caching layer on local output files so that write operations are larger. `(PR #4428) <https://github.com/apple/foundationdb/pull/4428>`_
+
 6.2.32
 ======
 * Fix an issue where symbolic links in cmake-built RPMs are broken if you unpack the RPM to a custom directory. `(PR #4380) <https://github.com/apple/foundationdb/pull/4380>`_

fdbclient/BackupContainer.actor.cpp

@@ -1819,21 +1819,30 @@ public:
 	class BackupFile : public IBackupFile, ReferenceCounted<BackupFile> {
 	public:
 		BackupFile(std::string fileName, Reference<IAsyncFile> file, std::string finalFullPath)
-		  : IBackupFile(fileName), m_file(file), m_finalFullPath(finalFullPath), m_writeOffset(0) {
+		  : IBackupFile(fileName), m_file(file), m_finalFullPath(finalFullPath), m_writeOffset(0),
-			m_buffer.reserve(m_buffer.arena(), CLIENT_KNOBS->BACKUP_LOCAL_FILE_WRITE_BLOCK);
+		    m_blockSize(CLIENT_KNOBS->BACKUP_LOCAL_FILE_WRITE_BLOCK) {
+			if (BUGGIFY) {
+				m_blockSize = deterministicRandom()->randomInt(100, 20000);
+			}
+			m_buffer.reserve(m_buffer.arena(), m_blockSize);
 		}
 
 		Future<Void> append(const void* data, int len) {
 			m_buffer.append(m_buffer.arena(), (const uint8_t*)data, len);
 
-			if (m_buffer.size() >= CLIENT_KNOBS->BACKUP_LOCAL_FILE_WRITE_BLOCK) {
+			if (m_buffer.size() >= m_blockSize) {
-				return flush(CLIENT_KNOBS->BACKUP_LOCAL_FILE_WRITE_BLOCK);
+				return flush(m_blockSize);
 			}
 
 			return Void();
 		}
 
 		Future<Void> flush(int size) {
+			// Avoid empty write
+			if (size == 0) {
+				return Void();
+			}
+
 			ASSERT(size <= m_buffer.size());
 
 			// Keep a reference to the old buffer
@@ -1854,7 +1863,7 @@ public:
 			wait(f->m_file->sync());
 			std::string name = f->m_file->getFilename();
 			f->m_file.clear();
-			renameFile(name, f->m_finalFullPath);
+			wait(IAsyncFileSystem::filesystem()->renameFile(name, f->m_finalFullPath));
 			return Void();
 		}
 
@@ -1870,11 +1879,12 @@ public:
 		Standalone<VectorRef<uint8_t>> m_buffer;
 		int64_t m_writeOffset;
 		std::string m_finalFullPath;
+		int m_blockSize;
 	};
 
-	Future<Reference<IBackupFile>> writeFile(std::string path) final {
+	Future<Reference<IBackupFile>> writeFile(std::string path) {
-		int flags = IAsyncFile::OPEN_NO_AIO | IAsyncFile::OPEN_CREATE | IAsyncFile::OPEN_ATOMIC_WRITE_AND_CREATE |
+		int flags = IAsyncFile::OPEN_NO_AIO | IAsyncFile::OPEN_UNCACHED | IAsyncFile::OPEN_CREATE |
-		            IAsyncFile::OPEN_READWRITE;
+		            IAsyncFile::OPEN_ATOMIC_WRITE_AND_CREATE | IAsyncFile::OPEN_READWRITE;
 		std::string fullPath = joinPath(m_path, path);
 		platform::createDirectory(parentDirectory(fullPath));
 		std::string temp = fullPath + "." + deterministicRandom()->randomUniqueID().toString() + ".temp";
@@ -2310,30 +2320,45 @@ ACTOR Future<Optional<int64_t>> timeKeeperEpochsFromVersion(Version v, Reference
 namespace backup_test {
 
 int chooseFileSize(std::vector<int>& sizes) {
-	int size = 1000;
 	if (!sizes.empty()) {
-		size = sizes.back();
+		int size = sizes.back();
 		sizes.pop_back();
-	}
 		return size;
 	}
+	return deterministicRandom()->randomInt(0, 2e6);
+}
 
-ACTOR Future<Void> writeAndVerifyFile(Reference<IBackupContainer> c, Reference<IBackupFile> f, int size) {
+ACTOR Future<Void> writeAndVerifyFile(Reference<IBackupContainer> c,
-	state Standalone<StringRef> content;
+                                      Reference<IBackupFile> f,
-	if (size > 0) {
+                                      int size,
-		content = makeString(size);
+                                      FlowLock* lock) {
-		for (int i = 0; i < content.size(); ++i)
+	state Standalone<VectorRef<uint8_t>> content;
-			mutateString(content)[i] = (uint8_t)deterministicRandom()->randomInt(0, 256);
 
-		wait(f->append(content.begin(), content.size()));
+	wait(lock->take(TaskPriority::DefaultYield, size));
+	state FlowLock::Releaser releaser(*lock, size);
+
+	printf("writeAndVerify size=%d file=%s\n", size, f->getFileName().c_str());
+
+	content.resize(content.arena(), size);
+	for (int i = 0; i < content.size(); ++i) {
+		content[i] = (uint8_t)deterministicRandom()->randomInt(0, 256);
+	}
+
+	state VectorRef<uint8_t> sendBuf = content;
+	while (sendBuf.size() > 0) {
+		state int n = std::min(sendBuf.size(), deterministicRandom()->randomInt(1, 16384));
+		wait(f->append(sendBuf.begin(), n));
+		sendBuf.pop_front(n);
 	}
 	wait(f->finish());
+
 	state Reference<IAsyncFile> inputFile = wait(c->readFile(f->getFileName()));
 	int64_t fileSize = wait(inputFile->size());
 	ASSERT(size == fileSize);
 	if (size > 0) {
-		state Standalone<StringRef> buf = makeString(size);
+		state Standalone<VectorRef<uint8_t>> buf;
-		int b = wait(inputFile->read(mutateString(buf), buf.size(), 0));
+		buf.resize(buf.arena(), fileSize);
+		int b = wait(inputFile->read(buf.begin(), buf.size(), 0));
 		ASSERT(b == buf.size());
 		ASSERT(buf == content);
 	}
@@ -2346,30 +2371,9 @@ Version nextVersion(Version v) {
 	return v + increment;
 }
 
-// Write a snapshot file with only begin & end key
+ACTOR Future<Void> testBackupContainer(std::string url) {
-ACTOR static Future<Void> testWriteSnapshotFile(Reference<IBackupFile> file, Key begin, Key end, uint32_t blockSize) {
+	state FlowLock lock(100e6);
-	ASSERT(blockSize > 3 * sizeof(uint32_t) + begin.size() + end.size());
 
-	uint32_t fileVersion = BACKUP_AGENT_SNAPSHOT_FILE_VERSION;
-	// write Header
-	wait(file->append((uint8_t*)&fileVersion, sizeof(fileVersion)));
-
-	// write begin key length and key
-	wait(file->appendStringRefWithLen(begin));
-
-	// write end key length and key
-	wait(file->appendStringRefWithLen(end));
-
-	int bytesLeft = blockSize - file->size();
-	if (bytesLeft > 0) {
-		Value paddings = fileBackup::makePadding(bytesLeft);
-		wait(file->append(paddings.begin(), bytesLeft));
-	}
-	wait(file->finish());
-	return Void();
-}
-
-ACTOR static Future<Void> testBackupContainer(std::string url) {
 	printf("BackupContainerTest URL %s\n", url.c_str());
 
 	state Reference<IBackupContainer> c = IBackupContainer::openContainer(url);
@@ -2393,7 +2397,11 @@ ACTOR static Future<Void> testBackupContainer(std::string url) {
 	state Version v = deterministicRandom()->randomInt64(0, std::numeric_limits<Version>::max() / 2);
 
 	// List of sizes to use to test edge cases on underlying file implementations
-	state std::vector<int> fileSizes = { 0, 10000000, 5000005 };
+	state std::vector<int> fileSizes = { 0 };
+	if (StringRef(url).startsWith(LiteralStringRef("blob"))) {
+		fileSizes.push_back(CLIENT_KNOBS->BLOBSTORE_MULTIPART_MIN_PART_SIZE);
+		fileSizes.push_back(CLIENT_KNOBS->BLOBSTORE_MULTIPART_MIN_PART_SIZE + 10);
+	}
 
 	loop {
 		state Version logStart = v;
@@ -2419,9 +2427,7 @@ ACTOR static Future<Void> testBackupContainer(std::string url) {
 
 			int size = chooseFileSize(fileSizes);
 			snapshotSizes.rbegin()->second += size;
-			// Write in actual range file format, instead of random data.
+			writes.push_back(writeAndVerifyFile(c, range, size, &lock));
-			// writes.push_back(writeAndVerifyFile(c, range, size));
-			wait(testWriteSnapshotFile(range, begin, end, blockSize));
 
 			if (deterministicRandom()->random01() < .2) {
 				writes.push_back(c->writeKeyspaceSnapshotFile(
@@ -2441,7 +2447,7 @@ ACTOR static Future<Void> testBackupContainer(std::string url) {
 		state Reference<IBackupFile> log = wait(c->writeLogFile(logStart, v, 10));
 		logs[logStart] = log->getFileName();
 		int size = chooseFileSize(fileSizes);
-		writes.push_back(writeAndVerifyFile(c, log, size));
+		writes.push_back(writeAndVerifyFile(c, log, size, &lock));
 
 		// Randomly stop after a snapshot has finished and all manually seeded file sizes have been used.
 		if (fileSizes.empty() && !snapshots.empty() && snapshots.rbegin()->second.empty() &&

fdbclient/FileBackupAgent.actor.cpp

@@ -1191,7 +1191,7 @@ struct BackupRangeTaskFunc : BackupTaskFuncBase {
 					wait(rangeFile.writeKey(nextKey));
 
 					if (BUGGIFY) {
-						rangeFile.padEnd();
+						wait(rangeFile.padEnd());
 					}
 
 					bool usedFile = wait(

fdbclient/Knobs.cpp

@@ -122,7 +122,7 @@ void ClientKnobs::initialize(bool randomize) {
 	init( TASKBUCKET_MAX_TASK_KEYS,               1000 ); if( randomize && BUGGIFY ) TASKBUCKET_MAX_TASK_KEYS = 20;
 
 	//Backup
-	init( BACKUP_LOCAL_FILE_WRITE_BLOCK,     1024*1024 ); if( randomize && BUGGIFY ) BACKUP_LOCAL_FILE_WRITE_BLOCK = 100;
+	init( BACKUP_LOCAL_FILE_WRITE_BLOCK,     1024*1024 );
 	init( BACKUP_CONCURRENT_DELETES,               100 );
 	init( BACKUP_SIMULATED_LIMIT_BYTES,		       1e6 ); if( randomize && BUGGIFY ) BACKUP_SIMULATED_LIMIT_BYTES = 1000;
 	init( BACKUP_GET_RANGE_LIMIT_BYTES,		       1e6 );

fdbrpc/AsyncFileNonDurable.actor.h

@@ -138,6 +138,8 @@ public:
 	// The address of the machine that opened the file
 	NetworkAddress openedAddress;
 
+	bool aio;
+
 private:
 	// The wrapped IAsyncFile
 	Reference<IAsyncFile> file;
@@ -175,8 +177,10 @@ private:
 	AsyncFileNonDurable(const std::string& filename,
 	                    Reference<IAsyncFile> file,
 	                    Reference<DiskParameters> diskParameters,
-	                    NetworkAddress openedAddress)
+	                    NetworkAddress openedAddress,
-	  : openedAddress(openedAddress), pendingModifications(uint64_t(-1)), approximateSize(0), reponses(false) {
+	                    bool aio)
+	  : openedAddress(openedAddress), pendingModifications(uint64_t(-1)), approximateSize(0), reponses(false),
+	    aio(aio) {
 
 		// This is only designed to work in simulation
 		ASSERT(g_network->isSimulated());
@@ -200,7 +204,8 @@ public:
 	ACTOR static Future<Reference<IAsyncFile>> open(std::string filename,
 	                                                std::string actualFilename,
 	                                                Future<Reference<IAsyncFile>> wrappedFile,
-	                                                Reference<DiskParameters> diskParameters) {
+	                                                Reference<DiskParameters> diskParameters,
+	                                                bool aio) {
 		state ISimulator::ProcessInfo* currentProcess = g_simulator.getCurrentProcess();
 		state TaskPriority currentTaskID = g_network->getCurrentTask();
 		state Future<Void> shutdown = success(currentProcess->shutdownSignal.getFuture());
@@ -227,7 +232,7 @@ public:
 			}
 
 			state Reference<AsyncFileNonDurable> nonDurableFile(
-			    new AsyncFileNonDurable(filename, file, diskParameters, currentProcess->address));
+			    new AsyncFileNonDurable(filename, file, diskParameters, currentProcess->address, aio));
 
 			// Causes the approximateSize member to be set
 			state Future<int64_t> sizeFuture = nonDurableFile->size();
@@ -464,20 +469,39 @@ private:
 
 		debugFileCheck("AsyncFileNonDurableWriteAfterWait", self->filename, dataCopy.begin(), offset, length);
 
-		// Only page-aligned writes are supported
+		// In AIO mode, only page-aligned writes are supported
-		ASSERT(offset % 4096 == 0 && length % 4096 == 0);
+		ASSERT(!self->aio || (offset % 4096 == 0 && length % 4096 == 0));
 
 		// Non-durable writes should introduce errors at the page level and corrupt at the sector level
 		// Otherwise, we can perform the entire write at once
-		int pageLength = saveDurable ? length : 4096;
+		int diskPageLength = saveDurable ? length : 4096;
-		int sectorLength = saveDurable ? length : 512;
+		int diskSectorLength = saveDurable ? length : 512;
 
 		vector<Future<Void>> writeFutures;
-		for (int writeOffset = 0; writeOffset < length; writeOffset += pageLength) {
+		for (int writeOffset = 0; writeOffset < length;) {
+			// Number of bytes until the next diskPageLength file offset within the write or the end of the write.
+			int pageLength = diskPageLength;
+			if (!self->aio && !saveDurable) {
+				// If not in AIO mode, and the save is not durable, then we can't perform the entire write all at once
+				// and the first and last pages touched by the write could be partial.
+				pageLength = std::min<int64_t>((int64_t)length - writeOffset,
+				                               diskPageLength - ((offset + writeOffset) % diskPageLength));
+			}
+
 			// choose a random action to perform on this page write (write correctly, corrupt, or don't write)
 			KillMode pageKillMode = (KillMode)deterministicRandom()->randomInt(0, self->killMode + 1);
 
-			for (int pageOffset = 0; pageOffset < pageLength; pageOffset += sectorLength) {
+			for (int pageOffset = 0; pageOffset < pageLength;) {
+				// Number of bytes until the next diskSectorLength file offset within the write or the end of the write.
+				int sectorLength = diskSectorLength;
+				if (!self->aio && !saveDurable) {
+					// If not in AIO mode, and the save is not durable, then we can't perform the entire write all at
+					// once and the first and last sectors touched by the write could be partial.
+					sectorLength =
+					    std::min<int64_t>((int64_t)length - (writeOffset + pageOffset),
+					                      diskSectorLength - ((offset + writeOffset + pageOffset) % diskSectorLength));
+				}
+
 				// If saving durable, then perform the write correctly.  Otherwise, perform the write correcly with a
 				// probability of 1/3. If corrupting the write, then this sector will be written correctly with a 1/4
 				// chance
@@ -552,7 +576,11 @@ private:
 					    .detail("Filename", self->filename);
 					TEST(true); // AsyncFileNonDurable dropped write
 				}
+
+				pageOffset += sectorLength;
 			}
+
+			writeOffset += pageLength;
 		}
 
 		wait(waitForAll(writeFutures));

fdbrpc/sim2.actor.cpp

@@ -684,8 +684,8 @@ private:
 			        opId.shortString().c_str(),
 			        size);
 
-		if (size == 0) {
 		// KAIO will return EINVAL, as len==0 is an error.
+		if ((self->flags & IAsyncFile::OPEN_NO_AIO) == 0 && size == 0) {
 			throw io_error();
 		}
 
@@ -2254,7 +2254,8 @@ Future<Reference<class IAsyncFile>> Sim2FileSystem::open(std::string filename, i
 			    AsyncFileNonDurable::open(filename,
 			                              actualFilename,
 			                              SimpleFile::open(filename, flags, mode, diskParameters, false),
-			                              diskParameters);
+			                              diskParameters,
+			                              (flags & IAsyncFile::OPEN_NO_AIO) == 0);
 		}
 		Future<Reference<IAsyncFile>> f = AsyncFileDetachable::open(machineCache[actualFilename]);
 		if (FLOW_KNOBS->PAGE_WRITE_CHECKSUM_HISTORY > 0)