Skip to content

Commit

Permalink
Merge pull request #18744 from hrydgard/memstick-move-speedup
Browse files Browse the repository at this point in the history 
Memstick folder move on Android: Speedup and safety
  • Loading branch information
@hrydgard
hrydgard committed Jan 22, 2024
2 parents 9fdcef9 + 6e587f5 commit 796787d
Show file tree
Hide file tree
Showing 50 changed files with 264 additions and 99 deletions.
37 changes: 14 additions & 23 deletions Common/File/FileUtil.cpp
View file
Expand Up @@ -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;
Expand Down Expand Up @@ -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();
Expand All @@ -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);
}

Expand Down
2 changes: 1 addition & 1 deletion Common/File/FileUtil.h
View file
Expand Up @@ -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);
Expand Down
1 change: 1 addition & 0 deletions Core/Util/GameManager.cpp
View file
Expand Up @@ -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);
}
Expand Down
203 changes: 139 additions & 64 deletions Core/Util/MemStick.cpp
View file
Expand Up @@ -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;
Expand All @@ -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());
Expand All @@ -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") {
Expand All @@ -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 };
}
12 changes: 7 additions & 5 deletions Core/Util/MemStick.h
View file
Expand Up @@ -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 {
Expand Down
4 changes: 3 additions & 1 deletion GPU/Common/TextureReplacer.cpp
View file
Expand Up @@ -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_);
Expand Down

0 comments on commit 796787d

Please sign in to comment.