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

Reimplement the read and the write operations #62

Merged
merged 1 commit into from
Jun 25, 2024

Conversation

HotMercury
Copy link
Collaborator

problem:
Currently, simplefs does not implement read and write operations and therefore uses the default implementation provided by the Linux kernel. Unfortunately, this implementation assumes that the data blocks are filled contiguously, which leads to potential performance and usability problems.

Implemented simplefs_file_read:

  • Reads data from the file based on the specified position and length.
  • Ensures that data is read only within the file size limits.
  • Directly reads data from the disk through extent blocks.

Implemented simplefs_file_write:

  • Writes data to the file starting from the specified position.
  • Allocates new blocks if necessary, managing extent-based allocation.
  • Updates inode size and block count correctly after writing.
  • Ensures data is written within the file system's maximum file size limit.
  • Directly writes data to the disk through extent blocks.

Both functions include proper error handling, resource management, and synchronization to ensure data store in disk.

Close #57

file.c Outdated Show resolved Hide resolved
file.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. Rewrite in shorter and disciplined manner.

@HotMercury HotMercury force-pushed the master branch 2 times, most recently from dde08bf to b631bb3 Compare June 25, 2024 12:08
file.c Outdated
@@ -310,5 +474,7 @@ const struct file_operations simplefs_file_ops = {
.read_iter = generic_file_read_iter,
.write_iter = generic_file_write_iter,
.fsync = generic_file_fsync,
.open = simplefs_file_open,
.open = simplefs_open,
.read = simplefs_read, /*if exist, read_iter will not be called*/
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does the comment "/if exist, read_iter will not be called/" make sense?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it time to drop generic_file_read_iter and generic_file_write_iter?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am not sure if it is time to drop these functions. I need to check more. I traced the read syscall and found the following call sequence:
ksys_read -> vfs_read.

if (file->f_op->read)
    ret = file->f_op->read(file, buf, count, pos);
else if (file->f_op->read_iter)
    ret = new_sync_read(file, buf, count, pos);
else
    ret = -EINVAL;

vfs_read

Copy link
Collaborator

Choose a reason for hiding this comment

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

You can utilize Ftrace along with some file system testers such as fio.

Copy link
Collaborator Author

@HotMercury HotMercury Jun 25, 2024

Choose a reason for hiding this comment

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

To test this issue, I used fio and ftrace to check if simplefs and generic_file_read_iter are being used. For convenience, I first wrapped test_generic_file_read_iter. Here are my test results, which show that generic_file_read_iter is indeed not being used.

wrap generic_file_read_iter

static ssize_t test_generic_file_read_iter(struct kiocb *iocb, struct iov_iter *to)
{
    return generic_file_read_iter(iocb, to);
}

fio in simplefs filesystem

$ fio --name=readtest --size=1M --filename=./testfile --bs=4k --rw=read --iodepth=4

ftrace setting

echo function > /sys/kernel/debug/tracing/current_tracer
echo test_generic_file_read_iter > /sys/kernel/debug/tracing/set_ftrace_filter
echo simplefs_read > /sys/kernel/debug/tracing/set_ftrace_filter

We can see that only simplefs_read is used.
output:

root@vm:/sys/kernel/debug/tracing# cat trace
# tracer: function
#
# entries-in-buffer/entries-written: 256/256   #P:4
#
#                                _-----=> irqs-off/BH-disabled
#                               / _----=> need-resched
#                              | / _---=> hardirq/softirq
#                              || / _--=> preempt-depth
#                              ||| / _-=> migrate-disable
#                              |||| /     delay
#           TASK-PID     CPU#  |||||  TIMESTAMP  FUNCTION
#              | |         |   |||||     |         |
             fio-44553   [001] ...1. 83186.031300: simplefs_read <-vfs_read
             fio-44553   [001] ...1. 83186.031325: simplefs_read <-vfs_read
             fio-44553   [001] ...1. 83186.031328: simplefs_read <-vfs_read
             fio-44553   [001] ...1. 83186.031332: simplefs_read <-vfs_read
             ...
             fio-44553   [001] ...1. 83186.031880: simplefs_read <-vfs_read

Copy link
Collaborator

Choose a reason for hiding this comment

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

We can see that only simplefs_read is used.

It sounds good. Let's move forward.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should I drop them in this PR?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Should I drop them in this PR?

Yes. And you can append the above experiments and observations.

This commit implements the 'simplefs_write' and 'simplefs_read'
functions for the simplefs filesystem. These functions handle
data writing and reading operations directly from the disk,
bypassing the page cache. To achieve this, we used the 'sb_bread'
and 'brelse' functions provided by the Linux kernel to read data
directly from the disk. For writing data, we used the
'mark_buffer_dirty' and 'sync_dirty_buffer' functions to ensure that
data is correctly written to the disk.

Removing 'generic_file_read_iter' and 'generic_file_write_iter'.
Through the following experiment, we can observe that once we
implement 'simplefs_read', 'generic_file_read_iter' is no longer
called, and the same applies to simplefs_write.

Experiment:
First, wrap 'generic_file_read_iter' with 'test_generic_file_read_iter'
and then set up the following:
$ echo function > current_tracer
$ echo test_generic_file_read_iter > set_ftrace_filter
$ echo simplefs_read > set_ftrace_filter

Run the fio command:
$ fio --name=readtest --size=1M --filename=./testfile --bs=4k \
--rw=read --iodepth=4

From the results, we can see that 'test_generic_file_read_iter' is
no longer used.

root@vm:/sys/kernel/debug/tracing# cat trace
tracer: function

entries-in-buffer/entries-written: 256/256   #P:4

                                _-----=> irqs-off/BH-disabled
                               / _----=> need-resched
                              | / _---=> hardirq/softirq
                              || / _--=> preempt-depth
                              ||| / _-=> migrate-disable
                              |||| /     delay
           TASK-PID     CPU#  |||||  TIMESTAMP  FUNCTION
              | |         |   |||||     |         |
             fio-44553   [001] ...1. 83186.031300: simplefs_read <-vfs_read
             fio-44553   [001] ...1. 83186.031325: simplefs_read <-vfs_read
             fio-44553   [001] ...1. 83186.031328: simplefs_read <-vfs_read
             fio-44553   [001] ...1. 83186.031332: simplefs_read <-vfs_read
             ...
             fio-44553   [001] ...1. 83186.031880: simplefs_read <-vfs_read

Close sysprog21#57
@jserv jserv merged commit 52cf2a1 into sysprog21:master Jun 25, 2024
2 checks passed
@jserv
Copy link
Collaborator

jserv commented Jun 25, 2024

Thank @HotMercury 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.

Reimplement the read and the write operations
2 participants