Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[STFS] Add critical section to file reading #2123

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[STFS] Add critical section to file reading #2123

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 8 additions & 5 deletions src/xenia/vfs/devices/stfs_container_file.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,12 +52,15 @@ X_STATUS StfsContainerFile::ReadSync(void* buffer, size_t buffer_length,
size_t read_length =
std::min(record.length - read_offset, remaining_length);

auto& file = entry_->files()->at(record.file);
xe::filesystem::Seek(file, record.offset + read_offset, SEEK_SET);
auto num_read = fread(p, 1, read_length, file);
auto global_lock = global_critical_region_.Acquire();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that a race between guess threads accessing the same guest file handle, or different guest handles implemented through one host file handle? If that's the latter, we should be opening the host container file multiple times, for each guest handle, or if possible, use a memory mapping. Using the global critical region here will create a lot of contention that may block other threads for milliseconds — the comment in xenia/base/mutex.h explicitly says that no I/O must be done there.

Copy link
Member Author

@Gliniak Gliniak Feb 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ohh my bad for not reading comments in mutex.h.
It uses multiple handles, so rewrites then to allow memory mapping probably (if possible)

Copy link
Member

@gibbed gibbed Feb 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's strange, I thought it was already using memory mapping for STFS? Maybe I'm forgetting when it got changed. I know I wanted the capability to do both, since we need the ability to load STFS packages nested within a non-host filesystem (ie, such as another STFS package).

{
auto& file = entry_->files()->at(record.file);
xe::filesystem::Seek(file, record.offset + read_offset, SEEK_SET);
auto num_read = fread(p, 1, read_length, file);

*out_bytes_read += num_read;
p += num_read;
*out_bytes_read += num_read;
p += num_read;
}
src_offset += record.length;
remaining_length -= read_length;
if (remaining_length == 0) {
Expand Down
2 changes: 2 additions & 0 deletions src/xenia/vfs/devices/stfs_container_file.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
#ifndef XENIA_VFS_DEVICES_STFS_CONTAINER_FILE_H_
#define XENIA_VFS_DEVICES_STFS_CONTAINER_FILE_H_

#include "xenia/base/mutex.h"
#include "xenia/vfs/file.h"

#include "xenia/xbox.h"
Expand All @@ -35,6 +36,7 @@ class StfsContainerFile : public File {
X_STATUS SetLength(size_t length) override { return X_STATUS_ACCESS_DENIED; }

private:
xe::global_critical_region global_critical_region_;
StfsContainerEntry* entry_;
};

Expand Down