From b151013201e2a98f37e90c3aa6c432291649d275 Mon Sep 17 00:00:00 2001 From: Vitaliy Filippov Date: Sun, 23 Feb 2025 02:31:19 +0300 Subject: [PATCH] Fix snapshot reads from a dirty write-back cache --- src/client/cluster_client.cpp | 53 +++++++++++++++++++++++++++----- src/client/cluster_client.h | 3 +- src/client/cluster_client_impl.h | 1 + src/client/cluster_client_wb.cpp | 15 ++++++++- src/osd/osd_primary.cpp | 3 +- 5 files changed, 65 insertions(+), 10 deletions(-) diff --git a/src/client/cluster_client.cpp b/src/client/cluster_client.cpp index 37d2f1e0..db52152c 100644 --- a/src/client/cluster_client.cpp +++ b/src/client/cluster_client.cpp @@ -792,6 +792,36 @@ bool cluster_client_t::check_rw(cluster_op_t *op) return false; } } + op->deoptimise_snapshot = false; + if (enable_writeback && (op->opcode == OSD_OP_READ || op->opcode == OSD_OP_READ_BITMAP || op->opcode == OSD_OP_READ_CHAIN_BITMAP)) + { + auto ino_it = st_cli.inode_config.find(op->inode); + if (ino_it != st_cli.inode_config.end()) + { + int chain_size = 0; + while (ino_it != st_cli.inode_config.end() && ino_it->second.parent_id) + { + // Check for loops - FIXME check it in etcd_state_client + if (ino_it->second.parent_id == op->inode || + chain_size > st_cli.inode_config.size()) + { + op->retval = -EINVAL; + auto cb = std::move(op->callback); + cb(op); + return false; + } + if (INODE_POOL(ino_it->second.parent_id) == INODE_POOL(ino_it->first) && + wb->has_inode(ino_it->second.parent_id)) + { + // Deoptimise reads - we have dirty data for one of the parent layer(s). + op->deoptimise_snapshot = true; + break; + } + chain_size++; + ino_it = st_cli.inode_config.find(ino_it->second.parent_id); + } + } + } return true; } @@ -923,12 +953,21 @@ resume_2: { // Check parent inode auto ino_it = st_cli.inode_config.find(op->cur_inode); - while (ino_it != st_cli.inode_config.end() && ino_it->second.parent_id && - INODE_POOL(ino_it->second.parent_id) == INODE_POOL(op->cur_inode) && - // Check for loops - ino_it->second.parent_id != op->inode) + // Skip parents from the same pool + int skipped = 0; + while (!op->deoptimise_snapshot && + ino_it != st_cli.inode_config.end() && ino_it->second.parent_id && + INODE_POOL(ino_it->second.parent_id) == INODE_POOL(op->cur_inode)) { - // Skip parents from the same pool + // Check for loops - FIXME check it in etcd_state_client + if (ino_it->second.parent_id == op->inode || + skipped > st_cli.inode_config.size()) + { + op->retval = -EINVAL; + erase_op(op); + return 1; + } + skipped++; ino_it = st_cli.inode_config.find(ino_it->second.parent_id); } if (ino_it != st_cli.inode_config.end() && @@ -1107,7 +1146,7 @@ void cluster_client_t::slice_rw(cluster_op_t *op) if (end == begin) { op->done_count++; - op->parts[i].flags = PART_DONE; + op->parts[i].flags = PART_SENT|PART_DONE; } } else if (op->opcode != OSD_OP_READ_BITMAP && op->opcode != OSD_OP_READ_CHAIN_BITMAP && op->opcode != OSD_OP_DELETE) @@ -1190,7 +1229,7 @@ int cluster_client_t::try_send(cluster_op_t *op, int i) pool_cfg.scheme == POOL_SCHEME_REPLICATED ? 1 : pool_cfg.pg_size-pool_cfg.parity_chunks ); uint64_t meta_rev = 0; - if (op->opcode != OSD_OP_READ_BITMAP && op->opcode != OSD_OP_DELETE) + if (op->opcode != OSD_OP_READ_BITMAP && op->opcode != OSD_OP_DELETE && !op->deoptimise_snapshot) { auto ino_it = st_cli.inode_config.find(op->cur_inode); if (ino_it != st_cli.inode_config.end()) diff --git a/src/client/cluster_client.h b/src/client/cluster_client.h index 632d8bf0..658e53a6 100644 --- a/src/client/cluster_client.h +++ b/src/client/cluster_client.h @@ -60,7 +60,8 @@ struct cluster_op_t protected: int state = 0; uint64_t cur_inode; // for snapshot reads - bool needs_reslice = false; + bool needs_reslice: 1; + bool deoptimise_snapshot: 1; int retry_after = 0; int inflight_count = 0, done_count = 0; timespec wait_up_until = {}; diff --git a/src/client/cluster_client_impl.h b/src/client/cluster_client_impl.h index 7fdd0509..14c30936 100644 --- a/src/client/cluster_client_impl.h +++ b/src/client/cluster_client_impl.h @@ -42,6 +42,7 @@ public: std::multimap flushed_buffers; // flush_id => refcnt ~writeback_cache_t(); + bool has_inode(uint64_t inode); dirty_buf_it_t find_dirty(uint64_t inode, uint64_t offset); bool is_left_merged(dirty_buf_it_t dirty_it); bool is_right_merged(dirty_buf_it_t dirty_it); diff --git a/src/client/cluster_client_wb.cpp b/src/client/cluster_client_wb.cpp index d21602c3..7f0cd866 100644 --- a/src/client/cluster_client_wb.cpp +++ b/src/client/cluster_client_wb.cpp @@ -17,6 +17,15 @@ writeback_cache_t::~writeback_cache_t() dirty_buffers.clear(); } +bool writeback_cache_t::has_inode(uint64_t inode) +{ + auto dirty_it = dirty_buffers.lower_bound((object_id){ + .inode = inode, + .stripe = 0, + }); + return dirty_it != dirty_buffers.end() && dirty_it->first.inode == inode; +} + dirty_buf_it_t writeback_cache_t::find_dirty(uint64_t inode, uint64_t offset) { auto dirty_it = dirty_buffers.lower_bound((object_id){ @@ -33,7 +42,11 @@ dirty_buf_it_t writeback_cache_t::find_dirty(uint64_t inode, uint64_t offset) break; } } - return dirty_it; + if (dirty_it != dirty_buffers.end() && dirty_it->first.inode == inode) + { + return dirty_it; + } + return dirty_buffers.end(); } bool writeback_cache_t::is_left_merged(dirty_buf_it_t dirty_it) diff --git a/src/osd/osd_primary.cpp b/src/osd/osd_primary.cpp index 98bf966a..89d370d8 100644 --- a/src/osd/osd_primary.cpp +++ b/src/osd/osd_primary.cpp @@ -57,6 +57,7 @@ bool osd_t::prepare_primary_rw(osd_op_t *cur_op) if (cur_op->req.hdr.opcode == OSD_OP_READ && cur_op->req.rw.meta_revision > 0) { // Chained read + // FIXME: Introduce an explicit opcode for chained reads auto inode_it = st_cli.inode_config.find(cur_op->req.rw.inode); if (inode_it->second.mod_revision != cur_op->req.rw.meta_revision) { @@ -70,7 +71,7 @@ bool osd_t::prepare_primary_rw(osd_op_t *cur_op) inode_it->second.parent_id && INODE_POOL(inode_it->second.parent_id) == pg_it->second.pool_id) { - // Check for loops + // Check for loops - FIXME check it in etcd_state_client if (inode_it->second.parent_id == cur_op->req.rw.inode || inode_it->second.parent_id == inode_it->second.num || chain_size > st_cli.inode_config.size())