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>
(cherry picked from commit e4b1e0466354942c935e9eca2ab2858e75049415)

Conflicts:
	src/librbd/io/ImageDispatch.cc [ ImageArea support not in
	  pacific ]
	src/librbd/io/ImageRequest.cc [ ditto ]
	src/librbd/io/ImageRequest.h [ ditto ]
	src/librbd/journal/Replay.cc [ ditto ]

diff --git a/src/librbd/io/ImageDispatch.cc b/src/librbd/io/ImageDispatch.cc
index cc8519abeeeb88193f1811f6a21157a93f1b331e..0c173dd2661febebcc21aa4534fb4c8bb64c2546 100644 (file)
--- a/src/librbd/io/ImageDispatch.cc
+++ b/src/librbd/io/ImageDispatch.cc
@@ -65,9 +65,8 @@ bool ImageDispatch<I>::write(
   start_in_flight_io(aio_comp);
 
   *dispatch_result = DISPATCH_RESULT_COMPLETE;
-  ImageRequest<I>::aio_write(
-    m_image_ctx, aio_comp, std::move(image_extents), std::move(bl),
-    io_context, op_flags, parent_trace);
+  ImageRequest<I>::aio_write(m_image_ctx, aio_comp, std::move(image_extents),
+                             std::move(bl), op_flags, parent_trace);
   return true;
 }
 
@@ -85,9 +84,8 @@ bool ImageDispatch<I>::discard(
   start_in_flight_io(aio_comp);
 
   *dispatch_result = DISPATCH_RESULT_COMPLETE;
-  ImageRequest<I>::aio_discard(
-    m_image_ctx, aio_comp, std::move(image_extents), discard_granularity_bytes,
-    io_context, parent_trace);
+  ImageRequest<I>::aio_discard(m_image_ctx, aio_comp, std::move(image_extents),
+                               discard_granularity_bytes, parent_trace);
   return true;
 }
 
@@ -104,9 +102,9 @@ bool ImageDispatch<I>::write_same(
   start_in_flight_io(aio_comp);
 
   *dispatch_result = DISPATCH_RESULT_COMPLETE;
-  ImageRequest<I>::aio_writesame(
-    m_image_ctx, aio_comp, std::move(image_extents), std::move(bl),
-    io_context, op_flags, parent_trace);
+  ImageRequest<I>::aio_writesame(m_image_ctx, aio_comp,
+                                 std::move(image_extents), std::move(bl),
+                                 op_flags, parent_trace);
   return true;
 }
 
@@ -124,9 +122,11 @@ bool ImageDispatch<I>::compare_and_write(
   start_in_flight_io(aio_comp);
 
   *dispatch_result = DISPATCH_RESULT_COMPLETE;
-  ImageRequest<I>::aio_compare_and_write(
-    m_image_ctx, aio_comp, std::move(image_extents), std::move(cmp_bl),
-    std::move(bl), mismatch_offset, io_context, op_flags, parent_trace);
+  ImageRequest<I>::aio_compare_and_write(m_image_ctx, aio_comp,
+                                         std::move(image_extents),
+                                         std::move(cmp_bl), std::move(bl),
+                                         mismatch_offset, op_flags,
+                                         parent_trace);
   return true;
 }
 

diff --git a/src/librbd/io/ImageRequest.cc b/src/librbd/io/ImageRequest.cc
index 671ec4e5284b4c378ec716faa1e117831ce47277..31405057ce97b68089d3bbc5797f4cd37b1ef8d1 100644 (file)
--- a/src/librbd/io/ImageRequest.cc
+++ b/src/librbd/io/ImageRequest.cc
@@ -239,11 +239,11 @@ void ImageRequest<I>::aio_read(I *ictx, AioCompletion *c,
 
 template <typename I>
 void ImageRequest<I>::aio_write(I *ictx, AioCompletion *c,
-                                Extents &&image_extents, bufferlist &&bl,
-                                IOContext io_context, int op_flags,
+                                Extents &&image_extents,
+                                bufferlist &&bl, int op_flags,
                                const ZTracer::Trace &parent_trace) {
-  ImageWriteRequest<I> req(*ictx, c, std::move(image_extents), std::move(bl),
-                           io_context, op_flags, parent_trace);
+  ImageWriteRequest<I> req(*ictx, c, std::move(image_extents),
+                           std::move(bl), op_flags, parent_trace);
   req.send();
 }
 
@@ -251,11 +251,9 @@ template <typename I>
 void ImageRequest<I>::aio_discard(I *ictx, AioCompletion *c,
                                   Extents &&image_extents,
                                   uint32_t discard_granularity_bytes,
-                                 IOContext io_context,
                                   const ZTracer::Trace &parent_trace) {
   ImageDiscardRequest<I> req(*ictx, c, std::move(image_extents),
-                             discard_granularity_bytes, io_context,
-                             parent_trace);
+                             discard_granularity_bytes, parent_trace);
   req.send();
 }
 
