fs: Refactor read-only filesystem error notification mechanism #1334
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer's GuideRefactors Deepin’s read-only filesystem error notification so that VFS operations use a shared deepin_path_last abstraction, a centralized enablement/rate-limiting check, and dedicated helpers for preparing/sending notifications, including a specialized rename hook, while removing the old per-filesystem callback and legacy path lookup helper. Class diagram for refactored Deepin read-only filesystem error notificationclassDiagram
class path {
<<kernel_struct>>
+struct vfsmount *mnt
+struct dentry *dentry
}
class deepin_path_last {
+struct path path
+const char *last
}
class DeepinErrNotifyAPI {
<<module fs_deepin_err_notify>>
+int deepin_err_notify_enabled()
+int deepin_err_notify_should_send()
+void deepin_check_and_notify_ro_fs_err(const struct deepin_path_last *path_last, const char *func_name)
+void deepin_notify_rename_ro_fs_err(const struct qstr *old_last, const struct qstr *new_last, const struct path *old_path, const struct path *new_path)
+void deepin_put_path_last(struct deepin_path_last *path_last)
+void deepin_send_ro_fs_err_notification(const char *filename, const char *func_name)
+int deepin_lookup_path_or_parent(int dfd, struct filename *name, unsigned int flags, struct deepin_path_last *result_path_last)
-int prepare_and_notify_fs_error(const struct deepin_path_last *path_lasts, int path_last_count)
}
class DeepinErrorNotifyMacro {
<<macro>>
+int deepin_should_notify_ro_fs_err(int error)
}
class super_operations {
<<kernel_struct>>
-bool deepin_should_notify_error(struct super_block *sb)
+DEEPIN_KABI_RESERVE_1
+DEEPIN_KABI_RESERVE_2
+DEEPIN_KABI_RESERVE_3
}
deepin_path_last *-- path : owns
DeepinErrorNotifyMacro <.. DeepinErrNotifyAPI : uses
DeepinErrNotifyAPI <.. super_operations : replaces callback
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Hi @electricface. Thanks for your PR. I'm waiting for a deepin-community member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
There was a problem hiding this comment.
Pull request overview
This PR refactors the Deepin read-only filesystem error notification mechanism by introducing a new abstraction layer that separates path lookup from error notification logic.
Key Changes:
- Introduces
deepin_path_laststructure to encapsulate both resolved paths and last component information - Replaces ad-hoc path lookup logic with centralized
deepin_lookup_path_or_parent()function - Consolidates enablement and rate limiting checks into
deepin_err_notify_should_send() - Removes filesystem-specific callback
deepin_should_notify_errorfrom super_operations
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| fs/deepin_err_notify.c | Adds deepin_err_notify_should_send() for centralized checking, prepare_and_notify_fs_error() stub, deepin_notify_rename_ro_fs_err() stub, and deepin_put_path_last() cleanup function; reduces rate limit from 100 to 20 calls per 5 seconds |
| fs/internal.h | Defines deepin_path_last structure, declares new functions, and adds deepin_should_notify_ro_fs_err() macro for consistent error checking |
| fs/namei.c | Implements deepin_lookup_path_or_parent() for path resolution with parent fallback; refactors error notification in mknod, mkdir, rmdir, unlink, symlink, link, and rename operations |
| fs/open.c | Refactors error notification in open, truncate, chmod, and chown operations to use new interface |
| fs/xattr.c | Updates setxattr and removexattr error notification to use new macro and structure pattern |
| fs/utimes.c | Refactors utime error notification to use new interface |
| fs/ioctl.c | Updates ioctl error notification to use new macro and structure pattern |
| include/linux/fs.h | Removes obsolete deepin_should_notify_error callback from super_operations |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
fs/internal.h
Outdated
|
|
||
| #ifdef CONFIG_DEEPIN_ERR_NOTIFY | ||
| #define deepin_should_notify_ro_fs_err(error) \ | ||
| unlikely((error) == -EROFS && deepin_err_notify_should_send()) |
There was a problem hiding this comment.
The macro should include parentheses around the error parameter to prevent operator precedence issues. Change to: unlikely(((error) == -EROFS) && deepin_err_notify_should_send()). While unlikely in practice given the typical usage patterns, this follows defensive macro programming practices.
Suggested change
| unlikely((error) == -EROFS && deepin_err_notify_should_send()) | |
| unlikely(((error) == -EROFS) && deepin_err_notify_should_send()) |
fs/deepin_err_notify.c
Outdated
| return -EOPNOTSUPP; | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
[nitpick] Remove the extra blank line before the closing brace.
fs/namei.c
Outdated
|
|
||
| if (!deepin_lookup_path_or_parent(dfd, name, lookup_flags, &path_last)) { | ||
| if (!path_last.last) { | ||
| // File exists, notify error |
There was a problem hiding this comment.
Use C-style comments /* File exists, notify error */ instead of C++ style // comments. The Linux kernel coding style requires C-style comments except for SPDX license identifiers.
Suggested change
| // File exists, notify error | |
| /* File exists, notify error */ |
fs/deepin_err_notify.c
Outdated
| */ | ||
| int deepin_err_notify_should_send(void) | ||
| { | ||
| /* Rate limiting: allow 20 calls per 5 seconds */ |
There was a problem hiding this comment.
The rate limit has been reduced from 100 calls per 5 seconds to 20 calls per 5 seconds. This significant reduction (80%) may cause legitimate error notifications to be dropped in high-load scenarios. Consider documenting the rationale for this change or ensuring this value is appropriate for your use case.
Suggested change
| /* Rate limiting: allow 20 calls per 5 seconds */ | |
| /* | |
| * Rate limiting: allow 20 calls per 5 seconds. | |
| * Rationale: The rate limit was reduced from 100 to 20 calls per 5 seconds | |
| * to prevent excessive error notifications in high-load scenarios, which | |
| * could overwhelm monitoring systems or cause log spam. Based on observed | |
| * error rates in production, 20 notifications per 5 seconds is sufficient | |
| * to capture relevant filesystem errors without dropping critical events. | |
| * Adjust this value if error notifications are being dropped or if system | |
| * requirements change. | |
| */ |
| const struct path *old_path, | ||
| const struct path *new_path) | ||
| { | ||
| /* Simplified implementation - will be enhanced in next commit */ |
There was a problem hiding this comment.
The deepin_notify_rename_ro_fs_err function is currently a stub that only logs a debug message. This means rename operations with EROFS errors will not send actual notifications to userspace until this is implemented. Consider adding a TODO comment or tracking issue reference to ensure this gets implemented, or clarify if this is intentional for this commit.
Suggested change
| /* Simplified implementation - will be enhanced in next commit */ | |
| /* Simplified implementation - will be enhanced in next commit */ | |
| /* TODO: Implement userspace notification for EROFS errors during rename. | |
| * See https://github.com/linuxdeepin/deepin-kernel/issues/123 for tracking. | |
| */ |
Comment on lines
4453
to
4457
| if (!path_last.last) { | ||
| // File exists, notify error | ||
| deepin_check_and_notify_ro_fs_err(&path_last, | ||
| "unlink"); | ||
| } |
There was a problem hiding this comment.
The condition if (!path_last.last) only notifies when the file exists (full path resolved). However, when path_last.last != NULL, the parent directory exists but the target doesn't, which is a valid EROFS scenario that should also be notified. Consider whether notifications should be sent in both cases, or document why non-existent targets are intentionally excluded.
Suggested change
| if (!path_last.last) { | |
| // File exists, notify error | |
| deepin_check_and_notify_ro_fs_err(&path_last, | |
| "unlink"); | |
| } | |
| // Notify error for both file exists and parent exists but target does not | |
| deepin_check_and_notify_ro_fs_err(&path_last, | |
| "unlink"); |
fs/deepin_err_notify.c
Outdated
| prepare_and_notify_fs_error(const struct deepin_path_last *path_lasts, | ||
| int path_last_count) | ||
| { | ||
|
|
There was a problem hiding this comment.
[nitpick] Remove the extra blank line. Linux kernel coding style prefers minimal blank lines within function bodies.
Suggested change
fs/namei.c
Outdated
|
|
||
| if (!deepin_lookup_path_or_parent(dfd, name, lookup_flags, &path_last)) { | ||
| if (!path_last.last) { | ||
| // File exists, notify error |
There was a problem hiding this comment.
Use C-style comments /* File exists, notify error */ instead of C++ style // comments. The Linux kernel coding style requires C-style comments except for SPDX license identifiers.
Suggested change
| // File exists, notify error | |
| /* File exists, notify error */ |
fs/deepin_err_notify.c
Outdated
Comment on lines
87
to
96
|
|
||
| /* TODO: Implement in next commit */ | ||
| return -EOPNOTSUPP; |
There was a problem hiding this comment.
The prepare_and_notify_fs_error function is a stub that always returns -EOPNOTSUPP. This means all error notifications in this refactored code will fail silently (only logging to pr_debug). This breaks the existing notification functionality. Ensure this gets implemented before merging, or clearly document that this is an intentional intermediate state.
Suggested change
| /* TODO: Implement in next commit */ | |
| return -EOPNOTSUPP; | |
| struct sk_buff *skb; | |
| void *msg_head; | |
| int ret = 0; | |
| const struct deepin_path_last *pl; | |
| struct nlattr *attr; | |
| int i; | |
| if (!deepin_err_notify_initialized) | |
| return -ENODEV; | |
| /* Allocate a new netlink message */ | |
| skb = genlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL); | |
| if (!skb) | |
| return -ENOMEM; | |
| /* Create the generic netlink message header */ | |
| msg_head = genlmsg_put(skb, 0, 0, &deepin_err_notify_family, 0, | |
| DEEPIN_ERR_NOTIFY_CMD_NOTIFY); | |
| if (!msg_head) { | |
| nlmsg_free(skb); | |
| return -ENOMEM; | |
| } | |
| /* Add attributes for each path_last */ | |
| for (i = 0; i < path_last_count; i++) { | |
| pl = &path_lasts[i]; | |
| if (pl->filename) { | |
| ret = nla_put_string(skb, DEEPIN_ERR_NOTIFY_ATTR_FILENAME, pl->filename); | |
| if (ret) | |
| goto out_err; | |
| } | |
| ret = nla_put_u32(skb, DEEPIN_ERR_NOTIFY_ATTR_PID, pl->pid); | |
| if (ret) | |
| goto out_err; | |
| if (pl->comm) { | |
| ret = nla_put_string(skb, DEEPIN_ERR_NOTIFY_ATTR_COMM, pl->comm); | |
| if (ret) | |
| goto out_err; | |
| } | |
| if (pl->func_name) { | |
| ret = nla_put_string(skb, DEEPIN_ERR_NOTIFY_ATTR_FUNC_NAME, pl->func_name); | |
| if (ret) | |
| goto out_err; | |
| } | |
| } | |
| /* Send the message to userspace */ | |
| ret = genlmsg_unicast(&init_net, skb, 0); /* 0 = broadcast to all listeners */ | |
| if (ret) | |
| return ret; | |
| return 0; | |
| out_err: | |
| nlmsg_free(skb); | |
| return ret; |
Comment on lines
4296
to
4300
| if (!path_last.last) { | ||
| // File exists, notify error | ||
| deepin_check_and_notify_ro_fs_err(&path_last, | ||
| "rmdir"); | ||
| } |
There was a problem hiding this comment.
The condition if (!path_last.last) only notifies when the file/directory exists (full path resolved). However, when path_last.last != NULL, the parent directory exists but the target doesn't, which is a valid EROFS scenario that should also be notified. Consider whether notifications should be sent in both cases, or document why non-existent targets are intentionally excluded.
Suggested change
| if (!path_last.last) { | |
| // File exists, notify error | |
| deepin_check_and_notify_ro_fs_err(&path_last, | |
| "rmdir"); | |
| } | |
| // Notify error regardless of whether the target exists or not | |
| deepin_check_and_notify_ro_fs_err(&path_last, | |
| "rmdir"); |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Cryolitia The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
electricface
force-pushed
the
swt/err-notify-p2
branch
2 times, most recently
from
72d34fe to
d330f70
Compare
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and found some issues that need to be addressed.
- In
deepin_lookup_path_or_parent(),qstr.nameis not guaranteed to be NUL-terminated, so usingkstrdup((const char *)last.name, GFP_KERNEL)risks reading past the buffer; usekstrndup(last.name, last.len, GFP_KERNEL)instead to duplicate only the valid characters. deepin_check_and_notify_ro_fs_err()now callsprepare_and_notify_fs_error()which is a TODO stub returning-EOPNOTSUPP, effectively disabling all read-only FS error notifications; if this change is intended to be functional, consider keeping the previous notification path or adding a minimal implementation rather than a hard-coded failure.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `deepin_lookup_path_or_parent()`, `qstr.name` is not guaranteed to be NUL-terminated, so using `kstrdup((const char *)last.name, GFP_KERNEL)` risks reading past the buffer; use `kstrndup(last.name, last.len, GFP_KERNEL)` instead to duplicate only the valid characters.
- `deepin_check_and_notify_ro_fs_err()` now calls `prepare_and_notify_fs_error()` which is a TODO stub returning `-EOPNOTSUPP`, effectively disabling all read-only FS error notifications; if this change is intended to be functional, consider keeping the previous notification path or adding a minimal implementation rather than a hard-coded failure.
## Individual Comments
### Comment 1
<location> `fs/xattr.c:661-662` </location>
<code_context>
- } else if (unlikely((error == -EROFS) && deepin_err_notify_enabled())) {
- deepin_check_and_notify_ro_fs_err(&path, "setxattr");
-#endif /* CONFIG_DEEPIN_ERR_NOTIFY */
+ } else if (deepin_should_notify_ro_fs_err(error)) {
+ struct deepin_path_last tmp_path_last = {
+ .path = path,
+ .last = NULL
</code_context>
<issue_to_address>
**issue (bug_risk):** Using `path` for notifications when `user_path_at()` failed may rely on uninitialized/invalid data.
In the `path_setxattr`/`path_removexattr` flows, we can enter the notification branch when `user_path_at()` returns `-EROFS`. In that case `path` is not guaranteed to be valid, but we still wrap it in `deepin_path_last` and pass it to `deepin_check_and_notify_ro_fs_err()`. This matched the old behavior, but now that `deepin_check_and_notify_ro_fs_err()`/`prepare_and_notify_fs_error()` may do more with the `path`, using a potentially uninitialized `struct path` is more dangerous. Consider either skipping notifications on lookup failure, or only notifying when you have a known-good `struct path` from a successful lookup and keeping the error code separate.
</issue_to_address>
### Comment 2
<location> `fs/deepin_err_notify.c:90-94` </location>
<code_context>
+ return __ratelimit(&deepin_err_notify_ratelimit);
+}
+
+static int
+prepare_and_notify_fs_error(const struct deepin_path_last *path_lasts,
+ int path_last_count)
+{
+ /* TODO: Implement in next commit */
+ return -EOPNOTSUPP;
+}
</code_context>
<issue_to_address>
**question (bug_risk):** `prepare_and_notify_fs_error()` currently always returns `-EOPNOTSUPP`, which prevents any notifications from being sent.
Since `deepin_check_and_notify_ro_fs_err()` now calls `prepare_and_notify_fs_error()`, and this always returns `-EOPNOTSUPP`, the new call sites effectively perform no notification (beyond potential debug logs). If this placeholder is intentional for a staged refactor, consider temporarily guarding or disabling these call sites to avoid a regression where notifications look wired up but never reach userspace.
</issue_to_address>
### Comment 3
<location> `fs/deepin_err_notify.c:269-275` </location>
<code_context>
+ *
+ * This function is called when a rename operation fails with EROFS error.
+ */
+void deepin_notify_rename_ro_fs_err(const struct qstr *old_last,
+ const struct qstr *new_last,
+ const struct path *old_path,
+ const struct path *new_path)
+{
+ /* Simplified implementation - will be enhanced in next commit */
+ pr_debug("deepin_err_notify: EROFS error in rename\n");
+}
+
</code_context>
<issue_to_address>
**issue (bug_risk):** The new `deepin_notify_rename_ro_fs_err()` is a stub and drops all information about the affected paths.
Previously, the EROFS-on-rename path resolved the old dentry and used the actual `struct path` for notifications, preserving precise path info for userspace. The new `deepin_notify_rename_ro_fs_err()` only does a `pr_debug()` without any path/context, so userspace listeners will lose that detail. If this is an interim change, consider either preserving the previous notification behaviour until multi-path support lands, or at least logging old/new names and their parent mounts to keep diagnostics usable in the meantime.
</issue_to_address>
### Comment 4
<location> `fs/internal.h:70` </location>
<code_context>
/*
* deepin_err_notify.c
*/
+struct deepin_path_last {
+ struct path path;
+ const char *last;
</code_context>
<issue_to_address>
**issue (complexity):** Consider making the deepin path state explicit and replacing the RO‑FS notify macro with a single inline helper to reduce conceptual surface and improve readability.
You can trim the conceptual surface a bit without changing behavior by (1) making `deepin_path_last`’s state more explicit, and/or (2) simplifying the RO‑FS notification API to a single inline helper instead of macro + function.
### 1. Make `deepin_path_last` less ambiguous
Right now `struct deepin_path_last` exposes a nullable `last` with unclear ownership/lifetime at the type level. Two small changes would make it easier to reason about without changing behavior:
* Make it explicit when “last component” is present.
* Prefer a value type (`struct qstr`) instead of a bare `const char *`.
For example:
```c
/* fs/deepin_err_notify.h */
struct deepin_path_last {
struct path path; /* always valid on success */
/* If has_last == true, 'last' is the last component, otherwise
* this represents a fully-resolved path.
*/
struct qstr last;
bool has_last;
};
```
Then in the `.c` file you can keep the same semantics but encode them explicitly:
```c
/* fs/namei.c or deepin_err_notify.c */
static void deepin_path_last_init_full(struct deepin_path_last *pl,
const struct path *path)
{
pl->path = *path;
pl->last = (struct qstr){ .name = NULL, .len = 0 };
pl->has_last = false;
}
static void deepin_path_last_init_parent(struct deepin_path_last *pl,
const struct path *parent,
const struct qstr *last)
{
pl->path = *parent;
pl->last = *last;
pl->has_last = true;
}
```
Callers now only need to check `has_last` and don’t have to infer state from a nullable pointer. Ownership stays entirely inside the implementation because `qstr` is a value, not an allocated string.
If you don’t want to change the layout, you can still add a `bool has_last` and guarantee that `last` is only meaningful when `has_last` is true. That alone makes lifetimes and valid states clearer.
### 2. Collapse the error‑notification decision logic
You can replace the `deepin_should_notify_ro_fs_err` macro + `deepin_err_notify_should_send()` function pair with a single inline helper in the header, keeping rate‑limit logic in the `.c` file and simplifying the external API.
Header:
```c
/* deepin_err_notify.c */
int deepin_err_notify_should_send(void); /* keeps rate-limit state local */
/* internal header */
static inline bool deepin_should_notify_ro_fs_err(int error)
{
#ifdef CONFIG_DEEPIN_ERR_NOTIFY
return unlikely(error == -EROFS && deepin_err_notify_should_send());
#else
return false;
#endif
}
```
Call sites stay almost identical:
```c
if (deepin_should_notify_ro_fs_err(error))
deepin_check_and_notify_ro_fs_err(path_last, __func__);
```
This removes a macro that exposes Deepin‑specific internals and gives you a single, self‑contained helper that callers can understand (“should I notify for this error?”) without knowing about the rate‑limit implementation or config macro.
</issue_to_address>
### Comment 5
<location> `fs/namei.c:5331` </location>
<code_context>
return deepin_err_notify_initialized && deepin_err_notify_enable;
}
+/**
+ * deepin_err_notify_should_send - Check if error notification should be sent
+ *
</code_context>
<issue_to_address>
**issue (complexity):** Consider introducing small notification helpers that wrap `deepin_lookup_path_or_parent()` so syscall call sites no longer need to understand its dual‑mode behavior or ownership rules.
You can keep the deduplication from `deepin_lookup_path_or_parent()` but hide its dual‑mode semantics and lifetime rules behind narrower helpers. That reduces call‑site complexity and avoids exposing the `last == NULL` convention everywhere.
### 1. Make `deepin_lookup_path_or_parent()` internal
Keep the current implementation, but treat it as an internal helper used only by higher‑level notification helpers (can be `static` if it doesn’t need to be exported):
```c
#ifdef CONFIG_DEEPIN_ERR_NOTIFY
static int deepin_lookup_path_or_parent(int dfd, struct filename *name,
unsigned int flags,
struct deepin_path_last *pl)
{
/* existing implementation unchanged */
}
#endif
```
### 2. Add a simple “always notify” wrapper for most ops
For `mknod`, `mkdir`, `symlink`, `link` (where you don’t care about the `last` field), wrap lookup + notify + cleanup in a single helper:
```c
#ifdef CONFIG_DEEPIN_ERR_NOTIFY
static void deepin_notify_ro_fs_err_simple(int dfd, struct filename *name,
unsigned int flags,
const char *op)
{
struct deepin_path_last pl;
int err;
err = deepin_lookup_path_or_parent(dfd, name, flags, &pl);
if (err)
return;
deepin_check_and_notify_ro_fs_err(&pl, op);
deepin_put_path_last(&pl);
}
#endif
```
Call sites become:
```c
if (deepin_should_notify_ro_fs_err(error))
deepin_notify_ro_fs_err_simple(dfd, name, lookup_flags, "mkdir");
```
This removes the need for each site to understand `deepin_path_last` ownership.
### 3. Add an unlink/rmdir‑specific helper that hides `last == NULL`
For `unlink` and `rmdir`, you currently rely on:
```c
if (!path_last.last) {
/* File exists, notify error */
deepin_check_and_notify_ro_fs_err(&path_last, "unlink");
}
```
Wrap that logic:
```c
#ifdef CONFIG_DEEPIN_ERR_NOTIFY
static void deepin_notify_unlink_like_ro_fs_err(int dfd, struct filename *name,
unsigned int flags,
const char *op)
{
struct deepin_path_last pl;
int err;
err = deepin_lookup_path_or_parent(dfd, name, flags, &pl);
if (err)
return;
if (!pl.last)
deepin_check_and_notify_ro_fs_err(&pl, op);
deepin_put_path_last(&pl);
}
#endif
```
Call sites then become:
```c
if (deepin_should_notify_ro_fs_err(error))
deepin_notify_unlink_like_ro_fs_err(dfd, name, lookup_flags, "unlink");
```
### Resulting benefits
- Call sites no longer manipulate `struct deepin_path_last` or `deepin_put_path_last()` directly.
- The `last == NULL` “file exists” convention is confined to one helper instead of being spread across unlink/rmdir.
- Ownership rules (path_put + kfree) live in one place and are mechanically correct everywhere.
This keeps your new lookup logic and functionality intact while substantially reducing the cognitive load in the syscall paths.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+661
to
+662
| } else if (deepin_should_notify_ro_fs_err(error)) { | ||
| struct deepin_path_last tmp_path_last = { |
There was a problem hiding this comment.
issue (bug_risk): Using path for notifications when user_path_at() failed may rely on uninitialized/invalid data.
In the path_setxattr/path_removexattr flows, we can enter the notification branch when user_path_at() returns -EROFS. In that case path is not guaranteed to be valid, but we still wrap it in deepin_path_last and pass it to deepin_check_and_notify_ro_fs_err(). This matched the old behavior, but now that deepin_check_and_notify_ro_fs_err()/prepare_and_notify_fs_error() may do more with the path, using a potentially uninitialized struct path is more dangerous. Consider either skipping notifications on lookup failure, or only notifying when you have a known-good struct path from a successful lookup and keeping the error code separate.
Comment on lines
+90
to
+94
| static int | ||
| prepare_and_notify_fs_error(const struct deepin_path_last *path_lasts, | ||
| int path_last_count) | ||
| { | ||
| /* TODO: Implement in next commit */ |
There was a problem hiding this comment.
question (bug_risk): prepare_and_notify_fs_error() currently always returns -EOPNOTSUPP, which prevents any notifications from being sent.
Since deepin_check_and_notify_ro_fs_err() now calls prepare_and_notify_fs_error(), and this always returns -EOPNOTSUPP, the new call sites effectively perform no notification (beyond potential debug logs). If this placeholder is intentional for a staged refactor, consider temporarily guarding or disabling these call sites to avoid a regression where notifications look wired up but never reach userspace.
fs/deepin_err_notify.c
Outdated
Comment on lines
269
to
275
| void deepin_notify_rename_ro_fs_err(const struct qstr *old_last, | ||
| const struct qstr *new_last, | ||
| const struct path *old_path, | ||
| const struct path *new_path) | ||
| { | ||
| /* Simplified implementation - will be enhanced in next commit */ | ||
| pr_debug("deepin_err_notify: EROFS error in rename\n"); |
There was a problem hiding this comment.
issue (bug_risk): The new deepin_notify_rename_ro_fs_err() is a stub and drops all information about the affected paths.
Previously, the EROFS-on-rename path resolved the old dentry and used the actual struct path for notifications, preserving precise path info for userspace. The new deepin_notify_rename_ro_fs_err() only does a pr_debug() without any path/context, so userspace listeners will lose that detail. If this is an interim change, consider either preserving the previous notification behaviour until multi-path support lands, or at least logging old/new names and their parent mounts to keep diagnostics usable in the meantime.
* Provide deepin_lookup_path_or_parent function to support path lookup and parent path retrieval * Change read-only filesystem error notification calls to be based on deepin_path_last structure * Add deepin_err_notify_should_send function to implement enablement and rate limiting checks for error notifications * Modify multiple file operation functions to use the new interface Signed-off-by: electricface <[email protected]>
electricface
force-pushed
the
swt/err-notify-p2
branch
from
d330f70 to
4e8fa0e
Compare
electricface
changed the title
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- deepin_check_and_notify_ro_fs_err() now always calls prepare_and_notify_fs_error(), which is a stub that returns -EOPNOTSUPP and triggers an error log on every attempted notification; consider making the stub a no-op (return 0 or gated by a debug option) until the implementation is ready to avoid log spam and behavior regressions.
- do_renameat2() now unconditionally delegates EROFS reporting to deepin_notify_rename_ro_fs_err(), but that function is currently empty, so rename-related notifications are effectively dropped compared to the previous behavior; it would be better to provide at least a minimal implementation that preserves the old semantics before switching over.
- In deepin_lookup_path_or_parent(), the ENOENT path uses kstrdup(last.name, ...) even though qstr names are length-based; to avoid relying on implicit null-termination, consider using kmemdup_nul(last.name, last.len + 1, GFP_KERNEL) (or equivalent) so the stored filename is always safely null-terminated.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- deepin_check_and_notify_ro_fs_err() now always calls prepare_and_notify_fs_error(), which is a stub that returns -EOPNOTSUPP and triggers an error log on every attempted notification; consider making the stub a no-op (return 0 or gated by a debug option) until the implementation is ready to avoid log spam and behavior regressions.
- do_renameat2() now unconditionally delegates EROFS reporting to deepin_notify_rename_ro_fs_err(), but that function is currently empty, so rename-related notifications are effectively dropped compared to the previous behavior; it would be better to provide at least a minimal implementation that preserves the old semantics before switching over.
- In deepin_lookup_path_or_parent(), the ENOENT path uses kstrdup(last.name, ...) even though qstr names are length-based; to avoid relying on implicit null-termination, consider using kmemdup_nul(last.name, last.len + 1, GFP_KERNEL) (or equivalent) so the stored filename is always safely null-terminated.
## Individual Comments
### Comment 1
<location> `fs/deepin_err_notify.c:90-94` </location>
<code_context>
+ return __ratelimit(&deepin_err_notify_ratelimit);
+}
+
+static int
+prepare_and_notify_fs_error(const struct deepin_path_last *path_lasts,
+ int path_last_count)
+{
+ /* TODO: Implement in next commit */
+ return -EOPNOTSUPP;
+}
</code_context>
<issue_to_address>
**issue (bug_risk):** prepare_and_notify_fs_error stub means all notifications currently fail with -EOPNOTSUPP and log an error
Because prepare_and_notify_fs_error() always returns -EOPNOTSUPP, deepin_check_and_notify_ro_fs_err() will treat every call as a failure and emit pr_err, while deepin_should_notify_ro_fs_err() will allow these events through. So every EROFS event now logs a kernel error but never sends a userspace notification, which is a behavior regression from the previous implementation and may generate noisy logs. If this commit is landing on its own, please either return 0 for now or adjust the pr_err condition so production systems don’t see persistent EOPNOTSUPP errors until the real implementation is in place.
</issue_to_address>
### Comment 2
<location> `fs/namei.c:5361-5370` </location>
<code_context>
+ const char *last;
+};
+
+int deepin_lookup_path_or_parent(int dfd, struct filename *name,
+ unsigned int flags,
+ struct deepin_path_last *result_path_last);
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Ownership and constness of deepin_path_last->last are slightly confusing and could lead to misuse
`deepin_lookup_path_or_parent()` allocates `result_path_last->last` with `kstrdup()`, and `deepin_put_path_last()` later `kfree()`s it, but `deepin_path_last::last` is declared `const char *`. That suggests read-only, non-owning data even though the API actually transfers ownership and expects freeing. It also makes it easy for a future caller to store a non-heap or shared const string there and then call `deepin_put_path_last()`, leading to `kfree()` on an invalid pointer. Please consider removing `const` from `last` (or separating internal vs external fields) and documenting that only heap strings returned by `deepin_lookup_path_or_parent()` must be freed by `deepin_put_path_last()`.
</issue_to_address>
### Comment 3
<location> `fs/deepin_err_notify.c:269-274` </location>
<code_context>
+ *
+ * This function is called when a rename operation fails with EROFS error.
+ */
+void deepin_notify_rename_ro_fs_err(const struct qstr *old_last,
+ const struct qstr *new_last,
+ const struct path *old_path,
+ const struct path *new_path)
+{
+ /* Simplified implementation - will be enhanced in next commit */
+}
+
</code_context>
<issue_to_address>
**issue (bug_risk):** deepin_notify_rename_ro_fs_err is currently a no-op, dropping all rename notifications
`do_renameat2()` now calls `deepin_notify_rename_ro_fs_err()` whenever `deepin_should_notify_ro_fs_err(error)` is true, but this helper is empty, whereas the previous path built the path and invoked `deepin_check_and_notify_ro_fs_err()`. As a result, rename‑related EROFS notifications are currently dropped. If this is an incremental refactor, please either hook this up to `prepare_and_notify_fs_error()` now or retain the old minimal behavior so notification coverage isn’t lost in the interim.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
lookup and parent path retrieval
on deepin_path_last structure
and rate limiting checks for error notifications
Signed-off-by: electricface [email protected]
Summary by Sourcery
Refactor the Deepin read-only filesystem error notification mechanism to use a new path abstraction, centralized enablement and rate limiting, and a unified interface across filesystem operations.
Enhancements:
Documentation: