Project

General

Profile

Bug #51725

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

Added by Ilya Dryomov over 2 years ago. Updated about 24 hours ago.

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

100%

Source:
Development
Tags:
backport_processed
Backport:
octopus,pacific
Regression:
No
Severity:
3 - minor
Reviewed:
Affected Versions:
ceph-qa-suite:
Component(RADOS):
Pull request ID:
Crash signature (v1):
Crash signature (v2):

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 Rejected
Copied to RADOS - Backport #52596: octopus: make bufferlist::c_str() skip rebuild when it isn't necessary Rejected

History

#2 Updated by Kefu Chai over 2 years 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 over 2 years 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 over 2 years ago

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

#5 Updated by Backport Bot over 2 years ago

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

#6 Updated by Backport Bot over 1 year ago

  • Tags set to backport_processed

#7 Updated by Konstantin Shalygin about 24 hours ago

  • Tracker changed from Feature to Bug
  • Status changed from Pending Backport to Resolved
  • % Done changed from 0 to 100
  • Source set to Development
  • Regression set to No
  • Severity set to 3 - minor

Also available in: Atom PDF