mds: catch damage to dentry's first field #49773

batrick · 2023-01-18T02:55:46Z

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. "pacific"), please refer to Submitting Patches to Ceph - Backports for the proper workflow.

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 retest this please
jenkins test classic perf
jenkins test crimson perf
jenkins test signed
jenkins test make check
jenkins test make check arm64
jenkins test submodules
jenkins test dashboard
jenkins test dashboard cephadm
jenkins test api
jenkins test docs
jenkins render docs
jenkins test ceph-volume all
jenkins test ceph-volume tox
jenkins test windows

lxbsz · 2023-01-18T03:19:26Z

src/mds/CDentry.cc

+{
+  auto&& snapclient = dir->mdcache->mds->snapclient;
+  auto next_snap = snapclient->get_last_created()+1;
+  if (first > last || (snapclient->is_server_ready() && first > next_snap)) {


Will this also be possible that the first equals to last, which are all head just like in https://tracker.ceph.com/issues/38452#note-10 ?

If so we also need to check this and abort the MDS ?

Could we also check CInode's corruption in this PR ?

If first > next_snap, it would trigger when first == SNAP_HEAD.

Could we also check CInode's corruption in this PR ?

It's not a bad idea but writing dedicated tests for that kind of corruption would take time. I believe the corruption is introduced via the dentries so it's best to get a fix in for this ASAP.

Makes sense +1

src/mds/CDentry.h

kotreshhr · 2023-01-19T13:32:54Z

The PR looks good to me. But I have not followed much on how postgress can corrupt the dentry which this PR is catching. A bit of information about that in commit msg would help.

batrick · 2023-01-19T18:40:08Z

Thanks @kotreshhr , I've left a small explanation in one of the commits.

vshankar

LGTM

src/mds/Server.cc

vshankar · 2023-01-25T09:39:43Z

src/mds/CDentry.cc

+{
+  auto&& snapclient = dir->mdcache->mds->snapclient;
+  auto next_snap = snapclient->get_last_created()+1;
+  if (first > last || (snapclient->is_server_ready() && first > next_snap)) {


Makes sense +1

rishabh-d-dave · 2023-01-31T10:06:31Z

jenkins test make check

rishabh-d-dave · 2023-01-31T10:06:38Z

jenkins test api

batrick · 2023-02-08T15:49:00Z

jenkins test api

vshankar · 2023-02-23T05:39:05Z

jenkins test api

batrick · 2023-02-28T16:18:45Z

jenkins test api

vshankar · 2023-03-06T13:53:04Z

@batrick I haven't gone through the test run, but this test failure caught my eye which might be related: https://pulpito.ceph.com/vshankar-2023-03-03_04:39:14-fs-wip-vshankar-testing-20230303.023823-testing-default-smithi/7192234

batrick · 2023-03-10T23:27:02Z

@batrick I haven't gone through the test run, but this test failure caught my eye which might be related: https://pulpito.ceph.com/vshankar-2023-03-03_04:39:14-fs-wip-vshankar-testing-20230303.023823-testing-default-smithi/7192234

This was a surprising failure I didn't see before in teuthology. We expect the mds to die but for some reason only recently see these errors AFAIK.

Anyway, I've fixed that.

I'm looking into another logical problem with this PR showing up in other test failures.

* refs/pull/49773/head: qa: add missing scan_links step for data scan recovery qa/tasks/cephfs: test damage to dentry's first is caught qa/tasks/cephfs: use rank_asok and allow specifying rank qa/tasks: allow specifying timeout command prefix to ceph mds: provide test configs for creating first corruption mds: catch damage to dentry's first field mds: add debugging for pre_cow_old_inode mds: cleanup code

batrick · 2023-03-28T15:12:53Z

jenkins test make check arm64

batrick · 2023-03-28T15:48:18Z

jenkins test make check arm64

vshankar · 2023-03-29T02:46:58Z

Changes look good. I'll have a final rundown on the test run before merging this. Thank you, @batrick.

batrick · 2023-03-29T12:46:12Z

https://pulpito.ceph.com/?branch=wip-pdonnell-testing-20230323.162417

batrick · 2023-03-29T12:47:51Z

#50692 should be merged with this PR or there will be QA failures in main.

batrick · 2023-03-29T13:07:27Z

@vshankar config added. Going to QA now.

batrick · 2023-03-29T13:07:36Z

cc @gregsfortytwo

batrick · 2023-03-29T13:46:12Z

jenkins test api

lxbsz · 2023-03-29T13:54:03Z

src/mds/CDir.cc

+    dout(1) << *dn << " " << dn->corrupt_first_loaded << dendl;
+    if (!dn->corrupt_first_loaded) {
+      dn->check_corruption(false);
+    }


Is this for debug only ? The debug level is 1 ?

... yes!

Good catch!

batrick · 2023-03-29T15:18:36Z

https://pulpito.ceph.com/pdonnell-2023-03-29_15:15:36-fs-wip-pdonnell-testing-20230329.131031-distro-default-smithi/

without latest SQUASH commit. This is just --suite fs --filter damage.

@vshankar do you want more tests run?

vshankar · 2023-03-29T15:41:44Z

https://pulpito.ceph.com/pdonnell-2023-03-29_15:15:36-fs-wip-pdonnell-testing-20230329.131031-distro-default-smithi/

without latest SQUASH commit. This is just --suite fs --filter damage.

@vshankar do you want more tests run?

Nope. Good to merge once tests pass.

Signed-off-by: Patrick Donnelly <pdonnell@redhat.com>

When possible. Abort the MDS before it can be written to the journal/directory. This is part of a series to address corruption first observed in [1]. How the corruption is introduced is yet unknown. [1] https://tracker.ceph.com/issues/38452#note-10 Fixes: http://tracker.ceph.com/issues/58482 Signed-off-by: Patrick Donnelly <pdonnell@redhat.com>

Signed-off-by: Patrick Donnelly <pdonnell@redhat.com>

This will use the more efficient: ceph tell mds.<fsname>:<rank> ... Signed-off-by: Patrick Donnelly <pdonnell@redhat.com>

Signed-off-by: Patrick Donnelly <pdonnell@redhat.com>

Without, the first field remains corrupt (HEAD). Signed-off-by: Patrick Donnelly <pdonnell@redhat.com>

So admin can restore access to files if necessary. Signed-off-by: Patrick Donnelly <pdonnell@redhat.com>

batrick · 2023-03-29T17:04:25Z

Tests look good: https://pulpito.ceph.com/pdonnell-2023-03-29_15:15:36-fs-wip-pdonnell-testing-20230329.131031-distro-default-smithi/

The warning should be ignorelisted. I added it to qa/tasks/cephfs: test damage to dentry's first is caught .

Will merge when jenkins test pass.

gregsfortytwo · 2023-03-29T17:27:00Z

The warning should be ignorelisted. I added it to qa/tasks/cephfs: test damage to dentry's first is caught .

Only for the tests we are deliberately inducing it in, right? If this turns up in the lab we definitely want to detect it!

batrick · 2023-03-29T17:43:12Z

The warning should be ignorelisted. I added it to qa/tasks/cephfs: test damage to dentry's first is caught .

Only for the tests we are deliberately inducing it in, right? If this turns up in the lab we definitely want to detect it!

Yes, it's induced.

batrick · 2023-03-29T17:46:21Z

jenkins test make check arm64

github-actions bot added cephfs Ceph File System common tests labels Jan 18, 2023

batrick added the needs-review label Jan 18, 2023

lxbsz reviewed Jan 18, 2023

View reviewed changes

src/mds/CDentry.h Outdated Show resolved Hide resolved

batrick force-pushed the postgres-encode branch from b74b8c3 to 229b872 Compare January 19, 2023 18:39

batrick force-pushed the postgres-encode branch from 229b872 to ed581bf Compare January 20, 2023 23:22

kotreshhr approved these changes Jan 23, 2023

View reviewed changes

vshankar reviewed Jan 25, 2023

View reviewed changes

batrick force-pushed the postgres-encode branch 2 times, most recently from cc17782 to 25b3ad9 Compare January 26, 2023 19:33

vshankar added wip-vshankar-backlog Backlog of CephFS PRs to pick for testing wip-vshankar-testing2 and removed wip-vshankar-backlog Backlog of CephFS PRs to pick for testing labels Feb 16, 2023

vshankar added wip-vshankar-backlog Backlog of CephFS PRs to pick for testing and removed wip-vshankar-testing2 labels Mar 7, 2023

batrick force-pushed the postgres-encode branch from 25b3ad9 to 210becf Compare March 10, 2023 23:26

batrick force-pushed the postgres-encode branch from 210becf to bf90995 Compare March 15, 2023 18:25

batrick added the wip-pdonnell-testing2 label Mar 15, 2023

batrick force-pushed the postgres-encode branch from bf90995 to 0c69de2 Compare March 15, 2023 19:03

batrick force-pushed the postgres-encode branch from e126f34 to 686b4b3 Compare March 28, 2023 14:02

batrick force-pushed the postgres-encode branch from 80fc323 to 78d0aca Compare March 29, 2023 13:09

lxbsz reviewed Mar 29, 2023

View reviewed changes

batrick added 9 commits March 29, 2023 13:02

mds: cleanup code

17d8265

Signed-off-by: Patrick Donnelly <pdonnell@redhat.com>

mds: add debugging for pre_cow_old_inode

c9d3605

Signed-off-by: Patrick Donnelly <pdonnell@redhat.com>

mds: provide test configs for creating first corruption

fe258dc

Signed-off-by: Patrick Donnelly <pdonnell@redhat.com>

qa/tasks: allow specifying timeout command prefix to ceph

dbf1a40

Signed-off-by: Patrick Donnelly <pdonnell@redhat.com>

qa/tasks/cephfs: use rank_asok and allow specifying rank

3ba1739

This will use the more efficient: ceph tell mds.<fsname>:<rank> ... Signed-off-by: Patrick Donnelly <pdonnell@redhat.com>

qa/tasks/cephfs: test damage to dentry's first is caught

293b90f

Signed-off-by: Patrick Donnelly <pdonnell@redhat.com>

qa: add missing scan_links step for data scan recovery

1b4d907

Without, the first field remains corrupt (HEAD). Signed-off-by: Patrick Donnelly <pdonnell@redhat.com>

mds: add config to decide whether to mark dentry bad

7ffa065

So admin can restore access to files if necessary. Signed-off-by: Patrick Donnelly <pdonnell@redhat.com>

batrick force-pushed the postgres-encode branch from 6375d9a to 7ffa065 Compare March 29, 2023 17:03

batrick merged commit 0c4d835 into ceph:main Mar 29, 2023
3 checks passed

batrick deleted the postgres-encode branch March 29, 2023 20:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mds: catch damage to dentry's first field #49773

mds: catch damage to dentry's first field #49773

batrick commented Jan 18, 2023 •

edited

lxbsz Jan 18, 2023 •

edited

lxbsz Jan 18, 2023

batrick Jan 19, 2023

vshankar Jan 25, 2023

kotreshhr commented Jan 19, 2023

batrick commented Jan 19, 2023

vshankar left a comment

vshankar Jan 25, 2023

rishabh-d-dave commented Jan 31, 2023

rishabh-d-dave commented Jan 31, 2023

batrick commented Feb 8, 2023

vshankar commented Feb 23, 2023

batrick commented Feb 28, 2023

vshankar commented Mar 6, 2023

batrick commented Mar 10, 2023

batrick commented Mar 28, 2023

batrick commented Mar 28, 2023

vshankar commented Mar 29, 2023

batrick commented Mar 29, 2023

batrick commented Mar 29, 2023

batrick commented Mar 29, 2023

batrick commented Mar 29, 2023

batrick commented Mar 29, 2023

lxbsz Mar 29, 2023

batrick Mar 29, 2023

batrick commented Mar 29, 2023

vshankar commented Mar 29, 2023

batrick commented Mar 29, 2023

gregsfortytwo commented Mar 29, 2023

batrick commented Mar 29, 2023

batrick commented Mar 29, 2023

mds: catch damage to dentry's first field #49773

mds: catch damage to dentry's first field #49773

Conversation

batrick commented Jan 18, 2023 • edited

Contribution Guidelines

Checklist

lxbsz Jan 18, 2023 • edited

Choose a reason for hiding this comment

lxbsz Jan 18, 2023

Choose a reason for hiding this comment

batrick Jan 19, 2023

Choose a reason for hiding this comment

vshankar Jan 25, 2023

Choose a reason for hiding this comment

kotreshhr commented Jan 19, 2023

batrick commented Jan 19, 2023

vshankar left a comment

Choose a reason for hiding this comment

vshankar Jan 25, 2023

Choose a reason for hiding this comment

rishabh-d-dave commented Jan 31, 2023

rishabh-d-dave commented Jan 31, 2023

batrick commented Feb 8, 2023

vshankar commented Feb 23, 2023

batrick commented Feb 28, 2023

vshankar commented Mar 6, 2023

batrick commented Mar 10, 2023

batrick commented Mar 28, 2023

batrick commented Mar 28, 2023

vshankar commented Mar 29, 2023

batrick commented Mar 29, 2023

batrick commented Mar 29, 2023

batrick commented Mar 29, 2023

batrick commented Mar 29, 2023

batrick commented Mar 29, 2023

lxbsz Mar 29, 2023

Choose a reason for hiding this comment

batrick Mar 29, 2023

Choose a reason for hiding this comment

batrick commented Mar 29, 2023

vshankar commented Mar 29, 2023

batrick commented Mar 29, 2023

gregsfortytwo commented Mar 29, 2023

batrick commented Mar 29, 2023

batrick commented Mar 29, 2023

batrick commented Jan 18, 2023 •

edited

lxbsz Jan 18, 2023 •

edited