-
Notifications
You must be signed in to change notification settings - Fork 61
Fix #1638: double CRC32C computation in RBD layer #1712
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
base: devel
Are you sure you want to change the base?
Conversation
- Add skip_rbd_crc_if_transport_digest config option to disable redundant RBD CRC computation - Implement config-driven logic in GatewayServer._initialize_rbd_crc32c() - Add unit tests validating all config scenarios Signed-off-by: Annmool <aydv.267@gmail.com>
ead43c9 to
dd5e8e7
Compare
|
|
||
| effective_rbd_crc = bool(rbd_with_crc32c) | ||
| if skip_if_transport_digest: | ||
| effective_rbd_crc = False |
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 seems to be adding a new config option (skip_rbd_crc_if_transport_digest) whose sole purpose is to override the existing config option (rbd_with_crc32c) in case it's set to true. Is there anything that is preventing the operator from manipulating rbd_with_crc32c option directly?
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 also don't understand what is the idea of this PR given that #1429 is handling the same issue exactly and is already merged.
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.
@idryomov You make a valid point. Looking at the current behavior, rbd_with_crc32c already defaults to False, so operators who want to skip RBD CRC computation can simply leave it unset or explicitly set it to False. The new skip_rbd_crc_if_transport_digest option is somewhat redundant from a functional perspective.
what we can do is
- Remove the new
skip_rbd_crc_if_transport_digestoption - Just rely on rbd_with_crc32c = False (already the default)
- Update documentation to clarify: operators who understand the optimization can disable RBD CRC by not enabling it
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.
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.
ok so I guess it need to be closed right?
Fix #1638
Problem: The NVMe-oF transport layer and RBD block device layer were both computing CRC32C checksums on the same data, wasting CPU cycles.
Solution
Config-Driven Optimization (server.py)
Modified
_initialize_rbd_crc32c()method to read a new config option:skip_rbd_crc_if_transport_digestWhen enabled, this flag disables RBD-side CRC32C computation entirely
The logic respects explicit
rbd_with_crc32csettings but allows the skip flag to override them for safetyAdded informative logging so operators know when the optimization is active
Configuration Option (ceph-nvmeof.conf)
Added
skip_rbd_crc_if_transport_digest = True/Falseoption under the [spdk] sectionIncluded safety warnings in comments explaining when to enable (trusted stacks only, no data transformations)
Result: Operators can now eliminate redundant CRC32C recomputation by setting one config flag, reducing CPU overhead on write-heavy workloads while maintaining safety through explicit opt-in and clear warnings.