From 4c0178f18066fee5d3a024860eec4ff1d1f85520 Mon Sep 17 00:00:00 2001 From: Vitaliy Filippov Date: Mon, 24 Feb 2020 01:01:34 +0300 Subject: [PATCH] Fix some memory freeing --- osd.cpp | 25 +++++++------- osd.h | 2 +- osd_peering.cpp | 13 +++++--- osd_peering_pg.h | 2 +- osd_primary.cpp | 6 ++-- osd_receive.cpp | 4 +-- osd_secondary.cpp | 83 +++++++++++++++++++++++------------------------ 7 files changed, 69 insertions(+), 66 deletions(-) diff --git a/osd.cpp b/osd.cpp index 30a80b06e..d925aadff 100644 --- a/osd.cpp +++ b/osd.cpp @@ -98,18 +98,19 @@ osd_op_t::~osd_op_t() { delete bs_op; } + if (op_data) + { + free(op_data); + } + if (rmw_buf) + { + free(rmw_buf); + } if (buf) { // Note: reusing osd_op_t WILL currently lead to memory leaks // So we don't reuse it, but free it every time - if (op_type == OSD_OP_IN && - req.hdr.opcode == OSD_OP_SHOW_CONFIG) - { - std::string *str = (std::string*)buf; - delete str; - } - else - free(buf); + free(buf); } } @@ -307,7 +308,6 @@ void osd_t::exec_op(osd_op_t *cur_op) delete cur_op; return; } - inflight_ops++; if (cur_op->req.hdr.magic != SECONDARY_OSD_OP_MAGIC || cur_op->req.hdr.opcode < OSD_OP_MIN || cur_op->req.hdr.opcode > OSD_OP_MAX || (cur_op->req.hdr.opcode == OSD_OP_SECONDARY_READ || cur_op->req.hdr.opcode == OSD_OP_SECONDARY_WRITE || @@ -315,10 +315,13 @@ void osd_t::exec_op(osd_op_t *cur_op) (cur_op->req.sec_rw.len > OSD_RW_MAX || cur_op->req.sec_rw.len % OSD_RW_ALIGN || cur_op->req.sec_rw.offset % OSD_RW_ALIGN)) { // Bad command - cur_op->bs_op->retval = -EINVAL; - secondary_op_callback(cur_op); + cur_op->reply.hdr.magic = SECONDARY_OSD_REPLY_MAGIC; + cur_op->reply.hdr.id = cur_op->req.hdr.id; + cur_op->reply.hdr.opcode = cur_op->req.hdr.opcode; + cur_op->reply.hdr.retval = -EINVAL; return; } + inflight_ops++; if (cur_op->req.hdr.opcode == OSD_OP_TEST_SYNC_STAB_ALL) { exec_sync_stab_all(cur_op); diff --git a/osd.h b/osd.h index 005297c2f..a8e91532e 100644 --- a/osd.h +++ b/osd.h @@ -102,6 +102,7 @@ struct osd_op_t osd_any_reply_t reply; blockstore_op_t *bs_op = NULL; void *buf = NULL; + void *rmw_buf = NULL; osd_primary_op_data_t* op_data = NULL; std::function callback; @@ -209,7 +210,6 @@ class osd_t void handle_read_op(osd_client_t *cl); void handle_read_reply(osd_client_t *cl); void send_replies(); - void make_reply(osd_op_t *op); void handle_send(ring_data_t *data, int peer_fd); void outbox_push(osd_client_t & cl, osd_op_t *op); diff --git a/osd_peering.cpp b/osd_peering.cpp index 5ad12a503..9958d3876 100644 --- a/osd_peering.cpp +++ b/osd_peering.cpp @@ -268,17 +268,22 @@ void osd_t::start_pg_peering(int pg_idx) it = pg.peering_state->list_ops.begin(); } } - for (auto & p: pg.peering_state->list_results) + for (auto it = pg.peering_state->list_results.begin(); it != pg.peering_state->list_results.end(); it++) { int role; for (role = 0; role < pg.cur_set.size(); role++) { - if (pg.cur_set[role] == p.first) + if (pg.cur_set[role] == it->first) break; } if (pg.state == PG_INCOMPLETE || role >= pg.cur_set.size()) { - pg.peering_state->list_results.erase(p.first); + if (it->second.buf) + { + free(it->second.buf); + } + pg.peering_state->list_results.erase(it); + it = pg.peering_state->list_results.begin(); } } } @@ -380,7 +385,7 @@ void osd_t::start_pg_peering(int pg_idx) .total_count = (uint64_t)op->reply.hdr.retval, .stable_count = op->reply.sec_list.stable_count, }; - // so it doesn't get freed. FIXME: do it better + // set op->buf to NULL so it doesn't get freed op->buf = NULL; ps->list_done++; ps->list_ops.erase(role_osd); diff --git a/osd_peering_pg.h b/osd_peering_pg.h index b515e6407..d0cb20c99 100644 --- a/osd_peering_pg.h +++ b/osd_peering_pg.h @@ -54,7 +54,7 @@ struct pg_osd_set_state_t struct pg_list_result_t { - obj_ver_id *buf; + obj_ver_id *buf = NULL; uint64_t total_count; uint64_t stable_count; }; diff --git a/osd_primary.cpp b/osd_primary.cpp index e0de2bd5e..799780888 100644 --- a/osd_primary.cpp +++ b/osd_primary.cpp @@ -19,8 +19,6 @@ struct osd_primary_op_data_t int degraded = 0, pg_size, pg_minsize; osd_rmw_stripe_t *stripes; osd_op_t *subops = NULL; - void *rmw_buf = NULL; - bool should_read_version = false; }; @@ -240,7 +238,7 @@ void osd_t::submit_read_subops(int read_pg_size, const uint64_t* osd_set, osd_op subops[subop].buf = stripes[role].read_buf; subops[subop].callback = [cur_op, this](osd_op_t *subop) { - // so it doesn't get freed. FIXME: do it better + // so it doesn't get freed subop->buf = NULL; handle_primary_read_subop(cur_op, subop->reply.hdr.retval == subop->req.sec_rw.len); }; @@ -276,7 +274,7 @@ void osd_t::exec_primary_write(osd_op_t *cur_op) // Check if there are other write requests to the same object // Determine blocks to read - op_data->rmw_buf = calc_rmw_reads(cur_op->buf, op_data->stripes, pg.cur_set.data(), pg.pg_size, pg.pg_minsize, pg.pg_cursize); + cur_op->rmw_buf = calc_rmw_reads(cur_op->buf, op_data->stripes, pg.cur_set.data(), pg.pg_size, pg.pg_minsize, pg.pg_cursize); op_data->should_read_version = true; // Read required blocks submit_read_subops(pg.pg_size, pg.cur_set.data(), cur_op); diff --git a/osd_receive.cpp b/osd_receive.cpp index a54e31300..1298291be 100644 --- a/osd_receive.cpp +++ b/osd_receive.cpp @@ -149,8 +149,8 @@ void osd_t::handle_read_reply(osd_client_t *cl) if (op->reply.hdr.opcode == OSD_OP_SECONDARY_READ && op->reply.hdr.retval > 0) { - // Read data - // FIXME: op->buf must be allocated + // Read data. In this case we assume that the buffer is preallocated by the caller (!) + assert(op->buf); cl->read_state = CL_READ_REPLY_DATA; cl->read_reply_id = op->req.hdr.id; cl->read_buf = op->buf; diff --git a/osd_secondary.cpp b/osd_secondary.cpp index d95558111..98d88078d 100644 --- a/osd_secondary.cpp +++ b/osd_secondary.cpp @@ -2,19 +2,46 @@ #include "json11/json11.hpp" -void osd_t::secondary_op_callback(osd_op_t *cur_op) +void osd_t::secondary_op_callback(osd_op_t *op) { inflight_ops--; - auto cl_it = clients.find(cur_op->peer_fd); + auto cl_it = clients.find(op->peer_fd); if (cl_it != clients.end()) { + op->reply.hdr.magic = SECONDARY_OSD_REPLY_MAGIC; + op->reply.hdr.id = op->req.hdr.id; + op->reply.hdr.opcode = op->req.hdr.opcode; + op->reply.hdr.retval = op->bs_op->retval; + if (op->req.hdr.opcode == OSD_OP_SECONDARY_LIST) + { + op->reply.sec_list.stable_count = op->bs_op->version; + } + else if (op->req.hdr.opcode == OSD_OP_SECONDARY_READ || + op->req.hdr.opcode == OSD_OP_SECONDARY_WRITE) + { + op->reply.sec_rw.version = op->bs_op->version; + } + else if (op->req.hdr.opcode == OSD_OP_SECONDARY_DELETE) + { + op->reply.sec_del.version = op->bs_op->version; + } + if (op->req.hdr.opcode == OSD_OP_SECONDARY_READ && + op->reply.hdr.retval > 0) + { + op->send_list.push_back(op->buf, op->reply.hdr.retval); + } + else if (op->req.hdr.opcode == OSD_OP_SECONDARY_LIST && + op->reply.hdr.retval > 0) + { + op->buf = op->bs_op->buf; // allocated by blockstore + op->send_list.push_back(op->buf, op->reply.hdr.retval * sizeof(obj_ver_id)); + } auto & cl = cl_it->second; - make_reply(cur_op); - outbox_push(cl, cur_op); + outbox_push(cl, op); } else { - delete cur_op; + delete op; } } @@ -74,10 +101,15 @@ void osd_t::exec_secondary(osd_op_t *cur_op) void osd_t::exec_show_config(osd_op_t *cur_op) { // FIXME: Send the real config, not its source - std::string *cfg_str = new std::string(std::move(json11::Json(config).dump())); - cur_op->buf = cfg_str; + std::string cfg_str = json11::Json(config).dump(); + cur_op->reply.hdr.magic = SECONDARY_OSD_REPLY_MAGIC; + cur_op->reply.hdr.id = cur_op->req.hdr.id; + cur_op->reply.hdr.opcode = cur_op->req.hdr.opcode; + cur_op->reply.hdr.retval = cfg_str.size()+1; + cur_op->buf = malloc(cfg_str.size()+1); + memcpy(cur_op->buf, cfg_str.c_str(), cfg_str.size()+1); auto & cl = clients[cur_op->peer_fd]; - make_reply(cur_op); + cur_op->send_list.push_back(cur_op->buf, cur_op->reply.hdr.retval); outbox_push(cl, cur_op); } @@ -104,38 +136,3 @@ void osd_t::exec_sync_stab_all(osd_op_t *cur_op) bs->enqueue_op(cur_op->bs_op); #endif } - -void osd_t::make_reply(osd_op_t *op) -{ - op->reply.hdr.magic = SECONDARY_OSD_REPLY_MAGIC; - op->reply.hdr.id = op->req.hdr.id; - op->reply.hdr.opcode = op->req.hdr.opcode; - if (op->req.hdr.opcode == OSD_OP_SHOW_CONFIG) - { - std::string *str = (std::string*)op->buf; - op->reply.hdr.retval = str->size()+1; - op->send_list.push_back((void*)str->c_str(), op->reply.hdr.retval); - } - else - { - op->reply.hdr.retval = op->bs_op->retval; - if (op->req.hdr.opcode == OSD_OP_SECONDARY_LIST) - op->reply.sec_list.stable_count = op->bs_op->version; - else if (op->req.hdr.opcode == OSD_OP_SECONDARY_READ || - op->req.hdr.opcode == OSD_OP_SECONDARY_WRITE) - op->reply.sec_rw.version = op->bs_op->version; - else if (op->req.hdr.opcode == OSD_OP_SECONDARY_DELETE) - op->reply.sec_del.version = op->bs_op->version; - } - if (op->req.hdr.opcode == OSD_OP_SECONDARY_READ && - op->reply.hdr.retval > 0) - { - op->send_list.push_back(op->buf, op->reply.hdr.retval); - } - else if (op->req.hdr.opcode == OSD_OP_SECONDARY_LIST && - op->reply.hdr.retval > 0) - { - op->buf = op->bs_op->buf; // allocated by blockstore - op->send_list.push_back(op->buf, op->reply.hdr.retval * sizeof(obj_ver_id)); - } -}