[Deepin-Kernel-SIG] [linux 6.6-y] [Upstream] shmem: fix recovery on rename failures #1383
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 GuideAdjusts shmem/simple directory offset handling so rename and rename-exchange operations pre-reserve maple_tree slots and avoid failing after whiteout or partially applied offset updates, ensuring consistent recovery when maple_tree insertions fail under low memory conditions. Sequence diagram for shmem_rename2 rename offset handlingsequenceDiagram
participant VFS
participant shmem_rename2
participant new_dir_ctx
participant old_dir_ctx
participant simple_offset_rename
participant shmem_whiteout
VFS->>shmem_rename2: shmem_rename2(idmap, old_dir, old_dentry, new_dir, new_dentry, flags)
activate shmem_rename2
alt invalid_flags_or_nonempty_target
shmem_rename2-->>VFS: error
deactivate shmem_rename2
else valid_and_empty
shmem_rename2->>new_dir_ctx: simple_offset_add(new_dir_ctx, new_dentry)
alt simple_offset_add_returns_EBUSY
shmem_rename2->>shmem_rename2: had_offset = true
else simple_offset_add_returns_error
shmem_rename2-->>VFS: error
activate shmem_rename2
deactivate shmem_rename2
end
alt flags_has_RENAME_WHITEOUT
shmem_rename2->>shmem_whiteout: shmem_whiteout(idmap, old_dir, old_dentry)
alt shmem_whiteout_error
shmem_rename2->>shmem_rename2: if !had_offset remove preinserted slot
shmem_rename2->>new_dir_ctx: simple_offset_remove(new_dir_ctx, new_dentry)
shmem_rename2-->>VFS: error
activate shmem_rename2
deactivate shmem_rename2
else shmem_whiteout_success
shmem_rename2->>simple_offset_rename: simple_offset_rename(old_dir, old_dentry, new_dir, new_dentry)
end
else no_RENAME_WHITEOUT
shmem_rename2->>simple_offset_rename: simple_offset_rename(old_dir, old_dentry, new_dir, new_dentry)
end
simple_offset_rename-->>shmem_rename2: void (offsets updated, no failures)
shmem_rename2->>shmem_rename2: if d_really_is_positive(new_dentry) then shmem_unlink, directory_updates
shmem_rename2-->>VFS: success
activate shmem_rename2
deactivate shmem_rename2
end
Updated class diagram for simple offset and shmem rename interactionsclassDiagram
class inode {
+i_mode: umode_t
+i_op: inode_operations*
}
class inode_operations {
+get_offset_ctx(inode inode*): offset_ctx*
}
class offset_ctx {
+mt: maple_tree
}
class maple_tree {
+root: void*
}
class SimpleOffsetOps {
+simple_offset_init(octx offset_ctx*): void
+simple_offset_add(octx offset_ctx*, dentry dentry*): int
+simple_offset_remove(octx offset_ctx*, dentry dentry*): void
+simple_offset_rename(old_dir inode*, old_dentry dentry*, new_dir inode*, new_dentry dentry*): void
+simple_offset_rename_exchange(old_dir inode*, old_dentry dentry*, new_dir inode*, new_dentry dentry*): int
+simple_offset_replace(octx offset_ctx*, dentry dentry*, index u32): int
}
class ShmemRename {
+shmem_rename2(idmap mnt_idmap*, old_dir inode*, old_dentry dentry*, new_dir inode*, new_dentry dentry*, flags unsigned int): int
+shmem_whiteout(idmap mnt_idmap*, dir inode*, dentry dentry*): int
+shmem_get_offset_ctx(dir inode*): offset_ctx*
}
class DentryHelpers {
+dentry2offset(dentry dentry*): u32
+offset_set(dentry dentry*, index u32): void
+d_really_is_positive(dentry dentry*): bool
}
class VfsRenameHelpers {
+simple_rename_exchange(old_dir inode*, old_dentry dentry*, new_dir inode*, new_dentry dentry*): int
+simple_empty(dentry dentry*): bool
}
class MapleTreeApi {
+mtree_store(mt maple_tree*, index u32, value void*, gfp gfp_t): int
}
inode --> inode_operations : uses
inode_operations --> offset_ctx : get_offset_ctx
offset_ctx --> maple_tree : has
ShmemRename --> offset_ctx : shmem_get_offset_ctx
ShmemRename --> SimpleOffsetOps : uses
ShmemRename --> DentryHelpers : uses
ShmemRename --> VfsRenameHelpers : uses
SimpleOffsetOps --> DentryHelpers : uses
SimpleOffsetOps --> MapleTreeApi : uses
MapleTreeApi --> maple_tree : operates_on
VfsRenameHelpers --> inode : uses
VfsRenameHelpers --> DentryHelpers : uses
Flow diagram for simple_offset_rename_exchange low-memory safe updatesflowchart TD
A["Start simple_offset_rename_exchange"] --> B["Get old_index = dentry2offset(old_dentry)"]
B --> C["Get new_index = dentry2offset(new_dentry)"]
C --> D{"old_index == 0 or new_index == 0?"}
D -->|Yes| E["WARN_ON and return -EINVAL"]
D -->|No| F["ret = mtree_store(new_ctx.mt, new_index, old_dentry)"]
F --> G{"ret != 0?"}
G -->|Yes| H["WARN_ON and return ret"]
G -->|No| I["ret = mtree_store(old_ctx.mt, old_index, new_dentry)"]
I --> J{"ret != 0?"}
J -->|Yes| K["Restore new_ctx: mtree_store(new_ctx.mt, new_index, new_dentry)"]
K --> L["return ret"]
J -->|No| M["offset_set(old_dentry, new_index)"]
M --> N["offset_set(new_dentry, old_index)"]
N --> O["simple_rename_exchange(old_dir, old_dentry, new_dir, new_dentry)"]
O --> P["return 0"]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![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 - here's some feedback:
- The new WARN_ON(!new_offset) / WARN_ON(!old_index || !new_index) assumptions in the offset helpers are subtle; consider adding brief comments documenting why a zero offset is impossible at these points (e.g., because callers have pre-grabbed slots), so future changes don’t inadvertently break these invariants.
- In simple_offset_rename_exchange(), you now rely on mtree_store() being effectively infallible when the index is already a singleton and use it for rollback; it would help maintainability to add a short code comment explicitly stating this constraint and that GFP_KERNEL is safe in these specific calls.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new WARN_ON(!new_offset) / WARN_ON(!old_index || !new_index) assumptions in the offset helpers are subtle; consider adding brief comments documenting why a zero offset is impossible at these points (e.g., because callers have pre-grabbed slots), so future changes don’t inadvertently break these invariants.
- In simple_offset_rename_exchange(), you now rely on mtree_store() being effectively infallible when the index is already a singleton and use it for rollback; it would help maintainability to add a short code comment explicitly stating this constraint and that GFP_KERNEL is safe in these specific calls.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Pull request overview
This PR backports an upstream fix from v6.19 to the 6.6-y kernel that addresses recovery failures during rename operations in the shmem filesystem. The fix ensures proper cleanup when maple_tree/xarray insertions fail due to memory shortage. However, this backport contains critical compilation errors that must be fixed before merging.
Key Changes:
- Pre-allocates offset slots for rename targets to avoid failure points after non-reversible operations
- Changes
simple_offset_rename()from returning int to void, moving error handling to callers - Updates
simple_offset_rename_exchange()to use guaranteed-success operations when slots already exist
Critical Issue: The backport incorrectly uses mtree_store() with non-existent mt fields instead of xa_store() with the actual xa fields in offset_ctx structure.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| mm/shmem.c | Pre-allocates offset slot before potentially failing operations (whiteout) and adds cleanup logic on failure |
| include/linux/fs.h | Changes simple_offset_rename() return type from int to void |
| fs/libfs.c | Refactors rename functions to eliminate failure points after slot pre-allocation; contains critical bugs using wrong API |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
fs/libfs.c
Outdated
| goto out_restore; | ||
| ret = mtree_store(&old_ctx->mt, old_index, new_dentry, GFP_KERNEL); | ||
| if (WARN_ON(ret)) { | ||
| mtree_store(&new_ctx->mt, new_index, new_dentry, GFP_KERNEL); |
There was a problem hiding this comment.
The code attempts to access new_ctx->mt but the offset_ctx structure only contains an xa (xarray) field, not an mt (maple_tree) field. This will cause a compilation error. The correct field to use is new_ctx->xa with xa_store instead of mtree_store.
| * (new) directory entry to be unchanged after a rename. | ||
| * | ||
| * Returns zero on success, a negative errno value on failure. | ||
| * Caller must have grabbed a slot for new_dentry in the maple_tree |
There was a problem hiding this comment.
The comment mentions "maple_tree" but the offset_ctx structure in this kernel version uses xarray (xa) not maple_tree. The comment should refer to "xarray" instead to match the actual data structure being used.
Suggested change
| * Caller must have grabbed a slot for new_dentry in the maple_tree | |
| * Caller must have grabbed a slot for new_dentry in the xarray |
Member
Author
There was a problem hiding this comment.
The comment mentions "maple_tree" but the
offset_ctxstructure in this kernel version uses xarray (xa) not maple_tree. The comment should refer to "xarray" instead to match the actual data structure being used.
No.
The linux-stable maintenance policy requires that, when backporting patches, you keep them as close to the original commit as possible — even if the comments no longer match the modified code logic.
fs/libfs.c
Outdated
| ret = simple_offset_replace(new_ctx, old_dentry, new_index); | ||
| if (ret) | ||
| goto out_restore; | ||
| ret = mtree_store(&new_ctx->mt, new_index, old_dentry, GFP_KERNEL); |
There was a problem hiding this comment.
The code attempts to access new_ctx->mt but the offset_ctx structure only contains an xa (xarray) field, not an mt (maple_tree) field. This will cause a compilation error. The correct field to use is new_ctx->xa with xa_store instead of mtree_store. This appears to be a backport error where the upstream v6.19 code (which may use maple_tree) was not properly adapted for the 6.6-y kernel (which uses xarray).
fs/libfs.c
Outdated
| if (ret) { | ||
| simple_offset_remove(new_ctx, old_dentry); | ||
| goto out_restore; | ||
| ret = mtree_store(&old_ctx->mt, old_index, new_dentry, GFP_KERNEL); |
There was a problem hiding this comment.
The code attempts to access old_ctx->mt but the offset_ctx structure only contains an xa (xarray) field, not an mt (maple_tree) field. This will cause a compilation error. The correct field to use is old_ctx->xa with xa_store instead of mtree_store.
[ Upstream commit e1b4c6a58304fd490124cc2b454d80edc786665c ] maple_tree insertions can fail if we are seriously short on memory; simple_offset_rename() does not recover well if it runs into that. The same goes for simple_offset_rename_exchange(). Moreover, shmem_whiteout() expects that if it succeeds, the caller will progress to d_move(), i.e. that shmem_rename2() won't fail past the successful call of shmem_whiteout(). Not hard to fix, fortunately - mtree_store() can't fail if the index we are trying to store into is already present in the tree as a singleton. For simple_offset_rename_exchange() that's enough - we just need to be careful about the order of operations. For simple_offset_rename() solution is to preinsert the target into the tree for new_dir; the rest can be done without any potentially failing operations. That preinsertion has to be done in shmem_rename2() rather than in simple_offset_rename() itself - otherwise we'd need to deal with the possibility of failure after successful shmem_whiteout(). Fixes: a2e4595 ("shmem: stable directory offsets") Reviewed-by: Christian Brauner <[email protected]> Reviewed-by: Chuck Lever <[email protected]> Signed-off-by: Al Viro <[email protected]> [ Backport from v6.19 ] [ Backport Adaptation Note: The upstream kernel has migrated the struct offset_ctx from using xarray to maple_tree, where the field is named mt and operations use mtree_store(). However, the current 6.6-y kernel still uses the original xarray-based implementation with the xa field and xa_store() API. This patch adapts the upstream commit by replacing the direct mtree_store(&ctx->mt, ...) calls with the existing simple_offset_replace() helper function, which internally performs xa_store(&octx->xa, ...). This approach is logically equivalent to the original commit because: *1. Both mtree_store() and xa_store() serve the same purpose: storing a dentry pointer at a given offset index in the directory's offset map. 2. The simple_offset_replace() function already exists in the 6.6-y codebase and correctly handles xarray operations including error checking via xa_is_err()/xa_err(). 3. Since simple_offset_replace() also calls offset_set() internally, the redundant offset_set() calls present after the store operations have been removed to avoid setting the offset twice. 4. The error handling and rollback logic remains identical: if the second store fails, the first operation is reverted by restoring the original dentry at its position. ] Signed-off-by: WangYuli <[email protected]>
Avenger-285714
force-pushed
the
shmem
branch
from
9a946a0 to
00f1c45
Compare
deepin pr auto review我来对这个git diff进行详细审查：
主要问题和建议：
if (WARN_ON(!new_offset))
return;建议在返回前进行适当的清理工作，即使这是一个不应该发生的情况。
if (WARN_ON(!old_index || !new_index))
return -EINVAL;这里的错误处理是合理的，但建议在返回前确保系统状态的一致性。
error = simple_offset_add(shmem_get_offset_ctx(new_dir), new_dentry);
if (error == -EBUSY)
had_offset = true;
else if (unlikely(error))
return error;这个改动增加了对 offset 的预分配，但错误处理可能需要更细致的考虑，特别是在 EBUSY 的情况下。
simple_rename_exchange(old_dir, old_dentry, new_dir, new_dentry);
return 0;这可能是一个潜在的问题，因为 总体建议：
这些改动整体上提高了代码的简洁性和可维护性，但需要注意确保错误处理的完整性。建议在合并前进行充分的测试，特别是在边界条件和错误情况下。 |
opsiff
approved these changes
Dec 17, 2025
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: opsiff The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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.
[ Upstream commit e1b4c6a58304fd490124cc2b454d80edc786665c ]
maple_tree insertions can fail if we are seriously short on memory; simple_offset_rename() does not recover well if it runs into that. The same goes for simple_offset_rename_exchange().
Moreover, shmem_whiteout() expects that if it succeeds, the caller will progress to d_move(), i.e. that shmem_rename2() won't fail past the successful call of shmem_whiteout().
Not hard to fix, fortunately - mtree_store() can't fail if the index we are trying to store into is already present in the tree as a singleton.
For simple_offset_rename_exchange() that's enough - we just need to be careful about the order of operations.
For simple_offset_rename() solution is to preinsert the target into the tree for new_dir; the rest can be done without any potentially failing operations.
That preinsertion has to be done in shmem_rename2() rather than in simple_offset_rename() itself - otherwise we'd need to deal with the possibility of failure after successful shmem_whiteout().
Fixes: a2e4595 ("shmem: stable directory offsets")
Reviewed-by: Christian Brauner [email protected]
Reviewed-by: Chuck Lever [email protected]
[ Backport from v6.19 ]
Summary by Sourcery
Improve shmem rename recovery when maple_tree operations fail under low-memory conditions.
Bug Fixes:
Enhancements: