Workspace virtualization fuse changes#1737
Conversation
65afa3e to
75d6e04
Compare
|
|
||
| void close() { | ||
| if (fd > 0) { | ||
| if (fd >= 0) { |
There was a problem hiding this comment.
seperate bug I found that was causing failures for my tests. Can file a sperate PR fixing this, if needed
There was a problem hiding this comment.
I kinda assumed this was quite intentional, to ensure we always had STDIN or something?
The job-cache code I removed looks like it might have depended on this behavior? If wake ran with stdin closed or thereabouts, and daemonize() was reached. Courtesy of auggie, but it's right re:daemonize() code I'm not 100% this was end-to-end reachable or not.
Since job_cache is where unique_fd originated, that makes sense.
(Additionally, we use BAD_FD as 0 which further suggests a consistent approach to this?)
unique_fd exists in a few places; none of them AFAICT have special consideration for 0.
I worry about fiddly dependencies on this lurking, but oh well.
This change looks right to me.
IMO it'd be better to make this change separately, yes, thanks.
|
|
||
| // Creates special files. Only used for job registration; regular mknod not supported. | ||
| // TODO: Revisit this for CAS implementation | ||
| static int wakefuse_mknod(const char *path, mode_t mode, dev_t rdev) { |
There was a problem hiding this comment.
Note sure exactly how we should support this under CAS
Will Dietz (dtzSiFive)
left a comment
There was a problem hiding this comment.
First round of comments!
Have you looked at executing some filesystem tests (e.g., https://github.com/kdave/xfstests or possibly https://github.com/tuxera/pjd-fstest)?
Mostly: There are a /lot/ of details to get right.
Just a thought re:above.
At least kicking the tires by some jobs that explicitly exercise filesystem behavior seems useful, even if full compliance is not our primary objective or realizable.
That said, those tests (without studying them) may also be insufficient, as they may not be written to expose the sorts of issues our implementation has due to its nature that other filesystems may not.
May help identify unexpected/unintended behavior, however, and likely there are others/better test suites.
| class StagedFilesStore { | ||
| public: | ||
| using JobMap = std::map<std::string, StagedItem>; | ||
| using StoreMap = std::map<std::string, JobMap>; |
There was a problem hiding this comment.
Can job_id be an integral type throughout?
It might not be reasonably put into that form at callers, just wondering.
| auto job_it = files_.find(job_id); | ||
| if (job_it == files_.end()) return false; | ||
| std::string prefix = dir + "/"; | ||
| auto it = job_it->second.lower_bound(prefix); |
There was a problem hiding this comment.
Are paths always canonical (/absolute), or no?
Just checking we're super sure we can check this using a string prefix approach 👍 .
|
|
||
| private: | ||
| StoreMap files_; | ||
| std::map<int, StagedItem *> fd_map_; // fd -> StagedItem* |
There was a problem hiding this comment.
Looks like this could probably be an unordered_map ? FWIW.
| // Encapsulates the nested map of staged files with helper methods | ||
| // Structure: job_id -> (path -> StagedItem) | ||
| // Also tracks fd -> StagedItem* for open file handles | ||
| class StagedFilesStore { |
There was a problem hiding this comment.
Consider explicitly = delete'ing copy-constructor and copy-assignment operators (and =default'ing the others?); the fd_map_ entries will point into wrong map as-is.
| } | ||
|
|
||
| // Get or create a staged item (for insertion) | ||
| StagedItem &operator()(const std::string &job_id, const std::string &path) { |
There was a problem hiding this comment.
Not a big deal but the default-constructed StagedItem is "invalid" (mostly the fields are empty, defaults to File but it's nothing); consider explicit insertion operation.
(and constructing so we can override them seems better served by an insert/emplace sort of API?)
If fits use case, assert not already present -- not sure if we expect to implicitly overwrite entries using this?
Just a consideration, no action needed here necessarily.
| @@ -0,0 +1,24 @@ | |||
| { | |||
There was a problem hiding this comment.
These tests I think can land separately and directly to master?
Is anything about them CAS-specific? I see they set USE_CAS, but they are testing wakebox in a CAS-agnostic way, right?
Wonder if could reasonably run tests both with and without USE_CAS or something, instead of having specific tests for each?
That said, once we're done we will only have the one path (is that right?), so it'll just be wakebox tests.
For your consideration 👍 .
I got nervous about all the hardcoded things like PATH and USER/HOME but haven't checked if there's precedent. Probably that's standard.
| fi | ||
|
|
||
| # Check that both files show expected content | ||
| # After append, hardlink should show both lines |
There was a problem hiding this comment.
Is it worth checking things the other way around? (write through hardlink.txt, see it in original.txt)
Anything interesting about chains/groups of hardlinks?
Haven't gotten to part where this is handled yet 🙃 .
This test seems a little imprecise w.r.t. what the comment here is saying. We cat original.txt and hardlink.txt and then hardlink.txt at end, producing this output:
shared content
shared content
shared content
appended
Without getting too fiddly (and esp not too fragile), can we more specifically check things?
| return std::visit(overloaded{ | ||
| [stbuf](const StagedFileData &f) { | ||
| // Stat the actual staging file | ||
| int res = stat(f.staging_path.c_str(), stbuf); |
There was a problem hiding this comment.
Do we care about st_nlink here? (or via hardlink above)?
There was a problem hiding this comment.
I'm guessing that generally speaking things like ls will be a little special (e.g., ls -i that mixes CAS and staging?).
| if (hash_it != it->second.visible_hashes.end() && !hash_it->second.empty()) { | ||
| std::string blob_path = cas_blob_path(hash_it->second); | ||
| if (!blob_path.empty()) { | ||
| int fd = open(blob_path.c_str(), O_RDONLY); |
There was a problem hiding this comment.
This seems like a problem if the open was O_WRONLY or something?
Not sure how far to go regarding ensuring these consistently appear read-only to jobs, but satisfying an open by yielding a fd with incompatible flags, hmm.
WDYT?
| } | ||
| } | ||
|
|
||
| // Output as file type (hardlinks become regular files in output) |
There was a problem hiding this comment.
Is this a necessary limitation?
Do symlinks persist?
|
Additionally, do we care about exposing files with their original mtime? Or really ever have two files with same hash that have different mtime? "Is there more to our input contract than literally reading from paths and getting specific content"? (see comments about dropping symlink-ness, hardlink-ness, so on) |
75d6e04
into
feature/workspace-virtualization
|
Accidentally merged these changes. I reverted this merge in the feature branch, and continuing work on: #1787 |
This PR:
Pathobject)WAKE_CASto be able to switch between CAS mode and writing directly to workspace (for backwards compatibility purposes).staging_fileskey, which has relevant metadata and distinction between files, directories, symlinks, and hardlinks like so:staging_filesoutput, hash the staged output files and insert them into the CASFuture PR's will need to remove the ability to directly read from the workspace (once we implement
Sourceinput files directly being added to the CAS)