Merge pull request #10985 from dillaman/wip-16921

rbd-nbd: fix kernel deadlock during teuthology testing

Reviewed-by: Mykola Golub <mgolub@mirantis.com>

src/librbd/operation/ResizeRequest.cc

@@ -175,6 +175,38 @@ Context *ResizeRequest<I>::handle_trim_image(int *result) {
     return this->create_context_finisher(*result);
   }
 
+  send_post_block_writes();
+  return nullptr;
+}
+
+template <typename I>
+void ResizeRequest<I>::send_flush_cache() {
+  I &image_ctx = this->m_image_ctx;
+  if (image_ctx.object_cacher == nullptr) {
+    send_trim_image();
+    return;
+  }
+
+  CephContext *cct = image_ctx.cct;
+  ldout(cct, 5) << this << " " << __func__ << dendl;
+
+  RWLock::RLocker owner_locker(image_ctx.owner_lock);
+  image_ctx.flush_cache(create_async_context_callback(
+    image_ctx, create_context_callback<
+      ResizeRequest<I>, &ResizeRequest<I>::handle_flush_cache>(this)));
+}
+
+template <typename I>
+Context *ResizeRequest<I>::handle_flush_cache(int *result) {
+  I &image_ctx = this->m_image_ctx;
+  CephContext *cct = image_ctx.cct;
+  ldout(cct, 5) << this << " " << __func__ << ": r=" << *result << dendl;
+
+  if (*result < 0) {
+    lderr(cct) << "failed to flush cache: " << cpp_strerror(*result) << dendl;
+    return this->create_context_finisher(*result);
+  }
+
   send_invalidate_cache();
   return nullptr;
 }
