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

use atomic to protect f_refs #15163

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

hujun260
Copy link
Contributor

@hujun260 hujun260 commented Dec 12, 2024

Summary

use atomic to protect f_refs
fix regresion from #14801

Impact

fs refs

Testing

ci ostest

@github-actions github-actions bot added Area: File System File System issues Size: S The size of the change in this PR is small labels Dec 12, 2024
@nuttxpr
Copy link

nuttxpr commented Dec 12, 2024

[Experimental Bot, please feedback here]

No, this PR does not fully meet the NuttX requirements. While it provides a summary and mentions testing, it lacks crucial details.

Here's what's missing:

  • Summary: Needs more detail. How does the small lock protect f_refs? What exactly was the regression from the linked PR? What functional part of the code changed (e.g., VFS, a specific filesystem)?
  • Impact: The current description is insufficient. Address all impact points with "YES" or "NO" and provide descriptions for any "YES" responses. Consider:
    • User Impact: Will applications using the affected filesystem notice any changes?
    • Build Impact: Are any new build dependencies introduced?
    • Hardware Impact: Does this affect any specific architectures or boards?
    • Documentation Impact: Does this change require documentation updates?
    • Security Impact: Does using a smaller lock introduce or mitigate any security vulnerabilities?
    • Compatibility Impact: Does this break compatibility with any existing code or applications?
  • Testing: "ci ostest" is not enough. Provide specific details about the build host (OS, CPU, compiler) and the target(s) (architecture, board, configuration). Include actual test logs before and after the change, demonstrating the issue and the fix. Just stating that it passed CI isn't sufficient for review; the reviewer needs to see the evidence.

The PR needs to be significantly expanded to meet the requirements and be properly reviewed.

@hujun260 hujun260 force-pushed the apache_13 branch 2 times, most recently from 07361cd to 634f337 Compare December 12, 2024 13:58
fs/inode/fs_files.c Outdated Show resolved Hide resolved
@hujun260 hujun260 changed the title use small lock to protect f_refs use atomic to protect f_refs Dec 13, 2024
fix regresion from apache#14801

Signed-off-by: hujun5 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: File System File System issues Size: S The size of the change in this PR is small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants