From: lixiaoy1 <xiaoyan.li@intel.com>
Date: Mon, 21 Sep 2020 15:03:09 +0000 (-0400)
Subject: rbd/io: split IO check
X-Git-Tag: v16.1.0~951^2~1
X-Git-Url: http://git-server-git.apps.pok.os.sepia.ceph.com/?a=commitdiff_plain;h=f18ca093b6a5375db469444d2cf2b006ca5d2a48;p=ceph.git

rbd/io: split IO check

Move IO check to interfaces in Utils.

Signed-off-by: Li, Xiaoyan <xiaoyan.li@intel.com>
---

diff --git a/src/librbd/internal.cc b/src/librbd/internal.cc
index d8bd8b7147e6..d895d1f35e6d 100644
--- a/src/librbd/internal.cc
+++ b/src/librbd/internal.cc
@@ -1571,11 +1571,17 @@ int validate_pool(IoCtx &io_ctx, CephContext *cct) {
   int clip_io(ImageCtx *ictx, uint64_t off, uint64_t *len)
   {
     ceph_assert(ceph_mutex_is_locked(ictx->image_lock));
-    uint64_t image_size = ictx->get_image_size(ictx->snap_id);
-    bool snap_exists = ictx->snap_exists;
 
-    if (!snap_exists)
-      return -ENOENT;
+    uint64_t image_size;
+    if (ictx->snap_id == CEPH_NOSNAP) {
+      image_size = ictx->get_image_size(CEPH_NOSNAP);
+    } else {
+      auto snap_info = ictx->get_snap_info(ictx->snap_id);
+      if (snap_info == nullptr) {
+	return -ENOENT;
+      }
+      image_size = snap_info->size;
+    }
 
     // special-case "len == 0" requests: always valid
     if (*len == 0)
diff --git a/src/librbd/io/ImageDispatcher.cc b/src/librbd/io/ImageDispatcher.cc
index 6c2863bccbb6..49f3bfc5ad04 100644
--- a/src/librbd/io/ImageDispatcher.cc
+++ b/src/librbd/io/ImageDispatcher.cc
@@ -14,6 +14,7 @@
 #include "librbd/io/QueueImageDispatch.h"
 #include "librbd/io/QosImageDispatch.h"
 #include "librbd/io/RefreshImageDispatch.h"
+#include "librbd/io/Utils.h"
 #include "librbd/io/WriteBlockImageDispatch.h"
 #include <boost/variant.hpp>
 
@@ -122,6 +123,59 @@ struct ImageDispatcher<I>::SendVisitor : public boost::static_visitor<bool> {
   }
 };
 
+template <typename I>
+struct ImageDispatcher<I>::PreprocessVisitor
+  : public boost::static_visitor<bool> {
+  ImageDispatcher<I>* image_dispatcher;
+  ImageDispatchSpec* image_dispatch_spec;
+
+  PreprocessVisitor(ImageDispatcher<I>* image_dispatcher,
+                    ImageDispatchSpec* image_dispatch_spec)
+    : image_dispatcher(image_dispatcher),
+      image_dispatch_spec(image_dispatch_spec) {
+  }
+
+  bool clip_request() const {
+    int r = util::clip_request(image_dispatcher->m_image_ctx,
+                               &image_dispatch_spec->image_extents);
+    if (r < 0) {
+      image_dispatch_spec->fail(r);
+      return true;
+    }
+    return false;
+  }
+
+  bool operator()(ImageDispatchSpec::Read& read) const {
+    if ((read.read_flags & READ_FLAG_DISABLE_CLIPPING) != 0) {
+      return false;
+    }
+    return clip_request();
+  }
+
+  bool operator()(ImageDispatchSpec::Flush&) const {
+    return clip_request();
+  }
+
+  bool operator()(ImageDispatchSpec::ListSnaps&) const {
+    return false;
+  }
+
+  template <typename T>
+  bool operator()(T&) const {
+    if (clip_request()) {
+      return true;
+    }
+
+    std::shared_lock image_locker{image_dispatcher->m_image_ctx->image_lock};
+    if (image_dispatcher->m_image_ctx->snap_id != CEPH_NOSNAP ||
+        image_dispatcher->m_image_ctx->read_only) {
+      image_dispatch_spec->fail(-EROFS);
+      return true;
+    }
+    return false;
+  }
+};
+
 template <typename I>
 ImageDispatcher<I>::ImageDispatcher(I* image_ctx)
   : Dispatcher<I, ImageDispatcherInterface>(image_ctx) {
@@ -203,6 +257,11 @@ bool ImageDispatcher<I>::send_dispatch(
     ImageDispatchSpec* image_dispatch_spec) {
   if (image_dispatch_spec->tid == 0) {
     image_dispatch_spec->tid = ++m_next_tid;
+
+    bool finished = preprocess(image_dispatch_spec);
+    if (finished) {
+      return true;
+    }
   }
 
   return boost::apply_visitor(
@@ -210,6 +269,14 @@ bool ImageDispatcher<I>::send_dispatch(
     image_dispatch_spec->request);
 }
 
+template <typename I>
+bool ImageDispatcher<I>::preprocess(
+    ImageDispatchSpec* image_dispatch_spec) {
+  return boost::apply_visitor(
+    PreprocessVisitor{this, image_dispatch_spec},
+    image_dispatch_spec->request);
+}
+
 } // namespace io
 } // namespace librbd
 
diff --git a/src/librbd/io/ImageDispatcher.h b/src/librbd/io/ImageDispatcher.h
index 176b2a229fe9..7ab248abdf7c 100644
--- a/src/librbd/io/ImageDispatcher.h
+++ b/src/librbd/io/ImageDispatcher.h
@@ -50,12 +50,15 @@ protected:
 
 private:
   struct SendVisitor;
+  struct PreprocessVisitor;
 
   std::atomic<uint64_t> m_next_tid{0};
 
   QosImageDispatch<ImageCtxT>* m_qos_image_dispatch = nullptr;
   WriteBlockImageDispatch<ImageCtxT>* m_write_block_dispatch = nullptr;
 
+  bool preprocess(ImageDispatchSpec* image_dispatch_spec);
+
 };
 
 } // namespace io
