osd: add has_manifest_chunk to count chunks in snapshot by myoungwon · Pull Request #38767 · ceph/ceph

myoungwon · 2021-01-05T12:52:08Z

cls_has_chunk does not cover snapshotted manifest object.
This leads to unexpected behavior during chunk scrub.

Signed-off-by: Myoungwon Oh myoungwon.oh@samsung.com

Checklist

References tracker ticket
Updates documentation if necessary
Includes tests for new functionality or reproducer for bug

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 api
jenkins test docs
jenkins render docs
jenkins test ceph-volume all
jenkins test ceph-volume tox

athanatos · 2021-01-08T00:12:45Z

src/osd/PrimaryLogPG.cc

+{
+  if (!obc->obs.oi.has_manifest()) {
+    return false;
+  }


Probably want to remove this check from cls_has_chunk as well.

athanatos · 2021-01-08T00:14:51Z

What is cls_has_chunk suppose to do? Why do we want to return true if any snapshot has that chunk? Do we use it to validate whether a particular chunk reference is live? Wouldn't it actually have to return the number of references to a particular chunk (an object might reference the same chunk twice at different offsets within the object)?

myoungwon · 2021-01-08T00:59:11Z

cls_has_chunk is used for chunk scrub by ceph_dedup_tool in order to find out the reference mismatch.
Assuming that there are manifest and chunk object in two tiers like:
base tier: manifest object A (has a chunk object CHUNKA)
low tier: chunk object CHUNKA (has a reference of the object A)

Due to the false-positive design on reference counting,

[From https://github.com//pull/24230] As you know, the key idea behind of reference counting is false-positive, which means (manifest object (no ref), chunk object(has ref)) can be possible instead of (manifest object (has ref), chunk 1(no ref)). So, if we want to add chunk-scrub, it would be enough to add scrub operation in the chunk pool.

what we should do here is to examine that CHUNKA exists in the low tier, and then check that the manifest object A contains CHUNKA's reference. If not, we should remove the CHUNKA.

athanatos · 2021-01-08T01:59:10Z

Ok, what about the multiple reference case I mention above?

myoungwon · 2021-01-08T06:09:14Z

As your comment, there is a case as below,
Due to the same chunk in two different offsets within the object A, CHUNKA has two references of object A.
But, the object A does not be updated correctly.
Base tier: manifest object A (has chunk object CHUNKA )
Low tier: chunk object CHUNKA (has a reference of the object A and the object A)

In this case, cl_has_chunk() should return the number of references to decrease the reference count in CHUNKA.
Did you point out this case?

athanatos · 2021-01-08T06:48:57Z

Yeah, that's what I'm asking about. Do we record how many references each backref holds?

myoungwon · 2021-01-08T07:16:13Z

Done. Can you take a look?

athanatos · 2021-01-08T20:30:56Z

src/osd/PrimaryLogPG.cc

  }
 }

+int PrimaryLogPG::has_manifest_chunk(ObjectContextRef obc, std::string& fp_oid) 


get_manifest_ref_count

src/osd/PrimaryLogPG.cc

athanatos · 2021-01-12T19:04:34Z

src/osd/PrimaryLogPG.cc

