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

fs_inode:Change the type of i_crefs to atomic_int #13443

Merged
merged 4 commits into from
Sep 23, 2024

Conversation

crafcat7
Copy link
Contributor

@crafcat7 crafcat7 commented Sep 14, 2024

Summary

Benefit from PR#13044, we can change the the inode->i_crefs to atomic. That we can avoid problems:
Deadlocks caused by the server requesting reads from the client and the client requesting writes from the server when operating a file system with multiple cores (e.g. RPMSGFS).

The main changes are as follows:
1.Modified the i_crefs from int16_t to atomic_int
2.Modified the i_crefs add, delete, read, and initialize interfaces to atomic operations

The main change of this PR is to change the i_crefs count of inode to atomic type and use atomic operations to maintain i_crefs.

Impact

Impact on user: NO, This is a change within VFS and should be transparent to external developers.
Impact on build: YES, This may affect compilation because the members of the inode structure have changed. Different compilers/platforms have different support for atomic, so some warnings are skipped.
Impact on compatibility: This implementation aims to be backward compatible. No existing functionality is expected to be broken.

Testing

Build Host(s): Linux x86
Target(s): sim/nsh

Basic operations on the file system can work normally

@anchao
Copy link
Contributor

anchao commented Sep 14, 2024

Please check that all i_crefs are replaced correctly, eg:
https://github.com/apache/nuttx/blob/master/drivers/serial/pty.c#L335

@anchao
Copy link
Contributor

anchao commented Sep 14, 2024

Ditto, please make sure to go through the code carefully
https://github.com/apache/nuttx/blob/master/fs/vfs/fs_dir.c#L83

@crafcat7 crafcat7 force-pushed the inode_atomic branch 2 times, most recently from acadd16 to e5535a7 Compare September 14, 2024 09:12
@crafcat7
Copy link
Contributor Author

Please check that all i_crefs are replaced correctly, eg: https://github.com/apache/nuttx/blob/master/drivers/serial/pty.c#L335

Done

Ditto, please make sure to go through the code carefully https://github.com/apache/nuttx/blob/master/fs/vfs/fs_dir.c#L83

Done

@crafcat7 crafcat7 force-pushed the inode_atomic branch 4 times, most recently from 75fbe24 to b291fb2 Compare September 14, 2024 16:33
@crafcat7 crafcat7 force-pushed the inode_atomic branch 3 times, most recently from c720470 to 4911fee Compare September 18, 2024 02:53
@crafcat7
Copy link
Contributor Author

I found that some platforms have the following warning when using different compilers

====================================================================================
Configuration/Tool: samd20-xplained/nsh,CONFIG_ARM_TOOLCHAIN_CLANG
2024-09-14 17:29:29
------------------------------------------------------------------------------------
  Cleaning...
  Configuring...
  Disabling CONFIG_ARM_TOOLCHAIN_GNU_EABI
  Enabling CONFIG_ARM_TOOLCHAIN_CLANG
  Building NuttX...
Error: inode/fs_inodeaddref.c:50:7: error: large atomic operation may incur significant performance penalty; the access size (4 bytes) exceeds the max lock-free size (0  bytes) [-Werror,-Watomic-alignment]
   50 |       atomic_fetch_add(&inode->i_crefs, 1);

In the current strategy, if the compiler itself does not support Atomic, Nuttx Base Atomic (low-performance version) will be used. So can we ignore it?

@crafcat7 crafcat7 force-pushed the inode_atomic branch 9 times, most recently from e90852e to 6b05d09 Compare September 18, 2024 17:23
@crafcat7 crafcat7 force-pushed the inode_atomic branch 2 times, most recently from 607130d to cf85911 Compare September 20, 2024 02:36
@github-actions github-actions bot added the Size: M The size of the change in this PR is medium label Sep 20, 2024
@crafcat7 crafcat7 force-pushed the inode_atomic branch 3 times, most recently from 22ae464 to bfc7828 Compare September 21, 2024 13:00
@github-actions github-actions bot added Arch: arm Issues related to ARM (32-bit) architecture Area: File System File System issues Area: OS Components OS Components issues Area: Crypto labels Sep 21, 2024
include/nuttx/atomic.h Outdated Show resolved Hide resolved
fs/vfs/fs_timerfd.c Outdated Show resolved Hide resolved
include/nuttx/fs/fs.h Outdated Show resolved Hide resolved
fs/shm/shmfs.c Outdated Show resolved Hide resolved
fs/shm/shmfs.c Outdated Show resolved Hide resolved
fs/vfs/fs_pseudofile.c Outdated Show resolved Hide resolved
Summary:
  1.Modified the i_crefs from int16_t to atomic_int
  2.Modified the i_crefs add, delete, read, and initialize interfaces to atomic operations
The purpose of this change is to avoid deadlock in cross-core scenarios, where A Core blocks B Core’s request for a write operation to A Core when A Core requests a read operation to B Core.

Signed-off-by: chenrun1 <[email protected]>
Error: vfs/fs_epoll.c:126:3: error: macro 'ATOMIC_VAR_INIT' has been marked as deprecated [-Werror,-Wdeprecated-pragma]
  ATOMIC_VAR_INIT(1),     /* i_crefs */
  ^
/Applications/Xcode_15.2.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/lib/clang/15.0.0/include/stdatomic.h:54:41: note: macro marked 'deprecated' here
                                        ^
1 error generated.
make[1]: *** [fs_epoll.o] Error 1
Error: socket/socket.c:78:3: error: macro 'ATOMIC_VAR_INIT' has been marked as deprecated [-Werror,-Wdeprecated-pragma]
  ATOMIC_VAR_INIT(1),     /* i_crefs */

Signed-off-by: chenrun1 <[email protected]>
…upport it

Summary:
  Configuration/Tool: zp214xpa/nsh,CONFIG_ARM_TOOLCHAIN_GNU_EABI
2024-09-14 06:11:00
------------------------------------------------------------------------------------
  Cleaning...
  Configuring...
  Disabling CONFIG_ARM_TOOLCHAIN_GNU_EABI
  Enabling CONFIG_ARM_TOOLCHAIN_GNU_EABI
  Building NuttX...
arm-none-eabi-ld: /github/workspace/sources/nuttx/staging/libfs.a(fs_inoderemove.o): in function `inode_remove':
fs_inoderemove.c:(.text.inode_remove+0x94): undefined reference to `__sync_synchronize'
arm-none-eabi-ld: fs_inoderemove.c:(.text.inode_remove+0x9c): undefined reference to `__sync_synchronize'
make[1]: *** [Makefile:212: nuttx] Error 1
make: *** [tools/Unix.mk:548: nuttx] Error 2
make: Target 'all' not remade because of errors.

Signed-off-by: chenrun1 <[email protected]>
When the toolchain does not support atomic, it will use the version implemented by NuttX (low performance version). This scenario is consistent with the original design, so we can ignore it.

see bug here:
https://bugs.llvm.org/show_bug.cgi?id=43603

Error: inode/fs_inodeaddref.c:50:7: error: large atomic operation may incur significant performance penalty; the access size (4 bytes) exceeds the max lock-free size (0  bytes) [-Werror,-Watomic-alignment]
   50 |       atomic_fetch_add(&inode->i_crefs, 1);
      |       ^
/tools/clang-arm-none-eabi/lib/clang/17/include/stdatomic.h:152:43: note: expanded from macro 'atomic_fetch_add'
  152 | #define atomic_fetch_add(object, operand) __c11_atomic_fetch_add(object, operand, __ATOMIC_SEQ_CST)
      |                                           ^
1 error generated.
make[1]: *** [Makefile:83: fs_inodeaddref.o] Error 1
Error: inode/fs_inodefind.c:74:7: error: large atomic operation may incur significant performance penalty; the access size (4 bytes) exceeds the max lock-free size (0  bytes) [-Werror,-Watomic-alignment]
   74 |       atomic_fetch_add(&node->i_crefs, 1);

Signed-off-by: chenrun1 <[email protected]>
@xiaoxiang781216 xiaoxiang781216 merged commit ab4d727 into apache:master Sep 23, 2024
29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Arch: arm Issues related to ARM (32-bit) architecture Area: File System File System issues Area: OS Components OS Components issues Size: M The size of the change in this PR is medium
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants