Skip to content

feat(fs): implement FileSystem::info() for FAT and ext4 to unblock statfs(2)#1853

Open
Deepak8858 wants to merge 1 commit intoDragonOS-Community:masterfrom
Deepak8858:fix/issue-1842-fs-info
Open

feat(fs): implement FileSystem::info() for FAT and ext4 to unblock statfs(2)#1853
Deepak8858 wants to merge 1 commit intoDragonOS-Community:masterfrom
Deepak8858:fix/issue-1842-fs-info

Conversation

@Deepak8858
Copy link
Copy Markdown

Fixes #1842

Implemented FileSystem::info() for both FAT and ext4 filesystems to populate the FsInfo struct. This resolves the kernel panics encountered when statfs(2) or fstatfs(2) is called on these mounts, enabling proper metadata querying.

@github-actions github-actions Bot added the enhancement New feature or request label Apr 10, 2026
@fslongjin
Copy link
Copy Markdown
Member

Hello, please add test cases and complete the relevant testing. All pull requests will only be merged after contributors have self-tested them.

@fslongjin
Copy link
Copy Markdown
Member

@claude review these changes

@dragonos-community-ai-pr-reviewer
Copy link
Copy Markdown

dragonos-community-ai-pr-reviewer Bot commented Apr 10, 2026

Claude finished @fslongjin's task in 5m 23s —— View job


Claude's Analysis

I'll review the changes in PR #1853 and provide feedback. Here's my plan:

  • Gather context: Read the changed files and understand the implementation
  • Analyze the changes: Review the FileSystem::info() implementations for FAT and ext4
  • Check for issues: Look for bugs, safety concerns, and correctness
  • Review coding standards: Ensure consistency with the codebase
  • Check test coverage: See if tests exist and need to be added
  • Provide recommendations: Suggest improvements or additional considerations
  • Final summary: Compile review findings

Review of PR #1853: feat(fs): implement FileSystem::info() for FAT and ext4 to unblock statfs(2)

Summary

The PR adds implementations of FileSystem::info() for the FAT and ext4 filesystems, returning a FsInfo struct with blk_dev_id and max_name_len. This resolves kernel panics when statfs(2)/fstatfs(2) are called on these mounts.

Code Changes

  1. kernel/src/filesystem/ext4/filesystem.rs:
    • info() returns FsInfo { blk_dev_id: self.raw_dev.data() as usize, max_name_len: 255 }
  2. kernel/src/filesystem/fat/fs.rs:
    • info() returns FsInfo { blk_dev_id: self.gendisk.device_num().data() as usize, max_name_len: 255 }

Analysis

  • Correctness: The implementations are semantically correct:
    • blk_dev_id is derived from the underlying block device's device number (DeviceNumber), which matches the purpose of the field ("文件系统所在的块设备的id").
    • max_name_len is set to 255, which is the standard maximum filename length for both FAT and ext4.
  • Safety: No unsafe code; the casts from u32 to usize are safe.
  • Impact: Previously these methods contained todo!(), causing kernel panics. Now they return valid data, allowing statfs/fstatfs syscalls to proceed.

Suggestions for Improvement

  1. Use existing constants:
    • FAT already defines FAT_MAX_NAMELEN: u64 = 255. Use it instead of the magic number 255 for consistency.
    • For ext4, consider defining a similar constant (e.g., EXT4_MAX_NAMELEN) or importing it from the another_ext4 crate if available.
  2. Consider fsid field: The SuperBlock struct has an fsid field (mapped to f_fsid in userspace statfs). Currently it remains zero. If blk_dev_id is intended to populate fsid, that should be handled in super_block() or statfs(). However, FsInfo is separate from SuperBlock, so this may be intentional.
  3. Testing: As noted by @fslongjin, test cases should be added. The existing user‑space tests (user/apps/c_unitest/test_statfs.c and test_fstatfs.c) likely now pass, but unit tests for the info() method itself would be valuable. Consider adding a simple test that:
    • Creates a mock filesystem instance (or uses a real mounted filesystem in a test environment)
    • Calls info() and verifies that blk_dev_id matches the expected device number and max_name_len is correct.

Overall Assessment

The changes are minimal, correct, and solve the reported issue. They follow the existing patterns seen in other filesystems (e.g., devfs, eventfd). The main missing piece is test coverage, which should be addressed before merging.

Recommendation: Approve with suggestions – the PR can be merged after adding the suggested constants and (if feasible) unit tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat(fs): implement FileSystem::info() for FAT and ext4 to unblock statfs(2)

2 participants