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.

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

Implement mounting and unmounting for external journal device #61

Merged
merged 1 commit into from
Jul 1, 2024

Conversation

jason50123
Copy link
Contributor

@jason50123 jason50123 commented Jun 22, 2024

This pull request continues from #59 (comment).
Due to some issues with Git, I have addressed them and created this new pull request.

Added a function simplefs_parse_options in the fill super process to obtain the external journal device and configure it using jbd2 related functions. Additionally, journal-related functionality was added to the write functions to record metadata "extent" information.

Also, added a "make journal" command in the Makefile, allowing users to create an external journal device image and mount it to simplefs.

super.c Outdated Show resolved Hide resolved
@jason50123 jason50123 force-pushed the external-journal branch 2 times, most recently from 5807d5c to 1c9b940 Compare June 24, 2024 04:40
@jserv
Copy link
Collaborator

jserv commented Jun 24, 2024

CI pipeline complains the following:

Testing cmd: touch 40919.txt...
Testing cmd: touch 40920.txt...Unmounting filesystem...
Remounting filesystem...
Remount succeeds.
Failed: file_1.txt content is incorrect.
Error: Process completed with exit code 2.

@jserv
Copy link
Collaborator

jserv commented Jun 24, 2024

This pull request continues from #59 (comment). Due to some issues with Git, I have addressed them and created this new pull request.

Don't do this. You should simply run git rebase -i and then git push --force.
If a workman wishes to do a good job, he must first sharpen his tools.

@jason50123
Copy link
Contributor Author

jason50123 commented Jun 24, 2024

@jserv
This version has not yet addressed issues related to Put super. So should I handle this part first and then submit a pull request?

CI pipeline complains the following:


Testing cmd: touch 40919.txt...

Testing cmd: touch 40920.txt...Unmounting filesystem...

Remounting filesystem...

Remount succeeds.

Failed: file_1.txt content is incorrect.

Error: Process completed with exit code 2.

@jserv
Copy link
Collaborator

jserv commented Jun 24, 2024

@jserv This version has not yet addressed issues related to Put super. So should I handle this part first and then submit a pull request?

Up to you. You should resolve the issue reported by GitHub Actions in advance. Later, you can create a new commit within the pull request (again, using git rebase -i) or submit a new pull request to consolidate the CI pipeline.

@jason50123
Copy link
Contributor Author

@jserv This version has not yet addressed issues related to Put super. So should I handle this part first and then submit a pull request?

Up to you. You should resolve the issue reported by GitHub Actions in advance. Later, you can create a new commit within the pull request (again, using git rebase -i) or submit a new pull request to consolidate the CI pipeline.

I understand, I will take care of it.

jserv

This comment was marked as outdated.

Makefile Show resolved Hide resolved
Copy link
Collaborator

@jserv jserv left a comment

Choose a reason for hiding this comment

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

Update top-level README.md for explaining journaling support.

@jason50123 jason50123 force-pushed the external-journal branch 2 times, most recently from 8bbed92 to 44ebcab Compare June 27, 2024 00:25
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
super.c Outdated Show resolved Hide resolved
super.c Outdated Show resolved Hide resolved
super.c Outdated Show resolved Hide resolved
@jserv
Copy link
Collaborator

jserv commented Jun 27, 2024

Can you provide journal specific test cases?

@jason50123 jason50123 changed the title Mount the external journal device and write accessible extent metadata Implement Mounting and Unmounting Functions for External Journal Device Jun 27, 2024
@jason50123
Copy link
Contributor Author

Can you provide journal specific test cases?

@jserv Due to certain issues, unexpected errors occur after journal data during mount-remount operations. Therefore, this commit only addresses the mounting and unmounting of the external journal device. The journaling metadata part will be submitted in a pull request after thorough testing is completed.

@jserv
Copy link
Collaborator

jserv commented Jun 27, 2024

Due to certain issues, unexpected errors occur after journal data during mount-remount operations. Therefore, this commit only addresses the mounting and unmounting of the external journal device. The journaling metadata part will be submitted in a pull request after thorough testing is completed.

Then, you should consolidate the git commit message and address the known issue. We do still need some preliminary test cases.

@jason50123 jason50123 requested a review from jserv June 27, 2024 07:34
Copy link
Collaborator

@jserv jserv left a comment

Choose a reason for hiding this comment

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

For the sake of Linux API changes, there are several implementation of simplefs_get_dev_journal function. Move some common code in a helper function, which is independent from Linux kernel versions, so that we can shrink the lines of code.

@jason50123 jason50123 requested a review from jserv June 27, 2024 20:43
@jserv jserv changed the title Implement Mounting and Unmounting Functions for External Journal Device Implement mounting and unmounting for external journal device Jun 28, 2024
super.c Outdated Show resolved Hide resolved
super.c Outdated Show resolved Hide resolved
super.c Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
super.c Outdated Show resolved Hide resolved
@jason50123 jason50123 force-pushed the external-journal branch 4 times, most recently from cdfe3bb to cf0d1bd Compare June 28, 2024 07:59
@jason50123 jason50123 requested a review from jserv June 28, 2024 08:00
super.c Outdated Show resolved Hide resolved
super.c Show resolved Hide resolved
super.c Outdated Show resolved Hide resolved
super.c Outdated Show resolved Hide resolved
Copy link
Collaborator

@jserv jserv left a comment

Choose a reason for hiding this comment

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

Improve the git commit messages to consist of several complete English sentences.

This update introduces the following changes:

1. Added a function simplefs_parse_options in the fill super process
to obtain the external journal device and configure it using jbd2
related functions. This allows the file system to correctly initialize
and manage the external journal device.

2. Implemented additional functions in put_super to handle the journal
device during the unmount process. This ensures that the journal device
is properly managed and released when the file system is unmounted.

3. Added a make journal command in the Makefile. This new command allows
 users to create an external journal device image and mount it to
 simplefs, simplifying the setup and usage of the external journal.

Future Work:
Currently, the external journal device size is fixed at 8MB. To use a
different size, corresponding variables in the code need to be modified.
Additionally, support for an internal journal (inode journal) can be
added in the future to further improve the file system's capabilities.
@jason50123 jason50123 requested a review from jserv June 29, 2024 01:10
@jserv jserv merged commit 403e1b8 into sysprog21:master Jul 1, 2024
2 checks passed
@jserv
Copy link
Collaborator

jserv commented Jul 1, 2024

Thank @jason50123 for contributing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants