filep Reference count #13296

Zhangshoukui · 2024-09-05T06:58:47Z

Summary

ncrease the reference count of filep，fs_getfilep adds +1 to the reference count. After the call is complete, run the fs_putfilep command to release the reference count.When the reference count reaches 0, the file is actually closed.

Impact

This prevents the file from being closed while it is being read or written

Testing

Zhangshoukui · 2024-09-05T07:05:12Z

Sorry, due to my irregular rebase, this PR is closed and a new trace is used.

./tools/configure.sh -l stm3240g-eval:knxwm
make -j7
I failed to build this configuration locally, any good suggestions？If not, I can only verify it through github's CI environment

xiaoxiang781216 · 2024-09-05T07:15:45Z

Sorry, due to my irregular rebase, this PR is closed and a new trace is used.

./tools/configure.sh -l stm3240g-eval:knxwm make -j7 I failed to build this configuration locally, any good suggestions？If not, I can only verify it through github's CI environment

the code size exceeds the reserved space, either:

optimize the code
extend the reserved space in ld script

anchao

this is a complete feature，please merge all patches into one

fs/Kconfig

fs/inode/fs_files.c

Zhangshoukui · 2024-09-06T02:31:25Z

@anchao ./tools/configure.sh -l stm3240g-eval:knxwm
There are also configurations that do not compile locally, is this a general configuration, and is it reasonable to add it to CI?

anchao · 2024-09-06T11:40:51Z

@anchao ./tools/configure.sh -l stm3240g-eval:knxwm There are also configurations that do not compile locally, is this a general configuration, and is it reasonable to add it to CI?

You could set the Kconfig default value of CONFIG_FS_REFCOUNT to n to keep the original configuration without any overhead.

xiaoxiang781216 · 2024-09-12T15:05:00Z

@Zhangshoukui please fix the conflict.

…eading writing... Signed-off-by: Shoukui Zhang <zhangshoukui@xiaomi.com>

60 | FAR struct inode *inode; | ^~~~~ Signed-off-by: Xiang Xiao <xiaoxiang@xiaomi.com>

task1: The refs is 0 but the inode has not been released task2: Executing files_duplist, get filep with refs is zero, but the inode has not yet released. [ 0.530600] [CPU1] [ 6] [ ALERT] [ap] sched_dumpstack: backtrace: [ 0.530800] [CPU1] [ 6] [ ALERT] [ap] sched_dumpstack: [ 6] [<0x4045172>] backtrace_unwind+0x241/0x244 [ 0.531200] [CPU1] [ 6] [ ALERT] [ap] sched_dumpstack: [ 6] [<0x4009150>] sched_backtrace+0x27/0x4c [ 0.531500] [CPU1] [ 6] [ ALERT] [ap] sched_dumpstack: [ 6] [<0x402f0fa>] sched_dumpstack+0x3d/0xa0 [ 0.531700] [CPU1] [ 6] [ ALERT] [ap] sched_dumpstack: [ 6] [<0x4002c90>] _assert+0x1e3/0x564 [ 0.532000] [CPU1] [ 6] [ ALERT] [ap] sched_dumpstack: [ 6] [<0x40221f6>] __assert+0x19/0x24 [ 0.532200] [CPU1] [ 6] [ ALERT] [ap] sched_dumpstack: [ 6] [<0x41fe376>] tcp_stop_monitor+0x61/0x6c [ 0.532500] [CPU1] [ 6] [ ALERT] [ap] sched_dumpstack: [ 6] [<0x41fe19e>] tcp_close+0xad/0xdc [ 0.532700] [CPU1] [ 6] [ ALERT] [ap] sched_dumpstack: [ 6] [<0x41ecc18>] inet_close+0x7b/0x8c [ 0.533000] [CPU1] [ 6] [ ALERT] [ap] sched_dumpstack: [ 6] [<0x41ec374>] psock_close+0x27/0x70 [ 0.533300] [CPU1] [ 6] [ ALERT] [ap] sched_dumpstack: [ 6] [<0x420b68e>] sock_file_close+0x15/0x3c [ 0.533500] [CPU1] [ 6] [ ALERT] [ap] sched_dumpstack: [ 6] [<0x4206704>] file_close+0x1f/0x80 [ 0.533800] [CPU1] [ 6] [ ALERT] [ap] sched_dumpstack: [ 6] [<0x4205278>] files_duplist+0x39f/0x45c [ 0.534100] [CPU1] [ 6] [ ALERT] [ap] sched_dumpstack: [ 6] [<0x4000e3c>] group_setuptaskfiles+0x57/0xc0 [ 0.534300] [CPU1] [ 6] [ ALERT] [ap] sched_dumpstack: [ 6] [<0x400bc6c>] nxtask_init+0x5b/0x13c Signed-off-by: zhangshoukui <zhangshoukui@xiaomi.com>

Zhangshoukui · 2024-09-13T04:21:20Z

@Zhangshoukui please fix the conflict.

done

Zhangshoukui · 2024-09-13T12:36:13Z

@anchao help me review this PR. thanks

anchao · 2024-09-13T16:45:03Z

@anchao help me review this PR. thanks

please set CONFIG_FS_REFCOUNT default value to n to keep the original configuration， we should not add unnecessary code size loads to IoT developers, especially for some chips that may only have 32k available RAM

Zhangshoukui · 2024-09-14T01:52:19Z

@anchao I do not think this code is unnecessary, for the operation of a system security is the first.At present, CI has passed, I think it can be approve

xiaoxiang781216 · 2024-09-14T02:20:11Z

@anchao help me review this PR. thanks

please set CONFIG_FS_REFCOUNT default value to n to keep the original configuration， we should not add unnecessary code size loads to IoT developers, especially for some chips that may only have 32k available RAM

The default value should keep as safe as possible, since not all users understand the fs internal implementation. @Zhangshoukui let's change default value to !DEFAULT_SMALL.

fs/inode/fs_files.c

Signed-off-by: zhangshoukui <zhangshoukui@xiaomi.com>

arm-none-eabi-ld: /github/workspace/sources/nuttx/nuttx section `.bss' will not fit in region `ksram' arm-none-eabi-ld: region `ksram' overflowed by 16 bytes Signed-off-by: zhangshoukui <zhangshoukui@xiaomi.com>

Zhangshoukui · 2024-09-14T10:21:10Z

@xiaoxiang781216 help me review this PR.thanks

yamt · 2024-10-21T08:38:32Z

This prevents the file from being closed while it is being read or written

what happens w/o this enabled? a crash?

yamt · 2024-10-21T08:40:11Z

fs/Kconfig

+	default !DEFAULT_SMALL
+	---help---
+		Enable will Records the number of filep references. The file is
+		actually closed when the count reaches 0


my impression is that this kind of things should not be a user-visible option.

In theory, it should be used by default and should not be configured by the user, but it actually increases the code size, so it is configurable.Any better suggestions? I can try to optimize

Zhangshoukui · 2024-10-21T08:50:49Z

This prevents the file from being closed while it is being read or written

what happens w/o this enabled? a crash?

Prevent multithreaded operations on the same filep when one is in r/w and the other is closed

yamt · 2024-10-21T08:52:05Z

This prevents the file from being closed while it is being read or written

what happens w/o this enabled? a crash?

Prevent multithreaded operations on the same filep when one is in r/w and the other is closed

my question is, why do you need to prevent it?

Zhangshoukui · 2024-10-21T09:18:15Z

This prevents the file from being closed while it is being read or written

what happens w/o this enabled? a crash?

Prevent multithreaded operations on the same filep when one is in r/w and the other is closed

my question is, why do you need to prevent it?

This commit can solve the above problem and avoid the crash, in turn, why let such an operation crash? Reference counting makes the read and write process more robust

yamt · 2024-10-21T09:24:13Z

This prevents the file from being closed while it is being read or written

what happens w/o this enabled? a crash?

Prevent multithreaded operations on the same filep when one is in r/w and the other is closed

my question is, why do you need to prevent it?

This commit can solve the above problem and avoid the crash, in turn, why let such an operation crash? Reference counting makes the read and write process more robust

if this is to fix a crash, i don't understand why it should be optional.

xiaoxiang781216 · 2024-10-21T09:59:49Z

