[PATCH] Avoid extra talloc in ctdb marshal buffer
Amitay Isaacs
amitay at gmail.com
Thu Jul 10 02:53:22 MDT 2014
On Wed, Jul 9, 2014 at 7:03 PM, Volker Lendecke <Volker.Lendecke at sernet.de>
wrote:
> 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.
>
>
Done.
> 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?
>
This patch converted the code without changing behavior. However, looking
at the code, it does seem wrong. I have added an extra patch in the
beginning that fails traverse if talloc_realloc fails. In that case, we
leave the function and clean up all temporary memory.
Amitay.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ctdb.patches
Type: application/octet-stream
Size: 11000 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20140710/34a1655f/attachment.obj>
More information about the samba-technical
mailing list