DNM: os/bluestore: improve move_ranges implementation #11561
Conversation
| } else { | ||
| skip_back = 0; | ||
| } | ||
| Extent *ne = new Extent( |
There was a problem hiding this comment.
Let's consider following disposition:
e(1090) -> blob(00x1000)
mv (10~90 - > 0x2000)
it results in
skip_front = 0, skip_back = 0
ne.logical_offset = 10 + 0 + 0x2000 - 10 = 0x2000
ne.blob_offset = 10 + 0 = 10
ne.length = 90 - 0 - 0 = 90
i.e.
new extent, located at alloc unit(AU) boundary(4K) has non zero blob offset. This violates our invariant that blobs are AU aligned and IMHO might have some negative impact on subsequent operations. Makes sense?
| Extent *ne = new Extent( | ||
| e.logical_offset + skip_front + dstoff - srcoff, | ||
| e.blob_offset + skip_front, | ||
| e.length - skip_front - skip_back, e.blob_depth, ep->blob); |
There was a problem hiding this comment.
I doubt blob_depth is inherited from the old object - that's rather a matter of disposition in the new object - hence needs recalculation( max over old_extents returned by punch_hole?)
|
Perhaps we should restrict this operation to move ranges at identical
offsets. That simplifies the API somewhat and avoids the alignment
issues...
|
|
@athanatos does fixing srcoff=dstoff seem ok to you?
|
|
@liewegas Hmm, ideally not. It would mean relying on the rollforward object being sparse. I supposed that could be ok. |
|
#11595 .. i'll rebase onto that |
liewegas
force-pushed
the
wip-bluestore-move-ranges
branch
2 times, most recently
from
ee3353a
to
7cc150b
Compare
|
Rebased and retested; ready for review |
| t.move_ranges_destroy_src(cid, hoid2, hoid, move_info); | ||
| cerr << "move temp object" << std::endl; | ||
| r = apply_transaction(store, &osr, std::move(t)); | ||
| ASSERT_EQ(r, 0); |
There was a problem hiding this comment.
Suggest to add content reading and verification for moved and unaffected extents.
| if (true) { | ||
| r = store->umount(); | ||
| ASSERT_EQ(0, r); | ||
| r = store->fsck(); |
There was a problem hiding this comment.
umount/mount perform fsck with the current settings. hence we have triple fsck here
| skip_back = 0; | ||
| } | ||
| Extent *ne = new Extent( | ||
| e.logical_offset + skip_front + offset - offset, |
| Extent *ne = new Extent( | ||
| e.logical_offset + skip_front + offset - offset, | ||
| e.blob_offset + skip_front, | ||
| e.length - skip_front - skip_back, e.blob_depth, ep->blob); |
There was a problem hiding this comment.
blob_depth can't be inherited from a source object
Signed-off-by: Sage Weil <sage@redhat.com>
Signed-off-by: Sage Weil <sage@redhat.com>
Signed-off-by: Sage Weil <sage@redhat.com>
This avoids needlessly making all the blobs shared. Signed-off-by: Sage Weil <sage@redhat.com>
Signed-off-by: Sage Weil <sage@redhat.com>
liewegas
force-pushed
the
wip-bluestore-move-ranges
branch
from
7cc150b
to
f2fbb3d
Compare
liewegas
changed the title
|
Gonna do something different. |
No description provided.