Memstick folder move on Android: Speedup and safety

Common/File/FileUtil.cpp

@@ -628,13 +628,12 @@ bool Rename(const Path &srcFilename, const Path &destFilename) {
 		break;
 	case PathType::CONTENT_URI:
 		// Content URI: Can only rename if in the same folder.
-		// TODO: Fallback to move + rename? Or do we even care about that use case?
+		// TODO: Fallback to move + rename? Or do we even care about that use case? We have MoveIfFast for such tricks.
 		if (srcFilename.GetDirectory() != destFilename.GetDirectory()) {
 			INFO_LOG(COMMON, "Content URI rename: Directories not matching, failing. %s --> %s", srcFilename.c_str(), destFilename.c_str());
 			return false;
 		}
 		INFO_LOG(COMMON, "Content URI rename: %s --> %s", srcFilename.c_str(), destFilename.c_str());
-
 		return Android_RenameFileTo(srcFilename.ToString(), destFilename.GetFilename()) == StorageError::SUCCESS;
 	default:
 		return false;
@@ -752,30 +751,26 @@ bool Copy(const Path &srcFilename, const Path &destFilename) {
 
 // Will overwrite the target.
 bool Move(const Path &srcFilename, const Path &destFilename) {
-	// Try a shortcut in Android Storage scenarios.
-	if (srcFilename.Type() == PathType::CONTENT_URI && destFilename.Type() == PathType::CONTENT_URI && srcFilename.CanNavigateUp() && destFilename.CanNavigateUp()) {
-		// We do not handle simultaneous renames here.
-		if (srcFilename.GetFilename() == destFilename.GetFilename()) {
-			Path srcParent = srcFilename.NavigateUp();
-			Path dstParent = destFilename.NavigateUp();
-			if (Android_MoveFile(srcFilename.ToString(), srcParent.ToString(), dstParent.ToString()) == StorageError::SUCCESS) {
-				return true;
-			}
-			// If failed, fall through and try other ways.
-		}
-	}
-
-	if (Rename(srcFilename, destFilename)) {
+	bool fast = MoveIfFast(srcFilename, destFilename);
+	if (fast) {
 		return true;
-	} else if (Copy(srcFilename, destFilename)) {
+	}
+	// OK, that failed, so fall back on a copy.
+	if (Copy(srcFilename, destFilename)) {
 		return Delete(srcFilename);
 	} else {
 		return false;
 	}
 }
 
 bool MoveIfFast(const Path &srcFilename, const Path &destFilename) {
-	if (srcFilename.Type() == PathType::CONTENT_URI && destFilename.Type() == PathType::CONTENT_URI && srcFilename.CanNavigateUp() && destFilename.CanNavigateUp()) {
+	if (srcFilename.Type() != destFilename.Type()) {
+		// No way it's gonna work.
+		return false;
+	}
+
+	// Only need to check one type here, due to the above check.
+	if (srcFilename.Type() == PathType::CONTENT_URI && srcFilename.CanNavigateUp() && destFilename.CanNavigateUp()) {
 		if (srcFilename.GetFilename() == destFilename.GetFilename()) {
 			Path srcParent = srcFilename.NavigateUp();
 			Path dstParent = destFilename.NavigateUp();
@@ -787,11 +782,7 @@ bool MoveIfFast(const Path &srcFilename, const Path &destFilename) {
 		}
 	}
 
-	if (srcFilename.Type() != destFilename.Type()) {
-		// No way it's gonna work.
-		return false;
-	}
-
+	// Try a traditional rename operation.
 	return Rename(srcFilename, destFilename);
 }
 

Common/File/FileUtil.h

@@ -97,7 +97,7 @@ bool DeleteDir(const Path &filename);
 // Deletes the given directory and anything under it. Returns true on success.
 bool DeleteDirRecursively(const Path &directory);
 
-// Renames file srcFilename to destFilename, returns true on success 
+// Renames/moves file srcFilename to destFilename, returns true on success 
 // Will usually only work with in the same partition or other unit of storage,
 // so you might have to fall back to copy/delete.
 bool Rename(const Path &srcFilename, const Path &destFilename);

Core/Util/GameManager.cpp

@@ -345,6 +345,7 @@ bool GameManager::InstallGame(const Path &url, const Path &fileName, bool delete
 			if (info.stripChars == 0) {
 				success = InstallMemstickZip(z, fileName, dest / "textures.zip", info, deleteAfter);
 			} else {
+				// TODO: Can probably remove this, as we now put .nomedia in /TEXTURES directly.
 				File::CreateEmptyFile(dest / ".nomedia");
 				success = InstallMemstickGame(z, fileName, dest, info, true, deleteAfter);
 			}

Core/Util/MemStick.cpp

@@ -62,15 +62,14 @@ bool SwitchMemstickFolderTo(Path newMemstickFolder) {
 	return true;
 }
 
-
 // Keep the size with the file, so we can skip overly large ones in the move.
 // The user will have to take care of them afterwards, it'll just take too long probably.
 struct FileSuffix {
 	std::string suffix;
 	u64 fileSize;
 };
 
-static bool ListFileSuffixesRecursively(const Path &root, const Path &folder, std::vector<std::string> &dirSuffixes, std::vector<FileSuffix> &fileSuffixes) {
+static bool ListFileSuffixesRecursively(const Path &root, const Path &folder, std::vector<std::string> &dirSuffixes, std::vector<FileSuffix> &fileSuffixes, MoveProgressReporter &progressReporter) {
 	std::vector<File::FileInfo> files;
 	if (!File::GetFilesInDir(folder, &files)) {
 		return false;
@@ -82,7 +81,8 @@ static bool ListFileSuffixesRecursively(const Path &root, const Path &folder, st
 			if (root.ComputePathTo(file.fullName, dirSuffix)) {
 				if (!dirSuffix.empty()) {
 					dirSuffixes.push_back(dirSuffix);
-					ListFileSuffixesRecursively(root, folder / file.name, dirSuffixes, fileSuffixes);
+					ListFileSuffixesRecursively(root, folder / file.name, dirSuffixes, fileSuffixes, progressReporter);
+					progressReporter.SetProgress(file.name, fileSuffixes.size(), (size_t)-1);
 				}
 			} else {
 				ERROR_LOG_REPORT(SYSTEM, "Failed to compute PathTo from '%s' to '%s'", root.c_str(), folder.c_str());
@@ -100,6 +100,43 @@ static bool ListFileSuffixesRecursively(const Path &root, const Path &folder, st
 	return true;
 }
 
+bool MoveChildrenFast(const Path &moveSrc, const Path &moveDest, MoveProgressReporter &progressReporter) {
+	std::vector<File::FileInfo> files;
+	if (!File::GetFilesInDir(moveSrc, &files)) {
+		return false;
+	}
+
+	for (size_t i = 0; i < files.size(); i++) {
+		auto &file = files[i];
+		// Construct destination path
+		Path fileSrc = file.fullName;
+		Path fileDest = moveDest / file.name;
+		progressReporter.SetProgress(file.name, i, files.size());
+		INFO_LOG(SYSTEM, "About to move PSP data from '%s' to '%s'", fileSrc.c_str(), fileDest.c_str());
+		bool result = File::MoveIfFast(fileSrc, fileDest);
+		if (!result) {
+			// TODO: Should we try to move back anything that succeeded before this one?
+			return false;
+		}
+	}
+	return true;
+}
+
+std::string MoveProgressReporter::Format() {
+	std::string str;
+	{
+		std::lock_guard<std::mutex> guard(mutex_);
+		if (max_ > 0) {
+			str = StringFromFormat("(%d/%d) ", count_, max_);
+		} else if (max_ < 0) {
+			str = StringFromFormat("(%d) ", count_);
+		}
+		str += progress_;
+	}
+	return str;
+}
+
+
 MoveResult *MoveDirectoryContentsSafe(Path moveSrc, Path moveDest, MoveProgressReporter &progressReporter) {
 	auto ms = GetI18NCategory(I18NCat::MEMSTICK);
 	if (moveSrc.GetFilename() != "PSP") {
@@ -112,94 +149,132 @@ MoveResult *MoveDirectoryContentsSafe(Path moveSrc, Path moveDest, MoveProgressR
 
 	INFO_LOG(SYSTEM, "About to move PSP data from '%s' to '%s'", moveSrc.c_str(), moveDest.c_str());
 
+	// First, we try the cheapest and safest way to move: Can we move files directly within the same device?
+	// We loop through the files/dirs in the source directory and just try to move them, it should work.
+	if (MoveChildrenFast(moveSrc, moveDest, progressReporter)) {
+		INFO_LOG(SYSTEM, "Quick-move succeeded");
+		progressReporter.SetProgress(ms->T("Done!"));
+		return new MoveResult{
+			true, ""
+		};
+	}
+
+	// If this doesn't work, we'll fall back on a recursive *copy* (disk space is less of a concern when
+	// moving from device to device, other than that everything fits on the destination).
+	// Then we verify the results before we delete the originals.
+
 	// Search through recursively, listing the files to move and also summing their sizes.
 	std::vector<FileSuffix> fileSuffixesToMove;
 	std::vector<std::string> directorySuffixesToCreate;
 
 	// NOTE: It's correct to pass moveSrc twice here, it's to keep the root in the recursion.
-	if (!ListFileSuffixesRecursively(moveSrc, moveSrc, directorySuffixesToCreate, fileSuffixesToMove)) {
+	if (!ListFileSuffixesRecursively(moveSrc, moveSrc, directorySuffixesToCreate, fileSuffixesToMove, progressReporter)) {
 		// TODO: Handle failure listing files.
 		std::string error = "Failed to read old directory";
 		INFO_LOG(SYSTEM, "%s", error.c_str());
-		progressReporter.Set(ms->T(error.c_str()));
+		progressReporter.SetProgress(ms->T(error.c_str()));
 		return new MoveResult{ false, error };
 	}
 
 	bool dryRun = false;  // Useful for debugging.
 
 	size_t failedFiles = 0;
-	size_t skippedFiles = 0;
 
 	// We're not moving huge files like ISOs during this process, unless
 	// they can be directly moved, without rewriting the file.
 	const uint64_t BIG_FILE_THRESHOLD = 24 * 1024 * 1024;
 
-	if (!moveSrc.empty()) {
-		// Better not interrupt the app while this is happening!
+	if (moveSrc.empty()) {
+		// Shouldn't happen.
+		return new MoveResult{ true, "", };
+	}
 
-		// Create all the necessary directories.
-		for (auto &dirSuffix : directorySuffixesToCreate) {
-			Path dir = moveDest / dirSuffix;
-			if (dryRun) {
-				INFO_LOG(SYSTEM, "dry run: Would have created dir '%s'", dir.c_str());
-			} else {
-				INFO_LOG(SYSTEM, "Creating dir '%s'", dir.c_str());
-				progressReporter.Set(dirSuffix);
-				// Just ignore already-exists errors.
-				File::CreateDir(dir);
-			}
+	// Better not interrupt the app while this is happening!
+
+	// Create all the necessary directories.
+	for (size_t i = 0; i < directorySuffixesToCreate.size(); i++) {
+		const auto &dirSuffix = directorySuffixesToCreate[i];
+		Path dir = moveDest / dirSuffix;
+		if (dryRun) {
+			INFO_LOG(SYSTEM, "dry run: Would have created dir '%s'", dir.c_str());
+		} else {
+			INFO_LOG(SYSTEM, "Creating dir '%s'", dir.c_str());
+			progressReporter.SetProgress(dirSuffix);
+			// Just ignore already-exists errors.
+			File::CreateDir(dir);
 		}
+	}
 
-		for (auto &fileSuffix : fileSuffixesToMove) {
-			progressReporter.Set(StringFromFormat("%s (%s)", fileSuffix.suffix.c_str(), NiceSizeFormat(fileSuffix.fileSize).c_str()));
-
-			Path from = moveSrc / fileSuffix.suffix;
-			Path to = moveDest / fileSuffix.suffix;
-
-			if (fileSuffix.fileSize > BIG_FILE_THRESHOLD) {
-				// We only move big files if it's fast to do so.
-				if (dryRun) {
-					INFO_LOG(SYSTEM, "dry run: Would have moved '%s' to '%s' (%d bytes) if fast", from.c_str(), to.c_str(), (int)fileSuffix.fileSize);
-				} else {
-					if (!File::MoveIfFast(from, to)) {
-						INFO_LOG(SYSTEM, "Skipped moving file '%s' to '%s' (%s)", from.c_str(), to.c_str(), NiceSizeFormat(fileSuffix.fileSize).c_str());
-						skippedFiles++;
-					} else {
-						INFO_LOG(SYSTEM, "Moved file '%s' to '%s'", from.c_str(), to.c_str());
-					}
-				}
+	for (size_t i = 0; i < fileSuffixesToMove.size(); i++) {
+		const auto &fileSuffix = fileSuffixesToMove[i];
+		progressReporter.SetProgress(StringFromFormat("%s (%s)", fileSuffix.suffix.c_str(), NiceSizeFormat(fileSuffix.fileSize).c_str()),
+			(int)i, (int)fileSuffixesToMove.size());
+
+		Path from = moveSrc / fileSuffix.suffix;
+		Path to = moveDest / fileSuffix.suffix;
+
+		if (dryRun) {
+			INFO_LOG(SYSTEM, "dry run: Would have moved '%s' to '%s' (%d bytes)", from.c_str(), to.c_str(), (int)fileSuffix.fileSize);
+		} else {
+			// Remove the "from" prefix from the path.
+			// We have to drop down to string operations for this.
+			if (!File::Copy(from, to)) {
+				ERROR_LOG(SYSTEM, "Failed to copy file '%s' to '%s'", from.c_str(), to.c_str());
+				failedFiles++;
+				// Should probably just bail?
 			} else {
-				if (dryRun) {
-					INFO_LOG(SYSTEM, "dry run: Would have moved '%s' to '%s' (%d bytes)", from.c_str(), to.c_str(), (int)fileSuffix.fileSize);
-				} else {
-					// Remove the "from" prefix from the path.
-					// We have to drop down to string operations for this.
-					if (!File::Move(from, to)) {
-						ERROR_LOG(SYSTEM, "Failed to move file '%s' to '%s'", from.c_str(), to.c_str());
-						failedFiles++;
-						// Should probably just bail?
-					} else {
-						INFO_LOG(SYSTEM, "Moved file '%s' to '%s'", from.c_str(), to.c_str());
-					}
-				}
+				INFO_LOG(SYSTEM, "Copied file '%s' to '%s'", from.c_str(), to.c_str());
 			}
 		}
+	}
 
-		// Delete all the old, now hopefully empty, directories.
-		// Hopefully DeleteDir actually fails if it contains a file...
-		for (auto &dirSuffix : directorySuffixesToCreate) {
-			Path dir = moveSrc / dirSuffix;
-			if (dryRun) {
-				INFO_LOG(SYSTEM, "dry run: Would have deleted dir '%s'", dir.c_str());
-			} else {
-				INFO_LOG(SYSTEM, "Deleting dir '%s'", dir.c_str());
-				progressReporter.Set(dirSuffix);
-				if (File::Exists(dir)) {
-					File::DeleteDir(dir);
-				}
-			}
+	if (failedFiles) {
+		return new MoveResult{ false, "", failedFiles };
+	}
+
+	// After the whole move, verify that all the files arrived correctly.
+	// If there's a single error, we do not delete the source data.
+	bool ok = true;
+	for (size_t i = 0; i < fileSuffixesToMove.size(); i++) {
+		const auto &fileSuffix = fileSuffixesToMove[i];
+		progressReporter.SetProgress(ms->T("Checking..."), (int)i, (int)fileSuffixesToMove.size());
+
+		Path to = moveDest / fileSuffix.suffix;
+
+		File::FileInfo info;
+		if (!File::GetFileInfo(to, &info)) {
+			ok = false;
+			break;
+		}
+
+		if (fileSuffix.fileSize != info.size) {
+			ERROR_LOG(SYSTEM, "Mismatched size in target file %s. Verification failed.", fileSuffix.suffix.c_str());
+			ok = false;
+			failedFiles++;
+			break;
 		}
 	}
 
-	return new MoveResult{ true, "", failedFiles };
+	if (!ok) {
+		return new MoveResult{ false, "", failedFiles };
+	}
+
+	INFO_LOG(SYSTEM, "Verification complete");
+
+	// Delete all the old, now hopefully empty, directories.
+	// Hopefully DeleteDir actually fails if it contains a file...
+	for (size_t i = 0; i < directorySuffixesToCreate.size(); i++) {
+		const auto &dirSuffix = directorySuffixesToCreate[i];
+		Path dir = moveSrc / dirSuffix;
+		if (dryRun) {
+			INFO_LOG(SYSTEM, "dry run: Would have deleted dir '%s'", dir.c_str());
+		} else {
+			INFO_LOG(SYSTEM, "Deleting dir '%s'", dir.c_str());
+			progressReporter.SetProgress(dirSuffix, i, directorySuffixesToCreate.size());
+			if (File::Exists(dir)) {
+				File::DeleteDir(dir);
+			}
+		}
+	}
+	return new MoveResult{ true, "", 0 };
 }

Core/Util/MemStick.h

@@ -3,24 +3,26 @@
 #include "Common/File/Path.h"
 
 #include <mutex>
+#include <string_view>
 
 // Utility functions moved out from MemstickScreen.
 
 class MoveProgressReporter {
 public:
-	void Set(const std::string &value) {
+	void SetProgress(std::string_view value, size_t count = 0, size_t maxVal = 0) {
 		std::lock_guard<std::mutex> guard(mutex_);
 		progress_ = value;
+		count_ = (int)count;
+		max_ = (int)maxVal;
 	}
 
-	std::string Get() {
-		std::lock_guard<std::mutex> guard(mutex_);
-		return progress_;
-	}
+	std::string Format();
 
 private:
 	std::string progress_;
 	std::mutex mutex_;
+	int count_;
+	int max_;
 };
 
 struct MoveResult {

GPU/Common/TextureReplacer.cpp

@@ -87,8 +87,10 @@ void TextureReplacer::NotifyConfigChanged() {
 
 		// If we're saving, auto-create the directory.
 		if (g_Config.bSaveNewTextures && !File::Exists(newTextureDir_)) {
+			INFO_LOG(G3D, "Creating new texture directory: '%s'", newTextureDir_.ToVisualString().c_str());
 			File::CreateFullPath(newTextureDir_);
-			File::CreateEmptyFile(newTextureDir_ / ".nomedia");
+			// We no longer create a nomedia file here, since we put one
+			// in the TEXTURES root.
 		}
 
 		enabled_ = File::IsDirectory(basePath_);