From a92c533e56e65906e34a3a61e2af0f66c23d2954 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) 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 ] --- src/librbd/io/ImageDispatch.cc | 24 ++++++------- src/librbd/io/ImageRequest.cc | 40 ++++++++++----------- src/librbd/io/ImageRequest.h | 64 ++++++++++++++++------------------ src/librbd/journal/Replay.cc | 12 +++---- 4 files changed, 65 insertions(+), 75 deletions(-) diff --git a/src/librbd/io/ImageDispatch.cc b/src/librbd/io/ImageDispatch.cc index cc8519abeeeb8..0c173dd2661fe 100644 --- a/src/librbd/io/ImageDispatch.cc +++ b/src/librbd/io/ImageDispatch.cc @@ -65,9 +65,8 @@ bool ImageDispatch::write( start_in_flight_io(aio_comp); *dispatch_result = DISPATCH_RESULT_COMPLETE; - ImageRequest::aio_write( - m_image_ctx, aio_comp, std::move(image_extents), std::move(bl), - io_context, op_flags, parent_trace); + ImageRequest::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::discard( start_in_flight_io(aio_comp); *dispatch_result = DISPATCH_RESULT_COMPLETE; - ImageRequest::aio_discard( - m_image_ctx, aio_comp, std::move(image_extents), discard_granularity_bytes, - io_context, parent_trace); + ImageRequest::aio_discard(m_image_ctx, aio_comp, std::move(image_extents), + discard_granularity_bytes, parent_trace); return true; } @@ -104,9 +102,9 @@ bool ImageDispatch::write_same( start_in_flight_io(aio_comp); *dispatch_result = DISPATCH_RESULT_COMPLETE; - ImageRequest::aio_writesame( - m_image_ctx, aio_comp, std::move(image_extents), std::move(bl), - io_context, op_flags, parent_trace); + ImageRequest::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::compare_and_write( start_in_flight_io(aio_comp); *dispatch_result = DISPATCH_RESULT_COMPLETE; - ImageRequest::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::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 671ec4e5284b4..31405057ce97b 100644 --- a/src/librbd/io/ImageRequest.cc +++ b/src/librbd/io/ImageRequest.cc @@ -239,11 +239,11 @@ void ImageRequest::aio_read(I *ictx, AioCompletion *c, template void ImageRequest::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 req(*ictx, c, std::move(image_extents), std::move(bl), - io_context, op_flags, parent_trace); + ImageWriteRequest req(*ictx, c, std::move(image_extents), + std::move(bl), op_flags, parent_trace); req.send(); } @@ -251,11 +251,9 @@ template void ImageRequest::aio_discard(I *ictx, AioCompletion *c, Extents &&image_extents, uint32_t discard_granularity_bytes, - IOContext io_context, const ZTracer::Trace &parent_trace) { ImageDiscardRequest 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::aio_flush(I *ictx, AioCompletion *c, template void ImageRequest::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 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::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), 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::ImageReadRequest(I &image_ctx, AioCompletion *aio_comp, int read_flags, const ZTracer::Trace &parent_trace) : ImageRequest(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::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::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::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::ImageListSnapsRequest( SnapIds&& snap_ids, int list_snaps_flags, SnapshotDelta* snapshot_delta, const ZTracer::Trace& parent_trace) : ImageRequest(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 2c05c3847f0d9..7d3f2339568bc 100644 --- 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::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(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( - 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 { 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( - 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(image_ctx, aio_comp, {}, {}, "flush", - parent_trace), + : ImageRequest(image_ctx, aio_comp, {}, + "flush", parent_trace), m_flush_source(flush_source) { } @@ -268,12 +264,12 @@ template class ImageWriteSameRequest : public AbstractImageWriteRequest { 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( - 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::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( - 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 db73edb610947..93f3d6ca95ef9 100644 --- a/src/librbd/journal/Replay.cc +++ b/src/librbd/journal/Replay.cc @@ -356,8 +356,7 @@ void Replay::handle_event(const journal::AioDiscardEvent &event, if (!clipped_io(event.offset, aio_comp)) { io::ImageRequest::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::handle_event(const journal::AioWriteEvent &event, io::ImageRequest::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::handle_event(const journal::AioWriteSameEvent &event, io::ImageRequest::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::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) { -- 2.39.5