From b856524e0c2c8dae9cdc661db7ef487b82763eda Mon Sep 17 00:00:00 2001
From: Vitaliy Filippov <vitalif@yourcmc.ru>
Date: Wed, 11 Dec 2024 01:09:25 +0300
Subject: [PATCH] Workaround for Linux bug: return post_op_attr for NFS-RDMA
 READ3

Linux NFS RDMA transport has a stupid bug - when the reply doesn't contain
post_op_attr, the data gets offsetted by 84 bytes (size of attributes) and
first 84 bytes are filled with probably random data.
---
 src/nfs/nfs_block.cpp      | 12 +++++++++++
 src/nfs/nfs_kv.cpp         | 32 ++++++++++++++++++++++++++---
 src/nfs/nfs_kv.h           |  4 +++-
 src/nfs/nfs_kv_create.cpp  |  4 ++--
 src/nfs/nfs_kv_getattr.cpp |  2 +-
 src/nfs/nfs_kv_link.cpp    |  4 ++--
 src/nfs/nfs_kv_lookup.cpp  |  2 +-
 src/nfs/nfs_kv_read.cpp    | 41 ++++++++++++++++++++++++++++++++++++++
 src/nfs/nfs_kv_readdir.cpp |  6 +++---
 src/nfs/nfs_kv_remove.cpp  |  2 +-
 src/nfs/nfs_kv_rename.cpp  |  4 ++--
 src/nfs/nfs_kv_setattr.cpp |  4 ++--
 src/nfs/nfs_kv_write.cpp   | 26 ++++++++++++------------
 13 files changed, 112 insertions(+), 31 deletions(-)

diff --git a/src/nfs/nfs_block.cpp b/src/nfs/nfs_block.cpp
index 245b5604..d8bc0136 100644
--- a/src/nfs/nfs_block.cpp
+++ b/src/nfs/nfs_block.cpp
@@ -334,10 +334,22 @@ static int block_nfs3_read_proc(void *opaque, rpc_op_t *rop)
         }
         else
         {
+            nfs_client_t *self = (nfs_client_t*)rop->client;
             auto & reply_ok = reply->resok;
             // reply_ok.data.data is already set above
             reply_ok.count = reply_ok.data.size;
             reply_ok.eof = 0;
+            if (self->rdma_conn)
+            {
+                // FIXME Linux NFS RDMA transport has a bug - when the reply
+                // doesn't contain post_op_attr, the data gets offsetted by
+                // 84 bytes (size of attributes)...
+                // So we have to fill it with RDMA. :-(
+                reply_ok.file_attributes = (post_op_attr){
+                    .attributes_follow = 1,
+                    .attributes = get_file_attributes(self, op->inode),
+                };
+            }
         }
         rpc_queue_reply(rop);
         delete op;
diff --git a/src/nfs/nfs_kv.cpp b/src/nfs/nfs_kv.cpp
index 2e64f444..791ec2b1 100644
--- a/src/nfs/nfs_kv.cpp
+++ b/src/nfs/nfs_kv.cpp
@@ -65,7 +65,7 @@ int kv_map_type(const std::string & type)
         (type == "fifo" ? NF3FIFO : -1)))))));
 }
 
-fattr3 get_kv_attributes(nfs_client_t *self, uint64_t ino, json11::Json attrs)
+fattr3 get_kv_attributes(nfs_proxy_t *proxy, uint64_t ino, json11::Json attrs)
 {
     auto type = kv_map_type(attrs["type"].string_value());
     auto mode = attrs["mode"].uint64_value();
@@ -86,7 +86,7 @@ fattr3 get_kv_attributes(nfs_client_t *self, uint64_t ino, json11::Json attrs)
         .rdev = (type == NF3BLK || type == NF3CHR
             ? (specdata3){ (uint32_t)attrs["major"].uint64_value(), (uint32_t)attrs["minor"].uint64_value() }
             : (specdata3){}),
-        .fsid = self->parent->fsid,
+        .fsid = proxy->fsid,
         .fileid = ino,
         .atime = atime,
         .mtime = mtime,
@@ -349,6 +349,27 @@ kv_fs_state_t::~kv_fs_state_t()
     }
 }
 
+void kv_fs_state_t::write_inode(inode_t ino, json11::Json value, bool hack_cache, std::function<void(int)> cb, std::function<bool(int, const std::string &)> cas_cb)
+{
+    if (!proxy->rdma_context)
+    {
+        proxy->db->set(kv_inode_key(ino), value.dump(), cb, cas_cb);
+        return;
+    }
+    // FIXME Linux NFS RDMA transport has a bug - it corrupts the data (by offsetting it 84 bytes)
+    // when the READ reply doesn't contain post_op_attr. So we have to fill post_op_attr with RDMA. :-(
+    // So we at least cache it to not repeat K/V requests every read.
+    read_hack_cache.erase(ino);
+    proxy->db->set(kv_inode_key(ino), value.dump(), [=](int res)
+    {
+        if (hack_cache || res != 0)
+            read_hack_cache.erase(ino);
+        else
+            read_hack_cache[ino] = value;
+        cb(res);
+    }, cas_cb);
+}
+
 void kv_fs_state_t::update_inode(inode_t ino, bool allow_cache, std::function<void(json11::Json::object &)> change, std::function<void(int)> cb)
 {
     // FIXME: Use "update" query
@@ -356,12 +377,15 @@ void kv_fs_state_t::update_inode(inode_t ino, bool allow_cache, std::function<vo
     {
         if (!res)
         {
+            read_hack_cache.erase(ino);
             auto ientry = attrs.object_items();
             change(ientry);
             bool *found = new bool;
             *found = true;
-            proxy->db->set(kv_inode_key(ino), json11::Json(ientry).dump(), [=](int res)
+            json11::Json ientry_json(ientry);
+            proxy->db->set(kv_inode_key(ino), ientry_json.dump(), [=](int res)
             {
+                read_hack_cache.erase(ino);
                 if (!*found)
                     res = -ENOENT;
                 delete found;
@@ -384,6 +408,8 @@ void kv_fs_state_t::update_inode(inode_t ino, bool allow_cache, std::function<vo
 
 void kv_fs_state_t::touch_inodes()
 {
+    // Clear RDMA read fattr3 "hack" cache every second
+    read_hack_cache.clear();
     std::set<inode_t> q = std::move(touch_queue);
     for (auto ino: q)
     {
diff --git a/src/nfs/nfs_kv.h b/src/nfs/nfs_kv.h
index 07ab9b69..c569c84a 100644
--- a/src/nfs/nfs_kv.h
+++ b/src/nfs/nfs_kv.h
@@ -75,6 +75,7 @@ struct kv_fs_state_t
     std::map<inode_t, kv_inode_extend_t> extends;
     std::set<inode_t> touch_queue;
     std::map<inode_t, uint64_t> volume_removed;
+    std::map<inode_t, json11::Json> read_hack_cache;
     uint64_t volume_stats_ctr = 0;
     uint64_t volume_touch_ctr = 0;
 
@@ -87,6 +88,7 @@ struct kv_fs_state_t
     void upgrade_db(std::function<void(int)> cb);
     void defrag_all(json11::Json cfg, std::function<void(int)> cb);
     void defrag_volume(inode_t ino, bool no_rm, bool dry_run, std::function<void(int, uint64_t, uint64_t, uint64_t)> cb);
+    void write_inode(inode_t ino, json11::Json value, bool hack_cache, std::function<void(int)> cb, std::function<bool(int, const std::string &)> cas_cb);
     ~kv_fs_state_t();
 };
 
@@ -116,7 +118,7 @@ nfstime3 nfstime_from_str(const std::string & s);
 std::string nfstime_to_str(nfstime3 t);
 std::string nfstime_now_str();
 int kv_map_type(const std::string & type);
-fattr3 get_kv_attributes(nfs_client_t *self, uint64_t ino, json11::Json attrs);
+fattr3 get_kv_attributes(nfs_proxy_t *proxy, uint64_t ino, json11::Json attrs);
 std::string kv_direntry_key(uint64_t dir_ino, const std::string & filename);
 std::string kv_direntry_filename(const std::string & key);
 std::string kv_inode_prefix_key(uint64_t ino, const char *prefix);
diff --git a/src/nfs/nfs_kv_create.cpp b/src/nfs/nfs_kv_create.cpp
index f1213c6e..c1a87a37 100644
--- a/src/nfs/nfs_kv_create.cpp
+++ b/src/nfs/nfs_kv_create.cpp
@@ -143,7 +143,7 @@ resume_2:
         cb(st->res);
         return;
     }
-    st->self->parent->db->set(kv_inode_key(st->new_id), st->attrs.dump().c_str(), [st](int res)
+    st->self->parent->kvfs->write_inode(st->new_id, st->attrs, false, [st](int res)
     {
         st->res = res;
         kv_continue_create(st, 3);
@@ -267,7 +267,7 @@ template<class T, class Tok> static void kv_create_reply(kv_create_state *st, in
                 },
                 .obj_attributes = {
                     .attributes_follow = 1,
-                    .attributes = get_kv_attributes(st->self, st->new_id, st->attrs),
+                    .attributes = get_kv_attributes(st->self->parent, st->new_id, st->attrs),
                 },
             },
         };
diff --git a/src/nfs/nfs_kv_getattr.cpp b/src/nfs/nfs_kv_getattr.cpp
index 7355594b..619ea3a2 100644
--- a/src/nfs/nfs_kv_getattr.cpp
+++ b/src/nfs/nfs_kv_getattr.cpp
@@ -68,7 +68,7 @@ int kv_nfs3_getattr_proc(void *opaque, rpc_op_t *rop)
             *reply = (GETATTR3res){
                 .status = NFS3_OK,
                 .resok = (GETATTR3resok){
-                    .obj_attributes = get_kv_attributes(self, ino, attrs),
+                    .obj_attributes = get_kv_attributes(self->parent, ino, attrs),
                 },
             };
         }
diff --git a/src/nfs/nfs_kv_link.cpp b/src/nfs/nfs_kv_link.cpp
index 3dffc0ab..a44eb68e 100644
--- a/src/nfs/nfs_kv_link.cpp
+++ b/src/nfs/nfs_kv_link.cpp
@@ -102,7 +102,7 @@ resume_2:
         new_ientry["ctime"] = nfstime_now_str();
         st->ientry = new_ientry;
     }
-    st->self->parent->db->set(kv_inode_key(st->ino), st->ientry.dump(), [st](int res)
+    st->self->parent->kvfs->write_inode(st->ino, st->ientry, false, [st](int res)
     {
         st->res = res;
         nfs_kv_continue_link(st, 3);
@@ -180,7 +180,7 @@ int kv_nfs3_link_proc(void *opaque, rpc_op_t *rop)
                 .resok = (LINK3resok){
                     .file_attributes = (post_op_attr){
                         .attributes_follow = 1,
-                        .attributes = get_kv_attributes(st->self, st->ino, st->ientry),
+                        .attributes = get_kv_attributes(st->self->parent, st->ino, st->ientry),
                     },
                 },
             };
diff --git a/src/nfs/nfs_kv_lookup.cpp b/src/nfs/nfs_kv_lookup.cpp
index 9636840b..5b8c60ef 100644
--- a/src/nfs/nfs_kv_lookup.cpp
+++ b/src/nfs/nfs_kv_lookup.cpp
@@ -55,7 +55,7 @@ int kv_nfs3_lookup_proc(void *opaque, rpc_op_t *rop)
                     .object = xdr_copy_string(rop->xdrs, kv_fh(ino)),
                     .obj_attributes = {
                         .attributes_follow = 1,
-                        .attributes = get_kv_attributes(self, ino, ientry),
+                        .attributes = get_kv_attributes(self->parent, ino, ientry),
                     },
                 },
             };
diff --git a/src/nfs/nfs_kv_read.cpp b/src/nfs/nfs_kv_read.cpp
index 78eae9fe..cf936ff9 100644
--- a/src/nfs/nfs_kv_read.cpp
+++ b/src/nfs/nfs_kv_read.cpp
@@ -37,6 +37,7 @@ static void nfs_kv_continue_read(nfs_kv_read_state *st, int state)
     else if (state == 1) goto resume_1;
     else if (state == 2) goto resume_2;
     else if (state == 3) goto resume_3;
+    else if (state == 4) goto resume_4;
     else
     {
         fprintf(stderr, "BUG: invalid state in nfs_kv_continue_read()");
@@ -141,6 +142,35 @@ resume_2:
             return;
         }
     }
+    else if (st->self->rdma_conn)
+    {
+        // Take ientry from read_hack_cache for RDMA connections
+        {
+            auto rh_it = st->self->parent->kvfs->read_hack_cache.find(st->ino);
+            if (rh_it != st->self->parent->kvfs->read_hack_cache.end())
+            {
+                st->ientry = rh_it->second;
+            }
+        }
+        if (st->ientry.is_null())
+        {
+            kv_read_inode(st->self->parent, st->ino, [st](int res, const std::string & value, json11::Json attrs)
+            {
+                st->res = res;
+                st->ientry = attrs;
+                nfs_kv_continue_read(st, 4);
+            }, st->allow_cache);
+            return;
+resume_4:
+            if (st->res < 0 || kv_map_type(st->ientry["type"].string_value()) != NF3REG)
+            {
+                auto cb = std::move(st->cb);
+                cb(st->res < 0 ? st->res : -EINVAL);
+                return;
+            }
+            st->self->parent->kvfs->read_hack_cache[st->ino] = st->ientry;
+        }
+    }
     st->aligned_offset = align_down(st->offset);
     st->aligned_size = align_up(st->offset+st->size) - st->aligned_offset;
     assert(!st->aligned_buf);
@@ -198,6 +228,17 @@ int kv_nfs3_read_proc(void *opaque, rpc_op_t *rop)
             reply->resok.data.size = st->size;
             reply->resok.count = st->size;
             reply->resok.eof = st->eof;
+            if (st->self->rdma_conn)
+            {
+                // FIXME Linux NFS RDMA transport has a bug - when the reply
+                // doesn't contain post_op_attr, the data gets offsetted by
+                // 84 bytes (size of attributes)...
+                // So we have to fill it with RDMA. :-(
+                reply->resok.file_attributes = (post_op_attr){
+                    .attributes_follow = 1,
+                    .attributes = get_kv_attributes(st->self->parent, st->ino, st->ientry),
+                };
+            }
         }
         rpc_queue_reply(st->rop);
         delete st;
diff --git a/src/nfs/nfs_kv_readdir.cpp b/src/nfs/nfs_kv_readdir.cpp
index cbb6b5e7..97cee3fe 100644
--- a/src/nfs/nfs_kv_readdir.cpp
+++ b/src/nfs/nfs_kv_readdir.cpp
@@ -57,7 +57,7 @@ static void kv_getattr_next(nfs_kv_readdir_state *st)
                 st->entries[idx].name_attributes = (post_op_attr){
                     // FIXME: maybe do not read parent attributes and leave them to a GETATTR?
                     .attributes_follow = 1,
-                    .attributes = get_kv_attributes(st->self, st->entries[idx].fileid, ientry),
+                    .attributes = get_kv_attributes(st->self->parent, st->entries[idx].fileid, ientry),
                 };
             }
             st->getattr_running--;
@@ -126,7 +126,7 @@ resume_1:
             dot.fileid = st->dir_ino;
             dot.name_attributes = (post_op_attr){
                 .attributes_follow = 1,
-                .attributes = get_kv_attributes(st->self, st->dir_ino, st->ientry),
+                .attributes = get_kv_attributes(st->self->parent, st->dir_ino, st->ientry),
             };
             dot.name_handle = (post_op_fh3){
                 .handle_follows = 1,
@@ -169,7 +169,7 @@ resume_2:
         dotdot.name_attributes = (post_op_attr){
             // FIXME: maybe do not read parent attributes and leave them to a GETATTR?
             .attributes_follow = 1,
-            .attributes = get_kv_attributes(st->self,
+            .attributes = get_kv_attributes(st->self->parent,
                 st->parent_ino ? st->parent_ino : st->dir_ino,
                 st->parent_ino ? st->parent_ientry : st->ientry),
         };
diff --git a/src/nfs/nfs_kv_remove.cpp b/src/nfs/nfs_kv_remove.cpp
index 031cd2a1..17a2eee7 100644
--- a/src/nfs/nfs_kv_remove.cpp
+++ b/src/nfs/nfs_kv_remove.cpp
@@ -197,7 +197,7 @@ resume_5:
         auto copy = st->ientry.object_items();
         copy["nlink"] = st->ientry["nlink"].uint64_value()-1;
         copy["ctime"] = nfstime_now_str();
-        st->self->parent->db->set(kv_inode_key(st->ino), json11::Json(copy).dump(), [st](int res)
+        st->self->parent->kvfs->write_inode(st->ino, copy, false, [st](int res)
         {
             st->res = res;
             nfs_kv_continue_delete(st, 6);
diff --git a/src/nfs/nfs_kv_rename.cpp b/src/nfs/nfs_kv_rename.cpp
index 7bd27c11..ce4fe570 100644
--- a/src/nfs/nfs_kv_rename.cpp
+++ b/src/nfs/nfs_kv_rename.cpp
@@ -240,7 +240,7 @@ resume_7:
                 copy["nlink"] = st->new_ientry["nlink"].uint64_value()-1;
                 copy["ctime"] = nfstime_now_str();
                 copy.erase("verf");
-                st->self->parent->db->set(kv_inode_key(st->new_direntry["ino"].uint64_value()), json11::Json(copy).dump(), [st](int res)
+                st->self->parent->kvfs->write_inode(st->new_direntry["ino"].uint64_value(), copy, false, [st](int res)
                 {
                     st->res = res;
                     nfs_kv_continue_rename(st, 8);
@@ -328,7 +328,7 @@ resume_11:
             ientry_new["parent_ino"] = st->new_dir_ino;
             ientry_new["ctime"] = nfstime_now_str();
             ientry_new.erase("verf");
-            st->self->parent->db->set(kv_inode_key(st->old_direntry["ino"].uint64_value()), json11::Json(ientry_new).dump(), [st](int res)
+            st->self->parent->kvfs->write_inode(st->old_direntry["ino"].uint64_value(), ientry_new, false, [st](int res)
             {
                 st->res = res;
                 nfs_kv_continue_rename(st, 12);
diff --git a/src/nfs/nfs_kv_setattr.cpp b/src/nfs/nfs_kv_setattr.cpp
index 4b3dee2f..20cfa61f 100644
--- a/src/nfs/nfs_kv_setattr.cpp
+++ b/src/nfs/nfs_kv_setattr.cpp
@@ -84,7 +84,7 @@ resume_1:
     }
     st->new_attrs.erase("verf");
     st->new_attrs["ctime"] = nfstime_now_str();
-    st->self->parent->db->set(kv_inode_key(st->ino), json11::Json(st->new_attrs).dump(), [st](int res)
+    st->self->parent->kvfs->write_inode(st->ino, st->new_attrs, false, [st](int res)
     {
         st->res = res;
         nfs_kv_continue_setattr(st, 2);
@@ -190,7 +190,7 @@ int kv_nfs3_setattr_proc(void *opaque, rpc_op_t *rop)
                     .obj_wcc = (wcc_data){
                         .after = (post_op_attr){
                             .attributes_follow = 1,
-                            .attributes = get_kv_attributes(st->self, st->ino, st->new_attrs),
+                            .attributes = get_kv_attributes(st->self->parent, st->ino, st->new_attrs),
                         },
                     },
                 },
diff --git a/src/nfs/nfs_kv_write.cpp b/src/nfs/nfs_kv_write.cpp
index b33011f5..17427f27 100644
--- a/src/nfs/nfs_kv_write.cpp
+++ b/src/nfs/nfs_kv_write.cpp
@@ -553,7 +553,7 @@ static void nfs_do_align_write(nfs_kv_write_state *st, uint64_t ino, uint64_t of
     }
 }
 
-static std::string new_normal_ientry(nfs_kv_write_state *st)
+static json11::Json new_normal_ientry(nfs_kv_write_state *st)
 {
     auto ni = st->ientry.object_items();
     ni.erase("empty");
@@ -564,10 +564,10 @@ static std::string new_normal_ientry(nfs_kv_write_state *st)
     ni["size"] = st->ext->cur_extend;
     ni["ctime"] = ni["mtime"] = nfstime_now_str();
     ni.erase("verf");
-    return json11::Json(ni).dump();
+    return ni;
 }
 
-static std::string new_moved_ientry(nfs_kv_write_state *st)
+static json11::Json new_moved_ientry(nfs_kv_write_state *st)
 {
     auto ni = st->ientry.object_items();
     ni.erase("empty");
@@ -578,10 +578,10 @@ static std::string new_moved_ientry(nfs_kv_write_state *st)
     ni["size"] = st->new_size;
     ni["ctime"] = ni["mtime"] = nfstime_now_str();
     ni.erase("verf");
-    return json11::Json(ni).dump();
+    return ni;
 }
 
-static std::string new_shared_ientry(nfs_kv_write_state *st)
+static json11::Json new_shared_ientry(nfs_kv_write_state *st)
 {
     auto ni = st->ientry.object_items();
     ni.erase("empty");
@@ -589,10 +589,10 @@ static std::string new_shared_ientry(nfs_kv_write_state *st)
     ni["ctime"] = ni["mtime"] = nfstime_now_str();
     ni["shared_ver"] = ni["shared_ver"].uint64_value()+1;
     ni.erase("verf");
-    return json11::Json(ni).dump();
+    return ni;
 }
 
-static std::string new_unshared_ientry(nfs_kv_write_state *st)
+static json11::Json new_unshared_ientry(nfs_kv_write_state *st)
 {
     auto ni = st->ientry.object_items();
     ni.erase("empty");
@@ -602,7 +602,7 @@ static std::string new_unshared_ientry(nfs_kv_write_state *st)
     ni.erase("shared_ver");
     ni["ctime"] = ni["mtime"] = nfstime_now_str();
     ni.erase("verf");
-    return json11::Json(ni).dump();
+    return ni;
 }
 
 static void nfs_kv_extend_inode(nfs_kv_write_state *st, int state, int base_state)
@@ -612,7 +612,7 @@ static void nfs_kv_extend_inode(nfs_kv_write_state *st, int state, int base_stat
     st->ext->cur_extend = st->ext->next_extend;
     st->ext->next_extend = 0;
     st->res2 = -EAGAIN;
-    st->proxy->db->set(kv_inode_key(st->ino), new_normal_ientry(st), [st, base_state](int res)
+    st->proxy->kvfs->write_inode(st->ino, new_normal_ientry(st), true, [st, base_state](int res)
     {
         st->res = res;
         nfs_kv_continue_write(st, base_state+1);
@@ -838,7 +838,7 @@ resume_4:
                 cb(st->res);
                 return;
             }
-            st->proxy->db->set(kv_inode_key(st->ino), new_moved_ientry(st), [st](int res)
+            st->proxy->kvfs->write_inode(st->ino, new_moved_ientry(st), true, [st](int res)
             {
                 st->res = res;
                 nfs_kv_continue_write(st, 5);
@@ -881,7 +881,7 @@ resume_7:
             }
 resume_8:
             // We always have to change inode entry on shared writes
-            st->proxy->db->set(kv_inode_key(st->ino), new_shared_ientry(st), [st](int res)
+            st->proxy->kvfs->write_inode(st->ino, new_shared_ientry(st), true, [st](int res)
             {
                 st->res = res;
                 nfs_kv_continue_write(st, 9);
@@ -930,7 +930,7 @@ resume_11:
                 return;
             }
         }
-        st->proxy->db->set(kv_inode_key(st->ino), new_unshared_ientry(st), [st](int res)
+        st->proxy->kvfs->write_inode(st->ino, new_unshared_ientry(st), true, [st](int res)
         {
             st->res = res;
             nfs_kv_continue_write(st, 12);
@@ -953,7 +953,7 @@ resume_12:
         }
         // Record removed part of the shared inode as obsolete in statistics
         st->proxy->kvfs->volume_removed[st->ientry["shared_ino"].uint64_value()] += st->ientry["shared_alloc"].uint64_value();
-        st->ientry_text = new_unshared_ientry(st);
+        st->ientry_text = new_unshared_ientry(st).dump();
     }
     // Non-shared write
     nfs_do_align_write(st, st->ino, st->offset, 0, 13);