diff --git a/src/librbd/io/ImageRequest.cc b/src/librbd/io/ImageRequest.cc
index 09a37c50bf4d..fc88357e2063 100644
--- a/src/librbd/io/ImageRequest.cc
+++ b/src/librbd/io/ImageRequest.cc
@@ -313,16 +313,6 @@ void ImageRequest<I>::send() {
   ldout(cct, 20) << get_request_type() << ": ictx=" << &image_ctx << ", "
                  << "completion=" << aio_comp << dendl;
 
-  int r = clip_request();
-  if (r < 0) {
-    m_aio_comp->fail(r);
-    return;
-  }
-
-  if (finish_request_early()) {
-    return;
-  }
-
   if (m_bypass_image_cache || m_image_ctx.image_cache == nullptr) {
     update_timestamp();
     send_request();
@@ -331,41 +321,6 @@ void ImageRequest<I>::send() {
   }
 }
 
-template <typename I>
-int ImageRequest<I>::clip_request() {
-  std::shared_lock image_locker{m_image_ctx.image_lock};
-  for (auto &image_extent : m_image_extents) {
-    auto clip_len = image_extent.second;
-    int r = clip_io(get_image_ctx(&m_image_ctx), image_extent.first, &clip_len);
-    if (r < 0) {
-      return r;
-    }
-
-    image_extent.second = clip_len;
-  }
-  return 0;
-}
-
-template <typename I>
-bool ImageRequest<I>::finish_request_early() {
-  auto total_bytes = get_total_length();
-  if (total_bytes == 0) {
-    auto *aio_comp = this->m_aio_comp;
-    aio_comp->set_request_count(0);
-    return true;
-  }
-  return false;
-}
-
-template <typename I>
-uint64_t ImageRequest<I>::get_total_length() const {
-  uint64_t total_bytes = 0;
-  for (auto& image_extent : this->m_image_extents) {
-    total_bytes += image_extent.second;
-  }
-  return total_bytes;
-}
-
 template <typename I>
 void ImageRequest<I>::update_timestamp() {
   bool modify = (get_aio_type() != AIO_TYPE_READ);
@@ -431,20 +386,6 @@ ImageReadRequest<I>::ImageReadRequest(I &image_ctx, AioCompletion *aio_comp,
   aio_comp->read_result = std::move(read_result);
 }
 
-template <typename I>
-int ImageReadRequest<I>::clip_request() {
-  if ((m_read_flags & READ_FLAG_DISABLE_CLIPPING) != 0) {
-    return 0;
-  }
-
-  int r = ImageRequest<I>::clip_request();
-  if (r < 0) {
-    return r;
-  }
-
-  return 0;
-}
-
 template <typename I>
 void ImageReadRequest<I>::send_request() {
   I &image_ctx = this->m_image_ctx;
@@ -508,21 +449,6 @@ void ImageReadRequest<I>::send_image_cache_request() {
                                   req_comp);
 }
 
-template <typename I>
-bool AbstractImageWriteRequest<I>::finish_request_early() {
-  AioCompletion *aio_comp = this->m_aio_comp;
-  {
-    std::shared_lock image_locker{this->m_image_ctx.image_lock};
-    if (this->m_image_ctx.snap_id != CEPH_NOSNAP ||
-        this->m_image_ctx.read_only) {
-      aio_comp->fail(-EROFS);
-      return true;
-    }
-  }
-
-  return ImageRequest<I>::finish_request_early();
-}
-
 template <typename I>
 void AbstractImageWriteRequest<I>::send_request() {
   I &image_ctx = this->m_image_ctx;
@@ -988,13 +914,6 @@ ImageListSnapsRequest<I>::ImageListSnapsRequest(
                     parent_trace),
     m_snap_ids(std::move(snap_ids)), m_list_snaps_flags(list_snaps_flags),
     m_snapshot_delta(snapshot_delta) {
-  this->set_bypass_image_cache();
-}
-
-template <typename I>
-int ImageListSnapsRequest<I>::clip_request() {
-  // permit arbitrary list-snaps requests (internal API)
-  return 0;
 }
 
 template <typename I>
@@ -1042,11 +961,6 @@ void ImageListSnapsRequest<I>::send_request() {
   }
 }
 
-template <typename I>
-void ImageListSnapsRequest<I>::send_image_cache_request() {
-  ceph_abort();
-}
-
 } // namespace io
 } // namespace librbd
 
