fs: use atomic_t store f_refs

[hujun260]

Summary

fs: use atomic_t store f_refs

Impact

fs

Testing

ci

[nuttxpr]

[Experimental Bot, please feedback here]

No, this PR does not fully meet the NuttX requirements. While it provides a summary of the change, it lacks crucial details. Here's a breakdown of what's missing:

• Summary: While the title "fs: use atomic_t store f_refs" is a good start, the summary needs more context. Why is this change necessary? What problem does it solve (e.g., race condition, performance improvement)? What specific part of the filesystem code is affected (e.g., file open/close operations, directory handling)? How does using atomic_t address the issue?

• Impact: The single word "fs" is insufficient. Each impact category needs a YES/NO answer followed by a description if the answer is YES. Even if the answer is NO, it's best to explicitly state it for clarity (e.g., "Impact on user: NO"). Consider the potential impact on performance, memory usage, and any other relevant aspects.

• Testing: "ci" is not enough. Specify the specific CI configurations used (e.g., which target architectures and boards). The "Testing logs before change" and "Testing logs after change" sections are completely empty. Provide actual logs or output demonstrating the issue before the change and the corrected behavior after the change. This is crucial for reviewers to understand the effect of the PR.

Example of how to improve the PR description:

Summary

This change replaces the existing reference counting mechanism for file structures (struct file) with an atomic_t to store f_refs. The previous implementation used a regular integer and was susceptible to race conditions when multiple threads accessed a file concurrently, potentially leading to incorrect reference counts and premature file closure. This change ensures thread-safe reference counting, preventing data corruption and crashes. This addresses issue #[NuttX Issue Number].

Impact

• Is new feature added? NO
• Is existing feature changed? YES. This change modifies the internal file reference counting mechanism.
• Impact on user: NO. Users should not observe any functional changes. Performance may slightly improve due to the atomic operations.
• Impact on build: NO.
• Impact on hardware: NO.
• Impact on documentation: NO.
• Impact on security: YES. This change mitigates a potential race condition that could lead to data corruption.
• Impact on compatibility: NO.
• Anything else to consider: A slight increase in memory usage due to the larger size of atomic_t compared to a regular integer.

Testing

I confirm that changes are verified on local setup and works as intended:

• Build Host(s): Linux (Ubuntu 20.04), x86_64, GCC 9.4.0
• Target(s): sim:qemu-x86_64, stm32f4discovery:nsh

Testing logs before change:

(demonstrate the race condition or incorrect behavior.  e.g., open a file in two threads, close it in one, and show that the other thread can still access it despite the reference count reaching zero).

Testing logs after change:

(repeat the same test and show that the issue is resolved. e.g., the second thread can no longer access the file after the first thread closes it).

By providing more detail and context, the PR becomes much easier to review and understand, increasing the chances of it being accepted.

[anchao]

race condition ... if ISR after read f_refs

[hujun260]

These few lines of code are already in spin_lock_irqsave

[anchao]

1. [CPU1]filep->f_refs == 1
2. [CPU1]atomic_read > 0
3. [CPU2]atomic_fetch_sub - 1 (fs_putfilep)
4. [CPU1]atomic_fetch_add + 1
5. [CPU2]file_close

[hujun260]

The issue you mentioned was introduced by me. Here's the fix:
#15163

[xiaoxiang781216]

let's create a new patch to remove fl_crefs at line 498

[hujun260]

ok