librbd: use an up-to-date snap context when owning the exclusive lock

By effectively moving capturing of the snap context to the API layer,
commit 1d0a3b17f590 ("librbd: pass IOContext to image-extent IO
dispatch methods") introduced a nasty regression.  The snap context can
be captured only after exclusive lock is safely held for the duration
of dealing with the image request and even then must be refreshed if
a snapshot creation request is accepted from a peer.  This is needed to
ensure correctness of the object map in general and fast-diff states in
particular (OBJECT_EXISTS vs OBJECT_EXISTS_CLEAN) and object deltas
computed based off of them.  Otherwise the object map that is forked
for the snapshot isn't guaranteed to accurately reflect the contents of
the snapshot when the snapshot is taken under I/O (as in disabling the
object map may lead to different results being returned for reads).

The regression affects mainly differential backup and snapshot-based
mirroring use cases with object-map and/or fast-diff enabled: since
some object deltas may be incomplete, the destination image may get
corrupted.

This commit represents a reasonable minimal fix: IOContext passed
through to ImageDispatch is effected only for reads and just gets
ignored for writes.  The next commit cleans up further by undoing the
passing of IOContext through the image dispatch layers for writes.

Fixes: https://tracker.ceph.com/issues/61616
Signed-off-by: Ilya Dryomov <idryomov@gmail.com>

diff --git a/src/librbd/io/ImageDispatch.cc b/src/librbd/io/ImageDispatch.cc
index ed3cc357077fcc1c923c2619fb3409903786bddc..699e85d5351824c9e0d746b2f4b43cbc0aadb567 100644 (file)
--- a/src/librbd/io/ImageDispatch.cc
+++ b/src/librbd/io/ImageDispatch.cc
@@ -75,8 +75,7 @@ bool ImageDispatch<I>::write(
 
   *dispatch_result = DISPATCH_RESULT_COMPLETE;
   ImageRequest<I>::aio_write(m_image_ctx, aio_comp, std::move(image_extents),
-                             area, std::move(bl), io_context, op_flags,
-                             parent_trace);
+                             area, std::move(bl), op_flags, parent_trace);
   return true;
 }
 
@@ -97,8 +96,7 @@ bool ImageDispatch<I>::discard(
 
   *dispatch_result = DISPATCH_RESULT_COMPLETE;
   ImageRequest<I>::aio_discard(m_image_ctx, aio_comp, std::move(image_extents),
-                               area, discard_granularity_bytes, io_context,
-                               parent_trace);
+                               area, discard_granularity_bytes, parent_trace);
   return true;
 }
 
@@ -119,7 +117,7 @@ bool ImageDispatch<I>::write_same(
   *dispatch_result = DISPATCH_RESULT_COMPLETE;
   ImageRequest<I>::aio_writesame(m_image_ctx, aio_comp,
                                  std::move(image_extents), area, std::move(bl),
-                                 io_context, op_flags, parent_trace);
+                                 op_flags, parent_trace);
   return true;
 }
 
@@ -142,7 +140,7 @@ bool ImageDispatch<I>::compare_and_write(
   ImageRequest<I>::aio_compare_and_write(m_image_ctx, aio_comp,
                                          std::move(image_extents), area,
                                          std::move(cmp_bl), std::move(bl),
-                                         mismatch_offset, io_context, op_flags,
+                                         mismatch_offset, op_flags,
                                          parent_trace);
   return true;
 }

diff --git a/src/librbd/io/ImageRequest.cc b/src/librbd/io/ImageRequest.cc
index ea6b7301a2a9fd08fd4868f6bae1952f4fe894a9..e4c41c22976a7458b8cb464392dfde91271568ee 100644 (file)
--- a/src/librbd/io/ImageRequest.cc
+++ b/src/librbd/io/ImageRequest.cc
@@ -241,11 +241,10 @@ void ImageRequest<I>::aio_read(I *ictx, AioCompletion *c,
 template <typename I>
 void ImageRequest<I>::aio_write(I *ictx, AioCompletion *c,
                                 Extents &&image_extents, ImageArea area,
-                                bufferlist &&bl, IOContext io_context,
-                                int op_flags,
+                                bufferlist &&bl, int op_flags,
                                const ZTracer::Trace &parent_trace) {
   ImageWriteRequest<I> req(*ictx, c, std::move(image_extents), area,
-                           std::move(bl), io_context, op_flags, parent_trace);
+                           std::move(bl), op_flags, parent_trace);
   req.send();
 }
 
