From c680531e070aef46bde50a8185d3a48ff9f2d9df Mon Sep 17 00:00:00 2001 From: Josh Durgin Date: Sat, 30 Mar 2013 17:25:18 -0700 Subject: [PATCH] librbd: change diff_iterate interface to be more C-friendly Use int instead of bool for the callback, and make it represent whether the data exists, rather than the opposite, since callers are likely to test for whether it's data instead of whether its zeroes. Change the return value to 0, since an int64_t will wrap around for large reads, and there's no value in reporting the length read when it will always be the length requested clipped to the size of the image. Signed-off-by: Josh Durgin --- src/include/rbd/librbd.hpp | 19 ++++++++++--------- src/librbd/internal.cc | 27 ++++++++++++++------------- src/librbd/internal.h | 8 ++++---- src/librbd/librbd.cc | 18 ++++++++++++++---- src/rbd.cc | 12 ++++++------ src/test/librbd/test_librbd.cc | 6 +++--- 6 files changed, 51 insertions(+), 39 deletions(-) diff --git a/src/include/rbd/librbd.hpp b/src/include/rbd/librbd.hpp index d061bdbe4eb86..600a3ccbcda00 100644 --- a/src/include/rbd/librbd.hpp +++ b/src/include/rbd/librbd.hpp @@ -161,22 +161,23 @@ public: * * This will return the differences between two versions of an image * via a callback, which gets the offset and length and a flag - * indicating whether the extent is known/defined to be zeros (a - * hole). If the source snapshot name is NULL, we interpret that as - * the beginning of time and return all allocated regions of the - * image. The end version is whatever is currently selected for the - * image handle (either a snapshot or the writeable head). + * indicating whether the extent exists (1), or is known/defined to + * be zeros (a hole, 0). If the source snapshot name is NULL, we + * interpret that as the beginning of time and return all allocated + * regions of the image. The end version is whatever is currently + * selected for the image handle (either a snapshot or the writeable + * head). * * @param fromsnapname start snapshot name, or NULL * @param ofs start offset * @param len len in bytes of region to report on * @param cb callback to call for each allocated region * @param arg argument to pass to the callback - * @returns len on success, or negative error code on error + * @returns 0 on success, or negative error code on error */ - int64_t diff_iterate(const char *fromsnapname, - uint64_t ofs, size_t len, - int (*cb)(uint64_t, size_t, bool, void *), void *arg); + int diff_iterate(const char *fromsnapname, + uint64_t ofs, size_t len, + int (*cb)(uint64_t, size_t, int, void *), void *arg); ssize_t write(uint64_t ofs, size_t len, ceph::bufferlist& bl); int discard(uint64_t ofs, uint64_t len); diff --git a/src/librbd/internal.cc b/src/librbd/internal.cc index 4daa5b801db3d..49573966f8ba3 100644 --- a/src/librbd/internal.cc +++ b/src/librbd/internal.cc @@ -2269,18 +2269,22 @@ reprotect_and_return_err: return total_read; } - int simple_diff_cb(uint64_t off, size_t len, bool exists, void *arg) + int simple_diff_cb(uint64_t off, size_t len, int exists, void *arg) { + // This reads the existing extents in a parent from the beginning + // of time. Since images are thin-provisioned, the extents will + // always represent data, not holes. + assert(exists); interval_set *diff = static_cast *>(arg); diff->insert(off, len); return 0; } - int64_t diff_iterate(ImageCtx *ictx, const char *fromsnapname, - uint64_t off, uint64_t len, - int (*cb)(uint64_t, size_t, bool, void *), - void *arg) + int diff_iterate(ImageCtx *ictx, const char *fromsnapname, + uint64_t off, uint64_t len, + int (*cb)(uint64_t, size_t, int, void *), + void *arg) { utime_t start_time, elapsed; @@ -2291,8 +2295,7 @@ reprotect_and_return_err: if (r < 0) return r; - uint64_t mylen = len; - r = clip_io(ictx, off, &mylen); + r = clip_io(ictx, off, &len); if (r < 0) return r; @@ -2347,9 +2350,8 @@ reprotect_and_return_err: return r; } - int64_t total_read = 0; uint64_t period = ictx->get_stripe_period(); - uint64_t left = mylen; + uint64_t left = len; while (left > 0) { uint64_t period_off = off - (off % period); @@ -2385,7 +2387,7 @@ reprotect_and_return_err: o.intersection_of(parent_diff); ldout(ictx->cct, 20) << " reporting parent overlap " << o << dendl; for (interval_set::iterator s = o.begin(); s != o.end(); ++s) { - cb(s.get_start(), s.get_len(), false, arg); + cb(s.get_start(), s.get_len(), true, arg); } } } @@ -2431,7 +2433,7 @@ reprotect_and_return_err: << " logical " << logical_off << "~" << s.get_len() << dendl; - cb(logical_off, s.get_len(), !end_exists, arg); + cb(logical_off, s.get_len(), end_exists, arg); } opos += r->second; } @@ -2439,12 +2441,11 @@ reprotect_and_return_err: } } - total_read += read_len; left -= read_len; off += read_len; } - return total_read; + return 0; } int simple_read_cb(uint64_t ofs, size_t len, const char *buf, void *arg) diff --git a/src/librbd/internal.h b/src/librbd/internal.h index 7f66006e12c43..e407d44af9e0d 100644 --- a/src/librbd/internal.h +++ b/src/librbd/internal.h @@ -168,10 +168,10 @@ namespace librbd { int64_t read_iterate(ImageCtx *ictx, uint64_t off, size_t len, int (*cb)(uint64_t, size_t, const char *, void *), void *arg); - int64_t diff_iterate(ImageCtx *ictx, const char *fromsnapname, - uint64_t off, uint64_t len, - int (*cb)(uint64_t, size_t, bool, void *), - void *arg); + int diff_iterate(ImageCtx *ictx, const char *fromsnapname, + uint64_t off, uint64_t len, + int (*cb)(uint64_t, size_t, int, void *), + void *arg); ssize_t read(ImageCtx *ictx, uint64_t off, size_t len, char *buf); ssize_t read(ImageCtx *ictx, const vector >& image_extents, char *buf, bufferlist *pbl); diff --git a/src/librbd/librbd.cc b/src/librbd/librbd.cc index b522288b6a7d9..20dbf62393b45 100644 --- a/src/librbd/librbd.cc +++ b/src/librbd/librbd.cc @@ -442,10 +442,10 @@ namespace librbd { return librbd::read_iterate(ictx, ofs, len, cb, arg); } - int64_t Image::diff_iterate(const char *fromsnapname, - uint64_t ofs, uint64_t len, - int (*cb)(uint64_t, size_t, bool, void *), - void *arg) + int Image::diff_iterate(const char *fromsnapname, + uint64_t ofs, uint64_t len, + int (*cb)(uint64_t, size_t, int, void *), + void *arg) { ImageCtx *ictx = (ImageCtx *)ctx; return librbd::diff_iterate(ictx, fromsnapname, ofs, len, cb, arg); @@ -1032,6 +1032,16 @@ extern "C" int64_t rbd_read_iterate(rbd_image_t image, uint64_t ofs, size_t len, return librbd::read_iterate(ictx, ofs, len, cb, arg); } +extern "C" int diff_iterate(rbd_image_t image, + const char *fromsnapname, + uint64_t ofs, uint64_t len, + int (*cb)(uint64_t, size_t, int, void *), + void *arg) +{ + librbd::ImageCtx *ictx = (librbd::ImageCtx *)image; + return librbd::diff_iterate(ictx, fromsnapname, ofs, len, cb, arg); +} + extern "C" ssize_t rbd_write(rbd_image_t image, uint64_t ofs, size_t len, const char *buf) { diff --git a/src/rbd.cc b/src/rbd.cc index f6cdb58390517..d776ef018cdf6 100644 --- a/src/rbd.cc +++ b/src/rbd.cc @@ -1035,14 +1035,14 @@ static int do_export(librbd::Image& image, const char *path) return r; } -static int export_diff_cb(uint64_t ofs, size_t _len, bool zero, void *arg) +static int export_diff_cb(uint64_t ofs, size_t _len, int exists, void *arg) { ExportContext *ec = static_cast(arg); int r; // extent bufferlist bl; - __u8 tag = zero ? 'z' : 'w'; + __u8 tag = exists ? 'w' : 'z'; ::encode(tag, bl); ::encode(ofs, bl); uint64_t len = _len; @@ -1051,7 +1051,7 @@ static int export_diff_cb(uint64_t ofs, size_t _len, bool zero, void *arg) if (r < 0) return r; - if (!zero) { + if (exists) { // read block bl.clear(); r = ec->image->read(ofs, len, bl); @@ -1071,7 +1071,7 @@ static int do_export_diff(librbd::Image& image, const char *fromsnapname, const char *endsnapname, const char *path) { - int64_t r; + int r; librbd::image_info_t info; int fd; @@ -1139,7 +1139,7 @@ static int do_export_diff(librbd::Image& image, const char *fromsnapname, return r; } -static int diff_cb(uint64_t ofs, size_t len, bool zero, void *arg) +static int diff_cb(uint64_t ofs, size_t len, int exists, void *arg) { cout << ofs << "\t" << len << "\t" << (zero ? "zero" : "data") << "\n"; @@ -1148,7 +1148,7 @@ static int diff_cb(uint64_t ofs, size_t len, bool zero, void *arg) static int do_diff(librbd::Image& image, const char *fromsnapname) { - int64_t r; + int r; librbd::image_info_t info; r = image.stat(info, sizeof(info)); diff --git a/src/test/librbd/test_librbd.cc b/src/test/librbd/test_librbd.cc index ff083ae817f48..c24f41ec95343 100644 --- a/src/test/librbd/test_librbd.cc +++ b/src/test/librbd/test_librbd.cc @@ -1503,7 +1503,7 @@ TEST(LibRBD, FlushAioPP) } -int iterate_cb(uint64_t off, size_t len, bool zero, void *arg) +int iterate_cb(uint64_t off, size_t len, int exists, void *arg) { cout << "iterate_cb " << off << "~" << len << std::endl; interval_set *diff = static_cast *>(arg); @@ -1564,7 +1564,7 @@ TEST(LibRBD, DiffIterate) cout << " wrote " << two << std::endl; interval_set diff; - ASSERT_EQ((int)size, image.diff_iterate("one", 0, size, iterate_cb, (void *)&diff)); + ASSERT_EQ(0, image.diff_iterate("one", 0, size, iterate_cb, (void *)&diff)); cout << " diff was " << diff << std::endl; if (!two.subset_of(diff)) { interval_set i; @@ -1620,7 +1620,7 @@ TEST(LibRBD, DiffIterateStress) cout << "from " << i << " to " << j << " diff " << diff << std::endl; image.snap_set(snap[j].c_str()); - ASSERT_EQ((int)size, image.diff_iterate(snap[i].c_str(), 0, size, iterate_cb, (void *)&actual)); + ASSERT_EQ(0, image.diff_iterate(snap[i].c_str(), 0, size, iterate_cb, (void *)&actual)); cout << " actual was " << actual << std::endl; if (!diff.subset_of(actual)) { interval_set i; -- 2.39.5