New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
osd: refcount for manifest object (redirect, chunked) #19935
Conversation
d5e5e56
to
6a36122
Compare
e18f958
to
6be622a
Compare
@liewegas In the previous discussion (#15482), we has discussed the following a issue.
This PR shows the concept of refcount for manifest object.
The target object which is referenced by other objects will be deleted if its refcount is 0. Can you take a look? Please let me know if I missed something. |
src/cls/refcount/cls_refcount.cc
Outdated
|
||
struct chunk_obj_refcount { | ||
map<chunk_refcount, bool> refs; | ||
set<chunk_refcount> retired_refs; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it doesn't seem like retired_refs is serving any purpose?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is my mistake. I will fix this.
src/cls/refcount/cls_refcount_ops.h
Outdated
@@ -125,5 +124,149 @@ struct cls_refcount_read_ret { | |||
}; | |||
WRITE_CLASS_ENCODER(cls_refcount_read_ret) | |||
|
|||
struct chunk_refcount { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
src/cls/refcount/cls_refcount.cc
Outdated
#define CHUNK_REFCOUNT_ATTR "chunk_refcount" | ||
|
||
struct chunk_obj_refcount { | ||
map<chunk_refcount, bool> refs; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/chunk_refcount/hobject_t/
src/osd/PrimaryLogPG.cc
Outdated
RefCountCallback *fin = new RefCountCallback( | ||
this, ctx, osd_op, get_last_peering_reset()); | ||
refcount_manifest(ctx->obc, target_oloc, target, SnapContext(), | ||
"chunk_get", fin, 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we really want to set ref counts for a vanilla redirect?
src/osd/PrimaryLogPG.cc
Outdated
object_locator_t target_oloc(oi.manifest.redirect_target); | ||
refcount_manifest(ctx->obc, target_oloc, oi.manifest.redirect_target, | ||
SnapContext(), "chunk_put", NULL, 0); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right. I will fix this as your comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like this shouldn't be based on need_reference but instead on just (oi.flags & object_info_t::FLAG_REDIRECT_HAS_REFERENCE)
@liewegas We need to discuss two issues.
|
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:
I lean toward 2... |
Note that hobject_t includes the pool id: https://github.com/ceph/ceph/blob/master/src/common/hobject.h#L49 |
@liewegas I think there are two types of flag.
|
I think just hobject_t is sufficient |
184d6bd
to
2c6011c
Compare
@liewegas I reworked existing commits as your comments. please review it again. |
src/cls/refcount/cls_refcount.cc
Outdated
#define CHUNK_REFCOUNT_ATTR "chunk_refcount" | ||
|
||
struct chunk_obj_refcount { | ||
map<hobject_t, bool> refs; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
set<hobject_t> ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
src/osd/osd_types.h
Outdated
@@ -4658,6 +4662,7 @@ struct object_info_t { | |||
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FLAG_REDIRECT_HAS_REFERENCE ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
src/osd/PrimaryLogPG.cc
Outdated
@@ -3303,6 +3304,34 @@ void PrimaryLogPG::do_proxy_chunked_op(OpRequestRef op, const hobject_t& missing | |||
} | |||
} | |||
|
|||
void PrimaryLogPG::refcount_manifest(ObjectContextRef obc, object_locator_t oloc, hobject_t soid, | |||
SnapContext snapc, string method, Context *cb, uint64_t offset) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/string method/bool get/ so we can avoid a string comparison here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
src/osd/PrimaryLogPG.cc
Outdated
} | ||
} else { | ||
cls_chunk_refcount_read_ret read_ret; | ||
int ref = read_refcount_manifest(ctx, read_ret); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right. Fixed.
b372439
to
a5517af
Compare
@liewegas I reworked this PR based on your recent comments. Any other comments? |
(Also, needs rebase!) |
f1a0b9b
to
2b0f376
Compare
@liewegas Your suggestion is make sense to me. I address all your comments. Please review again. |
@liewegas Ping. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
2b0f376
to
1f6697e
Compare
Rebase is done (I just resolved a conflict). |
hmm looks like there's a new conflict? that test result shoudl be fine! |
64248c0
to
1f6697e
Compare
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>
Signed-off-by: Myoungwon Oh <omwmw@sk.com>
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>
Signed-off-by: Myoungwon Oh <omwmw@sk.com>
1f6697e
to
5d67bb9
Compare
@liewegas A conflict is resolved. |
This PR introduces refcount operations for redirect and chunked object.
(please refer to #19294, #15482)
Signed-off-by: Myoungwon Oh omwmw@sk.com