@@ -253,11 +252,9 @@ template <typename I>
 void ImageRequest<I>::aio_discard(I *ictx, AioCompletion *c,
                                   Extents &&image_extents, ImageArea area,
                                   uint32_t discard_granularity_bytes,
-                                 IOContext io_context,
                                   const ZTracer::Trace &parent_trace) {
   ImageDiscardRequest<I> req(*ictx, c, std::move(image_extents), area,
-                             discard_granularity_bytes, io_context,
-                             parent_trace);
+                             discard_granularity_bytes, parent_trace);
   req.send();
 }
 
@@ -272,12 +269,10 @@ void ImageRequest<I>::aio_flush(I *ictx, AioCompletion *c,
 template <typename I>
 void ImageRequest<I>::aio_writesame(I *ictx, AioCompletion *c,
                                     Extents &&image_extents, ImageArea area,
-                                    bufferlist &&bl, IOContext io_context,
-                                    int op_flags,
+                                    bufferlist &&bl, int op_flags,
                                    const ZTracer::Trace &parent_trace) {
   ImageWriteSameRequest<I> req(*ictx, c, std::move(image_extents), area,
-                               std::move(bl), io_context, op_flags,
-                               parent_trace);
+                               std::move(bl), op_flags, parent_trace);
   req.send();
 }
 
@@ -288,12 +283,11 @@ void ImageRequest<I>::aio_compare_and_write(I *ictx, AioCompletion *c,
                                             bufferlist &&cmp_bl,
                                             bufferlist &&bl,
                                             uint64_t *mismatch_offset,
-                                            IOContext io_context, int op_flags,
+                                            int op_flags,
                                             const ZTracer::Trace &parent_trace) {
   ImageCompareAndWriteRequest<I> req(*ictx, c, std::move(image_extents), area,
                                      std::move(cmp_bl), std::move(bl),
-                                     mismatch_offset, io_context, op_flags,
-                                     parent_trace);
+                                     mismatch_offset, op_flags, parent_trace);
   req.send();
 }
 
@@ -372,8 +366,8 @@ ImageReadRequest<I>::ImageReadRequest(I &image_ctx, AioCompletion *aio_comp,
                                      int read_flags,
                                       const ZTracer::Trace &parent_trace)
   : ImageRequest<I>(image_ctx, aio_comp, std::move(image_extents), area,
-                    io_context, "read", parent_trace),
-    m_op_flags(op_flags), m_read_flags(read_flags) {
+                    "read", parent_trace),
+    m_io_context(io_context), m_op_flags(op_flags), m_read_flags(read_flags) {
   aio_comp->read_result = std::move(read_result);
 }
 
@@ -386,7 +380,7 @@ void ImageReadRequest<I>::send_request() {
   if (this->m_image_area == ImageArea::DATA &&
       image_ctx.cache && image_ctx.readahead_max_bytes > 0 &&
       !(m_op_flags & LIBRADOS_OP_FLAG_FADVISE_RANDOM)) {
-    readahead(get_image_ctx(&image_ctx), image_extents, this->m_io_context);
+    readahead(get_image_ctx(&image_ctx), image_extents, m_io_context);
   }
 
   // map image extents to object extents
@@ -417,7 +411,7 @@ void ImageReadRequest<I>::send_request() {
       aio_comp, {{oe.offset, oe.length, std::move(oe.buffer_extents)}});
     auto req = ObjectDispatchSpec::create_read(
       &image_ctx, OBJECT_DISPATCH_LAYER_NONE, oe.object_no,
-      &req_comp->extents, this->m_io_context, m_op_flags, m_read_flags,
+      &req_comp->extents, m_io_context, m_op_flags, m_read_flags,
       this->m_trace, nullptr, req_comp);
     req->send();
   }
@@ -482,7 +476,11 @@ void AbstractImageWriteRequest<I>::send_request() {
       journal_tid = append_journal_event(m_synchronous);
     }
 
-    send_object_requests(object_extents, this->m_io_context, journal_tid);
+    // it's very important that IOContext is captured here instead of
+    // e.g. at the API layer so that an up-to-date snap context is used
+    // when owning the exclusive lock
+    send_object_requests(object_extents, image_ctx.get_data_io_context(),
+                         journal_tid);
   }
 
   update_stats(clip_len);
@@ -846,8 +844,7 @@ ImageListSnapsRequest<I>::ImageListSnapsRequest(
     ImageArea area, SnapIds&& snap_ids, int list_snaps_flags,
     SnapshotDelta* snapshot_delta, const ZTracer::Trace& parent_trace)
   : ImageRequest<I>(image_ctx, aio_comp, std::move(image_extents), area,
-                    image_ctx.get_data_io_context(), "list-snaps",
-                    parent_trace),
+                    "list-snaps", parent_trace),
     m_snap_ids(std::move(snap_ids)), m_list_snaps_flags(list_snaps_flags),
     m_snapshot_delta(snapshot_delta) {
 }