diff --git a/src/librbd/io/ImageRequest.h b/src/librbd/io/ImageRequest.h
index dabda6b6d85a..49787874c501 100644
--- a/src/librbd/io/ImageRequest.h
+++ b/src/librbd/io/ImageRequest.h
@@ -91,11 +91,6 @@ protected:
     m_trace.event("start");
   }
 
-  uint64_t get_total_length() const;
-
-  virtual int clip_request();
-  virtual bool finish_request_early();
-
   virtual void update_timestamp();
   virtual void send_request() = 0;
   virtual void send_image_cache_request() = 0;
@@ -115,8 +110,6 @@ public:
                    const ZTracer::Trace &parent_trace);
 
 protected:
-  int clip_request() override;
-
   void send_request() override;
   void send_image_cache_request() override;
 
@@ -151,8 +144,6 @@ protected:
       m_synchronous(false) {
   }
 
-  bool finish_request_early() override;
-
   void send_request() override;
 
   virtual int prune_object_extents(
@@ -269,13 +260,6 @@ public:
 protected:
   using typename ImageRequest<ImageCtxT>::ObjectRequests;
 
-  int clip_request() override {
-    return 0;
-  }
-  bool finish_request_early() override {
-    return false;
-  }
-
   void update_timestamp() override {
   }
   void send_request() override;
@@ -387,11 +371,8 @@ public:
       SnapshotDelta* snapshot_delta, const ZTracer::Trace& parent_trace);
 
 protected:
-  int clip_request() override;
-
   void update_timestamp() override {}
   void send_request() override;
-  void send_image_cache_request() override;
 
   aio_type_t get_aio_type() const override {
     return AIO_TYPE_GENERIC;
diff --git a/src/librbd/io/Utils.cc b/src/librbd/io/Utils.cc
index 797006471ec3..ffa5de47b931 100644
--- a/src/librbd/io/Utils.cc
+++ b/src/librbd/io/Utils.cc
@@ -6,6 +6,8 @@
 #include "include/buffer.h"
 #include "include/rados/librados.hpp"
 #include "include/neorados/RADOS.hpp"
+#include "librbd/internal.h"
+#include "librbd/Utils.h"
 #include "librbd/io/AioCompletion.h"
 #include "librbd/io/ImageRequest.h"
 #include "osd/osd_types.h"
@@ -122,6 +124,30 @@ void read_parent(I *image_ctx, uint64_t object_no, const Extents &extents,
     image_ctx->parent->get_data_io_context(), 0, 0, trace);
 }
 
+template <typename I>
+int clip_request(I *image_ctx, Extents *image_extents) {
+  std::shared_lock image_locker{image_ctx->image_lock};
+  for (auto &image_extent : *image_extents) {
+    auto clip_len = image_extent.second;
+    int r = clip_io(librbd::util::get_image_ctx(image_ctx),
+                    image_extent.first, &clip_len);
+    if (r < 0) {
+      return r;
+    }
+
+    image_extent.second = clip_len;
+  }
+  return 0;
+}
+
+uint64_t extents_length(Extents &extents) {
+  uint64_t total_bytes = 0;
+  for (auto& image_extent : extents) {
+    total_bytes += image_extent.second;
+  }
+  return total_bytes;
+}
+
 } // namespace util
 } // namespace io
 } // namespace librbd
@@ -130,3 +156,5 @@ template void librbd::io::util::read_parent(
     librbd::ImageCtx *image_ctx, uint64_t object_no, const Extents &extents,
     librados::snap_t snap_id, const ZTracer::Trace &trace,
     ceph::bufferlist* data, Context* on_finish);