@@ -199,13 +231,16 @@ Context *ResizeRequest<I>::handle_invalidate_cache(int *result) {
   CephContext *cct = image_ctx.cct;
   ldout(cct, 5) << this << " " << __func__ << ": r=" << *result << dendl;
 
-  if (*result < 0) {
+  // ignore busy error -- writeback was successfully flushed so we might be
+  // wasting some cache space for trimmed objects, but they will get purged
+  // eventually. Most likely cause of the issue was a in-flight cache read
+  if (*result < 0 && *result != -EBUSY) {
     lderr(cct) << "failed to invalidate cache: " << cpp_strerror(*result)
                << dendl;
     return this->create_context_finisher(*result);
   }
 
-  send_post_block_writes();
+  send_trim_image();
   return nullptr;
 }
 
@@ -222,7 +257,7 @@ Context *ResizeRequest<I>::send_grow_object_map() {
   if (m_original_size == m_new_size) {
     return this->create_context_finisher(0);
   } else if (m_new_size < m_original_size) {
-    send_trim_image();
+    send_flush_cache();
     return nullptr;
   }
 

src/librbd/operation/ResizeRequest.h

@@ -80,12 +80,15 @@ class ResizeRequest : public Request<ImageCtxT> {
    *    |           STATE_UPDATE_HEADER ----------------------------\
    *    |                                                           |
    *    | (shrink)                                                  |
-   *    |\--------> STATE_TRIM_IMAGE                                |
+   *    |\--------> STATE_FLUSH_CACHE                               |
    *    |                 |                                         |
    *    |                 v                                         |
    *    |           STATE_INVALIDATE_CACHE                          |
    *    |                 |                                         |
    *    |                 v                                         |
+   *    |           STATE_TRIM_IMAGE                                |
+   *    |                 |                                         |
+   *    |                 v                                         |
    *    |           STATE_POST_BLOCK_WRITES                         |
    *    |                 |                                         |
    *    |                 v                                         |
@@ -121,6 +124,9 @@ class ResizeRequest : public Request<ImageCtxT> {
   Context *send_append_op_event();
   Context *handle_append_op_event(int *result);
 
+  void send_flush_cache();
+  Context *handle_flush_cache(int *result);
+
   void send_invalidate_cache();
   Context *handle_invalidate_cache(int *result);
 

src/test/librbd/mock/MockImageCtx.h

@@ -158,6 +158,7 @@ struct MockImageCtx {
   MOCK_METHOD1(flush_async_operations, void(Context *));
   MOCK_METHOD1(flush_copyup, void(Context *));
 
+  MOCK_METHOD1(flush_cache, void(Context *));
   MOCK_METHOD1(invalidate_cache, void(Context *));
   MOCK_METHOD1(shut_down_cache, void(Context *));
 

src/test/librbd/operation/test_mock_ResizeRequest.cc

@@ -116,6 +116,11 @@ class TestMockOperationResizeRequest : public TestMockFixture {
                   .WillOnce(FinishRequest(&mock_trim_request, r, &mock_image_ctx));
   }
 
+  void expect_flush_cache(MockImageCtx &mock_image_ctx, int r) {
+    EXPECT_CALL(mock_image_ctx, flush_cache(_)).WillOnce(CompleteContext(r, NULL));
+    expect_op_work_queue(mock_image_ctx);
+  }
+
   void expect_invalidate_cache(MockImageCtx &mock_image_ctx, int r) {
     EXPECT_CALL(mock_image_ctx, invalidate_cache(_))
                   .WillOnce(CompleteContext(r, NULL));
@@ -203,8 +208,9 @@ TEST_F(TestMockOperationResizeRequest, ShrinkSuccess) {
   expect_unblock_writes(mock_image_ctx);
 
   MockTrimRequest mock_trim_request;
-  expect_trim(mock_image_ctx, mock_trim_request, 0);
+  expect_flush_cache(mock_image_ctx, 0);
   expect_invalidate_cache(mock_image_ctx, 0);
+  expect_trim(mock_image_ctx, mock_trim_request, 0);
   expect_block_writes(mock_image_ctx, 0);
   expect_update_header(mock_image_ctx, 0);
   expect_shrink_object_map(mock_image_ctx);
@@ -264,11 +270,35 @@ TEST_F(TestMockOperationResizeRequest, TrimError) {
   expect_unblock_writes(mock_image_ctx);
 
   MockTrimRequest mock_trim_request;
+  expect_flush_cache(mock_image_ctx, 0);
+  expect_invalidate_cache(mock_image_ctx, -EBUSY);
   expect_trim(mock_image_ctx, mock_trim_request, -EINVAL);
   expect_commit_op_event(mock_image_ctx, -EINVAL);
   ASSERT_EQ(-EINVAL, when_resize(mock_image_ctx, ictx->size / 2, true, 0, false));
 }
 
+TEST_F(TestMockOperationResizeRequest, FlushCacheError) {
+  librbd::ImageCtx *ictx;
+  ASSERT_EQ(0, open_image(m_image_name, &ictx));
+
+  MockImageCtx mock_image_ctx(*ictx);
+  MockExclusiveLock mock_exclusive_lock;
+  MockJournal mock_journal;
+  MockObjectMap mock_object_map;
+  initialize_features(ictx, mock_image_ctx, mock_exclusive_lock, mock_journal,
+                      mock_object_map);
+
+  InSequence seq;
+  expect_block_writes(mock_image_ctx, 0);
+  expect_append_op_event(mock_image_ctx, true, 0);
+  expect_unblock_writes(mock_image_ctx);
+
+  MockTrimRequest mock_trim_request;
+  expect_flush_cache(mock_image_ctx, -EINVAL);
+  expect_commit_op_event(mock_image_ctx, -EINVAL);
+  ASSERT_EQ(-EINVAL, when_resize(mock_image_ctx, ictx->size / 2, true, 0, false));
+}
+
 TEST_F(TestMockOperationResizeRequest, InvalidateCacheError) {
   librbd::ImageCtx *ictx;
   ASSERT_EQ(0, open_image(m_image_name, &ictx));
@@ -286,7 +316,7 @@ TEST_F(TestMockOperationResizeRequest, InvalidateCacheError) {
   expect_unblock_writes(mock_image_ctx);
 
   MockTrimRequest mock_trim_request;
-  expect_trim(mock_image_ctx, mock_trim_request, 0);
+  expect_flush_cache(mock_image_ctx, 0);
   expect_invalidate_cache(mock_image_ctx, -EINVAL);
   expect_commit_op_event(mock_image_ctx, -EINVAL);
   ASSERT_EQ(-EINVAL, when_resize(mock_image_ctx, ictx->size / 2, true, 0, false));

src/tools/rbd_nbd/rbd-nbd.cc

@@ -185,13 +185,21 @@ class NBDServer
 
     dout(20) << __func__ << ": " << *ctx << dendl;
 
+    if (ret == -EINVAL) {
+      // if shrinking an image, a pagecache writeback might reference
+      // extents outside of the range of the new image extents
+      dout(5) << __func__ << ": masking IO out-of-bounds error" << dendl;
+      ctx->data.clear();
+      ret = 0;
+    }
+
     if (ret < 0) {
       ctx->reply.error = htonl(-ret);
     } else if ((ctx->command == NBD_CMD_READ) &&
                 ret < static_cast<int>(ctx->request.len)) {
       int pad_byte_count = static_cast<int> (ctx->request.len) - ret;
       ctx->data.append_zero(pad_byte_count);
-      dout(20) << __func__ << ": " << *ctx << ": Pad byte count: " 
+      dout(20) << __func__ << ": " << *ctx << ": Pad byte count: "
                << pad_byte_count << dendl;
       ctx->reply.error = 0;
     } else {
@@ -236,6 +244,7 @@ class NBDServer
       switch (ctx->command)
       {
         case NBD_CMD_DISC:
+          // RBD_DO_IT will return when pipe is closed
 	  dout(0) << "disconnect request received" << dendl;
           return;
         case NBD_CMD_WRITE:
@@ -650,9 +659,10 @@ static int do_unmap()
     return nbd;
   }
 
-  if (ioctl(nbd, NBD_DISCONNECT) < 0)
+  // alert the reader thread to shut down
+  if (ioctl(nbd, NBD_DISCONNECT) < 0) {
     cerr << "rbd-nbd: the device is not used" << std::endl;
-  ioctl(nbd, NBD_CLEAR_SOCK);
+  }
   close(nbd);
 
   return 0;
@@ -727,6 +737,7 @@ static int rbd_nbd(int argc, const char *argv[])
   env_to_vec(args);
   global_init(NULL, args, CEPH_ENTITY_TYPE_CLIENT, CODE_ENVIRONMENT_DAEMON,
               CINIT_FLAG_UNPRIVILEGED_DAEMON_DEFAULTS);
+  g_ceph_context->_conf->set_val_or_die("pid_file", "");
 
   std::vector<const char*>::iterator i;
   std::ostringstream err;