rbd: document DiffIterate Offset/Length semantics and add non-zero offset tests by kaovilai · Pull Request #1242 · ceph/go-ceph

kaovilai · 2026-03-24T22:57:46Z

Summary

Add doc comments to Offset and Length fields in DiffIterateConfig and DiffIterateByIDConfig clarifying that Length is the number of bytes to scan from Offset (range [Offset, Offset+Length)), not an absolute end position
Add nonZeroOffset tests for both DiffIterate and DiffIterateByID that verify only extents within the scanned range are reported

This ambiguity contributed to a bug in ceph-csi (ceph/ceph-csi#6200) where Length was set to the full volume size instead of (volumeSize - offset), causing zero blocks to be returned when using a non-zero offset (e.g. LUKS header skip).

Test plan

Verify nonZeroOffset tests pass for DiffIterate
Verify nonZeroOffset tests pass for DiffIterateByID
Verify existing tests still pass

Note

Responses generated with Claude

Copilot

Pull request overview

This PR clarifies how Offset/Length define the scan range for RBD diff-iterate operations and adds coverage to ensure non-zero offsets only report extents within the requested range (aligning behavior with expected usage by downstream consumers like ceph-csi).

Changes:

Document Offset and Length semantics for DiffIterateConfig and DiffIterateByIDConfig as a scan range [Offset, Offset+Length).
Add nonZeroOffset tests for both DiffIterate and DiffIterateByID to verify extents outside the scanned range are not reported.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.

File	Description
`rbd/diff_iterate.go`	Adds field docs clarifying `Offset`/`Length` scan-range semantics for `DiffIterate`.
`rbd/diff_iterate_by_id.go`	Adds analogous docs for `DiffIterateByID`.
`rbd/diff_iterate_test.go`	Adds `nonZeroOffset` test coverage for `DiffIterate`.
`rbd/diff_iterate_by_id_test.go`	Adds `nonZeroOffset` test coverage for `DiffIterateByID`.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-24T23:05:45Z

+	// Length is the number of bytes to scan starting from Offset.
+	// The scanned range is [Offset, Offset+Length). When using a
+	// non-zero Offset, Length should typically be (imageSize - Offset)
+	// to scan through the end of the image.


The new Length docs explain the [Offset, Offset+Length) semantics, but they don’t mention the constraint that Offset+Length must stay within the image size (librbd treats out-of-range requests as invalid) and that Length==0 scans an empty range. Adding a short note would make the API behavior less ambiguous for callers.

Suggested change

// to scan through the end of the image.

// to scan through the end of the image. Offset+Length must not

// exceed the image size; librbd treats out-of-range requests as

// invalid. A Length of 0 scans an empty range and the callback

// will not be invoked.

Length should typically be (imageSize - Offset) to scan through the end of the image.
Seems awkward to me. It sounds like a recommendation (which would be misleading) rather than a guideline. I would either leave it off or reword it as a gudeline (something like "Length must never exceed ...".

Copilot · 2026-03-24T23:05:46Z

+	// Length is the number of bytes to scan starting from Offset.
+	// The scanned range is [Offset, Offset+Length). When using a
+	// non-zero Offset, Length should typically be (imageSize - Offset)
+	// to scan through the end of the image.


Similar to DiffIterateConfig: consider extending the new Length docs to note that Offset+Length must be within the image bounds (otherwise librbd returns an error) and that Length==0 represents an empty scan range. This will help avoid callers assuming len=0 means “to end”.

Suggested change

// to scan through the end of the image.

// to scan through the end of the image. Offset+Length must be

// within the image bounds; otherwise the underlying librbd call

// will return an error. A Length of 0 specifies an empty scan

// range (no bytes are scanned) and does not mean "to the end".

Copilot · 2026-03-24T23:05:46Z

+// testDiffIterateNonZeroOffset verifies that DiffIterate works correctly with
+// a non-zero Offset. This exercises the scenario where a caller wants to skip
+// a portion of the image (e.g. a LUKS header) and only scan the remainder.
+// Length must be set to (imageSize - Offset), not imageSize.


This comment reads like a hard API requirement ("Length must be set to ..."), but it’s really guidance for the common “scan to end after skipping” case. Rewording to "to scan to the end, Length should be (imageSize-Offset)" would avoid implying other Length values are invalid.

Suggested change

// Length must be set to (imageSize - Offset), not imageSize.

// To scan to the end after skipping the initial portion, Length should be

// (imageSize - Offset) rather than imageSize.

Copilot · 2026-03-24T23:05:46Z

+// testDiffIterateByIDNonZeroOffset verifies that DiffIterateByID works
+// correctly with a non-zero Offset. This exercises the scenario where a caller
+// wants to skip a portion of the image (e.g. a LUKS header) and only scan
+// the remainder. Length must be set to (imageSize - Offset), not imageSize.


Same wording concern as the DiffIterate nonZeroOffset test: "Length must be set to ..." can be read as an API requirement rather than guidance for scanning to the end after an Offset. Consider rephrasing to make the intent explicit.

Suggested change

// the remainder. Length must be set to (imageSize - Offset), not imageSize.

// the remainder. In this scenario, Length is specified as (imageSize - Offset)

// so that the diff covers only the remaining range rather than the full imageSize.

phlogistonjohn · 2026-03-27T14:34:35Z

As a new contributor, please don't remove or replace the contribution checklist. It helps us ensure certain needed tasks have been considered by the submitter and if you don't understand one of those tasks one can feel free to ask.

Check off the items that are relevant to you and have considered done or you think don't apply (the patches don't touch public code for example).

phlogistonjohn · 2026-03-27T14:39:18Z

One other very important thing I just noticed:
We need a Signed-off-by trailer that must be attributed to a human individual (you, the submitter). (See also https://stackoverflow.com/questions/1962094/what-is-the-sign-off-feature-in-git-for)

kaovilai · 2026-03-27T15:27:40Z

You don't have DCO checks? Maybe those run after approval

nixpanic

LGTM, thanks!

phlogistonjohn · 2026-04-13T14:07:08Z

Hi, this change needs to be rebased and it needs to be formatted correctly and pass the formatting checks in the CI. Thank you.

phlogistonjohn · 2026-04-16T20:08:41Z

Hi @kaovilai can you please take a look at fixing and rebasing this PR? I would like to see it be merged within then next couple of weeks. Thanks!

kaovilai · 2026-04-16T20:09:48Z

okie

kaovilai · 2026-04-16T20:10:53Z

rebased

phlogistonjohn · 2026-04-16T20:13:23Z

Thanks for rebasing. However, we still need formatting fixes for the CI to pass, thanks.

phlogistonjohn · 2026-04-16T20:16:21Z

In case it helps, here's what the CI output shows:

--- /dev/fd/63	2026-04-16 20:14:44.982828631 +0000
+++ /dev/fd/62	2026-04-16 20:14:44.982828631 +0000
@@ -0,0 +1,24 @@
+diff rbd/diff_iterate.go.orig rbd/diff_iterate.go
+--- rbd/diff_iterate.go.orig
++++ rbd/diff_iterate.go
+@@ -60,7 +60,7 @@
+ 	// Length is the number of bytes to scan starting from Offset.
+ 	// The scanned range is [Offset, Offset+Length). Length must not
+ 	// exceed (imageSize - Offset).
+-	Length uint64
++	Length        uint64
+ 	IncludeParent DiffIncludeParent
+ 	WholeObject   DiffWholeObject
+ 	Callback      DiffIterateCallback
+diff rbd/diff_iterate_by_id.go.orig rbd/diff_iterate_by_id.go
+--- rbd/diff_iterate_by_id.go.orig
++++ rbd/diff_iterate_by_id.go
+@@ -56,7 +56,7 @@
+ 	// Length is the number of bytes to scan starting from Offset.
+ 	// The scanned range is [Offset, Offset+Length). Length must not
+ 	// exceed (imageSize - Offset).
+-	Length uint64
++	Length        uint64
+ 	IncludeParent DiffIncludeParent
+ 	WholeObject   DiffWholeObject
+ 	Callback      DiffIterateCallback
make: *** [Makefile:215: test-containers-test] Error 1
Error: Process completed with exit code 2.

…fset tests The Offset and Length fields in DiffIterateConfig and DiffIterateByIDConfig lacked documentation explaining that Length is the number of bytes to scan from Offset (range [Offset, Offset+Length)), not an absolute end position. This ambiguity contributed to a bug in ceph-csi (PR #6200) where Length was set to the full volume size instead of (volumeSize - offset), causing zero blocks to be returned when using a non-zero offset (e.g. LUKS header skip). Add doc comments clarifying the semantics and add tests exercising non-zero Offset values for both DiffIterate and DiffIterateByID, verifying that only extents within the [Offset, Offset+Length) range are reported. See: ceph/ceph-csi#6200 See: kaovilai/cephcsi-cbt-e2e#2 Generated with [Claude Code](https://claude.ai/code) via [Happy](https://happy.engineering) Signed-off-by: Tiger Kaovilai <passawit.kaovilai@gmail.com>

kaovilai · 2026-04-16T20:24:33Z

done

anoopcs9

lgtm.

mergify · 2026-04-17T12:24:23Z

kaovilai marked this pull request as ready for review March 24, 2026 22:59

Copilot AI review requested due to automatic review settings March 24, 2026 22:59

Copilot started reviewing on behalf of kaovilai March 24, 2026 23:00 View session

Copilot AI reviewed Mar 24, 2026

View reviewed changes

kaovilai mentioned this pull request Mar 24, 2026

CephCSI GetMetadataAllocated returns empty stream (0 blocks) despite rbd diff showing data kaovilai/cephcsi-cbt-e2e#2

Closed

4 tasks

kaovilai force-pushed the tkaovila/diff-iterate-offset-docs-and-tests branch from b8f3815 to 6c18b6a Compare March 28, 2026 20:38

nixpanic reviewed Mar 30, 2026

View reviewed changes

kaovilai force-pushed the tkaovila/diff-iterate-offset-docs-and-tests branch 2 times, most recently from 6f81f72 to 0572c22 Compare April 16, 2026 20:10

kaovilai force-pushed the tkaovila/diff-iterate-offset-docs-and-tests branch from 0572c22 to 4ab22ef Compare April 16, 2026 20:24

phlogistonjohn approved these changes Apr 17, 2026

View reviewed changes

phlogistonjohn requested a review from anoopcs9 April 17, 2026 12:05

phlogistonjohn added the API This PR includes a change to the public API of a go-ceph package label Apr 17, 2026

anoopcs9 approved these changes Apr 17, 2026

View reviewed changes

mergify bot added the queued label Apr 17, 2026

mergify bot merged commit 990706b into ceph:master Apr 17, 2026
15 of 16 checks passed

mergify bot removed the queued label Apr 17, 2026

-	// to scan through the end of the image.
+	// to scan through the end of the image. Offset+Length must not
+	// exceed the image size; librbd treats out-of-range requests as
+	// invalid. A Length of 0 scans an empty range and the callback
+	// will not be invoked.

-	// to scan through the end of the image.
+	// to scan through the end of the image. Offset+Length must be
+	// within the image bounds; otherwise the underlying librbd call
+	// will return an error. A Length of 0 specifies an empty scan
+	// range (no bytes are scanned) and does not mean "to the end".

	// Length must be set to (imageSize - Offset), not imageSize.
	// To scan to the end after skipping the initial portion, Length should be
	// (imageSize - Offset) rather than imageSize.

	// the remainder. Length must be set to (imageSize - Offset), not imageSize.
	// the remainder. In this scenario, Length is specified as (imageSize - Offset)
	// so that the diff covers only the remaining range rather than the full imageSize.

Conversation

kaovilai commented Mar 24, 2026

Summary

Test plan

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

phlogistonjohn Mar 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

phlogistonjohn commented Mar 27, 2026

Uh oh!

phlogistonjohn commented Mar 27, 2026

Uh oh!

kaovilai commented Mar 27, 2026

Uh oh!

nixpanic left a comment

Choose a reason for hiding this comment

Uh oh!

phlogistonjohn commented Apr 13, 2026

Uh oh!

phlogistonjohn commented Apr 16, 2026

Uh oh!

kaovilai commented Apr 16, 2026

Uh oh!

kaovilai commented Apr 16, 2026

Uh oh!

phlogistonjohn commented Apr 16, 2026

Uh oh!

phlogistonjohn commented Apr 16, 2026

Uh oh!

kaovilai commented Apr 16, 2026

Uh oh!

anoopcs9 left a comment

Choose a reason for hiding this comment

Uh oh!

mergify bot commented Apr 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merge Queue Status

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mergify bot commented Apr 17, 2026 •

edited

Loading