mds: scrub fix by sajibreadd-croit · Pull Request #6 · croit/ceph

sajibreadd-croit · 2025-12-19T11:04:04Z

remote link damage identification with reverse parent scrubbing
- remote link identification becomes tricky if inode is cached.
- Try to open the link normally, if issue while opening mark as damaged
- If openned successfully, it can be possible there is damage but inode
  is cached that's why it is succssful while opening. In that case take
  that openned inode, and scrub ancestors recursively. If any of the ancestor
  is damaged it remote link is marked as damaged.
- while scrubbing some flag is maintained in the inode,
  e.g. whether scrub is backward or forward or both
- his backward scrubbing will only work in read-only scrub that means
  without repair flag and mds_scrub_hard_link this ceph flag is turned on.
- A new type of damage introduced, using which multiple links point to same
  inode can be identified, which was not possible previously.
mds_damage_log_to_file and mds_damage_log_file is used to print out damages
in a file persistently as it's not safe to keep it in memory

Contribution Guidelines

To sign and title your commits, please refer to Submitting Patches to Ceph.
If you are submitting a fix for a stable branch (e.g. "quincy"), please refer to Submitting Patches to Ceph - Backports for the proper workflow.
When filling out the below checklist, you may click boxes directly in the GitHub web UI. When entering or editing the entire PR message in the GitHub web UI editor, you may also select a checklist item by adding an x between the brackets: [x]. Spaces and capitalization matter when checking off items this way.

Checklist

Tracker (select at least one)
- References tracker ticket
- Very recent bug; references commit where it was introduced
- New feature (ticket optional)
- Doc update (no ticket needed)
- Code cleanup (no ticket needed)
Component impact
- Affects Dashboard, opened tracker ticket
- Affects Orchestrator, opened tracker ticket
- No impact that needs to be tracked
Documentation (select at least one)
- Updates relevant documentation
- No doc update is appropriate
Tests (select at least one)
- Includes unit test(s)
- Includes integration test(s)
- Includes bug reproducer
- No tests

Show available Jenkins commands

jenkins test classic perf Jenkins Job | Jenkins Job Definition
jenkins test crimson perf Jenkins Job | Jenkins Job Definition
jenkins test signed Jenkins Job | Jenkins Job Definition
jenkins test make check Jenkins Job | Jenkins Job Definition
jenkins test make check arm64 Jenkins Job | Jenkins Job Definition
jenkins test submodules Jenkins Job | Jenkins Job Definition
jenkins test dashboard Jenkins Job | Jenkins Job Definition
jenkins test dashboard cephadm Jenkins Job | Jenkins Job Definition
jenkins test api Jenkins Job | Jenkins Job Definition
jenkins test docs ReadTheDocs | Github Workflow Definition
jenkins test ceph-volume all Jenkins Jobs | Jenkins Jobs Definition
jenkins test windows Jenkins Job | Jenkins Job Definition
jenkins test rook e2e Jenkins Job | Jenkins Job Definition

You must only issue one Jenkins command per-comment. Jenkins does not understand
comments with more than one command.

1. remote link damage identification with reverse parent scrubbing - remote link identification becomes tricky if inode is cached. - Try to open the link normally, if issue while opening mark as damaged - If openned successfully, it can be possible there is damage but inode is cached that's why it is succssful while opening. In that case take that openned inode, and scrub ancestors recursively. If any of the ancestor is damaged it remote link is marked as damaged. - while scrubbing some flag is maintained in the inode, e.g. whether scrub is backward or forward or both - his backward scrubbing will only work in read-only scrub that means without repair flag and mds_scrub_hard_link this ceph flag is turned on. - A new type of damage introduced, using which multiple links point to same inode can be identified, which was not possible previously. 2. mds_damage_log_to_file and mds_damage_log_file is used to print out damages in a file persistently as it's not safe to keep it in memory Signed-off-by: Md Mahamudur Rahaman Sajib <mahamudur.sajib@croit.io>

Signed-off-by: Igor Fedotov <igor.fedotov@croit.io>

…crub Also update ignorelist with the expected cluster warning. Fixes: https://tracker.ceph.com/issues/69345 Signed-off-by: Md Mahamudur Rahaman Sajib <mahamudur.sajib@croit.io> (cherry picked from commit 13eb6c6)

