Skip to content

Conversation

@mvvillarrealblozis
Copy link

The df command provides filesystem statistics such as total disk size, used space, available space, and inode information.

Key Features:

Basic Disk Usage Reporting:

The df command displays total blocks, used blocks, and free blocks for the entire filesystem. When run without arguments, it reports on all mounted filesystems. If one or more paths are provided, it reports only on the filesystems associated with those paths.

Human-Readable Output (-h option):

Outputs sizes in KB, MB, GB, etc., making it easier to understand large filesystems.

Inode Usage Information (-i option):

Displays inode usage instead of block usage, providing insight into the number of inodes available and used.

Usage:

df           # Reports disk usage for all mounted filesystems.
df <path>    # Reports disk usage for the specified path's filesystem.
df -h        # Outputs disk usage in human-readable format (KB, MB, GB, etc.).
df -i        # Displays inode usage instead of block usage. 

System Call Changes:

Added the statvfs system call to gather filesystem statistics from the kernel. This system call provides the df command with information about total blocks, free blocks, used blocks, total inodes, and free inodes.

Future Development:

Add filters to report only on specific filesystem types (e.g., ext2, ext3, etc.).
Implement color-coded output to highlight filesystems that are nearing capacity.

@Unbanneduser
Copy link

I will code review this.

@yahvi21
Copy link

yahvi21 commented Oct 2, 2024

I can code review this

@Unbanneduser
Copy link

fs.c
bmap_isfree() and bmap_free_blocks() Functions:
bread() should have error handling to ensure that reading from disk doesn’t fail silently.
free_inodes() Function:
Similar to bmap_isfree(), error handling should be considered around the inode structure fetching (iget()).

df.c
The sys_statvfs() function was added to provide filesystem statistics to userspace programs.
copyout() could fail silently if the destination address is invalid or protected. Error checking is present, but additional logging or error handling could be beneficial.

other utils are good

Most files are missing comments on the purpose of the new functions and structs.
Error Handling:
Several functions (eg. bmap_isfree, free_inodes, and sys_statvfs) lack robust error handling for external failures (eg. disk read failures).

High-Level Checks:
[✔️] PR fully resolves the task/issue.
[✔️] Does the PR have tests? (No specific test files added, but the df program allows for functionality testing.)
[✔️] Does the OS still compile and run? (No compilation issues noted.)
[ ] Does the PR follow project code formatting standards? (Minor formatting inconsistencies in a few files, such as missing comments.)
[✔️] Is documentation included?
Code Checks:
[✔️] Does the code achieve its intended purpose?
Were all edge cases considered? (Error handling for functions like bmap_isfree and sys_statvfs could be
[✔️] Is the code organized and maintainable?
[✔️] Does the code maintain the same level of performance? (No performance regressions noted.)

@eshadupz
Copy link

eshadupz commented Oct 5, 2024

I can code review this!

@eshadupz
Copy link

eshadupz commented Oct 5, 2024

High Level Checks:

  • PR fully resolves the task/issue
  • Does the PR have tests?
  • Does the PR follow project code formatting standards?
  • Does the OS still compile and run?
  • Is documentation included?

Code Checks:

  • Does the code achieve its intended purpose?
  • Were all edge cases considered?
  • Is the code organized and maintainable?
  • Does the code maintain the same level of performance? (no performance regressions)

Cool project! It's pretty straightforward and informative- an edge case that I found is when I put an invalid flag like df -p, it prints out the output for df, but I think there could be a more specific error message that says "flag doesn't exist" or something. For future advancements, you could probably add more error handling in terms of hardware issues the computer itself faces. Your code style and documentation looks good to me!

@malensek
Copy link
Contributor

malensek commented Oct 11, 2024

I think it's great that your project has made some changes to kernel space. There are a few problems with what is being claimed in the PR and what was proposed in the original issue and some code cleanup / refactoring is necessary. I'm quite surprised how much you know about the xv6 file system code, so at some point it'd be great to do a one-on-one code review on this so I can pick your brain.

When run without arguments, it reports on all mounted filesystems. If one or more paths are provided, it reports only on the filesystems associated with those paths.

This does not seem to be implemented. xv6 doesn't have a concept of file system mount points so I'm a bit confused how you would claim you support this. It's even in the usage:

df <path>    # Reports disk usage for the specified path's filesystem.

But not anywhere in the code. In the original issue (#63) it says would have some code to traverse from subdirectories up to the root of the fs.

Outputs sizes in KB, MB, GB, etc., making it easier to understand large filesystems.

This is not actually supported! I don't see any code for conversions. What if I up the FS size to 1 GB? It looks like everything is just divided by 2.

Add filters to report only on specific filesystem types (e.g., ext2, ext3, etc.).

Again, this isn't something that xv6 has. It has one FS type.

The first two things should've been caught by code reviewers. (-0.5 for both)

Code fixes:

  • Does not follow xv6 conventions
  • There is no documentation. This needs a minimal man page style text file in docs, at least.
  • Implemented sys_statvfs in sysproc.c instead of sysfile.c. These two locations are for syscall stubs, you'd implement your core business logic in fs.c and call it from sysfile.c instead of extern-ing data structures and including more headers. As it stands now, we've broken separation of concerns across several files.
  • Instead of copying used_blocks out of kernel space, this should be calculated in user space. No need to do this as a privileged operation and then have to copy more data out.
  • As mentioned above, the flag handling needs work.

You can fix the style/doc/organization issues above to get +1 back on your current score.

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.

5 participants