@@ -270,12 +268,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,
-                                    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),
-                               std::move(bl), io_context, op_flags,
-                               parent_trace);
+                               std::move(bl), op_flags, parent_trace);
   req.send();
 }
 
@@ -285,12 +281,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),
                                      std::move(cmp_bl), std::move(bl),
-                                     mismatch_offset, io_context, op_flags,
-                                     parent_trace);
+                                     mismatch_offset, op_flags, parent_trace);
   req.send();
 }
 
@@ -369,8 +364,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),
-                    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);
 }
 
@@ -382,7 +377,7 @@ void ImageReadRequest<I>::send_request() {
   auto &image_extents = this->m_image_extents;
   if (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
@@ -412,7 +407,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();
   }
@@ -477,7 +472,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);
@@ -840,8 +839,7 @@ ImageListSnapsRequest<I>::ImageListSnapsRequest(
     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),
-                    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 2c05c3847f0d98fb7733e0082fb63472713c1eb9..7d3f2339568bc91a0886be71977791d57577a2e6 100644 (file)
--- a/src/librbd/io/ImageRequest.h
+++ b/src/librbd/io/ImageRequest.h
@@ -38,27 +38,24 @@ public:
                        IOContext io_context, int op_flags, int read_flags,
                        const ZTracer::Trace &parent_trace);
   static void aio_write(ImageCtxT *ictx, AioCompletion *c,
-                        Extents &&image_extents, bufferlist &&bl,
-                        IOContext io_context, int op_flags,
+                        Extents &&image_extents,
+                        bufferlist &&bl, int op_flags,
                        const ZTracer::Trace &parent_trace);
   static void aio_discard(ImageCtxT *ictx, AioCompletion *c,
                           Extents &&image_extents,
                           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, bufferlist &&bl,
-                            IOContext io_context, int op_flags,
+                            Extents &&image_extents,
+                            bufferlist &&bl, int op_flags,
                             const ZTracer::Trace &parent_trace);
-
   static void aio_compare_and_write(ImageCtxT *ictx, AioCompletion *c,
                                     Extents &&image_extents,
-                                    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:
   ImageCtxT &m_image_ctx;
   AioCompletion *m_aio_comp;
   Extents m_image_extents;
-  IOContext m_io_context;
   ZTracer::Trace m_trace;
 
   ImageRequest(ImageCtxT &image_ctx, AioCompletion *aio_comp,
-               Extents &&image_extents, IOContext io_context,
-               const char *trace_name,
-              const ZTracer::Trace &parent_trace)
+               Extents &&image_extents, 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_io_context(io_context),
+      m_image_extents(std::move(image_extents)),
       m_trace(librbd::util::create_trace(image_ctx, trace_name, parent_trace)) {
     m_trace.event("start");
   }
@@ -112,7 +107,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;
 };
@@ -129,11 +126,11 @@ protected:
   using typename ImageRequest<ImageCtxT>::Extents;
 
   AbstractImageWriteRequest(ImageCtxT &image_ctx, AioCompletion *aio_comp,
-                            Extents &&image_extents, IOContext io_context,
+                            Extents &&image_extents,
                             const char *trace_name,
                            const ZTracer::Trace &parent_trace)
     : ImageRequest<ImageCtxT>(image_ctx, aio_comp, std::move(image_extents),
-                             io_context, trace_name, parent_trace),
+                              trace_name, parent_trace),
       m_synchronous(false) {
   }
 
@@ -164,11 +161,10 @@ public:
 
   ImageWriteRequest(ImageCtxT &image_ctx, AioCompletion *aio_comp,
                     Extents &&image_extents, 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), io_context, "write",
-        parent_trace),
+        image_ctx, aio_comp, std::move(image_extents),
+        "write", parent_trace),
       m_bl(std::move(bl)), m_op_flags(op_flags) {
   }
 
@@ -202,11 +198,11 @@ class ImageDiscardRequest : public AbstractImageWriteRequest<ImageCtxT> {
 public:
   ImageDiscardRequest(ImageCtxT &image_ctx, AioCompletion *aio_comp,
                       Extents&& image_extents,
-                     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), io_context, "discard",
-        parent_trace),
+        image_ctx, aio_comp, std::move(image_extents),
+        "discard", parent_trace),
       m_discard_granularity_bytes(discard_granularity_bytes) {
   }
 
