From 50f042d661ee78c91f5c7b0d1715c4303f35de73 Mon Sep 17 00:00:00 2001 From: Radoslaw Zarzynski Date: Tue, 6 Aug 2019 13:56:51 +0200 Subject: [PATCH] crimson/osd: OpsExecuter differentiates read and modifying operations. This is necessary to pass ClsHello.BadMethods. Signed-off-by: Radoslaw Zarzynski --- src/crimson/osd/ops_executer.cc | 90 ++++++++++++++++++--------------- src/crimson/osd/ops_executer.h | 22 ++++++++ src/crimson/osd/pg_backend.cc | 6 +-- src/crimson/osd/pg_backend.h | 4 +- 4 files changed, 76 insertions(+), 46 deletions(-) diff --git a/src/crimson/osd/ops_executer.cc b/src/crimson/osd/ops_executer.cc index 0a293835982..d668dbd12b6 100644 --- a/src/crimson/osd/ops_executer.cc +++ b/src/crimson/osd/ops_executer.cc @@ -74,35 +74,31 @@ seastar::future<> OpsExecuter::do_op_call(OSDOp& osd_op) #endif logger().debug("calling method {}.{}", cname, mname); -#if 0 - int prev_rd = ctx->num_read; - int prev_wr = ctx->num_write; -#endif - - - return seastar::async([this, &osd_op, method, indata=std::move(indata)]() mutable { + return seastar::async([this, &osd_op, flags, method, indata=std::move(indata)]() mutable { ceph::bufferlist outdata; + const auto prev_rd = num_read; + const auto prev_wr = num_write; const auto ret = method->exec(reinterpret_cast(this), indata, outdata); + if (num_read > prev_rd && !(flags & CLS_METHOD_RD)) { + logger().error("method tried to read object but is not marked RD"); + throw ceph::osd::input_output_error{}; + } + if (num_write > prev_wr && !(flags & CLS_METHOD_WR)) { + logger().error("method tried to update object but is not marked WR"); + throw ceph::osd::input_output_error{}; + } + + // for write calls we never return data expect errors. For details refer + // to cls/cls_hello.cc. + if (ret < 0 || (flags & CLS_METHOD_WR) == 0) { + logger().debug("method called response length={}", outdata.length()); + osd_op.op.extent.length = outdata.length(); + osd_op.outdata.claim_append(outdata); + } if (ret < 0) { throw ceph::osd::make_error(ret); } -#if 0 - if (ctx->num_read > prev_rd && !(flags & CLS_METHOD_RD)) { - derr << "method " << cname << "." << mname << " tried to read object but is not marked RD" << dendl; - result = -EIO; - break; - } - if (ctx->num_write > prev_wr && !(flags & CLS_METHOD_WR)) { - derr << "method " << cname << "." << mname << " tried to update object but is not marked WR" << dendl; - result = -EIO; - break; - } -#endif - - logger().debug("method called response length={}", outdata.length()); - osd_op.op.extent.length = outdata.length(); - osd_op.outdata.claim_append(outdata); }); } @@ -152,7 +148,6 @@ OpsExecuter::do_pgnls(ceph::bufferlist& indata, }); } -// TODO: split the method accordingly to os' constness needs seastar::future<> OpsExecuter::do_osd_op(OSDOp& osd_op) { @@ -164,27 +159,37 @@ OpsExecuter::do_osd_op(OSDOp& osd_op) case CEPH_OSD_OP_SYNC_READ: [[fallthrough]]; case CEPH_OSD_OP_READ: - return backend.read(os->oi, - op.extent.offset, - op.extent.length, - op.extent.truncate_size, - op.extent.truncate_seq, - op.flags).then([&osd_op](bufferlist bl) { - osd_op.rval = bl.length(); - osd_op.outdata = std::move(bl); - return seastar::now(); + return do_read_op([&osd_op] (auto& backend, const auto& os) { + return backend.read(os.oi, + osd_op.op.extent.offset, + osd_op.op.extent.length, + osd_op.op.extent.truncate_size, + osd_op.op.extent.truncate_seq, + osd_op.op.flags).then( + [&osd_op](bufferlist bl) { + osd_op.rval = bl.length(); + osd_op.outdata = std::move(bl); + return seastar::now(); + }); }); case CEPH_OSD_OP_GETXATTR: - return backend.getxattr(os, osd_op); + return do_read_op([&osd_op] (auto& backend, const auto& os) { + return backend.getxattr(os, osd_op); + }); case CEPH_OSD_OP_WRITE: - return backend.write(*os, osd_op, txn); + return do_write_op([&osd_op] (auto& backend, auto& os, auto& txn) { + return backend.write(os, osd_op, txn); + }); case CEPH_OSD_OP_WRITEFULL: - // XXX: os = backend.write(std::move(os), ...) instead? - return backend.writefull(*os, osd_op, txn); + return do_write_op([&osd_op] (auto& backend, auto& os, auto& txn) { + return backend.writefull(os, osd_op, txn); + }); case CEPH_OSD_OP_SETALLOCHINT: return seastar::now(); case CEPH_OSD_OP_SETXATTR: - return backend.setxattr(*os, osd_op, txn); + return do_write_op([&osd_op] (auto& backend, auto& os, auto& txn) { + return backend.setxattr(os, osd_op, txn); + }); case CEPH_OSD_OP_PGNLS: return do_pgnls(osd_op.indata, os->oi.soid.get_namespace(), op.pgls.count) .then([&osd_op](bufferlist bl) { @@ -192,11 +197,16 @@ OpsExecuter::do_osd_op(OSDOp& osd_op) return seastar::now(); }); case CEPH_OSD_OP_DELETE: - return backend.remove(*os, txn); + return do_write_op([&osd_op] (auto& backend, auto& os, auto& txn) { + return backend.remove(os, txn); + }); case CEPH_OSD_OP_CALL: return this->do_op_call(osd_op); case CEPH_OSD_OP_STAT: - return backend.stat(*os, osd_op); + // note: stat does not require RD + return do_const_op([&osd_op] (/* const */auto& backend, const auto& os) { + return backend.stat(os, osd_op); + }); default: logger().warn("unknown op {}", ceph_osd_op_name(op.op)); throw std::runtime_error( diff --git a/src/crimson/osd/ops_executer.h b/src/crimson/osd/ops_executer.h index 4804e20c8b2..b5b1cd8b001 100644 --- a/src/crimson/osd/ops_executer.h +++ b/src/crimson/osd/ops_executer.h @@ -33,12 +33,34 @@ class OpsExecuter { PGBackend& backend; ceph::os::Transaction txn; + size_t num_read = 0; ///< count read ops + size_t num_write = 0; ///< count update ops + seastar::future do_pgnls( ceph::bufferlist& indata, const std::string& nspace, uint64_t limit); seastar::future<> do_op_call(class OSDOp& osd_op); + template + auto do_const_op(Func&& f) { + // TODO: pass backend as read-only + return std::forward(f)(backend, const_cast(*os)); + } + + template + auto do_read_op(Func&& f) { + ++num_read; + // TODO: pass backend as read-only + return std::forward(f)(backend, const_cast(*os)); + } + + template + auto do_write_op(Func&& f) { + ++num_write; + return std::forward(f)(backend, *os, txn); + } + public: OpsExecuter(PGBackend::cached_os_t os, PG& pg) : os(std::move(os)), pg(pg), backend(pg.get_backend()) { diff --git a/src/crimson/osd/pg_backend.cc b/src/crimson/osd/pg_backend.cc index eb578199dca..e78c4ce4b93 100644 --- a/src/crimson/osd/pg_backend.cc +++ b/src/crimson/osd/pg_backend.cc @@ -247,7 +247,7 @@ seastar::future PGBackend::read(const object_info_t& oi, } seastar::future<> PGBackend::stat( - ObjectState& os, + const ObjectState& os, OSDOp& osd_op) { if (os.exists/* TODO: && !os.is_whiteout() */) { @@ -398,8 +398,6 @@ seastar::future<> PGBackend::setxattr( const OSDOp& osd_op, ceph::os::Transaction& txn) { - //++ctx->num_write; - if (local_conf()->osd_max_attr_size > 0 && osd_op.op.xattr.value_len > local_conf()->osd_max_attr_size) { throw ceph::osd::make_error(-EFBIG); @@ -429,7 +427,7 @@ seastar::future<> PGBackend::setxattr( } seastar::future<> PGBackend::getxattr( - ObjectState& os, + const ObjectState& os, OSDOp& osd_op) { std::string name; diff --git a/src/crimson/osd/pg_backend.h b/src/crimson/osd/pg_backend.h index 6a20e28dbfc..e88057757e2 100644 --- a/src/crimson/osd/pg_backend.h +++ b/src/crimson/osd/pg_backend.h @@ -48,7 +48,7 @@ public: uint32_t truncate_seq, uint32_t flags); seastar::future<> stat( - ObjectState& os, + const ObjectState& os, OSDOp& osd_op); seastar::future<> remove( ObjectState& os, @@ -77,7 +77,7 @@ public: const OSDOp& osd_op, ceph::os::Transaction& trans); seastar::future<> getxattr( - ObjectState& os, + const ObjectState& os, OSDOp& osd_op); virtual void got_rep_op_reply(const MOSDRepOpReply&) {} -- 2.39.5