+      obc_g ? &(obc_g->obs.oi.manifest) : nullptr ,
+      nullptr,
+      refs);
+    if (!refs.is_empty()) {


You don't need to do this check, the for loop before would just immediately terminate.

myoungwon · 2021-01-15T09:49:12Z

@athanatos I addressed your comment.

athanatos · 2021-01-20T23:26:29Z

Needs a rados suite run prior to merge.

cls_has_chunk does not cover snapshotted manifest object. This leads to unexpected behavior during chunk scrub. Signed-off-by: Myoungwon Oh <myoungwon.oh@samsung.com>

Signed-off-by: Myoungwon Oh <myoungwon.oh@samsung.com>

myoungwon · 2021-01-25T10:01:11Z

I just rebased this PR to run a rados suite on top of the latest master.

https://pulpito.ceph.com/myoungwon-2021-01-21_09:53:20-rados-wip-manifest-chunk-scrub-distro-basic-smithi/
https://pulpito.ceph.com/myoungwon-2021-01-24_05:30:33-rados-wip-manifest-chunk-scrub-distro-basic-smithi/

athanatos · 2021-01-25T19:19:18Z

@myoungwon There are a ton of dead and failed runs there.

myoungwon · 2021-01-26T03:04:29Z

https://pulpito.ceph.com/myoungwon-2021-01-25_12:40:07-rados-wip-manifest-chunk-scrub-distro-basic-smithi/

Hm.. The dead and fail cases are related to this PR?
It seems that all dead cases did not finish their job due to test environment as below.

2021-01-24T06:06:15.487 ERROR:teuthology.orchestra.daemon.state:Failed to send signal 1: None
Traceback (most recent call last):
File "/home/teuthworker/src/git.ceph.com_git_teuthology_master/teuthology/orchestra/daemon/state.py", line 108, in signal
self.proc.stdin.write(struct.pack('!b', sig))
File "/home/teuthworker/src/git.ceph.com_git_teuthology_master/virtualenv/lib/python3.6/site-packages/paramiko/file.py", line 405, in write
self._write_all(data)
File "/home/teuthworker/src/git.ceph.com_git_teuthology_master/virtualenv/lib/python3.6/site-packages/paramiko/file.py", line 522, in _write_all
count = self._write(data)
File "/home/teuthworker/src/git.ceph.com_git_teuthology_master/virtualenv/lib/python3.6/site-packages/paramiko/channel.py", line 1364, in _write
self.channel.sendall(data)
File "/home/teuthworker/src/git.ceph.com_git_teuthology_master/virtualenv/lib/python3.6/site-packages/paramiko/channel.py", line 846, in sendall
sent = self.send(s)
File "/home/teuthworker/src/git.ceph.com_git_teuthology_master/virtualenv/lib/python3.6/site-packages/paramiko/channel.py", line 801, in send
return self._send(s, m)
File "/home/teuthworker/src/git.ceph.com_git_teuthology_master/virtualenv/lib/python3.6/site-packages/paramiko/channel.py", line 1198, in _send
raise socket.error("Socket is closed")
OSError: Socket is closed

In addition, the failed case is already tracked by https://tracker.ceph.com/issues/48786, https://tracker.ceph.com/issues/48915.
Another failed case caused by _wctx_finish in bluestore.

tchaikov · 2021-01-29T07:28:23Z

github-actions bot added build/ops core tests labels Jan 5, 2021

myoungwon requested a review from athanatos January 6, 2021 00:34

athanatos reviewed Jan 8, 2021

View reviewed changes

myoungwon force-pushed the wip-manifest-chunk-scrub branch from c2865cc to fddc6a1 Compare January 8, 2021 01:17

myoungwon requested a review from a team as a code owner January 8, 2021 07:13

github-actions bot added the crimson label Jan 8, 2021

myoungwon removed the request for review from a team January 8, 2021 07:15

myoungwon force-pushed the wip-manifest-chunk-scrub branch 2 times, most recently from 5bc4aea to 0cc6aa0 Compare January 8, 2021 13:18

athanatos reviewed Jan 8, 2021

View reviewed changes

src/osd/PrimaryLogPG.cc Show resolved Hide resolved

myoungwon force-pushed the wip-manifest-chunk-scrub branch from 0cc6aa0 to 78ed661 Compare January 9, 2021 06:11

athanatos reviewed Jan 12, 2021

View reviewed changes

myoungwon mentioned this pull request Jan 18, 2021

src/test: fix to avoid fail notification when testing manifest refcount #38937

Merged

3 tasks

athanatos approved these changes Jan 18, 2021

View reviewed changes

athanatos added the needs-qa label Jan 20, 2021

myoungwon added 4 commits January 21, 2021 15:12

osd: add has_manifest_chunk() to check chunks in snapshot

bdcedd4

cls_has_chunk does not cover snapshotted manifest object. This leads to unexpected behavior during chunk scrub. Signed-off-by: Myoungwon Oh <myoungwon.oh@samsung.com>

test: add cls_has_chunk test to count chunk in snapshot

11b52b4

Signed-off-by: Myoungwon Oh <myoungwon.oh@samsung.com>

osd, test: return the number of references of chunked object

cb7cf5b

Signed-off-by: Myoungwon Oh <myoungwon.oh@samsung.com>

osd: remove unnecessary check

1109a59

Signed-off-by: Myoungwon Oh <myoungwon.oh@samsung.com>

myoungwon force-pushed the wip-manifest-chunk-scrub branch from a860d7f to 1109a59 Compare January 25, 2021 09:56

tchaikov added the wip-kefu-testing label Jan 26, 2021

athanatos merged commit d972c3e into ceph:master Jan 29, 2021

myoungwon mentioned this pull request Mar 2, 2021

pacific: osd, test: fix to avoid fail notification when testing manifest refcount #39773

Merged

3 tasks

Conversation

myoungwon commented Jan 5, 2021

Checklist

Uh oh!

athanatos Jan 8, 2021

Choose a reason for hiding this comment

Uh oh!

athanatos commented Jan 8, 2021

Uh oh!

myoungwon commented Jan 8, 2021

Uh oh!

athanatos commented Jan 8, 2021

Uh oh!

myoungwon commented Jan 8, 2021

Uh oh!

athanatos commented Jan 8, 2021

Uh oh!

myoungwon commented Jan 8, 2021

Uh oh!

athanatos Jan 8, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

athanatos Jan 12, 2021

Choose a reason for hiding this comment

Uh oh!

myoungwon commented Jan 15, 2021

Uh oh!

athanatos commented Jan 20, 2021

Uh oh!

myoungwon commented Jan 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

athanatos commented Jan 25, 2021

Uh oh!

myoungwon commented Jan 26, 2021

Uh oh!

tchaikov commented Jan 29, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

myoungwon commented Jan 25, 2021 •

edited

Loading