@@ -240,8 +236,8 @@ public:
   ImageFlushRequest(ImageCtxT &image_ctx, AioCompletion *aio_comp,
                     FlushSource flush_source,
                     const ZTracer::Trace &parent_trace)
-    : ImageRequest<ImageCtxT>(image_ctx, aio_comp, {}, {}, "flush",
-                              parent_trace),
+    : ImageRequest<ImageCtxT>(image_ctx, aio_comp, {},
+                              "flush", parent_trace),
       m_flush_source(flush_source) {
   }
 
@@ -268,12 +264,12 @@ template <typename ImageCtxT = ImageCtx>
 class ImageWriteSameRequest : public AbstractImageWriteRequest<ImageCtxT> {
 public:
   ImageWriteSameRequest(ImageCtxT &image_ctx, AioCompletion *aio_comp,
-                        Extents&& image_extents, bufferlist &&bl,
-                        IOContext io_context, int op_flags,
+                        Extents&& image_extents,
+                        bufferlist &&bl, int op_flags,
                         const ZTracer::Trace &parent_trace)
     : AbstractImageWriteRequest<ImageCtxT>(
-       image_ctx, aio_comp, std::move(image_extents), io_context, "writesame",
-        parent_trace),
+        image_ctx, aio_comp, std::move(image_extents),
+        "writesame", parent_trace),
       m_data_bl(std::move(bl)), m_op_flags(op_flags) {
   }
 
@@ -304,12 +300,12 @@ public:
   using typename ImageRequest<ImageCtxT>::ObjectRequests;
 
   ImageCompareAndWriteRequest(ImageCtxT &image_ctx, AioCompletion *aio_comp,
-                              Extents &&image_extents, bufferlist &&cmp_bl,
-                              bufferlist &&bl, uint64_t *mismatch_offset,
-                              IOContext io_context, int op_flags,
-                               const ZTracer::Trace &parent_trace)
+                              Extents &&image_extents,
+                              bufferlist &&cmp_bl, bufferlist &&bl,
+                              uint64_t *mismatch_offset, int op_flags,
+                              const ZTracer::Trace &parent_trace)
       : AbstractImageWriteRequest<ImageCtxT>(
-          image_ctx, aio_comp, std::move(image_extents), io_context,
+          image_ctx, aio_comp, std::move(image_extents),
           "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 db73edb610947e6f77258a5ccdeeacb65892ed23..93f3d6ca95ef97eb31fc37bd6715f17c8433529d 100644 (file)
--- a/src/librbd/journal/Replay.cc
+++ b/src/librbd/journal/Replay.cc
@@ -356,8 +356,7 @@ void Replay<I>::handle_event(const journal::AioDiscardEvent &event,
   if (!clipped_io(event.offset, aio_comp)) {
     io::ImageRequest<I>::aio_discard(&m_image_ctx, aio_comp,
                                      {{event.offset, event.length}},
-                                     event.discard_granularity_bytes,
-                                     m_image_ctx.get_data_io_context(), {});
+                                     event.discard_granularity_bytes, {});
   }
 
   if (flush_required) {
@@ -392,7 +391,7 @@ void Replay<I>::handle_event(const journal::AioWriteEvent &event,
     io::ImageRequest<I>::aio_write(&m_image_ctx, aio_comp,
                                    {{event.offset, event.length}},
                                    std::move(data),
-                                   m_image_ctx.get_data_io_context(), 0, {});
+                                   0, {});
   }
 
   if (flush_required) {
@@ -446,8 +445,7 @@ void Replay<I>::handle_event(const journal::AioWriteSameEvent &event,
     io::ImageRequest<I>::aio_writesame(&m_image_ctx, aio_comp,
                                        {{event.offset, event.length}},
                                        std::move(data),
-                                       m_image_ctx.get_data_io_context(), 0,
-                                       {});
+                                       0, {});
   }
 
   if (flush_required) {
@@ -481,9 +479,7 @@ void Replay<I>::handle_event(const journal::AioWriteSameEvent &event,
                                                {{event.offset, event.length}},
                                                std::move(cmp_data),
                                                std::move(write_data),
-                                               nullptr,
-                                               m_image_ctx.get_data_io_context(),
-                                               0, {});
+                                               nullptr, 0, {});
   }
 
   if (flush_required) {