Project

General

Profile

Feature #51725

make bufferlist::c_str() skip rebuild when it isn't necessary

Added by Ilya Dryomov 5 months ago. Updated 3 months ago.

Status:
Pending Backport
Priority:
Normal
Category:
Dev Interfaces
Target version:
-
% Done:

0%

Source:
Tags:
Backport:
octopus,pacific
Reviewed:
Affected Versions:
Component(RADOS):
Pull request ID:

Description

The ceph_msg_header2 on the other hand, the bufferlist comes from the segment data, which is also contiguous, but splicing out the segment crc causes the bufferlist to be composed of two buffers, the other one being an empty carriage.
So ceph_msg_header2 always have to be rebuilt, while the preamble and the epilogue do not.

I wonder if either `splice()` or `rebuild()` should be made smarter. From a `bufferlist` user POV, a splice from the end of the contiguous buffer leading to `c_str()` ending up in `rebuild()` and actually reallocating is counter-intuitive.

Good point.
I definitely think that `c_str()` can be made a little smarter to detect more cases that do not require rebuild.
Given that, perhaps it's a better approach to always prefer `reinterpret_cast`, and use `ceph::decode` only as a fallback.
In that case, this commit [https://github.com/ceph/ceph/pull/42280] will be reverted.


Related issues

Copied to RADOS - Backport #52595: pacific: make bufferlist::c_str() skip rebuild when it isn't necessary New
Copied to RADOS - Backport #52596: octopus: make bufferlist::c_str() skip rebuild when it isn't necessary New

History

#2 Updated by Kefu Chai 5 months ago

  • Status changed from In Progress to Fix Under Review
  • Assignee set to Radoslaw Zarzynski
  • Pull request ID set to 42417

#3 Updated by Ilya Dryomov 3 months ago

  • Status changed from Fix Under Review to Pending Backport
  • Backport set to octopus,pacific

This spares a heap allocation on every received message. Marking for backporting to both octopus and pacific as a low-risk performance improvement.

#4 Updated by Backport Bot 3 months ago

  • Copied to Backport #52595: pacific: make bufferlist::c_str() skip rebuild when it isn't necessary added

#5 Updated by Backport Bot 3 months ago

  • Copied to Backport #52596: octopus: make bufferlist::c_str() skip rebuild when it isn't necessary added

Also available in: Atom PDF