librbd: fix zero length request handling

Zero-length writes would hang because the completion was never
called. Reads would hit an assert about zero length in
Striper::file_to_exents().

Fix all of these cases by skipping zero-length extents. The completion
is created and finished when finish_adding_requests() is called. This
is slightly different from usual completions since it comes from the
same thread as the one scheduling the request, but zero-length aio
requests should never happen from things that might care about this,
like QEMU.

Writes and discards have had this bug since the beginning of
librbd. Reads might have avoided it until stripingv2 was added.

Fixes: #5469
Signed-off-by: Josh Durgin <josh.durgin@inktank.com>

diff --git a/src/librbd/internal.cc b/src/librbd/internal.cc
index 8056fab587b62f772104dd4efe8918e72161ec40..127be38cf34682a55eda15d997a339d6bafd700b 100644 (file)
--- a/src/librbd/internal.cc
+++ b/src/librbd/internal.cc
@@ -2884,9 +2884,6 @@ reprotect_and_return_err:
     ldout(cct, 20) << "aio_write " << ictx << " off = " << off << " len = "
                   << len << " buf = " << (void*)buf << dendl;
 
-    if (!len)
-      return 0;
-
     int r = ictx_check(ictx);
     if (r < 0)
       return r;
@@ -2912,14 +2909,16 @@ reprotect_and_return_err:
 
     // map
     vector<ObjectExtent> extents;
-    Striper::file_to_extents(ictx->cct, ictx->format_string, &ictx->layout, off, mylen, 0, extents);
+    if (len > 0) {
+      Striper::file_to_extents(ictx->cct, ictx->format_string,
+                              &ictx->layout, off, mylen, 0, extents);
+    }
 
     c->get();
     c->init_time(ictx, AIO_TYPE_WRITE);
     for (vector<ObjectExtent>::iterator p = extents.begin(); p != extents.end(); ++p) {
       ldout(cct, 20) << " oid " << p->oid << " " << p->offset << "~" << p->length
                     << " from " << p->buffer_extents << dendl;
-
       // assemble extent
       bufferlist bl;
       for (vector<pair<uint64_t,uint64_t> >::iterator q = p->buffer_extents.begin();
@@ -2966,9 +2965,6 @@ reprotect_and_return_err:
     ldout(cct, 20) << "aio_discard " << ictx << " off = " << off << " len = "
                   << len << dendl;
 
-    if (!len)
-      return 0;
-
     int r = ictx_check(ictx);
     if (r < 0)
       return r;
@@ -2992,7 +2988,10 @@ reprotect_and_return_err:
 
     // map
     vector<ObjectExtent> extents;
-    Striper::file_to_extents(ictx->cct, ictx->format_string, &ictx->layout, off, len, 0, extents);
+    if (len > 0) {
+      Striper::file_to_extents(ictx->cct, ictx->format_string,
+                              &ictx->layout, off, len, 0, extents);
+    }
 
     c->get();
     c->init_time(ictx, AIO_TYPE_DISCARD);
@@ -3086,6 +3085,8 @@ reprotect_and_return_err:
       r = clip_io(ictx, p->first, &len);
       if (r < 0)
        return r;
+      if (len == 0)
+       continue;
 
       Striper::file_to_extents(ictx->cct, ictx->format_string, &ictx->layout,
                               p->first, len, 0, object_extents, buffer_ofs);

diff --git a/src/test/librbd/test_librbd.cc b/src/test/librbd/test_librbd.cc
index d0b9c994f12f563585937089ac6aed9d3cdaff58..7f354187277ade26ab78aafa9b7dd64149baa00b 100644 (file)
--- a/src/test/librbd/test_librbd.cc
+++ b/src/test/librbd/test_librbd.cc
@@ -1777,6 +1777,88 @@ TEST(LibRBD, DiffIterateStress)
   ASSERT_EQ(0, destroy_one_pool_pp(pool_name, rados));
 }
 
+TEST(LibRBD, ZeroLengthWrite)
+{
+  rados_t cluster;
+  rados_ioctx_t ioctx;
+  string pool_name = get_temp_pool_name();
+  ASSERT_EQ("", create_one_pool(pool_name, &cluster));
+  rados_ioctx_create(cluster, pool_name.c_str(), &ioctx);
+
+  rbd_image_t image;
+  int order = 0;
+  const char *name = "testimg";
+  uint64_t size = 2 << 20;
+
+  ASSERT_EQ(0, create_image(ioctx, name, size, &order));
+  ASSERT_EQ(0, rbd_open(ioctx, name, &image, NULL));
+
+  char read_data[1];
+  ASSERT_EQ(0, rbd_write(image, 0, 0, NULL));
+  ASSERT_EQ(1, rbd_read(image, 0, 1, read_data));
+  ASSERT_EQ('\0', read_data[0]);
+
+  ASSERT_EQ(0, rbd_close(image));
+
+  rados_ioctx_destroy(ioctx);
+  ASSERT_EQ(0, destroy_one_pool(pool_name, &cluster));
+}
+
+
+TEST(LibRBD, ZeroLengthDiscard)
+{
+  rados_t cluster;
+  rados_ioctx_t ioctx;
+  string pool_name = get_temp_pool_name();
+  ASSERT_EQ("", create_one_pool(pool_name, &cluster));
+  rados_ioctx_create(cluster, pool_name.c_str(), &ioctx);
+
+  rbd_image_t image;
+  int order = 0;
+  const char *name = "testimg";
+  uint64_t size = 2 << 20;
+
+  ASSERT_EQ(0, create_image(ioctx, name, size, &order));
+  ASSERT_EQ(0, rbd_open(ioctx, name, &image, NULL));
+
+  const char *data = "blah";
+  char read_data[strlen(data)];
+  ASSERT_EQ((int)strlen(data), rbd_write(image, 0, strlen(data), data));
+  ASSERT_EQ(0, rbd_discard(image, 0, 0));
+  ASSERT_EQ((int)strlen(data), rbd_read(image, 0, strlen(data), read_data));
+  ASSERT_EQ(0, memcmp(data, read_data, strlen(data)));
+
+  ASSERT_EQ(0, rbd_close(image));
+
+  rados_ioctx_destroy(ioctx);
+  ASSERT_EQ(0, destroy_one_pool(pool_name, &cluster));
+}
+
+TEST(LibRBD, ZeroLengthRead)
+{
+  rados_t cluster;
+  rados_ioctx_t ioctx;
+  string pool_name = get_temp_pool_name();
+  ASSERT_EQ("", create_one_pool(pool_name, &cluster));
+  rados_ioctx_create(cluster, pool_name.c_str(), &ioctx);
+
+  rbd_image_t image;
+  int order = 0;
+  const char *name = "testimg";
+  uint64_t size = 2 << 20;
+
+  ASSERT_EQ(0, create_image(ioctx, name, size, &order));
+  ASSERT_EQ(0, rbd_open(ioctx, name, &image, NULL));
+
+  char read_data[1];
+  ASSERT_EQ(0, rbd_read(image, 0, 0, read_data));
+
+  ASSERT_EQ(0, rbd_close(image));
+
+  rados_ioctx_destroy(ioctx);
+  ASSERT_EQ(0, destroy_one_pool(pool_name, &cluster));
+}
+
 int main(int argc, char **argv)
 {
   ::testing::InitGoogleTest(&argc, argv);