+template int librbd::io::util::clip_request(
+    librbd::ImageCtx *image_ctx, Extents *image_extents);
diff --git a/src/librbd/io/Utils.h b/src/librbd/io/Utils.h
index 81161adc8313..63e0affcd1b4 100644
--- a/src/librbd/io/Utils.h
+++ b/src/librbd/io/Utils.h
@@ -34,6 +34,11 @@ void read_parent(ImageCtxT *image_ctx, uint64_t object_no, const Extents &extent
                  librados::snap_t snap_id, const ZTracer::Trace &trace,
                  ceph::bufferlist* data, Context* on_finish);
 
+template <typename ImageCtxT = librbd::ImageCtx>
+int clip_request(ImageCtxT *image_ctx, Extents *image_extents);
+
+uint64_t extents_length(Extents &extents);
+
 } // namespace util
 } // namespace io
 } // namespace librbd
diff --git a/src/test/librbd/io/test_mock_ImageRequest.cc b/src/test/librbd/io/test_mock_ImageRequest.cc
index 968207fb2182..e39409e68a80 100644
--- a/src/test/librbd/io/test_mock_ImageRequest.cc
+++ b/src/test/librbd/io/test_mock_ImageRequest.cc
@@ -196,40 +196,6 @@ TEST_F(TestMockIoImageRequest, AioWriteModifyTimestamp) {
   ASSERT_EQ(0, aio_comp_ctx_2.wait());
 }
 
-TEST_F(TestMockIoImageRequest, AioWriteEarlyFinish) {
-  librbd::ImageCtx *ictx;
-  ASSERT_EQ(0, open_image(m_image_name, &ictx));
-
-  MockTestImageCtx mock_image_ctx(*ictx);
-
-  C_SaferCond aio_comp_ctx_1, aio_comp_ctx_2;
-  AioCompletion *aio_comp_1 = AioCompletion::create_and_start(
-    &aio_comp_ctx_1, ictx, AIO_TYPE_WRITE);
-  AioCompletion *aio_comp_2 = AioCompletion::create_and_start(
-    &aio_comp_ctx_2, ictx, AIO_TYPE_WRITE);
-
-  bufferlist bl;
-  MockImageWriteRequest mock_aio_image_write_1(
-    mock_image_ctx, aio_comp_1, {{0, 0}}, std::move(bl),
-    mock_image_ctx.get_data_io_context(), 0, {});
-  {
-    std::shared_lock owner_locker{mock_image_ctx.owner_lock};
-    mock_aio_image_write_1.send();
-  }
-  ASSERT_EQ(0, aio_comp_ctx_1.wait());
-
-  mock_image_ctx.snap_id = 123;
-  bl.append("1");
-  MockImageWriteRequest mock_aio_image_write_2(
-    mock_image_ctx, aio_comp_2, {{0, 1}}, std::move(bl),
-    mock_image_ctx.get_data_io_context(), 0, {});
-  {
-    std::shared_lock owner_locker{mock_image_ctx.owner_lock};
-    mock_aio_image_write_2.send();
-  }
-  ASSERT_EQ(-EROFS, aio_comp_ctx_2.wait());
-}
-
 TEST_F(TestMockIoImageRequest, AioReadAccessTimestamp) {
   REQUIRE_FORMAT_V2();
 
diff --git a/src/test/librbd/test_librbd.cc b/src/test/librbd/test_librbd.cc
index f62065099fad..5c3e27fd67b4 100644
--- a/src/test/librbd/test_librbd.cc
+++ b/src/test/librbd/test_librbd.cc
@@ -2964,6 +2964,36 @@ TEST_F(TestLibRBD, TestIOToSnapshot)
   rados_ioctx_destroy(ioctx);
 }
 
+TEST_F(TestLibRBD, TestSnapshotDeletedIo)
+{
+  rados_ioctx_t ioctx;
+  rados_ioctx_create(_cluster, m_pool_name.c_str(), &ioctx);
+
+  rbd_image_t image;
+  int order = 0;
+  std::string name = get_temp_image_name();
+  uint64_t isize = 2 << 20;
+
+  int r;
+
+  ASSERT_EQ(0, create_image(ioctx, name.c_str(), isize, &order));
+  ASSERT_EQ(0, rbd_open(ioctx, name.c_str(), &image, NULL));
+  ASSERT_EQ(0, rbd_snap_create(image, "orig"));
+
+  r = rbd_snap_set(image, "orig");
+  ASSERT_EQ(r, 0);
+
+  ASSERT_EQ(0, rbd_snap_remove(image, "orig"));
+  char test[20];
+  ASSERT_EQ(-ENOENT, rbd_read(image, 20, 20, test));
+
+  r = rbd_snap_set(image, NULL);
+  ASSERT_EQ(r, 0);
+
+  ASSERT_EQ(0, rbd_close(image));
+  rados_ioctx_destroy(ioctx);
+}
+
 TEST_F(TestLibRBD, TestClone)
 {
   REQUIRE_FEATURE(RBD_FEATURE_LAYERING);