From a8330f5cfddaab853a1844afe43ee9a71f96d0c3 Mon Sep 17 00:00:00 2001 From: Josh Durgin Date: Tue, 8 Apr 2014 17:38:50 -0700 Subject: [PATCH] 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 --- src/librbd/internal.cc | 19 ++++---- src/test/librbd/test_librbd.cc | 82 ++++++++++++++++++++++++++++++++++ 2 files changed, 92 insertions(+), 9 deletions(-) diff --git a/src/librbd/internal.cc b/src/librbd/internal.cc index 8056fab587b62..127be38cf3468 100644 --- 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 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::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 >::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 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 d0b9c994f12f5..7f354187277ad 100644 --- 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); -- 2.39.5