…damage-file candidate selection, type filtering, and bounded window controls (`--extent-period`, `--extent-limit`) into DataScan while preserving legacy full-scan behavior. Add cls-assisted accumulated-metadata flushing semantics and robust CephFS QA fixtures/tests that validate manual expectations, inode/damage parity, multi-pool edge cases, and argument validation with deterministic QA coverage. Signed-off-by: Md Mahamudur Rahaman Sajib <mahamudur.sajib@croit.io>

Signed-off-by: Md Mahamudur Rahaman Sajib <mahamudur.sajib@croit.io>

ifed01 · 2026-04-29T16:40:12Z

+    std::string damage_file_path;
+    std::string damage_type_expr;
+    std::vector<std::string> damage_type_tokens;
+    bool damage_type_filter_set;


Looks redundant, could be replaced with damage_type_tocken.size()

ifed01 · 2026-04-29T16:40:21Z

+    std::string damage_type_expr;
+    std::vector<std::string> damage_type_tokens;
+    bool damage_type_filter_set;
+    bool damage_type_all;


Isn't that always equal to !damage_type_filter_set ?

ifed01 · 2026-04-29T16:52:02Z

+    // Parser state for scan_extents damage/inode inputs.
+    std::string damage_file_path;
+    std::string damage_type_expr;
+    std::vector<std::string> damage_type_tokens;


why not using std::set instead?

ifed01 · 2026-04-29T16:57:51Z

+    bool damage_type_all;
+    std::string inode_file_path;
+    uint64_t extent_period;
+    bool extent_period_set;


IMO redundant, extent_period == 0 should be sufficient to indicate unset val

I am keeping the default value of extent_period = 1 such that if someone doesn't add that parameter then default value will be kept. But also I want to protect other commands having extent_period. That's why a sanity boolean is used.

ifed01 · 2026-04-29T16:58:01Z

+    uint64_t extent_period;
+    bool extent_period_set;
+    uint64_t extent_limit;
+    bool extent_limit_set;


ifed01 · 2026-04-29T20:17:25Z

