Fix snapshot reads from a dirty write-back cache

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 &&
+            // Skip parents from the same pool
-                INODE_POOL(ino_it->second.parent_id) == INODE_POOL(op->cur_inode) &&
+            int skipped = 0;
-                // Check for loops
+            while (!op->deoptimise_snapshot &&
-                ino_it->second.parent_id != op->inode)
+                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())

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 = {};

src/client/cluster_client_impl.h

@@ -42,6 +42,7 @@ public:
     std::multimap<uint64_t, uint64_t*> 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);

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)

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())