test: fix TierFlushDuringFlush to wait until dedup_tier is set on base pool by ssdohammer-sl · Pull Request #45035 · ceph/ceph

ssdohammer-sl · 2022-02-15T01:52:04Z

When start_dedup() is called while the base pool is not set the dedup_tier,
it is not possible to know the target pool of the chunk object.

User set the dedup_tier on a base pool by mon_command().
User issues tier_flush on the object which has a manifest (base pool)
before the dedup_tier is applied on the base pool.
OSD calls start_dedup() to flush the chunk objects to chunk pool.
OSD calls get_dedup_tier() to get the chunk pool of the base pool,
but it is not possible to know the chunk pool.
get_dedup_tier() returns 0 because it is not applied on the base pool yet.
This makes refcount_manifest() lost it's way to chunk pool.

To prevent this issue, start_dedup() has to be called after dedup_tier is set
on the base pool. To do so, this commit prohibits getting chunk pool id if
dedup_tier is not set.

Fixes: http://tracker.ceph.com/issues/53855

Signed-off-by: Sungmin Lee sung_min.lee@samsung.com

Checklist

References tracker ticket
No impact that needs to be tracked
Includes bug reproducer

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

ljflores · 2022-02-15T22:55:29Z

jenkins test api

ljflores · 2022-02-15T22:55:37Z

jenkins test make check

src/osd/PrimaryLogPG.cc

athanatos · 2022-02-18T02:59:55Z

src/osd/PrimaryLogPG.cc

+  // check if dedup_tier isn't set
+  if (oloc.pool <= 0) {
+    dout(10) << __func__ << " failed to get  pool info of the chunk object" << dendl;
+    return hobject_t();


The caller doesn't check for hobject_t() as a return value. The assert here was fine, but start_dedup needs to check for the validity of oloc.pool and return an error that will actually be propagated to the client from the CEPH_OSD_OP_CACHE_FLUSH case. We also need a test for that case to confirm that the correct error gets propagated.

Returning an hobject_t() is not a valid way to signal an error. get_fpoid_from_chunk() should not be called in the first place if the dedup tier isn't set. Please check this in start_dedup prior to calling get_fpoid_from_chunk and put the assert back.

Thank you for further explanation. I misunderstood what you intended.
Changed to check dedup_tier is set in start_dedup and put assert back in get_fpoid_from_chunk.

myoungwon · 2022-02-18T08:19:23Z

src/osd/PrimaryLogPG.cc

 	    result = -EAGAIN;
+    else if (result == -EINVAL)
+      goto fail;
 	} else {


I don't think this is needed here because op is CEPH_OSD_OP_CACHE_FLUSH---not related to dedup work.

src/osd/PrimaryLogPG.cc

athanatos · 2022-02-23T21:30:56Z

src/osd/PrimaryLogPG.cc

  for (auto p : chunks) {
    hobject_t target = mop->new_manifest.chunk_map[p.first].oid;
    if (refs.find(target) == refs.end()) {
+      if (target.get_hash() == 0) {


This check is incorrect, 1 in 1<<32 objects will have a hash of 0.

You're right. Thank you for explanation.
It was a depreciated check which was intended to handle former bug fix before you commented.
So, I should have removed this code in advance.
Sorry for confusing you.

myoungwon · 2022-02-24T00:55:38Z

src/osd/PrimaryLogPG.cc

    if (refs.find(target) == refs.end()) {
+      if (target.get_hash() == 0) {
+        return -EINVAL;
+      }


This change is not necessary. What is the purpose of this?

This change should have been removed in advance, but it was my mistake.
Sorry for confusing you.

…e pool When start_dedup() is called while the base pool is not set the dedup_tier, it is not possible to know the target pool of the chunk object. 1. User set the dedup_tier on a base pool by mon_command(). 2. User issues tier_flush on the object which has a manifest (base pool) before the dedup_tier is applied on the base pool. 3. OSD calls start_dedup() to flush the chunk objects to chunk pool. 4. OSD calls get_dedup_tier() to get the chunk pool of the base pool, but it is not possible to know the chunk pool. 5. get_dedup_tier() returns 0 because it is not applied on the base pool yet. 6. This makes refcount_manifest() lost it's way to chunk pool. To prevent this issue, start_dedup() has to be called after dedup_tier is set on the base pool. To do so, this commit prohibits getting chunk pool id if dedup_tier is not set. Fixes: http://tracker.ceph.com/issues/53855 Signed-off-by: Sungmin Lee <sung_min.lee@samsung.com>

athanatos · 2022-02-24T04:24:03Z

retest this please

athanatos · 2022-02-24T04:25:12Z

Ok, that looks reasonable. Thanks for taking the time to track down and fix it!

myoungwon added bug-fix tests labels Feb 15, 2022

ljflores added needs-review wip-lflores-testing labels Feb 15, 2022

neha-ojha requested review from athanatos and neha-ojha February 16, 2022 18:32

athanatos reviewed Feb 16, 2022

View reviewed changes

src/osd/PrimaryLogPG.cc Show resolved Hide resolved

ssdohammer-sl force-pushed the bug-fix-TierFlushDuringFlush branch from 96c790f to 6932880 Compare February 18, 2022 02:49

github-actions bot added the core label Feb 18, 2022

athanatos reviewed Feb 18, 2022

View reviewed changes

ssdohammer-sl force-pushed the bug-fix-TierFlushDuringFlush branch from 6932880 to 4f6f1e3 Compare February 18, 2022 08:05

myoungwon reviewed Feb 18, 2022

View reviewed changes

ssdohammer-sl force-pushed the bug-fix-TierFlushDuringFlush branch 2 times, most recently from efca14b to 6e5a8c7 Compare February 22, 2022 02:33

ljflores removed the wip-lflores-testing label Feb 23, 2022

ljflores requested review from athanatos and myoungwon February 23, 2022 15:35

athanatos reviewed Feb 23, 2022

View reviewed changes

src/osd/PrimaryLogPG.cc Outdated Show resolved Hide resolved

athanatos reviewed Feb 23, 2022

View reviewed changes

myoungwon reviewed Feb 24, 2022

View reviewed changes

ssdohammer-sl force-pushed the bug-fix-TierFlushDuringFlush branch from 6e5a8c7 to 66ad91e Compare February 24, 2022 01:40

athanatos self-requested a review February 24, 2022 04:24

athanatos approved these changes Feb 24, 2022

View reviewed changes

athanatos added the needs-qa label Feb 24, 2022

myoungwon approved these changes Feb 24, 2022

View reviewed changes

myoungwon removed the needs-review label Feb 24, 2022

neha-ojha added the wip-yuri4-testing label Mar 1, 2022

yuriw merged commit 1f703bc into ceph:master Mar 3, 2022

ljflores mentioned this pull request Mar 24, 2022

quincy: test: fix TierFlushDuringFlush to wait until dedup_tier is set on base pool #45624

Merged

myoungwon mentioned this pull request Jun 20, 2022

pacific: test: fix TierFlushDuringFlush to wait until dedup_tier is set on bas… #46748

Merged

14 tasks

ssdohammer-sl deleted the bug-fix-TierFlushDuringFlush branch May 4, 2023 00:43

Conversation

ssdohammer-sl commented Feb 15, 2022 • edited by ljflores Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

ljflores commented Feb 15, 2022

Uh oh!

ljflores commented Feb 15, 2022

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!

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!

athanatos commented Feb 24, 2022

Uh oh!

athanatos commented Feb 24, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ssdohammer-sl commented Feb 15, 2022 •

edited by ljflores

Loading