os/bluestore: implement object content recompression/defragmentation when scrubbing #57631
Conversation
| @@ -11776,11 +11841,131 @@ int BlueStore::read( | |||
| if (offset == length && offset == 0) | |||
| length = o->onode.size; | |||
|
|
|||
| r = _do_read(c, o, offset, length, bl, op_flags); | |||
| r = _do_read(c, o, offset, length, bl, op_flags, 0, | |||
| might_need_reformatting ? &span_stat : nullptr); | |||
There was a problem hiding this comment.
If we can potentially defrag on any read, it could be interesting once the extents are read off disk to make a decision about whether or not an object is fragmented enough to warrant defragmentation on-the-fly.
There was a problem hiding this comment.
@markhpc I am excited to see it. Opens interesting paths.
|
I'm not sure "data_layout_reformatting" is very clear to the users what it is doing. Maybe instead use "optimize_during_scrub" with options "defrag", "recompress", "all", ... The second option is flexible if we want to allow additional optimizations or modes in the future. |
src/mon/OSDMonitor.cc
Outdated
| @@ -6156,7 +6157,8 @@ bool OSDMonitor::preprocess_command(MonOpRequestRef op) | |||
| {"dedup_chunk_algorithm", DEDUP_CHUNK_ALGORITHM}, | |||
| {"dedup_cdc_chunk_size", DEDUP_CDC_CHUNK_SIZE}, | |||
| {"bulk", BULK}, | |||
| {"read_ratio", READ_RATIO} | |||
| {"read_ratio", READ_RATIO}, | |||
| {"data_layout_reformatting", DATA_LAYOUT_REFORMATTING}, | |||
There was a problem hiding this comment.
This is missing corresponding "data_reformatting" get and set entries in MonCommands.h (easy to miss!)
Very much dislike "optimize_during_scrub". I'd favor the boolean flag approach with something like "scrub_defragmentation", and "scrub_recompression". The number of pool options we have and the way we display them in the command help has become pretty unwieldy though. This doesn't do anything to help it. Having said that, I agree that this is best configurable at the pool level, especially if we want to allow lazy compression of objects on scrub. |
|
I did a very minimal initial test where I created 4 128GB RBD images, pre-filled them with 4MB writes, then did 5 minutes of 4K random writes to them in a pool with 512 PGs. This is on NVMe, so it's pretty quick. I then looked at a certain PG 2.1ff:
and ran deep-scrub on it 3 times:
And the fragmentation score and reformat counters at this point is: Next set the pool data_reformatting to defragment and rerun deep-scrub on 2.1ff:
Next set the pool data_reformatting to both and rerun deep-scrub on 2.1ff:
Next set the pool compression_mode to passive and rerun deep-scrub on 2.1ff. Results are similar to above.
Running another deep scrub on that PG results in an attempt to recompress the objects again: A couple of thoughts:
Morning update: I tried deep-scrubbing another PG this morning with data_reformatting set to both and watched onode_extents:
It looks like the number of extents actually went up? |
|
Here's a debug_bluestore = 10 log while deep-scrubbing one of the other PGs (2.1fb). |
Are you referring to onode_extens perf counter here? IMO that's a wrong metric as it's an amount of logical (not physical) extents in the cache(!) |
Looks good to me, the majority of reads has log output like: |
I could see no initial compression, e.g. sc (aka storage compressed) = 0 2024-05-23T12:58:04.864+0000 7fed59ea4700 10 bluestore(/tmp/cbt/mnt/osd-device-2-data) read recompress info, 0x80000 vs. 0x80000 the above line shows no saving due to recompression - recompressed data needs the same 512K of space. So I presume your input data flow isn't compressible. Or something wrong with compression settings... |
Yikes! That's confusing. I don't know if I knew that at one point, but clearly I didn't remember it. :) We should rename it. I'll try to take a look with the histogram. |
There was no initial compression. I wrote the data out uncompressed with data_reformatting off, then set it to "defragment", then "both". Next I set the compression mode to passive, then aggressive, then force. Each time, I ran a deep scrub on different PGs to see what would happen. |
This is a great point. I think the intuitive user expectation is that this feature could be used to compress old objects written uncompressed before this feature was available. Also earlier we spoke about a feature like:
|
Signed-off-by: Garry Drankovich <garry.drankovich@clyso.com>
Signed-off-by: Garry Drankovich <garry.drankovich@clyso.com>
Signed-off-by: Garry Drankovich <garry.drankovich@clyso.com>
gardran
force-pushed
the
wip-gdran-defregment
branch
from
462da8c
to
56f74a1
Compare
Signed-off-by: Garry Drankovich <garry.drankovich@clyso.com>
| @@ -11741,28 +11787,47 @@ int BlueStore::set_collection_opts( | |||
| } | |||
|
|
|||
| int BlueStore::read( | |||
There was a problem hiding this comment.
Using ObjectStore::read() as interface is problematic. For proof of concept / quality testing is clearly ok.
The current algorithm can be extended, at least in following way:
- Data compressed possibly does not need to be recompressed. It could be moved. As a result, we do not need to get it decoded. But having it tied to
read()forces us to decode to provide outputbufferlist - During scrub we could create lists of objects that need recompression and defragmentation. Actual action to do so could be separate step, applied after.
- I have a dream to implement "optimize-snaps" method. For head+snaps in reencodes them so head gets continuous encoding and snaps became diffs to head. This possibly requires passing head+snaps in one go.
My general proposal is to expand ObjectStore interface to allow for object optimization.
There was a problem hiding this comment.
Using
ObjectStore::read()as interface is problematic. For proof of concept / quality testing is clearly ok.
I would rather disagree - having object reformatting coupled with reading allows to have it optimized - one can omit object's content reading/decompression which are required if alternative/standalone API is utilized.
The current algorithm can be extended, at least in following way:
- Data compressed possibly does not need to be recompressed. It could be moved. As a result, we do not need to get it decoded. But having it tied to
read()forces us to decode to provide outputbufferlist
That's a valid point if object reformatting is triggered independently. But when we piggy back a read we need decompression anyway - so this doesn't create an additional overhead due to that.
- During scrub we could create lists of objects that need recompression and defragmentation. Actual action to do
so could be separate step, applied after.
Theoretically this looks good but in practice this would complicate the code (as there will be an additional entity to track and apply deferred reformatting) while the benefits are unclear.
- I have a dream to implement "optimize-snaps" method. For head+snaps in reencodes them so head gets continuous encoding and snaps became diffs to head. This possibly requires passing head+snaps in one go.
My general proposal is to expand ObjectStore interface to allow for object optimization.
This sounds like you're suggesting an additional standalone procedure (to a major degree similar to deep-scrub) to perform object layout optimization. IMO this is nice as a design concept but it would provide an additional (and likely severe) burden on the cluster - existing deep-scrubbing tend to be a pretty heavy-weight process itself and having another background process (comparable in terms of performed operations) doesn't look like a good option to me.
With the proposed solution we're trying to decrease that burden by piggy-backing the deep scrub instead.
| @@ -11776,11 +11841,134 @@ int BlueStore::read( | |||
| if (offset == length && offset == 0) | |||
| length = o->onode.size; | |||
|
|
|||
| r = _do_read(c, o, offset, length, bl, op_flags); | |||
| r = _do_read(c, o, offset, length, bl, op_flags, 0, | |||
There was a problem hiding this comment.
The implementation is very strongly tied to existing BlueStore execution paths.
This is a problem, because it gives very little room to actually work on optimization algorithms.
There was a problem hiding this comment.
That's true but we really want to save some cycles by not doing independent standalone optimization.
| IOContext* ioc) | ||
| { | ||
| IOContext* ioc, | ||
| span_stat_t* res_span_stat) |
There was a problem hiding this comment.
_prepare_read_ioc is modified to provide scanning for optimization.
We should have a separate function that will scan object and decide action.
For example it might happen that in object we have 3 chunks for total of 4MB,
and in attempt to defragment it we allocated 3 chunks.
There was a problem hiding this comment.
_prepare_read_iocis modified to provide scanning for optimization. We should have a separate function that will scan object and decide action.
That's again an attempt to design things for an independent optimization process. Which still sounds questionable to me.
For example it might happen that in object we have 3 chunks for total of 4MB, and in attempt to defragment it we allocated 3 chunks.
Not sure I understand this example. When speaking of defragmentation only - it would happen if amount of new fragments is less than original one for a specific data span (512K when triggered from deep scrub).
If recompression is enabled as well -reformatting unconditionally occurs when new required space for a data span is less than the original one. Fragmentation is taken into consideration when recompression is not beneficial only. This recompression+defragmentation prioritization might look a bit inflexible - one might want prefer defragmentation over recompression but for now it looks like an overkill to me.
There was a problem hiding this comment.
The big reason I think we want to piggyback on deep-scrub is because we're already reading the content of the object to do the scrub, so it's the perfect time to make decisions about de-fragmentation and compression as the most expensive part of this can easily be reading the fragmented extents. I don't think we should scan objects as a separate loop unless we have a very good reason. It's much better in my mind to opportunistically defrag/compress when we've already done the expensive fragmented read.
There was a problem hiding this comment.
@markhpc
I (now) agree that performing optimization to object as an extra step during deep-scrub is good.
"I don't think we should scan objects as a separate loop unless we have a very good reason."
The code here is scanning the object - it's just piggybacking _read_cache to do it.
There is no change to behaviour of _read_cache.
The filling of span_stat_t could be done in separate loop at a cost of a loop.
Similarly, we now load shared blobs in _prepare_read_ioc - useless for reading process.
That would made optimization sense, if significant part of processing was reused - but its only the loop.
@gardran My opinion is that whenever we make a new feature we should make it as detached from existing code as possible to minimize accidental bug injection. Within reason of course...
src/os/bluestore/BlueStore.cc
Outdated
| // object operations | ||
| std::unique_lock l(c->lock); | ||
| ceph_assert(o->exists); | ||
| _assign_nid(txc, o); |
There was a problem hiding this comment.
I guess we already have nid?
src/os/bluestore/BlueStore.cc
Outdated
| l_bluestore_allocator_lat, | ||
| mono_clock::now() - start, | ||
| cct->_conf->bluestore_log_op_age); | ||
| might_need_defragment = preallocated >= (int64_t)need && prealloc.size() < span_stat.frags; |
There was a problem hiding this comment.
We need to create diligent testing against all allocators we have.
My doubt is that we compare fragments in object with fragments in free space.
My thinking is - if I optimized object 5 chunks -> 2 chunks, then I probably created 5 small free chunks and removed 2 larger free chunks. There might be an emergent pattern that ultimately keeps both objects and free space more concise, but its impossible to infer it from above.
There was a problem hiding this comment.
One should realize that this PR is intended primarily for object optimization not a whole free space one. So yes hopefully object defragmentation could result in free space defragmentation as well but generally speaking that's rather a different task.
Plus a few more PR comment resolutions from Adam. Signed-off-by: Garry Drankovich <garry.drankovich@clyso.com>
| if (r == -EIO) { | ||
| logger->inc(l_bluestore_read_eio); | ||
| } | ||
| } | ||
| if (might_need_reformatting) { |
There was a problem hiding this comment.
This should be under same lock as _do_read() is executed.
Taking collection lock later is not ok, since some transaction could be injected in-between.
| _choose_write_options(c, o, op_flags, &wctx); | ||
| } | ||
| // do reformat if | ||
| // - object isn't cached (meaning it's not being written at the moment), |
There was a problem hiding this comment.
When we write to object, we keep data in Buffers at least until the data is readable from disk.
This is to make sure that if one reads object he will receive the proper data, leaving BlueStore free to delay storing data to disk (aka deferred writes).
As a result one can always read cached data and it will be the proper data.
Hence - there is no need to exclude objects that have unstored data.
The closest scenario to a problem I could imagine would be:
- Transaction X to object A submitted
- Buffers updated, Onode metadata updated
- Data X is not yet written
- Deep scrub reformats object A
- Onode metadata updated (Buffers not changed)
- Data from reformatting gets to the disk before data from transaction
- Power-off or similar
In this case we would never signal that transaction X is completed, yet it will be there.
But it's not a problem, it is normal.
Could you elaborate what prompted you to exclude objects that are being written to?
| @@ -16681,6 +16964,41 @@ void BlueStore::_choose_write_options( | |||
| wctx->target_blob_size < min_alloc_size * 2) { | |||
| wctx->target_blob_size = min_alloc_size * 2; | |||
| } | |||
| if (wctx->compress) { | |||
There was a problem hiding this comment.
+1 for moving selection of compressor to _choose_write_options.
| { | ||
| // build blob-wise list to of stuff read (that isn't cached) | ||
| unsigned left = length; | ||
| uint64_t pos = offset; | ||
| span_stat_t dummy_span_stat; | ||
| span_stat_t& span_stat = res_span_stat ? *res_span_stat : dummy_span_stat; | ||
| span_stat.stored = length; | ||
| auto lp = o->extent_map.seek_lextent(offset); | ||
| while (left > 0 && lp != o->extent_map.extent_map.end()) { |
There was a problem hiding this comment.
Need to span_stat.stored -= left; after we exit the loop to factor in the case that we reached end() earlier.
| case COMP_PASSIVE_LAZY: return "passive_lazy"; | ||
| case COMP_AGGRESSIVE_LAZY: return "aggressive_lazy"; | ||
| case COMP_FORCE_LAZY: return "force_lazy"; | ||
|
|
||
| default: return "???"; |
There was a problem hiding this comment.
I think we need following combination of compressions for regular operation and for scrub.
| regular-compression | scrub-compression |
|---|---|
| none | none |
| none | passive |
| none | agressive |
| none | force |
| passive | passive |
| passive | agresive |
| passive | force |
| aggresive | agresive |
| aggresive | force |
| force | force |
| case COMP_PASSIVE_LAZY: return "passive_lazy"; | ||
| case COMP_AGGRESSIVE_LAZY: return "aggressive_lazy"; | ||
| case COMP_FORCE_LAZY: return "force_lazy"; | ||
|
|
||
| default: return "???"; | ||
| } | ||
| } |
There was a problem hiding this comment.
The separate discussion should be if we shouldn't give different conf.bluestore_compression_required_ratio for scrub recompression.
| if (r == -EIO) { | ||
| logger->inc(l_bluestore_read_eio); | ||
| } | ||
| } | ||
| if (might_need_reformatting) { | ||
| _maybe_reformat_object(c, o, offset, length, bl, op_flags, span_stat); | ||
| } |
There was a problem hiding this comment.
Now we will add latency of reformat to l_bluestore_read_lat. I am not sure whether we want it or not.
This PR adds an ability for BlueStore to perform object content recompression and defragmentation when handling read requests during deep-scrubbing.
After reading object's chunk BlueStore assesses if it can improve space saving by recompressing an object and/or it can optimize physical layout of the object.
Per-pool option 'data_layout_reformatting' has been introduced to control the behavior. It support the following options:
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 retest this pleasejenkins test classic perfjenkins test crimson perfjenkins test signedjenkins test make checkjenkins test make check arm64jenkins test submodulesjenkins test dashboardjenkins test dashboard cephadmjenkins test apijenkins test docsjenkins render docsjenkins test ceph-volume alljenkins test ceph-volume toxjenkins test windowsjenkins test rook e2e