The patch tries to fix this common issue: thread 1 call read(fd, ...) and block, thread2 call close(fd).
without this patch, read will access the memory which is freed by close.

xiaoxiang781216 · 2024-10-21T10:02:33Z

This prevents the file from being closed while it is being read or written

what happens w/o this enabled? a crash?

Prevent multithreaded operations on the same filep when one is in r/w and the other is closed

my question is, why do you need to prevent it?

This commit can solve the above problem and avoid the crash, in turn, why let such an operation crash? Reference counting makes the read and write process more robust

if this is to fix a crash, i don't understand why it should be optional.

Yes, I also think it should be always enabled, but @anchao think this is a corner case and should give the user the freedom to disable it.

anchao · 2024-10-21T12:21:59Z

This commit can solve the above problem and avoid the crash, in turn, why let such an operation crash? Reference counting makes the read and write process more robust

if this is to fix a crash, i don't understand why it should be optional.

Yes, I also think it should be always enabled, but @anchao think this is a corner case and should give the user the freedom to disable it.

But this feature consumes a lot of rom size. e.g on ARM32 platform, the flash usage will increase by ~500 bytes caused by bl/blx instructions are inserted in many places in the fs.

anchao · 2024-10-21T12:24:08Z

We should ensure that NuttX remains compact and lightweight, rather than expanding endlessly like Linux. @xiaoxiang781216 @yamt

yamt · 2024-10-21T12:28:07Z

This commit can solve the above problem and avoid the crash, in turn, why let such an operation crash? Reference counting makes the read and write process more robust

if this is to fix a crash, i don't understand why it should be optional.

Yes, I also think it should be always enabled, but @anchao think this is a corner case and should give the user the freedom to disable it.

But this feature consumes a lot of rom size. e.g on ARM32 platform, the flash usage will increase by ~500 bytes caused by bl/blx instructions are inserted in many places in the fs.

do you have a suggestion of an alternative fix for the crash?
to me, saving 500 bytes doesn't sound like a good reason to leave a crashing bug.

yamt · 2024-10-21T12:36:14Z

We should ensure that NuttX remains compact and lightweight, rather than expanding endlessly like Linux. @xiaoxiang781216 @yamt

i agree.
i hope it doesn't mean to leave crashing bugs though.
also, i suspect moderate continuous size increase is natural for a software like this.

anchao · 2024-10-21T12:47:57Z

Most RTOS developers typically do not do the following things:

Open and close file descriptors after system bringup frequently.
Open and close file descriptors via different task contexts.

FS_REFCOUNT enhances the detection of such issues.

do you have a suggestion of an alternative fix for the crash? to me, saving 500 bytes doesn't sound like a good reason to leave a crashing bug.

if disable FS_REFCOUNT, I think we could add some error catch and print error log on failure case, which will help developers identify issues more quickly.

yamt · 2024-10-23T05:29:39Z

Most RTOS developers typically do not do the following things:
1. Open and close file descriptors after system bringup frequently.

2. Open and close file descriptors via different task contexts.
FS_REFCOUNT enhances the detection of such issues.

i guess what you need is a config option or whatever which allows users to
limit themselves to a small restricted subset of posix semantics, which doesn't need
these reference counting.

as we don't have such a mechanism right now, i suppose we need to enable this reference counting
(or whatever fix for the crash) unconditionally.

do you have a suggestion of an alternative fix for the crash? to me, saving 500 bytes doesn't sound like a good reason to leave a crashing bug.

if disable FS_REFCOUNT, I think we could add some error catch and print error log on failure case, which will help developers identify issues more quickly.

do you have an idea how to detect the failure cases?

anchao requested changes Sep 5, 2024

View reviewed changes

fs/Kconfig Outdated Show resolved Hide resolved

fs/inode/fs_files.c Outdated Show resolved Hide resolved

Zhangshoukui force-pushed the master branch 3 times, most recently from 5873bd9 to a356d27 Compare September 12, 2024 06:40

GUIDINGLI approved these changes Sep 12, 2024

View reviewed changes

Zhangshoukui force-pushed the master branch from 329589d to a356d27 Compare September 12, 2024 13:49

