From ba6aa697c7831b04d03bb8031de5b247600ae03e Mon Sep 17 00:00:00 2001 From: Ilya Dryomov Date: Mon, 12 Jun 2023 21:45:03 +0200 Subject: [PATCH] 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 (cherry picked from commit e4b1e0466354942c935e9eca2ab2858e75049415) --- src/librbd/io/ImageDispatch.cc | 10 ++++---- src/librbd/io/ImageRequest.cc | 37 +++++++++++++--------------- src/librbd/io/ImageRequest.h | 44 ++++++++++++++++------------------ src/librbd/journal/Replay.cc | 12 ++++------ 4 files changed, 45 insertions(+), 58 deletions(-) diff --git a/src/librbd/io/ImageDispatch.cc b/src/librbd/io/ImageDispatch.cc index ed3cc357077fc..699e85d535182 100644 --- a/src/librbd/io/ImageDispatch.cc +++ b/src/librbd/io/ImageDispatch.cc @@ -75,8 +75,7 @@ bool ImageDispatch::write( *dispatch_result = DISPATCH_RESULT_COMPLETE; ImageRequest::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::discard( *dispatch_result = DISPATCH_RESULT_COMPLETE; ImageRequest::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::write_same( *dispatch_result = DISPATCH_RESULT_COMPLETE; ImageRequest::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::compare_and_write( ImageRequest::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 ea6b7301a2a9f..e4c41c22976a7 100644 --- a/src/librbd/io/ImageRequest.cc +++ b/src/librbd/io/ImageRequest.cc @@ -241,11 +241,10 @@ void ImageRequest::aio_read(I *ictx, AioCompletion *c, template void ImageRequest::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 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 void ImageRequest::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 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::aio_flush(I *ictx, AioCompletion *c, template void ImageRequest::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 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::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 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::ImageReadRequest(I &image_ctx, AioCompletion *aio_comp, int read_flags, const ZTracer::Trace &parent_trace) : ImageRequest(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::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::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::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::ImageListSnapsRequest( ImageArea area, SnapIds&& snap_ids, int list_snaps_flags, SnapshotDelta* snapshot_delta, const ZTracer::Trace& parent_trace) : ImageRequest(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 cd7b203e80cd6..2668c1acb2cda 100644 --- 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(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 { 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( - 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 { 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( - 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(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 { 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( - 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( - 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 c35b44bfcd727..42acf5eb24137 100644 --- a/src/librbd/journal/Replay.cc +++ b/src/librbd/journal/Replay.cc @@ -357,8 +357,7 @@ void Replay::handle_event(const journal::AioDiscardEvent &event, io::ImageRequest::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::handle_event(const journal::AioWriteEvent &event, io::ImageRequest::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::handle_event(const journal::AioWriteSameEvent &event, io::ImageRequest::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::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) { -- 2.39.5