From: Radoslaw Zarzynski <rzarzyns@redhat.com>
Date: Wed, 18 Sep 2024 13:14:45 +0000 (+0000)
Subject: os: get rid of the Transaction::decode_bp()
X-Git-Tag: v20.0.0~209^2
X-Git-Url: http://git.apps.os.sepia.ceph.com/?a=commitdiff_plain;h=e3a6680915780e4b03a380c0cbd20bd8771ef9f2;p=ceph.git

os: get rid of the Transaction::decode_bp()

`os::Transaction::decode_bp()` has only one user: `_setattrs()`
of `BlueStore`. It uses that for optimization purposes: keeping
up contigous space instead of potentially fragmented `bufferlist`
that would require rectifying memcpy later.
The problem is `_setattrs()` also needs to avoid keeping large
raw buffers with only small subset being referenced. It achieves
this by copying the data if `bufferptr:::is_partial()` returns
`true`. However, this means the memcpy happens virtually always
as it's hard to even imagine the `val`, decoded from the wire,
can fulfill the 0 waste requirement.
Therefore the optimization doesn't make sense; it only imposes
costs in terms of complexity breaking the symmetry between encode
and decode in `os::Transation` (there is no `encode_bp()`).

This commit kills the optimization and simplifies `os::Transaction`.

Signed-off-by: Radoslaw Zarzynski <rzarzyns@redhat.com>
---

diff --git a/src/os/Transaction.h b/src/os/Transaction.h
index f349a29c9f120..19896c79f95e2 100644
--- a/src/os/Transaction.h
+++ b/src/os/Transaction.h
@@ -711,10 +711,6 @@ public:
       decode(s, data_bl_p);
       return s;
     }
-    void decode_bp(ceph::buffer::ptr& bp) {
-	using ceph::decode;
-      decode(bp, data_bl_p);
-    }
     void decode_bl(ceph::buffer::list& bl) {
 	using ceph::decode;
       decode(bl, data_bl_p);
diff --git a/src/os/bluestore/BlueStore.cc b/src/os/bluestore/BlueStore.cc
index 44a171873c08c..791d03559db8a 100644
--- a/src/os/bluestore/BlueStore.cc
+++ b/src/os/bluestore/BlueStore.cc
@@ -15690,9 +15690,9 @@ void BlueStore::_txc_add_transaction(TransContext *txc, Transaction *t)
     case Transaction::OP_SETATTR:
       {
         string name = i.decode_string();
-        bufferptr bp;
-        i.decode_bp(bp);
-	r = _setattr(txc, c, o, name, bp);
+        bufferlist bl;
+        i.decode_bl(bl);
+	r = _setattr(txc, c, o, name, bl);
       }
       break;
 
@@ -17733,18 +17733,22 @@ int BlueStore::_setattr(TransContext *txc,
 			CollectionRef& c,
 			OnodeRef& o,
 			const string& name,
-			bufferptr& val)
+			bufferlist& val)
 {
   dout(15) << __func__ << " " << c->cid << " " << o->oid
 	   << " " << name << " (" << val.length() << " bytes)"
 	   << dendl;
   int r = 0;
-  if (val.is_partial()) {
-    auto& b = o->onode.attrs[name.c_str()] = bufferptr(val.c_str(),
-						       val.length());
+  if (!val.length()) {
+    auto& b = o->onode.attrs[name.c_str()] = bufferptr("", 0);
     b.reassign_to_mempool(mempool::mempool_bluestore_cache_meta);
-  } else {
-    auto& b = o->onode.attrs[name.c_str()] = val;
+  } else if (!val.is_contiguous()) {
+    val.rebuild();
+    auto& b = o->onode.attrs[name.c_str()] = val.front();
+    b.reassign_to_mempool(mempool::mempool_bluestore_cache_meta);
+  } else if (val.front().is_partial()) {
+    val.rebuild();
+    auto& b = o->onode.attrs[name.c_str()] = val.front();
     b.reassign_to_mempool(mempool::mempool_bluestore_cache_meta);
   }
   txc->write_onode(o);
diff --git a/src/os/bluestore/BlueStore.h b/src/os/bluestore/BlueStore.h
index 207ae2ec7a255..c5fc66fd16cb6 100644
--- a/src/os/bluestore/BlueStore.h
+++ b/src/os/bluestore/BlueStore.h
@@ -3862,7 +3862,7 @@ private:
 	       CollectionRef& c,
 	       OnodeRef& o,
 	       const std::string& name,
-	       ceph::buffer::ptr& val);
+	       ceph::buffer::list& val);
   int _setattrs(TransContext *txc,
 		CollectionRef& c,
 		OnodeRef& o,