diff --git a/src/librbd/io/ImageRequest.h b/src/librbd/io/ImageRequest.h
index cd7b203e80cd666b794f2a465b640e1631ea2a39..2668c1acb2cda6231af6996f9e0b61f403d0ff33 100644 (file)
--- a/src/librbd/io/ImageRequest.h
+++ b/src/librbd/io/ImageRequest.h
@@ -38,26 +38,23 @@ public:
                        const ZTracer::Trace &parent_trace);
   static void aio_write(ImageCtxT *ictx, AioCompletion *c,
                         Extents &&image_extents, ImageArea area,
-                        bufferlist &&bl, IOContext io_context, int op_flags,
+                        bufferlist &&bl, int op_flags,
                        const ZTracer::Trace &parent_trace);
   static void aio_discard(ImageCtxT *ictx, AioCompletion *c,
                           Extents &&image_extents, ImageArea area,
                           uint32_t discard_granularity_bytes,
-                          IOContext io_context,
                           const ZTracer::Trace &parent_trace);
   static void aio_flush(ImageCtxT *ictx, AioCompletion *c,
                         FlushSource flush_source,
                         const ZTracer::Trace &parent_trace);
   static void aio_writesame(ImageCtxT *ictx, AioCompletion *c,
                             Extents &&image_extents, ImageArea area,
-                            bufferlist &&bl, IOContext io_context, int op_flags,
+                            bufferlist &&bl, int op_flags,
                             const ZTracer::Trace &parent_trace);
-
   static void aio_compare_and_write(ImageCtxT *ictx, AioCompletion *c,
                                     Extents &&image_extents, ImageArea area,
-                                    bufferlist &&cmp_bl,
-                                    bufferlist &&bl, uint64_t *mismatch_offset,
-                                    IOContext io_context, int op_flags,
+                                    bufferlist &&cmp_bl, bufferlist &&bl,
+                                    uint64_t *mismatch_offset, int op_flags,
                                     const ZTracer::Trace &parent_trace);
 
   void send();
@@ -73,15 +70,13 @@ protected:
   AioCompletion *m_aio_comp;
   Extents m_image_extents;
   ImageArea m_image_area;
-  IOContext m_io_context;
   ZTracer::Trace m_trace;
 
   ImageRequest(ImageCtxT &image_ctx, AioCompletion *aio_comp,
-               Extents &&image_extents, ImageArea area, IOContext io_context,
-               const char *trace_name, const ZTracer::Trace &parent_trace)
+               Extents &&image_extents, ImageArea area, const char *trace_name,
+               const ZTracer::Trace &parent_trace)
     : m_image_ctx(image_ctx), m_aio_comp(aio_comp),
       m_image_extents(std::move(image_extents)), m_image_area(area),
-      m_io_context(io_context),
       m_trace(librbd::util::create_trace(image_ctx, trace_name, parent_trace)) {
     m_trace.event("start");
   }
@@ -110,7 +105,9 @@ protected:
   const char *get_request_type() const override {
     return "aio_read";
   }
+
 private:
+  IOContext m_io_context;
   int m_op_flags;
   int m_read_flags;
 };
@@ -127,10 +124,10 @@ protected:
 
   AbstractImageWriteRequest(ImageCtxT &image_ctx, AioCompletion *aio_comp,
                             Extents &&image_extents, ImageArea area,
-                            IOContext io_context, const char *trace_name,
+                            const char *trace_name,
                            const ZTracer::Trace &parent_trace)
     : ImageRequest<ImageCtxT>(image_ctx, aio_comp, std::move(image_extents),
-                              area, io_context, trace_name, parent_trace),
+                              area, trace_name, parent_trace),
       m_synchronous(false) {
   }
 
@@ -159,10 +156,9 @@ class ImageWriteRequest : public AbstractImageWriteRequest<ImageCtxT> {
 public:
   ImageWriteRequest(ImageCtxT &image_ctx, AioCompletion *aio_comp,
                     Extents &&image_extents, ImageArea area, bufferlist &&bl,
-                    IOContext io_context, int op_flags,
-                   const ZTracer::Trace &parent_trace)
+                    int op_flags, const ZTracer::Trace &parent_trace)
     : AbstractImageWriteRequest<ImageCtxT>(
-        image_ctx, aio_comp, std::move(image_extents), area, io_context,
+        image_ctx, aio_comp, std::move(image_extents), area,
         "write", parent_trace),
       m_bl(std::move(bl)), m_op_flags(op_flags) {
   }
