[PATCH] Avoid extra talloc in ctdb marshal buffer
Volker Lendecke
Volker.Lendecke at SerNet.DE
Wed Jul 9 03:03:04 MDT 2014
On Wed, Jul 09, 2014 at 01:24:04PM +1000, Amitay Isaacs wrote:
> Hi,
>
> Refactor ctdb_marshall_add() to get rid of extra talloc and re-use these
> functions instead of duplicating marshaling code.
>
> Please review and push if ok.
Looks good! A few comments inside:
> From c46c3646c8f7329abbef37b39723473689b16465 Mon Sep 17 00:00:00 2001
> From: Amitay Isaacs <amitay at gmail.com>
> Date: Tue, 6 May 2014 18:26:41 +1000
> Subject: [PATCH 1/3] ctdb-util: Refactor record marshalling routines to avoid
> extra talloc
>
> Signed-off-by: Amitay Isaacs <amitay at gmail.com>
> ---
> ctdb/common/ctdb_util.c | 92 +++++++++++++++++++++++++++++--------------------
> 1 file changed, 55 insertions(+), 37 deletions(-)
>
> diff --git a/ctdb/common/ctdb_util.c b/ctdb/common/ctdb_util.c
> index f71f74a..58ee2f5 100644
> --- a/ctdb/common/ctdb_util.c
> +++ b/ctdb/common/ctdb_util.c
> @@ -179,44 +179,67 @@ void ctdb_reqid_remove(struct ctdb_context *ctdb, uint32_t reqid)
> }
>
>
> +static uint32_t ctdb_marshall_record_size(TDB_DATA key,
> + struct ctdb_ltdb_header *header,
> + TDB_DATA data)
> +{
> + return offsetof(struct ctdb_rec_data, data) + key.dsize +
> + data.dsize + (header ? sizeof(*header) : 0);
> +}
> +
> +static void ctdb_marshall_record_copy(struct ctdb_rec_data *rec,
> + uint32_t reqid,
> + TDB_DATA key,
> + struct ctdb_ltdb_header *header,
> + TDB_DATA data,
> + uint32_t length)
> +{
> + uint32_t offset;
> +
> + rec->length = length;
> + rec->reqid = reqid;
> + rec->keylen = key.dsize;
> + memcpy(&rec->data[0], key.dptr, key.dsize);
> + offset = key.dsize;
> +
> + if (header) {
> + rec->datalen = data.dsize + sizeof(*header);
> + memcpy(&rec->data[offset], header, sizeof(*header));
> + offset += sizeof(*header);
> + } else {
> + rec->datalen = data.dsize;
> + }
> + memcpy(&rec->data[offset], data.dptr, data.dsize);
> +}
> +
> /*
> form a ctdb_rec_data record from a key/data pair
>
> note that header may be NULL. If not NULL then it is included in the data portion
> of the record
> */
> -struct ctdb_rec_data *ctdb_marshall_record(TALLOC_CTX *mem_ctx, uint32_t reqid,
> - TDB_DATA key,
> +struct ctdb_rec_data *ctdb_marshall_record(TALLOC_CTX *mem_ctx, uint32_t reqid,
> + TDB_DATA key,
> struct ctdb_ltdb_header *header,
> TDB_DATA data)
> {
> size_t length;
> struct ctdb_rec_data *d;
>
> - length = offsetof(struct ctdb_rec_data, data) + key.dsize +
> - data.dsize + (header?sizeof(*header):0);
> + length = ctdb_marshall_record_size(key, header, data);
> +
> d = (struct ctdb_rec_data *)talloc_size(mem_ctx, length);
> if (d == NULL) {
> return NULL;
> }
> - d->length = length;
> - d->reqid = reqid;
> - d->keylen = key.dsize;
> - memcpy(&d->data[0], key.dptr, key.dsize);
> - if (header) {
> - d->datalen = data.dsize + sizeof(*header);
> - memcpy(&d->data[key.dsize], header, sizeof(*header));
> - memcpy(&d->data[key.dsize+sizeof(*header)], data.dptr, data.dsize);
> - } else {
> - d->datalen = data.dsize;
> - memcpy(&d->data[key.dsize], data.dptr, data.dsize);
> - }
> +
> + ctdb_marshall_record_copy(d, reqid, key, header, data, length);
> return d;
> }
>
>
> /* helper function for marshalling multiple records */
> -struct ctdb_marshall_buffer *ctdb_marshall_add(TALLOC_CTX *mem_ctx,
> +struct ctdb_marshall_buffer *ctdb_marshall_add(TALLOC_CTX *mem_ctx,
> struct ctdb_marshall_buffer *m,
> uint64_t db_id,
> uint32_t reqid,
> @@ -225,36 +248,31 @@ struct ctdb_marshall_buffer *ctdb_marshall_add(TALLOC_CTX *mem_ctx,
> TDB_DATA data)
> {
> struct ctdb_rec_data *r;
> - size_t m_size, r_size;
> struct ctdb_marshall_buffer *m2;
> + uint32_t length, offset;
>
> - r = ctdb_marshall_record(mem_ctx, reqid, key, header, data);
> - if (r == NULL) {
> - talloc_free(m);
> - return NULL;
> - }
> + length = ctdb_marshall_record_size(key, header, data);
>
> if (m == NULL) {
> - m = talloc_zero_size(mem_ctx, offsetof(struct ctdb_marshall_buffer, data));
> - if (m == NULL) {
> - return NULL;
> - }
> - m->db_id = db_id;
> + offset = offsetof(struct ctdb_marshall_buffer, data);
> + m2 = talloc_zero_size(mem_ctx, offset + length);
> + } else {
> + offset = talloc_get_size(m);
> + m2 = talloc_realloc_size(mem_ctx, m, offset + length);
> }
> -
> - m_size = talloc_get_size(m);
> - r_size = talloc_get_size(r);
> -
> - m2 = talloc_realloc_size(mem_ctx, m, m_size + r_size);
> if (m2 == NULL) {
> - talloc_free(m);
> + if (m != NULL) {
> + talloc_free(m);
> + }
Here we nowadays have the TALLOC_FREE macro doing exactly
this.
> return NULL;
> }
>
> - memcpy(m_size + (uint8_t *)m2, r, r_size);
> -
> - talloc_free(r);
> + if (m == NULL) {
> + m2->db_id = db_id;
> + }
>
> + r = (struct ctdb_rec_data *)((uint8_t *)m2 + offset);
> + ctdb_marshall_record_copy(r, reqid, key, header, data, length);
> m2->count++;
>
> return m2;
> --
> 1.9.3
>
> From 03e9df78ca987ee2ddc473a001b4414f25cee55f Mon Sep 17 00:00:00 2001
> From: Amitay Isaacs <amitay at gmail.com>
> Date: Tue, 6 May 2014 18:39:25 +1000
> Subject: [PATCH 2/3] ctdb-vacuum: Use ctdb_marshall_add to add a record to
> marshall buffer
>
> This avoids duplicate code and extra talloc in ctdb_marshall_record.
>
> Signed-off-by: Amitay Isaacs <amitay at gmail.com>
> ---
> ctdb/server/ctdb_vacuum.c | 36 +++++++++---------------------------
> 1 file changed, 9 insertions(+), 27 deletions(-)
>
> diff --git a/ctdb/server/ctdb_vacuum.c b/ctdb/server/ctdb_vacuum.c
> index ce3c600..d5a77ae 100644
> --- a/ctdb/server/ctdb_vacuum.c
> +++ b/ctdb/server/ctdb_vacuum.c
> @@ -181,35 +181,23 @@ static int add_record_to_vacuum_fetch_list(struct vacuum_data *vdata,
> TDB_DATA key)
> {
> struct ctdb_context *ctdb = vdata->ctdb;
> - struct ctdb_rec_data *rec;
> uint32_t lmaster;
> - size_t old_size;
> struct ctdb_marshall_buffer *vfl;
>
> lmaster = ctdb_lmaster(ctdb, &key);
>
> vfl = vdata->vacuum_fetch_list[lmaster];
>
> - rec = ctdb_marshall_record(vfl, ctdb->pnn, key, NULL, tdb_null);
> - if (rec == NULL) {
> + vfl = ctdb_marshall_add(ctdb, vfl, vfl->db_id, ctdb->pnn,
> + key, NULL, tdb_null);
> + if (vfl == NULL) {
> DEBUG(DEBUG_ERR,(__location__ " Out of memory\n"));
> vdata->traverse_error = true;
> return -1;
> }
>
> - old_size = talloc_get_size(vfl);
> - vfl = talloc_realloc_size(NULL, vfl, old_size + rec->length);
> - if (vfl == NULL) {
> - DEBUG(DEBUG_ERR,(__location__ " Failed to expand\n"));
> - vdata->traverse_error = true;
> - return -1;
> - }
> vdata->vacuum_fetch_list[lmaster] = vfl;
>
> - vfl->count++;
> - memcpy(old_size+(uint8_t *)vfl, rec, rec->length);
> - talloc_free(rec);
> -
> return 0;
> }
>
> @@ -294,23 +282,17 @@ static int delete_marshall_traverse(void *param, void *data)
> {
> struct delete_record_data *dd = talloc_get_type(data, struct delete_record_data);
> struct delete_records_list *recs = talloc_get_type(param, struct delete_records_list);
> - struct ctdb_rec_data *rec;
> - size_t old_size;
> + struct ctdb_marshall_buffer *m;
>
> - rec = ctdb_marshall_record(dd, recs->records->db_id, dd->key, &dd->hdr, tdb_null);
> - if (rec == NULL) {
> + m = ctdb_marshall_add(NULL, recs->records, recs->records->db_id,
> + recs->records->db_id,
> + dd->key, &dd->hdr, tdb_null);
> + if (m == NULL) {
> DEBUG(DEBUG_ERR, (__location__ " failed to marshall record\n"));
> return 0;
While here, doesn't this leave rec->records as a dangling
pointer? Shouldn't we set rec->records=NULL here as well?
> }
>
> - old_size = talloc_get_size(recs->records);
> - recs->records = talloc_realloc_size(NULL, recs->records, old_size + rec->length);
> - if (recs->records == NULL) {
> - DEBUG(DEBUG_ERR,(__location__ " Failed to expand\n"));
> - return 0;
> - }
> - recs->records->count++;
> - memcpy(old_size+(uint8_t *)(recs->records), rec, rec->length);
> + recs->records = m;
> return 0;
> }
>
> --
> 1.9.3
>
> From 5c2dbcbfbef0f5680b16d0fae19de98d94a6a0f4 Mon Sep 17 00:00:00 2001
> From: Amitay Isaacs <amitay at gmail.com>
> Date: Tue, 6 May 2014 18:52:54 +1000
> Subject: [PATCH 3/3] ctdb-vacuum: Use existing function ctdb_marshall_finish
>
> Signed-off-by: Amitay Isaacs <amitay at gmail.com>
Reviewed-by: Volker Lendecke <vl at samba.org>
Volker
--
SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
phone: +49-551-370000-0, fax: +49-551-370000-9
AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
http://www.sernet.de, mailto:kontakt at sernet.de
More information about the samba-technical
mailing list