+    librados::IoCtx &ctx, inodeno_t inode_no, const uint64_t obj_index,
+    const uint64_t obj_size, const int64_t obj_pool_id, const time_t mtime,
+    AccumulateResult *accum_res, const bool immediate_write) {
+  if (!immediate_write) {


Why are you extending accumulate_inode_metadata() interface with two additional parameter to perform completely different in-mem accumulation instead of having/using standalone method, e.g. AccumulatedResult::update_accumulate_result or something.

ifed01 · 2026-04-29T20:21:18Z

+    const uint64_t obj_size, const int64_t obj_pool_id, const time_t mtime,
+    AccumulateResult *accum_res, const bool immediate_write) {
+  if (!immediate_write) {
+    if (accum_res == nullptr) {


IMO one can assert here and get rid off any return value

ifed01 · 2026-04-29T20:23:25Z

+        r = ClsCephFSClient::accumulate_inode_metadata(data_io, ino, extent_id,
+                                                       size, obj_pool_id, mtime,
+                                                       &accum_res, false);
+        if (r < 0) {


One can get rid off any error handling here if using standalone update_accumulate_result() method for in-mem update as per my comment above.

ifed01 · 2026-04-29T20:38:45Z

  return ctx.operate(zeroth_object.name, &op);
 }

+int ClsCephFSClient::flush_inode_accumulate_result(


I have a feeling that everything this function does could be performed through the original accumulate_inode_metadata() call. Am I missing something?
If thats' the case then all these new code to support set_accumulated_inode_metadata method and its parameter structure could be removed.

Actually it won't be possible (I also initially think that way). Let's assume a case where no chunk exist but the hard_link (basically as dentry) exist. In that case I would like to create a fake inode using force_create_head_inode to resolve the issue. We could remove that dentry from omap as well but it would cause extra corruption in link count. Those fake inode eventually will go into directory lost+found (check scan_inode_from_oid function).

ifed01 · 2026-04-29T20:42:52Z

+      }
+    }
+
+    if (!window_productive && all_stat_failed) {


Isn't window_production check redundant here? IIUC when all_stat_failed is true window_productive is always false.

ifed01 · 2026-04-29T21:23:32Z

+                                 uint64_t obj_name_offset,
+                                 bool force_restore_ancestors,
+                                 std::unordered_set<uint64_t> &&inode_set) {
+  if (force_restore_ancestors &&


do we really need this check here or we can rely on the same inside inject_with_backtrace?

Did you mean the cycle check is unnecessary or the whole scan_frag_from_oid call inside inject_with_backtrace is unnecessary?

ifed01 · 2026-04-29T21:28:57Z

      // should be fixed up using a separate tool scanning the metadata
      // pool.
-      break;
+      if (force_inject_ancestors) {


Is the comment above still valid with the new code?

This will not be true according to the code I did. When I found a dir_frag, I used that directory inode object's backtrace to restore not the outdated backtrace of child inode.

My argument here is, if this idea have risk then whole scan_frags is in risk.

Signed-off-by: Md Mahamudur Rahaman Sajib mahamudur.sajib@croit.io

github-actions Bot added cephfs common config-change labels Dec 19, 2025

sajibreadd-croit commented Dec 19, 2025

View reviewed changes

Comment thread src/mds/ScrubStack.cc Outdated

sajibreadd-croit commented Dec 19, 2025

View reviewed changes

Comment thread src/mds/ScrubStack.cc Outdated

sajibreadd-croit requested a review from ifed01 December 19, 2025 11:11

sajibreadd-croit force-pushed the wip-washu-scrub-fix-v18.2.4 branch 5 times, most recently from 280aec3 to 3fdcc5b Compare December 19, 2025 12:44

sajibreadd-croit force-pushed the wip-washu-scrub-fix-v18.2.4 branch from 3fdcc5b to 9e16a8e Compare January 7, 2026 08:10

github-actions Bot added core mon api-change build/ops documentation mgr pybind cephadm orchestrator rook bluestore crimson dashboard CI rbd nvmeof rgw ceph-volume labels Jan 7, 2026

sajibreadd-croit force-pushed the wip-washu-scrub-fix-v18.2.4 branch 6 times, most recently from a9a992f to 81d0299 Compare February 25, 2026 15:10

sajibreadd-croit and others added 3 commits February 25, 2026 19:53

mds: refactor hard link scrubbing

890f117

Signed-off-by: Igor Fedotov <igor.fedotov@croit.io>

sajibreadd-croit force-pushed the wip-washu-scrub-fix-v18.2.4 branch 5 times, most recently from 3efad5c to d9ddfd5 Compare February 28, 2026 09:24

sajibreadd-croit force-pushed the wip-washu-scrub-fix-v18.2.4 branch from d9ddfd5 to 4fa1e49 Compare March 1, 2026 11:42

sajibreadd-croit force-pushed the wip-washu-scrub-fix-v18.2.4 branch 6 times, most recently from 3a3e0de to 057ae50 Compare March 18, 2026 11:06

sajibreadd-croit added 2 commits March 18, 2026 14:50

Added targeted scanning for scan_inodes

971ce2a

Signed-off-by: Md Mahamudur Rahaman Sajib <mahamudur.sajib@croit.io>

removed inodes should be removed from injected_inos as well.

b7cf88a

Signed-off-by: Md Mahamudur Rahaman Sajib <mahamudur.sajib@croit.io>

sajibreadd-croit force-pushed the wip-washu-scrub-fix-v18.2.4 branch from 55f6b5b to b7cf88a Compare March 18, 2026 13:51

Parallelized scan_links

a14127f

Signed-off-by: Md Mahamudur Rahaman Sajib <mahamudur.sajib@croit.io>

ifed01 reviewed Apr 29, 2026

View reviewed changes

Review resolved (Will squash this commit later)

c85740a

Signed-off-by: Md Mahamudur Rahaman Sajib mahamudur.sajib@croit.io

Conversation

sajibreadd-croit commented Dec 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution Guidelines

Checklist

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sajibreadd-croit May 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sajibreadd-croit commented Dec 19, 2025 •

edited

Loading

sajibreadd-croit May 12, 2026 •

edited

Loading