cephfs: add support for cephfs file block diff api

[iPraveenParihar]

cephfs: add support for cephfs file block diff api

This commit adds support for cephfs file block diff
APIs which gives a list of changed blocks with in a
file between two snapshots.

Checklist

• Added tests for features and functional changes
• Public functions and types are documented
• Standard formatting is applied to Go code
• Is this a new API? Added a new file that begins with //go:build ceph_preview
• Ran make api-update to record new APIs

New or infrequent contributors may want to review the go-ceph Developer's Guide including the section on how we track API Status and the API Stability Plan.

The go-ceph project uses mergify. View the mergify command guide for information on how to interact with mergify. Add a comment with @Mergifyio rebase to rebase your PR when github indicates that the PR is out of date with the base branch.

[mergify]

This pull request now has conflicts with the target branch.
Could you please resolve conflicts and force push the corrected
changes? 🙏

[iPraveenParihar]

@anoopcs9 PTAL

[phlogistonjohn]

Is there any case where the bool is true and the pointer is nil? If not the bool seems a bit redundant. Is there something I'm overlooking?

[iPraveenParihar]

@phlogistonjohn, no we don't have such a case.
bool will be true if there are more changed blocks entries and is such a case the pointer will never be nil.

[phlogistonjohn]

IIUC, it seems like we can then simply the return types by dropping the boolean and having the callers test for nil/non-nil in all the cases that were previously using the boolean.

[iPraveenParihar]

The pointer points to the current set of changed blocks (nil on error), while the boolean clearly indicates if more entries remain. Without the boolean, callers would need to infer that from the pointer and error, which makes the flow less clear.

[Rakshith-R]

IIUC, it seems like we can then simply the return types by dropping the boolean and having the callers test for nil/non-nil in all the cases that were previously using the boolean.

The boolean is used by cephfs function to indicate if the user needs to call ReadFileBlockDiff again for more entries.
We should preserve the behavior intended by cephfs team here.

@vshankar had pointed to this test case in ceph repo so that I could understand the intended usage.
Hope it helps clear the confusion.

https://github.com/ceph/ceph/blob/060b69c6605d88ad06bd1a3b81697abff2bd4ca5/src/test/libcephfs/snapdiff.cc#L410-L430

[phlogistonjohn]

Now, I'm even more confused. There's no boolean in the api we're wrapping it returns ret (an int) and info (input) and blocks (output) arguments.
No bool there at all.

Without the boolean, callers would need to infer that from the pointer and error, which makes the flow less clear

This doesn't seem right to me. Error indicates error, period. At that point the loop should stop and handle the error. If error is not nil the other return value(s) are irrelevant. If there is no error then the client can check the nil-ity of the blocks value or not to determine if the loop needs to continue or not. The extra bool value is redundant.

[Rakshith-R]

The ret(an int) indicated by r is used in the following manner:

• r > 0 : there will be more changed blocks returned in the next call
• r == 0: no more changed blocks to be returned
• r < 0: there's an errorr

It is confusing but that's how the return value of the function ceph_file_blockdiff is meant to be used.

The following is testcase for this function from https://github.com/ceph/ceph/blob/060b69c6605d88ad06bd1a3b81697abff2bd4ca5/src/test/libcephfs/snapdiff.cc#L410-L430

    r = 1;
    while (r > 0) {
      ceph_file_blockdiff_changedblocks blocks;
      r = ceph_file_blockdiff(&info, &blocks);
      if (r < 0) {
	std::cerr << " Failed to get next changed block, ret:" << r << std::endl;
	return r;
      }

      int nr_blocks = blocks.num_blocks;
      struct cblock *b = blocks.b;
      while (nr_blocks > 0) {
	std::cout << " == [" << b->offset << "~" << b->len << "] == " << std::endl;
	if (expected) {
	  expected->erase(b->offset, b->len);
	}
	++b;
	--nr_blocks;
      }

      ceph_free_file_blockdiff_buffer(&blocks);
    }

