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

v9fs:Add socket support #15597

Merged
merged 4 commits into from
Jan 20, 2025
Merged

v9fs:Add socket support #15597

merged 4 commits into from
Jan 20, 2025

Conversation

crafcat7
Copy link
Contributor

Summary

In #13001, I added a new distributed file system in NuttX, which only supports VIRTIO as a data transmission method.
In this PR, I implemented a Socket-based transmission method, so that V9FS can access the content on the Host based on the Socket method, which is suitable for more scenarios.
I also added the V9FS document to explain how to use it.
In short:
- mplemented a socket-based v9fs driver
- Enhanced Net-related, now TCP supports multi-segment IOV sending
- Added V9FS Document

Impact

  1. Enhanced NET related changes, now TCP supports multi-segment IOV mode sending.
  2. V9FS supports socket as a transmission medium
  3. Added V9FS Document

Testing

Build Host(s): Linux x86
Target(s): qemu
Passed local file system related tests

@github-actions github-actions bot added Area: Documentation Improvements or additions to documentation Area: Networking Effects networking subsystem Area: File System File System issues Size: L The size of the change in this PR is large labels Jan 18, 2025
@nuttxpr
Copy link

nuttxpr commented Jan 18, 2025

[Experimental Bot, please feedback here]

This PR mostly meets the NuttX requirements, but could be improved by providing more specific details. Here's a breakdown:

Strengths:

  • Clear summary of changes: The summary explains the "why," "what," and "how" of the changes. Referencing the prior PR is also helpful.
  • Acknowledges impact areas: The impact section touches on several key aspects, like functionality, documentation, and networking changes.
  • Provides basic testing information: The testing section indicates the host and target environments.

Weaknesses & Suggestions for Improvement:

  • Missing Issue References: While the prior PR is mentioned, specific NuttX or NuttX Apps issues should be linked if applicable. Even if there isn't a directly related issue, creating one for tracking the feature request/bug fix is recommended.

  • Impact Section Lacks Detail: The impact section is too brief. Instead of just stating "Enhanced NET related changes," describe specifically how the multi-segment IOV sending impacts users, builds, etc. For example:

    • Impact on user: Users can now potentially see performance improvements in network file system operations.
    • Impact on build: No changes to the build process itself, but new code is added.
    • Impact on hardware: No direct hardware impact, but affects network driver usage.
    • Impact on security: Consider potential security implications of socket-based V9FS (e.g., authentication, data integrity). Even if there are none, explicitly state this.
    • Impact on compatibility: Does this change break anything? Is it compatible with all existing network configurations?
  • Missing "Before/After" Testing Logs: The testing section states tests passed, but provides no actual logs. Include relevant log snippets demonstrating the change's effect (e.g., mounting the V9FS filesystem, performing file operations, demonstrating the multi-segment IOV sending). Show how the behavior differs before and after your changes.

  • Limited Testing Information: While QEMU is a good start, testing on actual hardware is highly recommended if possible. Specify the QEMU configuration used (e.g., qemu-system-x86_64 -M virt). More testing details like the specific tests run (if any automated test suite was used) would strengthen the PR.

In short: The PR is on the right track, but needs more detail, particularly in the Impact and Testing sections, to fully meet the NuttX requirements. Be as explicit and thorough as possible to ensure reviewers can properly assess the changes.

@xiaoxiang781216
Copy link
Contributor

@crafcat7 please fix the document error

@crafcat7 crafcat7 changed the title V9fs socket v9fs:Add socket support Jan 18, 2025
@crafcat7
Copy link
Contributor Author

@crafcat7 please fix the document error

Done,now it has passed the document check.

net/tcp/tcp_recvfrom.c Outdated Show resolved Hide resolved
net/socket/recvmsg.c Show resolved Hide resolved
fs/v9fs/socket_9p.c Outdated Show resolved Hide resolved
fs/v9fs/Kconfig Show resolved Hide resolved
crafcat7 and others added 2 commits January 19, 2025 00:34
Summary:
  1.Add new api for socket parsing header - v9fs_parse_size
  2.Add socket driver for 9pfs

Signed-off-by: chenrun1 <[email protected]>
To prepare for supporting multiple iov in each protocol.

Signed-off-by: Zhe Weng <[email protected]>
@crafcat7 crafcat7 force-pushed the v9fs_socket branch 2 times, most recently from 7a1cdc6 to 3c389a6 Compare January 18, 2025 16:43
Summary:
  Implement multiple iovlen recvmsg in inet_sockif, only TCP mode
  support at current.

Signed-off-by: chenrun1 <[email protected]>
Signed-off-by: chenrun1 <[email protected]>
@xiaoxiang781216 xiaoxiang781216 merged commit 2375a55 into apache:master Jan 20, 2025
40 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Documentation Improvements or additions to documentation Area: File System File System issues Area: Networking Effects networking subsystem Size: L The size of the change in this PR is large
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants