osd: refcount for manifest object (redirect, chunked) by myoungwon · Pull Request #19935 · ceph/ceph

myoungwon · 2018-01-12T12:51:29Z

This PR introduces refcount operations for redirect and chunked object.
(please refer to #19294, #15482)

Signed-off-by: Myoungwon Oh omwmw@sk.com

myoungwon · 2018-02-22T16:59:43Z

@liewegas In the previous discussion (#15482), we has discussed the following a issue.

how to handle ref cleanup (e.g., when we promote or when we delete the object with the manifest)

This PR shows the concept of refcount for manifest object.
The target object have chuck_refcount{source_oid, pool_id, source_offset}
and chunk_refcount is handled as the following example.

set_chunk or set_redirect is sent
increase target object's refcount (add chunk_refcount)
a. send set_chunk or set_redirect to the target object
b. (if old reference exists) decrease target object's refcount first and then, send set_chunk or set_redirect to the target object

The target object which is referenced by other objects will be deleted if its refcount is 0.
Also, the message for decreasing refcount will be sent before the redirected or chunked object is removed,

Can you take a look? Please let me know if I missed something.

liewegas · 2018-02-22T19:54:53Z

src/cls/refcount/cls_refcount.cc

+
+struct chunk_obj_refcount {
+  map<chunk_refcount, bool> refs;
+  set<chunk_refcount> retired_refs;


it doesn't seem like retired_refs is serving any purpose?

This is my mistake. I will fix this.

liewegas · 2018-02-22T19:59:41Z

src/cls/refcount/cls_refcount_ops.h

 };
 WRITE_CLASS_ENCODER(cls_refcount_read_ret)

+struct chunk_refcount {


i would drop this type and just use hobject_t. i don't think we need to know which part of the referring object is pointing at this chunk, and we're missing the hash value here, which would be useful for a space-efficient coarse-grained ref map.

liewegas · 2018-02-22T19:59:50Z

src/cls/refcount/cls_refcount.cc

+#define CHUNK_REFCOUNT_ATTR "chunk_refcount"
+
+struct chunk_obj_refcount {
+  map<chunk_refcount, bool> refs;


s/chunk_refcount/hobject_t/

liewegas · 2018-02-22T22:44:36Z

src/osd/PrimaryLogPG.cc

+	  RefCountCallback *fin = new RefCountCallback(
+	    this, ctx, osd_op, get_last_peering_reset());
+	  refcount_manifest(ctx->obc, target_oloc, target, SnapContext(),
+			    "chunk_get", fin, 0);


do we really want to set ref counts for a vanilla redirect?

liewegas · 2018-02-22T22:45:59Z

src/osd/PrimaryLogPG.cc

+	    object_locator_t target_oloc(oi.manifest.redirect_target);
+	    refcount_manifest(ctx->obc, target_oloc, oi.manifest.redirect_target, 
+			      SnapContext(), "chunk_put", NULL, 0);
+	  }


I think this has to happen in a context after the local operation commits. otherwise we might crash, failing to commit locally, but still decrement the ref on the chunk.

Right. I will fix this as your comment.

It seems like this shouldn't be based on need_reference but instead on just (oi.flags & object_info_t::FLAG_REDIRECT_HAS_REFERENCE)

myoungwon · 2018-02-23T08:00:05Z

@liewegas We need to discuss two issues.

chunk_refcount or hobject_t ?
At this point, hobject_t is reasonable. But, what about future direction? I think this depends on the policy. If we need to consider that multiple objects in different pools indicate a single chunk, pool_id is needed. Because the different pools can have a same object id. Also, in the case of source_offset, different offset in a single object can have the same contents. For example, offset 0 and offset 1024 in the object A can have a same chunk. So, we might need source_offset (but, this is only for dedup tier).
Do we really want to set ref counts for a vanilla redirect?
please look at following example.
object A -(redirect)-> object X
object B -(redirect)-> object X
Do we allow this? In this case, if object A receives a delete message, the message is forwarded to object X. So, I think we need to prevent deleting object X by ref count.

liewegas · 2018-02-23T12:26:03Z

I think we might have a more general question to answer: how/when do we know from a given object_info_t manifest that we own references to the target object that we need to increment/decrement. I see a few options:

Decide that chunk manifests always own refs, redirects always do not, or similar. This seems limiting because you might want to dedup on entire objects where a redirect is sufficient. Or, in many/most cases, you might use a redirect without wanting to refcount at all (e.g., maybe the target pool is read-only).
Have a flag in the object_info_t indicating whether we own a ref. This would work for redirect, but for the chunk map, we might own refs for some of them and not all? I forget. This reminds me of my confusion when we had an hobject_t for the target but the target was possibly stale or out of date. Perhaps we want a per-chunk flag in the manifest for whether we own a ref?
Have refcounting (yes or no) be a property of the target pool somehow.

I lean toward 2...

liewegas · 2018-02-23T14:18:47Z

Note that hobject_t includes the pool id: https://github.com/ceph/ceph/blob/master/src/common/hobject.h#L49

myoungwon · 2018-02-26T08:15:38Z

@liewegas I think there are two types of flag.
First, enable/disable counting reference. Second, indicate whether it own a ref (but, refcouting is enabled)
Which one is you intend for?
Anyway, a flag (a per-chunk flag in the case of chunked object) in the object_info_t is reasonable to me.

chunk_refcount or hobject_t ?
As you mentioned, pool_id already is included in hobject_t. So it doesn't need to be included. What do you think about source_offset?
struct refcount {hobject_t; uint64_t source_offset} or hobject_t?

liewegas · 2018-02-26T15:17:58Z

I think just hobject_t is sufficient

myoungwon · 2018-02-28T10:48:53Z

@liewegas I reworked existing commits as your comments. please review it again.

liewegas · 2018-03-05T22:57:48Z

src/cls/refcount/cls_refcount.cc

+#define CHUNK_REFCOUNT_ATTR "chunk_refcount"
+
+struct chunk_obj_refcount {
+  map<hobject_t, bool> refs;


set<hobject_t> ?

liewegas · 2018-03-05T22:59:37Z

src/osd/osd_types.h

    FLAG_CACHE_PIN   = 1<<6, // pin the object in cache tier
    FLAG_MANIFEST    = 1<<7, // has manifest
    FLAG_USES_TMAP   = 1<<8, // deprecated; no longer used
+    FLAG_HAS_REFERENCE = 1<<9, // has reference


FLAG_REDIRECT_HAS_REFERENCE ?

liewegas · 2018-03-05T23:00:25Z

src/osd/PrimaryLogPG.cc

 }

+void PrimaryLogPG::refcount_manifest(ObjectContextRef obc, object_locator_t oloc, hobject_t soid,
+                                     SnapContext snapc, string method, Context *cb, uint64_t offset)


s/string method/bool get/ so we can avoid a string comparison here

liewegas · 2018-03-05T23:06:33Z

src/osd/PrimaryLogPG.cc

+	  } 
+	} else {
+	  cls_chunk_refcount_read_ret read_ret;
+	  int ref = read_refcount_manifest(ctx, read_ret);


Hmm, I don't think we should interfere with delete here. If someone goes and sends delete ops to a rados pool with refcounted objects that's their mistake.

If we're worried about it, perhaps a pool property that disallows delete ops, or caps that only allow read operations and the refcount cls.

You are right. Fixed.

myoungwon · 2018-03-06T14:30:29Z

@liewegas I reworked this PR based on your recent comments. Any other comments?

liewegas · 2018-03-17T16:38:25Z

(Also, needs rebase!)

myoungwon · 2018-03-19T10:26:51Z

@liewegas Your suggestion is make sense to me. I address all your comments. Please review again.
And I will make a new PR regarding to unset operation for manifest object.

myoungwon · 2018-04-09T13:17:28Z

@liewegas Ping.

liewegas

lgtm!

myoungwon · 2018-04-13T07:18:26Z

Rebase is done (I just resolved a conflict).
Test result ( http://pulpito.ceph.com/myoungwon-2018-04-13_04:45:30-rados:thrash-wip-manifest-ref-count-distro-basic-smithi/ ).
Do I need more rados test results for this PR?

liewegas · 2018-04-13T15:54:30Z

hmm looks like there's a new conflict? that test result shoudl be fine!

refcount() for chunked object is added (based on source id, pool id and offset) Signed-off-by: Myoungwon Oh <omwmw@sk.com>

Signed-off-by: Myoungwon Oh <omwmw@sk.com>

…bject is referenced Signed-off-by: Myoungwon Oh <omwmw@sk.com>

Signed-off-by: Myoungwon Oh <omwmw@sk.com>

myoungwon · 2018-04-13T18:37:51Z

@liewegas A conflict is resolved.

tchaikov · 2018-04-15T14:51:08Z

the failure is addressed by #21433

myoungwon added feature core labels Jan 12, 2018

myoungwon force-pushed the wip-manifest-ref-count branch 2 times, most recently from d5e5e56 to 6a36122 Compare January 15, 2018 11:29

myoungwon force-pushed the wip-manifest-ref-count branch 3 times, most recently from e18f958 to 6be622a Compare February 22, 2018 16:19

liewegas self-requested a review February 22, 2018 17:50

liewegas reviewed Feb 22, 2018

View reviewed changes

myoungwon force-pushed the wip-manifest-ref-count branch 2 times, most recently from 184d6bd to 2c6011c Compare February 28, 2018 03:04

myoungwon requested a review from liewegas March 1, 2018 15:01

liewegas reviewed Mar 5, 2018

View reviewed changes

myoungwon force-pushed the wip-manifest-ref-count branch 2 times, most recently from b372439 to a5517af Compare March 6, 2018 14:03

myoungwon force-pushed the wip-manifest-ref-count branch 3 times, most recently from f1a0b9b to 2b0f376 Compare March 19, 2018 09:39

myoungwon changed the title ~~WIP: osd: refcount for manifest object (redirect, chunked)~~ osd: refcount for manifest object (redirect, chunked) Mar 19, 2018

myoungwon requested a review from liewegas April 2, 2018 11:20

liewegas approved these changes Apr 9, 2018

View reviewed changes

liewegas added the needs-qa label Apr 9, 2018

myoungwon force-pushed the wip-manifest-ref-count branch from 2b0f376 to 1f6697e Compare April 13, 2018 02:27

myoungwon force-pushed the wip-manifest-ref-count branch from 64248c0 to 1f6697e Compare April 13, 2018 17:21

myoungwon added 8 commits April 14, 2018 02:33

cls/refcount: add chunk_refcount

70828e1

refcount() for chunked object is added (based on source id, pool id and offset) Signed-off-by: Myoungwon Oh <omwmw@sk.com>

librados: add with_reference flag when redirect or chunk is enabled

12feefc

Signed-off-by: Myoungwon Oh <omwmw@sk.com>

osd/osd_types.h: add HAS_REFERENCE flag

22b54c5

Signed-off-by: Myoungwon Oh <omwmw@sk.com>

osd: add refcount_manifest() for the object that has manifest

6d1948c

Signed-off-by: Myoungwon Oh <omwmw@sk.com>

osd: increase refcount if redirect or chunked object is enabled

cd60537

Signed-off-by: Myoungwon Oh <omwmw@sk.com>

osd: decrease refcount when object is deleted (has manifest) if the o…

19b5703

…bject is referenced Signed-off-by: Myoungwon Oh <omwmw@sk.com>

src/test: add a unit test for manifest refcount

3ff9bd4

Signed-off-by: Myoungwon Oh <omwmw@sk.com>

cls/refcount: add chunk_refcount_client()

5d67bb9

Signed-off-by: Myoungwon Oh <omwmw@sk.com>

myoungwon force-pushed the wip-manifest-ref-count branch from 1f6697e to 5d67bb9 Compare April 13, 2018 17:37

liewegas added this to the mimic milestone Apr 13, 2018

tchaikov added the wip-kefu-testing label Apr 14, 2018

tchaikov merged commit 7720ef5 into ceph:master Apr 15, 2018

myoungwon mentioned this pull request May 15, 2018

osd, librados: add unset-manifest op #21999

Merged

tchaikov mentioned this pull request Sep 10, 2018

luminous: deep scrub cannot find the bitrot if the object is cached #23980

Closed

Conversation

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

Uh oh!

Uh oh!

myoungwon commented Feb 22, 2018

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!

myoungwon commented Feb 23, 2018

Uh oh!

liewegas commented Feb 23, 2018

Uh oh!

liewegas commented Feb 23, 2018

Uh oh!

myoungwon commented Feb 26, 2018

Uh oh!

liewegas commented Feb 26, 2018

Uh oh!

myoungwon commented Feb 28, 2018

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!

myoungwon commented Mar 6, 2018

Uh oh!

liewegas commented Mar 17, 2018

Uh oh!

myoungwon commented Mar 19, 2018

Uh oh!

myoungwon commented Apr 9, 2018

Uh oh!

liewegas left a comment

Choose a reason for hiding this comment

Uh oh!

myoungwon commented Apr 13, 2018

Uh oh!

liewegas commented Apr 13, 2018

Uh oh!

myoungwon commented Apr 13, 2018

Uh oh!

tchaikov commented Apr 15, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

myoungwon commented Jan 12, 2018 •

edited

Loading