@@ -197,10 +193,10 @@ class ImageDiscardRequest : public AbstractImageWriteRequest<ImageCtxT> {
 public:
   ImageDiscardRequest(ImageCtxT &image_ctx, AioCompletion *aio_comp,
                       Extents&& image_extents, ImageArea area,
-                     uint32_t discard_granularity_bytes, IOContext io_context,
+                      uint32_t discard_granularity_bytes,
                       const ZTracer::Trace &parent_trace)
     : AbstractImageWriteRequest<ImageCtxT>(
-        image_ctx, aio_comp, std::move(image_extents), area, io_context,
+        image_ctx, aio_comp, std::move(image_extents), area,
         "discard", parent_trace),
       m_discard_granularity_bytes(discard_granularity_bytes) {
   }
@@ -237,7 +233,7 @@ public:
                     const ZTracer::Trace &parent_trace)
     : ImageRequest<ImageCtxT>(image_ctx, aio_comp, {},
                               ImageArea::DATA /* dummy for {} */,
-                              {}, "flush", parent_trace),
+                              "flush", parent_trace),
       m_flush_source(flush_source) {
   }
 
@@ -265,10 +261,10 @@ class ImageWriteSameRequest : public AbstractImageWriteRequest<ImageCtxT> {
 public:
   ImageWriteSameRequest(ImageCtxT &image_ctx, AioCompletion *aio_comp,
                         Extents&& image_extents, ImageArea area,
-                        bufferlist &&bl, IOContext io_context, int op_flags,
+                        bufferlist &&bl, int op_flags,
                         const ZTracer::Trace &parent_trace)
     : AbstractImageWriteRequest<ImageCtxT>(
-        image_ctx, aio_comp, std::move(image_extents), area, io_context,
+        image_ctx, aio_comp, std::move(image_extents), area,
         "writesame", parent_trace),
       m_data_bl(std::move(bl)), m_op_flags(op_flags) {
   }
@@ -302,10 +298,10 @@ public:
   ImageCompareAndWriteRequest(ImageCtxT &image_ctx, AioCompletion *aio_comp,
                               Extents &&image_extents, ImageArea area,
                               bufferlist &&cmp_bl, bufferlist &&bl,
-                              uint64_t *mismatch_offset, IOContext io_context,
-                              int op_flags, const ZTracer::Trace &parent_trace)
+                              uint64_t *mismatch_offset, int op_flags,
+                              const ZTracer::Trace &parent_trace)
       : AbstractImageWriteRequest<ImageCtxT>(
-          image_ctx, aio_comp, std::move(image_extents), area, io_context,
+          image_ctx, aio_comp, std::move(image_extents), area,
           "compare_and_write", parent_trace),
         m_cmp_bl(std::move(cmp_bl)), m_bl(std::move(bl)),
         m_mismatch_offset(mismatch_offset), m_op_flags(op_flags) {

diff --git a/src/librbd/journal/Replay.cc b/src/librbd/journal/Replay.cc
index c35b44bfcd727002729c89faa5b38c19484fe796..42acf5eb24137ab1e810bdb256b2df10ad939c58 100644 (file)
--- a/src/librbd/journal/Replay.cc
+++ b/src/librbd/journal/Replay.cc
@@ -357,8 +357,7 @@ void Replay<I>::handle_event(const journal::AioDiscardEvent &event,
     io::ImageRequest<I>::aio_discard(&m_image_ctx, aio_comp,
                                      {{event.offset, event.length}},
                                      io::ImageArea::DATA,
-                                     event.discard_granularity_bytes,
-                                     m_image_ctx.get_data_io_context(), {});
+                                     event.discard_granularity_bytes, {});
   }
 
   if (flush_required) {
@@ -393,7 +392,7 @@ void Replay<I>::handle_event(const journal::AioWriteEvent &event,
     io::ImageRequest<I>::aio_write(&m_image_ctx, aio_comp,
                                    {{event.offset, event.length}},
                                    io::ImageArea::DATA, std::move(data),
-                                   m_image_ctx.get_data_io_context(), 0, {});
+                                   0, {});
   }
 
   if (flush_required) {
@@ -447,8 +446,7 @@ void Replay<I>::handle_event(const journal::AioWriteSameEvent &event,
     io::ImageRequest<I>::aio_writesame(&m_image_ctx, aio_comp,
                                        {{event.offset, event.length}},
                                        io::ImageArea::DATA, std::move(data),
-                                       m_image_ctx.get_data_io_context(), 0,
-                                       {});
+                                       0, {});
   }
 
   if (flush_required) {
@@ -483,9 +481,7 @@ void Replay<I>::handle_event(const journal::AioWriteSameEvent &event,
                                                io::ImageArea::DATA,
                                                std::move(cmp_data),
                                                std::move(write_data),
-                                               nullptr,
-                                               m_image_ctx.get_data_io_context(),
-                                               0, {});
+                                               nullptr, 0, {});
   }
 
   if (flush_required) {