[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