Zhangshoukui and others added 3 commits September 13, 2024 09:29

vfs/file: add reference counting to prevent accidental close during r…

e4b483e

…eading writing... Signed-off-by: Shoukui Zhang <zhangshoukui@xiaomi.com>

Fix vfs/fs_fstatfs.c:60:21: warning: variable 'inode' set but not used

609a1e1

60 | FAR struct inode *inode; | ^~~~~ Signed-off-by: Xiang Xiao <xiaoxiang@xiaomi.com>

Zhangshoukui force-pushed the master branch from a356d27 to 3691ab7 Compare September 13, 2024 02:08

xiaoxiang781216 requested a review from anchao September 13, 2024 05:24

Zhangshoukui force-pushed the master branch from 3691ab7 to da01077 Compare September 14, 2024 03:18

anchao reviewed Sep 14, 2024

View reviewed changes

fs/inode/fs_files.c Show resolved Hide resolved

Zhangshoukui added 2 commits September 14, 2024 15:19

Add reference count configuration

b78bfd0

Signed-off-by: zhangshoukui <zhangshoukui@xiaomi.com>

stm3240g-eval:knxwm and olimex-stm32-p407:kmodule remove FS_REFCOUNT

d4c6513

arm-none-eabi-ld: /github/workspace/sources/nuttx/nuttx section `.bss' will not fit in region `ksram' arm-none-eabi-ld: region `ksram' overflowed by 16 bytes Signed-off-by: zhangshoukui <zhangshoukui@xiaomi.com>

Zhangshoukui force-pushed the master branch from da01077 to d4c6513 Compare September 14, 2024 07:22

anchao approved these changes Sep 14, 2024

View reviewed changes

xiaoxiang781216 approved these changes Sep 17, 2024

View reviewed changes

xiaoxiang781216 merged commit 756faf3 into apache:master Sep 17, 2024

anchao mentioned this pull request Sep 18, 2024

fs/reference_count: set default reference count option depends on DISABLE_PTHREAD #13519

Merged

anchao mentioned this pull request Oct 21, 2024

Revert "fs/inode: add pre-allocated task files to avoid allocator access #14416

Closed

yamt reviewed Oct 21, 2024

View reviewed changes

filep Reference count #13296

filep Reference count #13296

Uh oh!

Conversation

Zhangshoukui commented Sep 5, 2024

Summary

Impact

Testing

Uh oh!

Zhangshoukui commented Sep 5, 2024

Uh oh!

xiaoxiang781216 commented Sep 5, 2024

Uh oh!

anchao left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Zhangshoukui commented Sep 6, 2024

Uh oh!

anchao commented Sep 6, 2024

Uh oh!

xiaoxiang781216 commented Sep 12, 2024

Uh oh!

Zhangshoukui commented Sep 13, 2024

Uh oh!

Zhangshoukui commented Sep 13, 2024

Uh oh!

anchao commented Sep 13, 2024

Uh oh!

Zhangshoukui commented Sep 14, 2024

Uh oh!

xiaoxiang781216 commented Sep 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Zhangshoukui commented Sep 14, 2024

Uh oh!

yamt commented Oct 21, 2024

Uh oh!

yamt Oct 21, 2024

Choose a reason for hiding this comment

Uh oh!

Zhangshoukui Oct 21, 2024

Choose a reason for hiding this comment

Uh oh!

Zhangshoukui commented Oct 21, 2024

Uh oh!

yamt commented Oct 21, 2024

Uh oh!

Zhangshoukui commented Oct 21, 2024

Uh oh!

yamt commented Oct 21, 2024

Uh oh!

xiaoxiang781216 commented Oct 21, 2024

Uh oh!

xiaoxiang781216 commented Oct 21, 2024

Uh oh!

anchao commented Oct 21, 2024

Uh oh!

anchao commented Oct 21, 2024

Uh oh!

yamt commented Oct 21, 2024

Uh oh!

yamt commented Oct 21, 2024

Uh oh!

anchao commented Oct 21, 2024

Uh oh!

yamt commented Oct 23, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

xiaoxiang781216 commented Sep 14, 2024 •

edited

Loading