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 <josh.durgin@inktank.com>

diff --git a/src/include/rbd/librbd.hpp b/src/include/rbd/librbd.hpp
index d061bdbe4eb867012083b9e6bcde9ed9399c6eaa..600a3ccbcda009d056d1eb9430ce4b962be4609f 100644 (file)
--- 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 4daa5b801db3ddd427efcd847e7395567d144379..49573966f8ba341f2ea4a18d25cb45272367da96 100644 (file)
--- 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<uint64_t> *diff = static_cast<interval_set<uint64_t> *>(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<uint64_t>::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 7f66006e12c43e7bc6e46d8499e4b70a2dbe2717..e407d44af9e0ddb13d5219e1b04c7507b3820955 100644 (file)
--- 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<pair<uint64_t,uint64_t> >& image_extents,
               char *buf, bufferlist *pbl);

diff --git a/src/librbd/librbd.cc b/src/librbd/librbd.cc
index b522288b6a7d97bf600fef71142ce1c661cfb681..20dbf62393b451fcdfd6b3646b57b4b7a88923bc 100644 (file)
--- 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 f6cdb5839051725290bb4470aa342c6fd22fc6b7..d776ef018cdf6fbda6f50576172b63ff0849c9b6 100644 (file)
--- 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<ExportContext *>(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 ff083ae817f4846b24a1e5e171ee3961ffcdec5a..c24f41ec9534342aac8d4bedd8c4398834b08d3c 100644 (file)
--- 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<uint64_t> *diff = static_cast<interval_set<uint64_t> *>(arg);
@@ -1564,7 +1564,7 @@ TEST(LibRBD, DiffIterate)
     cout << " wrote " << two << std::endl;
 
     interval_set<uint64_t> 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<uint64_t> 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<uint64_t> i;