fix(osdtrace): make the ops map an LRU hash so leaked entries can't stop tracing - #192
Draft
ray5273 wants to merge 2 commits into
Draft
fix(osdtrace): make the ops map an LRU hash so leaked entries can't stop tracing#192ray5273 wants to merge 2 commits into
ray5273 wants to merge 2 commits into
Conversation
…fails uprobe_log_subop_stats and uprobe_repop_commit returned early when reading the write length failed after the ops lookup had already succeeded, skipping the bpf_map_delete_elem(&ops, &key) that follows the submit. The sub-op has completed by then, so the entry is dead either way and nothing will ever delete it. Delete the entry on that failure path too, as taodd#188 did for the ring buffer reserve failure path. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RawRtYaKZCaZZQMvsDyWV7
uprobe_enqueue_op inserts every client op and replica sub-op into the ops
map, and only the completion probes (log_op_stats, log_subop_stats,
repop_commit, ec_submit_transaction) delete them. Several ordinary OSD
paths finish an op without reaching any of those probes:
- reads that fail: PrimaryLogPG::complete_read_ctx only calls
log_op_stats for result >= 0, so every -ENOENT read leaks
- writes that fail (record_write_error, then return)
- the reply_op_error paths in do_op and friends
- dup/resent ops answered from the pg log (already_complete)
- ops dropped by can_discard_request
Client tids are monotonic, so the orphan handling in uprobe_enqueue_op
never sees these keys again. With the plain 8192-entry hash the leaked
entries accumulate until bpf_map_update_elem fails with -E2BIG for every
new op; from then on no op is tracked, every completion lookup misses, and
osdtrace silently stops emitting events while still looking healthy. A
workload with a steady stream of stats/reads on missing objects gets
there in seconds.
Switch the map to BPF_MAP_TYPE_LRU_HASH so a full map evicts instead of
rejecting inserts. Lookups set the LRU reference bit, so ops that are
still moving through the probes are preferred over leaked entries that
are never touched again. The trade-off is that an op left untouched long
enough while the map is under pressure (e.g. still waiting in the op
queue) can be evicted and go missing from the output, and eviction can
begin before max_entries is reached because of the per-CPU free lists.
Plugging the leaking paths themselves is still worthwhile; this change
makes sure the ones we have not found cannot take the tracer down.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RawRtYaKZCaZZQMvsDyWV7
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
In full mode (the default), osdtrace can silently stop emitting events on a live OSD and never recover until it is restarted. The
opsmap is a plain 8192-entry hash, and several ordinary OSD code paths finish an op without reaching any of the completion probes that delete its entry. Once enough entries have leaked,bpf_map_update_elem()inuprobe_enqueue_opfails with-E2BIGfor every new op. No op is tracked after that, every completion lookup misses, and the output goes quiet while the process still looks healthy.This PR:
uprobe_log_subop_statsanduprobe_repop_commitreturned early on a failed length read after the lookup had succeeded, skipping the delete. This is the same pattern perf(osdtrace): submit ring buffer events with BPF_RB_NO_WAKEUP #188 fixed for the ring buffer reserve failure path.opstoBPF_MAP_TYPE_LRU_HASH, so a full map evicts entries instead of rejecting inserts. The OSD-side leaks listed below are not something the BPF program can see, so this keeps them from taking the tracer down.Where the entries leak
uprobe_enqueue_opinserts everyMSG_OSD_OP/MSG_OSD_REPOP. The entry is only deleted bylog_op_stats,log_subop_stats,repop_commitorec_submit_transaction. From the Ceph (squid) source, these paths finish an op without reaching any of them:-ENOENTstat/read on a missing object)PrimaryLogPG::complete_read_ctxonly callslog_op_statswhenresult >= 0execute_ctx→record_write_error()→close_op_ctx()→returnreply_op_error()call sites indo_opand friends (-EINVAL,-EPERM,-EBLOCKLISTED,-EAGAIN, ...)do_op:already_complete(version)→reply_op_error(...)do_op:can_discard_request(op)→returnClient tids are monotonic, so the orphan handling added in
uprobe_enqueue_opnever sees these keys again. A workload with a steady stream of reads or stats on missing objects (common with RGW and RBD) fills 8192 entries within seconds.Why LRU, and what it costs
(pid, owner, tid), and ops do not complete in insertion order.dequeue_op,execute_ctx,mark_flag_point, ...) keep getting referenced. Leaked entries are never touched again after insert, so they are evicted first.target_free = clamp(max_entries / ncpus / 2, 1, 128)), eviction can also begin beforemax_entriesis reached. Losing some trace lines under pressure seemed clearly preferable to losing all of them permanently.max_entriesis unchanged (8192). The map is shared by every OSD traced by one osdtrace process.Validation
Environment:
Reproducing the stall on main (full mode)
Normal workload still traced correctly
rados benchwrite / rand-read: rows emitted and well-formed on this PR (tests/lib/verify-trace-output.sh)Performance
uprobe_enqueue_opuprobe_dequeue_opuprobe_log_op_statsuprobe_repop_commitClient IOPS: main ______ / this PR ______
Notes / follow-ups
opsmap is also a plainBPF_MAP_TYPE_HASH; the same reasoning may apply there and can be handled separately.🤖 Generated with Claude Code
https://claude.ai/code/session_01RawRtYaKZCaZZQMvsDyWV7