[PATCH] Avoid extra talloc in ctdb marshal buffer
Amitay Isaacs
amitay at gmail.com
Wed Jul 16 02:06:02 MDT 2014
On Thu, Jul 10, 2014 at 6:53 PM, Amitay Isaacs <amitay at gmail.com> wrote:
>
> 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.
>
Volker,
Do you have any comments on the extra patch? May be Michael has some
suggestion?
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/20140716/40b39da2/attachment.obj>
More information about the samba-technical
mailing list