In go-ceph implementation to make it simpler,
a boolean is used to indicate if the next function call needs to be made or not.

	if ret < 0 {
		return false, nil, getError(ret)
	}
	// if ret == 0 indicates there is no more entries after this call.
	noMoreEntries = (ret == 0)

Again,

We should preserve the behavior intended by cephfs team here.

Making any change to the behavior at this go-ceph binding layer may lead to unexpected behavior later on.

[anoopcs9]

Well, if we go for an extra round I hope we’ll discover that num_blocks is 0 and/or b is NULL which I guess is what John was hinting at.

just another thought: Can we think of a More() method to basically return whether more processing is required or not? Let's say we have a boolean noMoreEntries inside FileBlockDiffInfo which is set to true during init and subsequently updated during read based on the return code for ceph_file_blockdiff() inside ReadFileBlockDiff().

[phlogistonjohn]

Now, I'm even more confused. There's no boolean in the api we're wrapping it returns ret (an int) and info (input) and blocks (output) arguments.
No bool there at all.

Without the boolean, callers would need to infer that from the pointer and error, which makes the flow less clear

This doesn't seem right to me. Error indicates error, period. At that point the loop should stop and handle the error. If error is not nil the other return value(s) are irrelevant. If there is no error then the client can check the nil-ity of the blocks value or not to determine if the loop needs to continue or not. The extra bool value is redundant.

[iPraveenParihar]

@nixpanic @anoopcs9 PTAL.
Thanks! 😄

[iPraveenParihar]

rebased the PR now.
Could I please have a review again?

[anoopcs9]

Well, if we go for an extra round I hope we’ll discover that num_blocks is 0 and/or b is NULL which I guess is what John was hinting at.

just another thought: Can we think of a More() method to basically return whether more processing is required or not? Let's say we have a boolean noMoreEntries inside FileBlockDiffInfo which is set to true during init and subsequently updated during read based on the return code for ceph_file_blockdiff() inside ReadFileBlockDiff().

[iPraveenParihar]

@anoopcs9, addressed your reviews.
PTAL. Thanks!

[anoopcs9]

@Mergifyio rebase

[mergify]

rebase

✅ Branch has been successfully rebased

[mergify]

This pull request now has conflicts with the target branch.
Could you please resolve conflicts and force push the corrected
changes? 🙏

[anoopcs9]

btw, your latest update seems to have overwritten the space/tab alignment fixes I had pushed earlier. Please make sure that you fetch the latest version from PR before making any changes.

[anoopcs9]

I’ve added a comment about the extra care needed for struct ceph_file_blockdiff_result.

Thanks for your patience over the past months, it’s really appreciated.

[phlogistonjohn]

@Mergifyio rebase

[mergify]

rebase

✅ Branch has been successfully rebased

[anoopcs9]

@Mergifyio rebase

[mergify]

rebase

✅ Branch has been successfully rebased

[phlogistonjohn]

lgtm, thanks

[mergify]

Merge Queue Status

✅ The pull request has been merged at 21934d9

This pull request spent 6 seconds in the queue, with no time running CI.
The checks were run in-place.

Required conditions to merge

• #approved-reviews-by >= 1 [🛡 GitHub branch protection]
  • cephfs: add support for cephfs file block diff api #1191
• #changes-requested-reviews-by = 0 [🛡 GitHub branch protection]
  • cephfs: add support for cephfs file block diff api #1191
• any of [🛡 GitHub branch protection]:
  • check-success = check
  • check-neutral = check
  • check-skipped = check
• any of [🛡 GitHub branch protection]:
  • check-success = test-suite (pacific)
  • check-neutral = test-suite (pacific)
  • check-skipped = test-suite (pacific)
• any of [🛡 GitHub branch protection]:
  • check-success = test-suite (quincy)
  • check-neutral = test-suite (quincy)
  • check-skipped = test-suite (quincy)
• any of [🛡 GitHub branch protection]:
  • check-success = test-suite (reef)
  • check-neutral = test-suite (reef)
  • check-skipped = test-suite (reef)
• any of [🛡 GitHub branch protection]:
  • check-success = test-suite (squid)
  • check-neutral = test-suite (squid)
  • check-skipped = test-suite (squid)