Merge branch 'wip-16689' into wip-mgolub-testing

librbd: optimize away unnecessary object map updates #10332

src/librbd/AioObjectRequest.cc

@@ -402,22 +402,21 @@ namespace librbd {
       } else {
         // should have been flushed prior to releasing lock
         assert(m_ictx->exclusive_lock->is_lock_owner());
-
         m_object_exist = m_ictx->object_map->object_may_exist(m_object_no);
 
-        ldout(m_ictx->cct, 20) << "send_pre " << this << " " << m_oid << " "
-          		       << m_object_off << "~" << m_object_len << dendl;
-        m_state = LIBRBD_AIO_WRITE_PRE;
-
         uint8_t new_state;
-        boost::optional<uint8_t> current_state;
         pre_object_map_update(&new_state);
 
         RWLock::WLocker object_map_locker(m_ictx->object_map_lock);
-        if ((*m_ictx->object_map)[m_object_no] != new_state) {
+        if (m_ictx->object_map->update_required(m_object_no, new_state)) {
+          ldout(m_ictx->cct, 20) << "send_pre " << this << " " << m_oid << " "
+                                 << m_object_off << "~" << m_object_len
+                                 << dendl;
+          m_state = LIBRBD_AIO_WRITE_PRE;
+
           Context *ctx = util::create_context_callback<AioObjectRequest>(this);
           bool updated = m_ictx->object_map->aio_update(m_object_no, new_state,
-                                                        current_state, ctx);
+                                                        {}, ctx);
           assert(updated);
         } else {
           write = true;
@@ -442,17 +441,15 @@ namespace librbd {
     // should have been flushed prior to releasing lock
     assert(m_ictx->exclusive_lock->is_lock_owner());
 
-    ldout(m_ictx->cct, 20) << "send_post " << this << " " << m_oid << " "
-			   << m_object_off << "~" << m_object_len << dendl;
-    m_state = LIBRBD_AIO_WRITE_POST;
-
     RWLock::WLocker object_map_locker(m_ictx->object_map_lock);
-    uint8_t current_state = (*m_ictx->object_map)[m_object_no];
-    if (current_state != OBJECT_PENDING ||
-        current_state == OBJECT_NONEXISTENT) {
+    if (!m_ictx->object_map->update_required(m_object_no, OBJECT_NONEXISTENT)) {
       return true;
     }
 
+    ldout(m_ictx->cct, 20) << "send_post " << this << " " << m_oid << " "
+			   << m_object_off << "~" << m_object_len << dendl;
+    m_state = LIBRBD_AIO_WRITE_POST;
+
     Context *ctx = util::create_context_callback<AioObjectRequest>(this);
     bool updated = m_ictx->object_map->aio_update(m_object_no,
                                                   OBJECT_NONEXISTENT,

src/librbd/ObjectMap.cc

@@ -90,6 +90,18 @@ bool ObjectMap::object_may_exist(uint64_t object_no) const
   return exists;
 }
 
+bool ObjectMap::update_required(uint64_t object_no, uint8_t new_state) {
+  assert(m_image_ctx.object_map_lock.is_wlocked());
+  uint8_t state = (*this)[object_no];
+
+  if ((state == new_state) ||
+      (new_state == OBJECT_PENDING && state == OBJECT_NONEXISTENT) ||
+      (new_state == OBJECT_NONEXISTENT && state != OBJECT_PENDING)) {
+    return false;
+  }
+  return true;
+}
+
 void ObjectMap::open(Context *on_finish) {
   object_map::RefreshRequest<> *req = new object_map::RefreshRequest<>(
     m_image_ctx, &m_object_map, m_snap_id, on_finish);

src/librbd/ObjectMap.h

@@ -39,6 +39,7 @@ class ObjectMap {
   void close(Context *on_finish);
 
   bool object_may_exist(uint64_t object_no) const;
+  bool update_required(uint64_t object_no, uint8_t new_state);
 
   void aio_save(Context *on_finish);
   void aio_resize(uint64_t new_size, uint8_t default_object_state,

src/rbd_replay/ActionTypes.cc

@@ -288,12 +288,18 @@ void ActionEntry::decode(__u8 version, bufferlist::iterator &it) {
   case ACTION_TYPE_WRITE:
     action = WriteAction();
     break;
+  case ACTION_TYPE_DISCARD:
+    action = DiscardAction();
+    break;
   case ACTION_TYPE_AIO_READ:
     action = AioReadAction();
     break;
   case ACTION_TYPE_AIO_WRITE:
     action = AioWriteAction();
     break;
+  case ACTION_TYPE_AIO_DISCARD:
+    action = AioDiscardAction();
+    break;
   case ACTION_TYPE_OPEN_IMAGE:
     action = OpenImageAction();
     break;
@@ -330,12 +336,18 @@ void ActionEntry::generate_test_instances(std::list<ActionEntry *> &o) {
   o.push_back(new ActionEntry(WriteAction()));
   o.push_back(new ActionEntry(WriteAction(1, 123456789, dependencies, 3, 4,
                                           5)));
+  o.push_back(new ActionEntry(DiscardAction()));
+  o.push_back(new ActionEntry(DiscardAction(1, 123456789, dependencies, 3, 4,
+                                            5)));
   o.push_back(new ActionEntry(AioReadAction()));
   o.push_back(new ActionEntry(AioReadAction(1, 123456789, dependencies, 3, 4,
                                             5)));
   o.push_back(new ActionEntry(AioWriteAction()));
   o.push_back(new ActionEntry(AioWriteAction(1, 123456789, dependencies, 3, 4,
                                              5)));
+  o.push_back(new ActionEntry(AioDiscardAction()));
+  o.push_back(new ActionEntry(AioDiscardAction(1, 123456789, dependencies, 3, 4,
+                                               5)));
 
   o.push_back(new ActionEntry(OpenImageAction()));
   o.push_back(new ActionEntry(OpenImageAction(1, 123456789, dependencies, 3,
@@ -372,12 +384,18 @@ std::ostream &operator<<(std::ostream &out,
   case ACTION_TYPE_WRITE:
     out << "Write";
     break;
+  case ACTION_TYPE_DISCARD:
+    out << "Discard";
+    break;
   case ACTION_TYPE_AIO_READ:
     out << "AioRead";
     break;
   case ACTION_TYPE_AIO_WRITE:
     out << "AioWrite";
     break;
+  case ACTION_TYPE_AIO_DISCARD:
+    out << "AioDiscard";
+    break;
   case ACTION_TYPE_OPEN_IMAGE:
     out << "OpenImage";
     break;

src/rbd_replay/ActionTypes.h

@@ -72,6 +72,8 @@ enum ActionType {
   ACTION_TYPE_CLOSE_IMAGE     = 7,
   ACTION_TYPE_AIO_OPEN_IMAGE  = 8,
   ACTION_TYPE_AIO_CLOSE_IMAGE = 9,
+  ACTION_TYPE_DISCARD         = 10,
+  ACTION_TYPE_AIO_DISCARD     = 11
 };
 
 struct ActionBase {
@@ -170,6 +172,18 @@ struct WriteAction : public IoActionBase {
   }
 };
 
+struct DiscardAction : public IoActionBase {
+  static const ActionType ACTION_TYPE = ACTION_TYPE_DISCARD;
+
+  DiscardAction() {
+  }
+  DiscardAction(action_id_t id, thread_id_t thread_id,
+                const Dependencies &dependencies, imagectx_id_t imagectx_id,
+                uint64_t offset, uint64_t length)
+    : IoActionBase(id, thread_id, dependencies, imagectx_id, offset, length) {
+  }
+};
+
 struct AioReadAction : public IoActionBase {
   static const ActionType ACTION_TYPE = ACTION_TYPE_AIO_READ;
 
@@ -194,6 +208,18 @@ struct AioWriteAction : public IoActionBase {
   }
 };
 
+struct AioDiscardAction : public IoActionBase {
+  static const ActionType ACTION_TYPE = ACTION_TYPE_AIO_DISCARD;
+
+  AioDiscardAction() {
+  }
+  AioDiscardAction(action_id_t id, thread_id_t thread_id,
+                   const Dependencies &dependencies, imagectx_id_t imagectx_id,
+                   uint64_t offset, uint64_t length)
+    : IoActionBase(id, thread_id, dependencies, imagectx_id, offset, length) {
+  }
+};
+
 struct OpenImageAction : public ImageActionBase {
   static const ActionType ACTION_TYPE = ACTION_TYPE_OPEN_IMAGE;
 
@@ -272,8 +298,10 @@ typedef boost::variant<StartThreadAction,
                        StopThreadAction,
                        ReadAction,
                        WriteAction,
+                       DiscardAction,
                        AioReadAction,
                        AioWriteAction,
+                       AioDiscardAction,
                        OpenImageAction,
                        CloseImageAction,
                        AioOpenImageAction,

src/rbd_replay/actions.cc

@@ -57,6 +57,14 @@ struct ConstructVisitor : public boost::static_visitor<Action::ptr> {
     return Action::ptr(new AioWriteAction(action));
   }
 
+  inline Action::ptr operator()(const action::DiscardAction &action) const {
+    return Action::ptr(new DiscardAction(action));
+  }
+
+  inline Action::ptr operator()(const action::AioDiscardAction &action) const {
+    return Action::ptr(new AioDiscardAction(action));
+  }
+
   inline Action::ptr operator()(const action::OpenImageAction &action) const {
     return Action::ptr(new OpenImageAction(action));
   }
@@ -118,7 +126,6 @@ void ReadAction::perform(ActionCtx &worker) {
   worker.remove_pending(io);
 }
 
-
 void AioWriteAction::perform(ActionCtx &worker) {
   static const std::string fake_data(create_fake_data());
   dout(ACTION_LEVEL) << "Performing " << *this << dendl;
@@ -152,6 +159,31 @@ void WriteAction::perform(ActionCtx &worker) {
   worker.remove_pending(io);
 }
 
+void AioDiscardAction::perform(ActionCtx &worker) {
+  dout(ACTION_LEVEL) << "Performing " << *this << dendl;
+  librbd::Image *image = worker.get_image(m_action.imagectx_id);
+  PendingIO::ptr io(new PendingIO(pending_io_id(), worker));
+  worker.add_pending(io);
+  if (worker.readonly()) {
+    worker.remove_pending(io);
+  } else {
+    int r = image->aio_discard(m_action.offset, m_action.length, &io->completion());
+    assertf(r >= 0, "id = %d, r = %d", id(), r);
+  }
+}
+
+void DiscardAction::perform(ActionCtx &worker) {
+  dout(ACTION_LEVEL) << "Performing " << *this << dendl;
+  librbd::Image *image = worker.get_image(m_action.imagectx_id);
+  PendingIO::ptr io(new PendingIO(pending_io_id(), worker));
+  worker.add_pending(io);
+  if (!worker.readonly()) {
+    ssize_t r = image->discard(m_action.offset, m_action.length);
+    assertf(r >= 0, "id = %d, r = %d", id(), r);
+  }
+  worker.remove_pending(io);
+}
+
 void OpenImageAction::perform(ActionCtx &worker) {
   dout(ACTION_LEVEL) << "Performing " << *this << dendl;
   PendingIO::ptr io(new PendingIO(pending_io_id(), worker));

src/rbd_replay/actions.hpp

@@ -251,6 +251,36 @@ class WriteAction : public TypedAction<action::WriteAction> {
 };
 
 
+class AioDiscardAction : public TypedAction<action::AioDiscardAction> {
+public:
+  explicit AioDiscardAction(const action::AioDiscardAction &action)
+    : TypedAction<action::AioDiscardAction>(action) {
+  }
+
+  virtual void perform(ActionCtx &ctx);
+
+protected:
+  virtual const char *get_action_name() const {
+    return "AioDiscardAction";
+  }
+};
+
+
+class DiscardAction : public TypedAction<action::DiscardAction> {
+public:
+  explicit DiscardAction(const action::DiscardAction &action)
+    : TypedAction<action::DiscardAction>(action) {
+  }
+
+  virtual void perform(ActionCtx &ctx);
+
+protected:
+  virtual const char *get_action_name() const {
+    return "DiscardAction";
+  }
+};
+
+
 class OpenImageAction : public TypedAction<action::OpenImageAction> {
 public:
   explicit OpenImageAction(const action::OpenImageAction &action)

src/rbd_replay/ios.cc

@@ -111,6 +111,18 @@ void WriteIO::write_debug(std::ostream& out) const {
   out << ", imagectx=" << m_imagectx << ", offset=" << m_offset << ", length=" << m_length << "]";
 }
 
+void DiscardIO::encode(bufferlist &bl) const {
+  action::Action action((action::DiscardAction(
+    ionum(), thread_id(), convert_dependencies(start_time(), dependencies()),
+    m_imagectx, m_offset, m_length)));
+  ::encode(action, bl);
+}
+
+void DiscardIO::write_debug(std::ostream& out) const {
+  write_debug_base(out, "discard");
+  out << ", imagectx=" << m_imagectx << ", offset=" << m_offset << ", length=" << m_length << "]";
+}
+
 void AioReadIO::encode(bufferlist &bl) const {
   action::Action action((action::AioReadAction(
     ionum(), thread_id(), convert_dependencies(start_time(), dependencies()),
@@ -135,6 +147,18 @@ void AioWriteIO::write_debug(std::ostream& out) const {
   out << ", imagectx=" << m_imagectx << ", offset=" << m_offset << ", length=" << m_length << "]";
 }
 
+void AioDiscardIO::encode(bufferlist &bl) const {
+  action::Action action((action::AioDiscardAction(
+    ionum(), thread_id(), convert_dependencies(start_time(), dependencies()),
+    m_imagectx, m_offset, m_length)));
+  ::encode(action, bl);
+}
+
+void AioDiscardIO::write_debug(std::ostream& out) const {
+  write_debug_base(out, "aio discard");
+  out << ", imagectx=" << m_imagectx << ", offset=" << m_offset << ", length=" << m_length << "]";
+}
+
 void OpenImageIO::encode(bufferlist &bl) const {
   action::Action action((action::OpenImageAction(
     ionum(), thread_id(), convert_dependencies(start_time(), dependencies()),

src/rbd_replay/ios.hpp

@@ -186,6 +186,31 @@ class WriteIO : public IO {
   uint64_t m_length;
 };
 
+class DiscardIO : public IO {
+public:
+  DiscardIO(action_id_t ionum,
+	    uint64_t start_time,
+	    thread_id_t thread_id,
+            const io_set_t& deps,
+	    imagectx_id_t imagectx,
+	    uint64_t offset,
+	    uint64_t length)
+    : IO(ionum, start_time, thread_id, deps),
+      m_imagectx(imagectx),
+      m_offset(offset),
+      m_length(length) {
+  }
+
+  virtual void encode(bufferlist &bl) const;
+
+  void write_debug(std::ostream& out) const;
+
+private:
+  imagectx_id_t m_imagectx;
+  uint64_t m_offset;
+  uint64_t m_length;
+};
+
 class AioReadIO : public IO {
 public:
   AioReadIO(action_id_t ionum,
@@ -236,6 +261,31 @@ class AioWriteIO : public IO {
   uint64_t m_length;
 };
 
+class AioDiscardIO : public IO {
+public:
+  AioDiscardIO(action_id_t ionum,
+	       uint64_t start_time,
+	       thread_id_t thread_id,
+               const io_set_t& deps,
+	       imagectx_id_t imagectx,
+	       uint64_t offset,
+	       uint64_t length)
+    : IO(ionum, start_time, thread_id, deps),
+      m_imagectx(imagectx),
+      m_offset(offset),
+      m_length(length) {
+  }
+
+  virtual void encode(bufferlist &bl) const;
+
+  void write_debug(std::ostream& out) const;
+
+private:
+  imagectx_id_t m_imagectx;
+  uint64_t m_offset;
+  uint64_t m_length;
+};
+
 class OpenImageIO : public IO {
 public:
   OpenImageIO(action_id_t ionum,