osd: recalculate coll_t::_str during decode() to fix stale values#63938
osd: recalculate coll_t::_str during decode() to fix stale values#63938
Conversation
|
@ceph/core hi core maintainers , could you please help review this change? |
3 similar comments
|
@ceph/core hi core maintainers , could you please help review this change? |
|
@ceph/core hi core maintainers , could you please help review this change? |
|
@ceph/core hi core maintainers , could you please help review this change? |
|
This sounds really bad. Was it not happening often in practice? (I'm confused because there's no ticket attached to it.) What was the real consequence of these values being uninitialized here? It looks like this may be a common problem in the compatibility checker code. Is the referenced patch to change to newly-allocated instances available? |
Fix compatibility issue where coll_t::_str retained stale values after decoding v1/v2 format blobs, causing confusing debug output and tool messages. The _str field was not recalculated when decoding older formats: - v1 format (pre-Ceph v0.21, before commit a108774) - v2 format (meta/regular PGs) - v3 format always includes _str (temp PGs) This primarily affected debugging scenarios since _str is only used for log messages and BlueStore low-level tool output. The issue went undetected because existing tests reused struct instances, preserving field values across encode/decode cycles. An upcoming test change will allocate fresh instances for each decode, which would expose these stale values. Since _str can be derived from always-encoded fields, recalculate it during decode() when missing from the encoded data. Signed-off-by: Kefu Chai <tchaikov@gmail.com>
050d02d to
d01003e
Compare
|
@gregsfortytwo Hi Greg, thanks for reviewing this change. Instead of recalculating the non-persisted field in
It does happen in practice, but it doesn't cause functional issues if we don't rely on comparing logging messages with actual state. See #64591 for more details on the impact of the uninitialized
Yes, it's available in #63910. |
|
jenkins test make check |
|
@ceph/core hi core maintainers , could you please help review this change? |
|
We found an issue with another PR in the batch. I'm having @SrinivasaBharath rebuild and retest to unblock this one. Please stand by... Testing ref: https://tracker.ceph.com/issues/72299 |
Fix compatibility issue where
coll_t::_strretained stale values afterdecoding v1/v2 format blobs, causing confusing debug output and tool
messages.
The
_strfield was not recalculated when decoding older formats:_str(temp PGs)This primarily affected debugging scenarios since _str is only used for
log messages and BlueStore low-level tool output. The issue went
undetected because existing tests reused struct instances, preserving
field values across encode/decode cycles.
An upcoming test change will allocate fresh instances for each decode, which would expose these stale values. Since
_strcan be derived from always-encoded fields, recalculate it duringdecode()when missing from the encoded data.Contribution Guidelines
To sign and title your commits, please refer to Submitting Patches to Ceph.
If you are submitting a fix for a stable branch (e.g. "quincy"), please refer to Submitting Patches to Ceph - Backports for the proper workflow.
When filling out the below checklist, you may click boxes directly in the GitHub web UI. When entering or editing the entire PR message in the GitHub web UI editor, you may also select a checklist item by adding an
xbetween the brackets:[x]. Spaces and capitalization matter when checking off items this way.Checklist
Show available Jenkins commands
jenkins test classic perfJenkins Job | Jenkins Job Definitionjenkins test crimson perfJenkins Job | Jenkins Job Definitionjenkins test signedJenkins Job | Jenkins Job Definitionjenkins test make checkJenkins Job | Jenkins Job Definitionjenkins test make check arm64Jenkins Job | Jenkins Job Definitionjenkins test submodulesJenkins Job | Jenkins Job Definitionjenkins test dashboardJenkins Job | Jenkins Job Definitionjenkins test dashboard cephadmJenkins Job | Jenkins Job Definitionjenkins test apiJenkins Job | Jenkins Job Definitionjenkins test docsReadTheDocs | Github Workflow Definitionjenkins test ceph-volume allJenkins Jobs | Jenkins Jobs Definitionjenkins test windowsJenkins Job | Jenkins Job Definitionjenkins test rook e2eJenkins Job | Jenkins Job Definition