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

sched/sem_rw: convert read-lock to write-lock when self already holds write-lock #13797

Merged
merged 3 commits into from
Oct 10, 2024

Conversation

Donny9
Copy link
Contributor

@Donny9 Donny9 commented Oct 2, 2024

Summary

  1. sched/sem_rw: convert read-lock to write-lock when self already holds write-lock
  2. fs/inode: using read-lock to protect inode_find

The PR is releated to #13736 (comment), to fix issue about:

In some scenarios, inode_find and inode_remove are used in combination, such as in shm_open and shm_unlink. The remove operation requires a write lock. If we were to switch to a read lock in inode_find, then a lock would need to be added in inode_remove, and additional checks would be required elsewhere.

Impact

convert read-lock to write-lock when self already holds write-lock

Testing

Local test

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

nuttxpr commented Oct 2, 2024

[Experimental Bot, please feedback here]

The PR summary partially meets the requirements, but is missing some key information:

What's Missing in Summary:

  • Why is the change necessary? The summary mentions a related discussion, but doesn't explicitly state the problem the PR solves. It should clearly articulate the bug or improvement this PR addresses.
  • How does the change exactly work? The summary provides a high-level description but lacks specifics. Explain the mechanism used to convert read-locks to write-locks and how using a read-lock in inode_find addresses the original issue.

Impact Section is Insufficient:

  • The current impact section is overly brief. It only mentions "convert read-lock to write-lock" which duplicates information from the summary.
  • Address ALL Impact Points: Go through each impact category listed in the requirements (user impact, build impact, hardware impact, etc.) and specify "NO" or provide details if applicable. For example, this change seems likely to impact compatibility, so explain the backward/forward compatibility implications.

Testing Section is Inadequate:

  • Insufficient Detail: "Local test" is too vague. Specify the host OS, CPU architecture, compiler, target architecture, board configuration, and any specific test cases used.
  • Missing Logs: The template asks for "Testing logs before change" and "Testing logs after change". Provide relevant snippets from your test runs demonstrating the problem before the PR and the successful outcome after.

In summary, while the PR addresses a specific issue, the provided description lacks the necessary detail and completeness expected in a NuttX PR. Expand the summary, thoroughly address all impact points, and provide specific details and logs for the testing section.

@github-actions github-actions bot added the Area: Board support Board support issues label Oct 3, 2024
sched/semaphore/sem_rw.c Outdated Show resolved Hide resolved
sched/semaphore/sem_rw.c Outdated Show resolved Hide resolved
@acassis
Copy link
Contributor

acassis commented Oct 3, 2024

@Donny9 please include the Summary content in the commit log message

… a write-lock

If the write lock is already held by oneself and sine the write
lock can be recursively held, so, this operation can be converted to a write
lock to avoid deadlock.

Signed-off-by: dongjiuzhu1 <[email protected]>
inode_find don't need to modify inode tree

Signed-off-by: dongjiuzhu1 <[email protected]>
@Donny9
Copy link
Contributor Author

Donny9 commented Oct 10, 2024

@Donny9 please include the Summary content in the commit log message

Done~

@GUIDINGLI GUIDINGLI merged commit ae19554 into apache:master Oct 10, 2024
37 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Board support Board support issues Area: File System File System issues Area: OS Components OS Components issues Board: arm 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.

5 participants