New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

osd: new op for calculating an extent checksum #14256

Merged
merged 3 commits into from Apr 19, 2017

Conversation

Projects
None yet
4 participants
@dillaman
Contributor

dillaman commented Mar 30, 2017

This new op will be utilized by the rbd-mirror "deep scrub" feature to detect replication mismatches between object clones by exchanging only crc32c values vs requiring full object reads.

* @return negative error code on failure
*/
CEPH_RADOS_API int rados_checksum(rados_ioctx_t io, const char *oid,
size_t len, uint64_t off, uint32_t *pcrc);

This comment has been minimized.

@liewegas

liewegas Apr 3, 2017

Member

I think this should have an argument for the checksum to use (crc32c, xxhash, etc.). Possibly an initialization value (usually -1 for crc32c; not sure about the others). And the output should be a char *out, size_t size in case it's not a 32-bit hash.

or this method should be called crc32c. but even then it should still have an initial value instead of assuming -1.

This comment has been minimized.

@dillaman

dillaman Apr 3, 2017

Contributor

Fair enough -- I was just trying to get away w/ re-using the existing extent structure instead of rolling my own "checksum" struct with duplicated extents. :-p

}
uint32_t crc = read_op.outdata.crc32c(-1);
::encode(crc, osd_op.outdata);

This comment has been minimized.

@liewegas

liewegas Apr 3, 2017

Member

We should try to use Checksummer.h to generalize this to all supported checksum algorithms

@dillaman

This comment has been minimized.

Contributor

dillaman commented Apr 5, 2017

@liewegas tweaks pushed for feedback

*
* le32 num_checksum_chunks
* {
* checksum-type checksum checksum result for chunk

This comment has been minimized.

@liewegas

liewegas Apr 5, 2017

Member

"checksum-type checksum checksum result for chunk" is a bit confusing. we probably also want to specify that it is little-endian word order.

how about "leXX checksum for chunk (where XX = appropriate size for the checksum type)"

rados_read_op_t op2 = rados_create_read_op();
rados_read_op_read(op2, 0, sizeof(buf2), buf2, NULL, NULL);
rados_read_op_set_flags(op2, LIBRADOS_OP_FLAG_FADVISE_DONTNEED |

This comment has been minimized.

@liewegas

liewegas Apr 5, 2017

Member

i think FADVISE_NOCACHE ("I won't use this data again") instead of DONTNEED ("nobody (me or anyone else) will use this data again")

This comment has been minimized.

@dillaman

dillaman Apr 5, 2017

Contributor

No worries -- I just translated the existing "RoundTripPP3" case to the C API.

@liewegas

just the comment nit and fadvise hint; otherwise looks good!

@liewegas

This comment has been minimized.

Member

liewegas commented Apr 5, 2017

Oh, we should add a simple test to test/librados/misc.cc that writes a multiple chunks worth of data, fetches a checksum, and verifies the result is correct. should probably do a few ops with offset == 0, == first chunk, etc.

@dillaman

This comment has been minimized.

Contributor

dillaman commented Apr 5, 2017

@liewegas addressed comments and added test case coverage for all three checksum algs w/ offset-based and chunk-based results.

@liewegas

This comment has been minimized.

Member

liewegas commented Apr 5, 2017

retest this please

@yuriw

This comment has been minimized.

Contributor

yuriw commented Apr 7, 2017

@dillaman @liewegas seems causing issues see run and rerun, removed from testing batch for now

http://pulpito.ceph.com/yuriw-2017-04-06_19:59:50-rados-wip-yuri-testing_2017_4_7-distro-basic-smithi/

http://pulpito.ceph.com/yuriw-2017-04-07_15:47:40-rados-wip-yuri-testing_2017_4_7---basic-smithi/

Removed needs-qa tag
Let me know if you want to test it by itself.

@dillaman

This comment has been minimized.

Contributor

dillaman commented Apr 8, 2017

The only issues that look related this PR are EC pool test cases since SYNC_READ isn't supported on an EC pool and thus returns -EOPNOTSUPP. @liewegas Is it safe for me to swap out the internal SYNC_READ for a READ op?

@dillaman

This comment has been minimized.

Contributor

dillaman commented Apr 11, 2017

updated ceph_test_rados to properly detect when it's being invoked against an EC pool (and the test failed to pass the "--ec-pool" option).

@dillaman

This comment has been minimized.

Contributor

dillaman commented Apr 11, 2017

retest this please

dillaman added some commits Apr 3, 2017

common/Checksummer: allow the initial/seed value to be supplied
Signed-off-by: Jason Dillaman <dillaman@redhat.com>
osd: new op for retrieving an extent checksum
Signed-off-by: Jason Dillaman <dillaman@redhat.com>
librados: expose new checksum osd operation
Signed-off-by: Jason Dillaman <dillaman@redhat.com>
@dillaman

This comment has been minimized.

Contributor

dillaman commented Apr 18, 2017

@liewegas @jdurgin now with extra EC fun

@jdurgin

This comment has been minimized.

Member

jdurgin commented Apr 18, 2017

very nice!

@jdurgin jdurgin added the needs-qa label Apr 18, 2017

@liewegas liewegas merged commit 5080aff into ceph:master Apr 19, 2017

3 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodifed Submodules submodules for project are unmodified
Details
default Build finished.
Details

@dillaman dillaman deleted the dillaman:wip-19297 branch Apr 20, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment