Misc cleanups and optimalisations

PATCH New Zdenek Kabelac 1 participant 9 messages Mar 22, 2011

Share & tools

Zdenek Kabelac

author

2011-03-22 21:34 UTC #0 lore raw

cover Misc cleanups and optimalisations

Shorter patchset to address few different issue - combined into one patchset - each patch is however separate and unrelated to others.

Zdenek Kabelac (8):
Optimise write of error to _lvm_errmsg
Fix sending random content in the cluster message Proposal - char alignment on word
Use created hash tables for quick check of LV, PV. Simplify boundary checks
Use id_equal instead of strncmp()
Proposal - avoid parsing same data
Few more files filtered from memory locking

 lib/cache/lvmcache.c          |   17 +++++--
 lib/cache/lvmcache.h          |    1 +
 lib/locking/cluster_locking.c |    6 ++-
 lib/log/log.c                 |   31 ++++++++++---
 lib/metadata/metadata.c       |   98 +++++++++++++++++++----------------------
 lib/mm/memlock.c              |    2 +
 libdm/mm/pool-fast.c          |    6 +-
 libdm/mm/pool.c               |    4 +-
 8 files changed, 94 insertions(+), 71 deletions(-)

Signature · 1 line

1.7.4.1

Zdenek Kabelac

author

2011-03-22 21:34 UTC #1 lore raw

PATCH 1/8 Optimise write of error to _lvm_errmsg

Isn't usually perfomance critical - but when used for debuging, this code noticable slows down the processing.

I may also ask what is the purpose of having some large unbound buffer? (clvmd getting some error could leak out of memory I guess)

So added just 512KB limit.

TODO: create _lvm_errmsg buffer only when lvm2api needs it.

Signed-off-by Zdenek Kabelac <[email protected]>

---
 lib/log/log.c |   31 +++++++++++++++++++++++--------
 1 files changed, 23 insertions(+), 8 deletions(-)

lib/log/log.c +23 -8

index ec72b7a..b84c604 100644
@@ -43,6 +43,9 @@ static lvm2_log_fn_t _lvm2_log_fn = NULL;
 static int _lvm_errno = 0;
 static int _store_errmsg = 0;
 static char *_lvm_errmsg = NULL;
+static size_t _lvm_errmsg_size = 0;
+static size_t _lvm_errmsg_len = 0;
+#define MAX_ERRMSG_LEN (512 * 1024)  /* max size of error buffer 512KB */
 
 void init_log_fn(lvm2_log_fn_t log_fn)
 {

@@ -154,6 +157,7 @@ void reset_lvm_errno(int store_errmsg)
 	if (_lvm_errmsg) {
 		dm_free(_lvm_errmsg);
 		_lvm_errmsg = NULL;
+		_lvm_errmsg_size = _lvm_errmsg_len = 0;
 	}
 
 	_store_errmsg = store_errmsg;

@@ -189,6 +193,7 @@ void print_log(int level, const char *file, int line, int dm_errno,
 	int use_stderr = level & _LOG_STDERR;
 	int log_once = level & _LOG_ONCE;
 	int fatal_internal_error = 0;
+	size_t msglen;
 
 	level &= ~(_LOG_STDERR|_LOG_ONCE);

@@ -229,14 +234,24 @@ void print_log(int level, const char *file, int line, int dm_errno,
 		message = &buf2[0];
 	}
 
-	if (_store_errmsg && (level <= _LOG_ERR)) {
-		if (!_lvm_errmsg)
-			_lvm_errmsg = dm_strdup(message);
-		else if ((newbuf = dm_realloc(_lvm_errmsg,
- 					      strlen(_lvm_errmsg) +
-					      strlen(message) + 2))) {
-			_lvm_errmsg = strcat(newbuf, "\n");
-			_lvm_errmsg = strcat(newbuf, message);
+	if (_store_errmsg && (level <= _LOG_ERR) &&
+	    _lvm_errmsg_len < MAX_ERRMSG_LEN) {
+		msglen = strlen(message);
+		if ((_lvm_errmsg_len + msglen + 1) >= _lvm_errmsg_size) {
+			_lvm_errmsg_size = 2 * (_lvm_errmsg_len + msglen + 1);
+			if ((newbuf = dm_realloc(_lvm_errmsg,
+						 _lvm_errmsg_size)))
+				_lvm_errmsg = newbuf;
+			else
+				_lvm_errmsg_size = _lvm_errmsg_len;
+		}
+		if (_lvm_errmsg &&
+		    (_lvm_errmsg_len + msglen + 2) < _lvm_errmsg_size) {
+			/* prepend '\n' and copy with '\0' but do not count in */
+                        if (_lvm_errmsg_len)
+				_lvm_errmsg[_lvm_errmsg_len++] = '\n';
+			memcpy(_lvm_errmsg + _lvm_errmsg_len, message, msglen + 1);
+			_lvm_errmsg_len += msglen;
 		}
 	}

Signature · 1 line

1.7.4.1

Zdenek Kabelac

author

2011-03-22 21:34 UTC #2 lore raw

PATCH 2/8 Fix sending random content in the cluster message

In reply to Zdenek Kabelac

Another one to make the passed messages around cluster more consistent. Initilize missing header part.
And compensate 1 extra byte attached to the message without right content.

Signed-off-by Zdenek Kabelac <[email protected]>

---
 lib/locking/cluster_locking.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

lib/locking/cluster_locking.c +4 -2

index 4dac05f..074f145 100644
@@ -173,6 +173,7 @@ static void _build_header(struct clvm_header *head, int clvmd_cmd, const char *n
 	head->cmd = clvmd_cmd;
 	head->status = 0;
 	head->flags = 0;
+	head->xid = 0;
 	head->clientid = 0;
 	head->arglen = len;

@@ -216,11 +217,12 @@ static int _cluster_request(char clvmd_cmd, const char *node, void *data, int le
 	if (_clvmd_sock == -1)
 		return 0;
 
-	_build_header(head, clvmd_cmd, node, len);
+	/* 1 byte is used from struct  clmd_header.argc[1], so -> len - 1 */
+	_build_header(head, clvmd_cmd, node, len - 1);
 	memcpy(head->node + strlen(head->node) + 1, data, len);
 
 	status = _send_request(outbuf, sizeof(struct clvm_header) +
-			      strlen(head->node) + len, &retbuf);
+			      strlen(head->node) + len - 1, &retbuf);
 	if (!status)
 		goto out;

Signature · 1 line

1.7.4.1

Zdenek Kabelac

author

2011-03-22 21:34 UTC #3 lore raw

PATCH 3/8 Proposal - char alignment on word

In reply to Zdenek Kabelac

Align strdup char* allocation just on 2 bytes.
It looks like wasting space to aling strings on 8 bytes. (Could be even 1byte - but for hashing it might eventually get better perfomance - but probably hardly measurable).

Signed-off-by Zdenek Kabelac <[email protected]>

---
 libdm/mm/pool.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

libdm/mm/pool.c +2 -2

index 825f7ca..608826b 100644
@@ -27,7 +27,7 @@ void dm_pools_check_leaks(void);
 
 char *dm_pool_strdup(struct dm_pool *p, const char *str)
 {
-	char *ret = dm_pool_alloc(p, strlen(str) + 1);
+	char *ret = dm_pool_alloc_aligned(p, strlen(str) + 1, 2);
 
 	if (ret)
 		strcpy(ret, str);

@@ -37,7 +37,7 @@ char *dm_pool_strdup(struct dm_pool *p, const char *str)
 
 char *dm_pool_strndup(struct dm_pool *p, const char *str, size_t n)
 {
-	char *ret = dm_pool_alloc(p, n + 1);
+	char *ret = dm_pool_alloc_aligned(p, n + 1, 2);
 
 	if (ret) {
 		strncpy(ret, str, n);

Signature · 1 line

1.7.4.1

Zdenek Kabelac

author

2011-03-22 21:34 UTC #4 lore raw

PATCH 4/8 Use created hash tables for quick check of LV, PV.

In reply to Zdenek Kabelac

Instead of searching linear list of all LVs, PVs - use create hash table for quick mapping between LV.

(Note - for small number of PVs or LVs the overhead of the hash is bigger)

Signed-off-by Zdenek Kabelac <[email protected]>

---
 lib/metadata/metadata.c |   91 ++++++++++++++++++++++-------------------------
 1 files changed, 43 insertions(+), 48 deletions(-)

lib/metadata/metadata.c +43 ~~-48~~

index cdd6089..1d711bc 100644
@@ -2182,6 +2182,12 @@ void lv_calculate_readahead(const struct logical_volume *lv, uint32_t *read_ahea
 	}
 }
 
+struct validate_hash {
+	struct dm_hash_table *lvname;
+	struct dm_hash_table *lvid;
+	struct dm_hash_table *pvid;
+};
+
 /*
  * Check that an LV and all its PV references are correctly listed in vg->lvs
  * and vg->pvs, respectively. This only looks at a single LV, but *not* at the

@@ -2191,21 +2197,14 @@ void lv_calculate_readahead(const struct logical_volume *lv, uint32_t *read_ahea
 static int _lv_validate_references_single(struct logical_volume *lv, void *data)
 {
 	struct volume_group *vg = lv->vg;
+	struct validate_hash *vhash = data;
 	struct lv_segment *lvseg;
-	struct pv_list *pvl;
-	struct lv_list *lvl;
+	struct physical_volume *pv;
 	int s;
 	int r = 1;
-	int ok = 0;
-
-	dm_list_iterate_items(lvl, &vg->lvs) {
-		if (lvl->lv == lv) {
-			ok = 1;
-			break;
-		}
-	}
 
-	if (!ok) {
+	if (lv != dm_hash_lookup_binary(vhash->lvid, &lv->lvid.id[1],
+					sizeof(lv->lvid.id[1]))) {
 		log_error(INTERNAL_ERROR
 			  "Referenced LV %s not listed in VG %s.",
 			  lv->name, vg->name);

@@ -2214,22 +2213,16 @@ static int _lv_validate_references_single(struct logical_volume *lv, void *data)
 
 	dm_list_iterate_items(lvseg, &lv->segments) {
 		for (s = 0; s < lvseg->area_count; ++s) {
-			if (seg_type(lvseg, s) == AREA_PV) {
-				ok = 0;
-				/* look up the reference in vg->pvs */
-				dm_list_iterate_items(pvl, &vg->pvs) {
-					if (pvl->pv == seg_pv(lvseg, s)) {
-						ok = 1;
-						break;
-					}
-				}
-
-				if (!ok) {
-					log_error(INTERNAL_ERROR
-						  "Referenced PV %s not listed in VG %s.",
-						  pv_dev_name(seg_pv(lvseg, s)), vg->name);
-					r = 0;
-				}
+			if (seg_type(lvseg, s) != AREA_PV)
+				continue;
+			pv = seg_pv(lvseg, s);
+			/* look up the reference in vg->pvs */
+			if (pv != dm_hash_lookup_binary(vhash->pvid, &pv->id,
+							sizeof(pv->id))) {
+				log_error(INTERNAL_ERROR
+					  "Referenced PV %s not listed in VG %s.",
+					  pv_dev_name(pv), vg->name);
+				r = 0;
 			}
 		}
 	}

@@ -2247,9 +2240,7 @@ int vg_validate(struct volume_group *vg)
 	uint32_t hidden_lv_count = 0, lv_count = 0, lv_visible_count = 0;
 	uint32_t pv_count = 0;
 	uint32_t num_snapshots = 0;
-	struct dm_hash_table *lvname_hash;
-	struct dm_hash_table *lvid_hash;
-	struct dm_hash_table *pvid_hash;
+	struct validate_hash vhash = { NULL };
 
 	if (vg->alloc == ALLOC_CLING_BY_TAGS) {
 		log_error(INTERNAL_ERROR "VG %s allocation policy set to invalid cling_by_tags.",

@@ -2258,7 +2249,7 @@ int vg_validate(struct volume_group *vg)
 	}
 
 	/* FIXME Also check there's no data/metadata overlap */
-	if (!(pvid_hash = dm_hash_create(vg->pv_count))) {
+	if (!(vhash.pvid = dm_hash_create(vg->pv_count))) {
 		log_error("Failed to allocate pvid hash.");
 		return 0;
 	}

@@ -2283,7 +2274,7 @@ int vg_validate(struct volume_group *vg)
 			r = 0;
 		}
 
-		if (dm_hash_lookup_binary(pvid_hash, &pvl->pv->id,
+		if (dm_hash_lookup_binary(vhash.pvid, &pvl->pv->id,
 					  sizeof(pvl->pv->id))) {
 			if (!id_write_format(&pvl->pv->id, uuid,
 					     sizeof(uuid)))

@@ -2295,7 +2286,7 @@ int vg_validate(struct volume_group *vg)
 			r = 0;
 		}
 
-		if (!dm_hash_insert_binary(pvid_hash, &pvl->pv->id,
+		if (!dm_hash_insert_binary(vhash.pvid, &pvl->pv->id,
 					   sizeof(pvl->pv->id), pvl->pv)) {
 			log_error("Failed to hash pvid.");
 			r = 0;

@@ -2303,7 +2294,6 @@ int vg_validate(struct volume_group *vg)
 		}
 	}
 
-	dm_hash_destroy(pvid_hash);
 
 	if (!check_pv_segments(vg)) {
 		log_error(INTERNAL_ERROR "PV segments corrupted in %s.",

@@ -2370,28 +2360,29 @@ int vg_validate(struct volume_group *vg)
 
 	/* Avoid endless loop if lv->segments list is corrupt */
 	if (!r)
-		return r;
+		goto out;
 
-	if (!(lvname_hash = dm_hash_create(lv_count))) {
+	if (!(vhash.lvname = dm_hash_create(lv_count))) {
 		log_error("Failed to allocate lv_name hash");
-		return 0;
+		r = 0;
+		goto out;
 	}
 
-	if (!(lvid_hash = dm_hash_create(lv_count))) {
+	if (!(vhash.lvid = dm_hash_create(lv_count))) {
 		log_error("Failed to allocate uuid hash");
-		dm_hash_destroy(lvname_hash);
-		return 0;
+		r = 0;
+		goto out;
 	}
 
 	dm_list_iterate_items(lvl, &vg->lvs) {
-		if (dm_hash_lookup(lvname_hash, lvl->lv->name)) {
+		if (dm_hash_lookup(vhash.lvname, lvl->lv->name)) {
 			log_error(INTERNAL_ERROR
 				  "Duplicate LV name %s detected in %s.",
 				  lvl->lv->name, vg->name);
 			r = 0;
 		}
 
-		if (dm_hash_lookup_binary(lvid_hash, &lvl->lv->lvid.id[1],
+		if (dm_hash_lookup_binary(vhash.lvid, &lvl->lv->lvid.id[1],
 					  sizeof(lvl->lv->lvid.id[1]))) {
 			if (!id_write_format(&lvl->lv->lvid.id[1], uuid,
 					     sizeof(uuid)))

@@ -2408,13 +2399,13 @@ int vg_validate(struct volume_group *vg)
 			r = 0;
 		}
 
-		if (!dm_hash_insert(lvname_hash, lvl->lv->name, lvl)) {
+		if (!dm_hash_insert(vhash.lvname, lvl->lv->name, lvl)) {
 			log_error("Failed to hash lvname.");
 			r = 0;
 			break;
 		}
 
-		if (!dm_hash_insert_binary(lvid_hash, &lvl->lv->lvid.id[1],
+		if (!dm_hash_insert_binary(vhash.lvid, &lvl->lv->lvid.id[1],
 					   sizeof(lvl->lv->lvid.id[1]), lvl->lv)) {
 			log_error("Failed to hash lvid.");
 			r = 0;

@@ -2422,10 +2413,7 @@ int vg_validate(struct volume_group *vg)
 		}
 	}
 
-	dm_hash_destroy(lvname_hash);
-	dm_hash_destroy(lvid_hash);
-
-	if (!_lv_postorder_vg(vg, _lv_validate_references_single, NULL)) {
+	if (!_lv_postorder_vg(vg, _lv_validate_references_single, &vhash)) {
 		stack;
 		r = 0;
 	}

@@ -2459,6 +2447,13 @@ int vg_validate(struct volume_group *vg)
 
 	if (vg_max_lv_reached(vg))
 		stack;
+out:
+	if (vhash.lvid)
+		dm_hash_destroy(vhash.lvid);
+	if (vhash.lvname)
+		dm_hash_destroy(vhash.lvname);
+	if (vhash.pvid)
+		dm_hash_destroy(vhash.pvid);
 
 	return r;
 }

Signature · 1 line

1.7.4.1

Zdenek Kabelac

author

2011-03-22 21:34 UTC #5 lore raw

PATCH 5/8 Simplify boundary checks

In reply to Zdenek Kabelac

As we do not support such large allocation they could wrap around the pointer size - I think this optimisation is correct and make the code easier to read and understand.

Signed-off-by Zdenek Kabelac <[email protected]>

---
 libdm/mm/pool-fast.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

libdm/mm/pool-fast.c +3 -3

index b651449..29d61a4 100644
@@ -92,7 +92,7 @@ void *dm_pool_alloc_aligned(struct dm_pool *p, size_t s, unsigned alignment)
 		_align_chunk(c, alignment);
 
 	/* have we got room ? */
-	if (!c || (c->begin > c->end) || (c->end - c->begin < s)) {
+	if (!c || ((c->begin + s) > c->end)) {
 		/* allocate new chunk */
 		size_t needed = s + alignment + sizeof(struct chunk);
 		c = _new_chunk(p, (needed > p->chunk_size) ?

@@ -169,7 +169,7 @@ int dm_pool_begin_object(struct dm_pool *p, size_t hint)
 	if (c)
 		_align_chunk(c, align);
 
-	if (!c || (c->begin > c->end) || (c->end - c->begin < hint)) {
+	if (!c || ((c->begin + hint) > c->end)) {
 		/* allocate a new chunk */
 		c = _new_chunk(p,
 			       hint > (p->chunk_size - sizeof(struct chunk)) ?

@@ -192,7 +192,7 @@ int dm_pool_grow_object(struct dm_pool *p, const void *extra, size_t delta)
 	if (!delta)
 		delta = strlen(extra);
 
-	if (c->end - (c->begin + p->object_len) < delta) {
+	if ((c->begin + p->object_len + delta) > c->end) {
 		/* move into a new chunk */
 		if (p->object_len + delta > (p->chunk_size / 2))
 			nc = _new_chunk(p, (p->object_len + delta) * 2);

Signature · 1 line

1.7.4.1

Zdenek Kabelac

author

2011-03-22 21:34 UTC #6 lore raw

PATCH 6/8 Use id_equal instead of strncmp()

In reply to Zdenek Kabelac

It's better for code reading and code consistency.

Signed-off-by Zdenek Kabelac <[email protected]>

---
 lib/metadata/metadata.c |    7 +++----
 1 files changed, 3 insertions(+), 4 deletions(-)

lib/metadata/metadata.c +3 -4

index 1d711bc..f4d4671 100644
@@ -3038,8 +3038,7 @@ static struct volume_group *_vg_read(struct cmd_context *cmd,
 				continue;
 			}
 
-			if (strncmp((char *)vg->id.uuid,
-			    (char *)correct_vg->id.uuid, ID_LEN)) {
+			if (!id_equal(&vg->id, &correct_vg->id)) {
 				inconsistent = 1;
 				inconsistent_vgid = 1;
 			}

@@ -3287,7 +3286,7 @@ static struct volume_group *_vg_read_by_vgid(struct cmd_context *cmd,
 	    vginfo->vgname && !is_orphan_vg(vginfo->vgname)) {
 		if ((vg = _vg_read(cmd, NULL, vgid, 1,
 				   &consistent, precommitted)) &&
-		    !strncmp((char *)vg->id.uuid, vgid, ID_LEN)) {
+		    id_equal(&vg->id, (const struct id *)vgid)) {
 			if (!consistent)
 				log_error("Volume group %s metadata is "
 					  "inconsistent", vg->name);

@@ -3318,7 +3317,7 @@ static struct volume_group *_vg_read_by_vgid(struct cmd_context *cmd,
 		consistent = 0;
 		if ((vg = _vg_read(cmd, vgname, vgid, 1, &consistent,
 				   precommitted)) &&
-		    !strncmp((char *)vg->id.uuid, vgid, ID_LEN)) {
+		    id_equal(&vg->id, (const struct id *)vgid)) {
 			if (!consistent) {
 				log_error("Volume group %s metadata is "
 					  "inconsistent", vgname);

Signature · 1 line

1.7.4.1

Zdenek Kabelac

author

2011-03-22 21:34 UTC #7 lore raw

PATCH 7/8 Proposal - avoid parsing same data

In reply to Zdenek Kabelac

Instead of regenerating config tree and parsing same data again, check whether export_vg_to_buffer does not produce same string as the one already cached - in this case keep it, otherwise throw cached content away.

For the code simplicity calling _free_cached_vgmetadata() with vgmetadata == NULL as the function handles this itself.

Note: sometimes export_vg_to_buffer() generates almost the same data with just different time stamp, but for the patch simplicity, data are reparsed in this case.

This patch currently helps for vgrefresh.

Signed-off-by Zdenek Kabelac <[email protected]>

---
 lib/cache/lvmcache.c |   17 +++++++++++++----
 lib/cache/lvmcache.h |    1 +
 2 files changed, 14 insertions(+), 4 deletions(-)

lib/cache/lvmcache.c +13 -4

index 4e2e158..037fc4a 100644
@@ -99,6 +99,7 @@ static void _store_metadata(struct volume_group *vg, unsigned precommitted)
 {
 	char uuid[64] __attribute__((aligned(8)));
 	struct lvmcache_vginfo *vginfo;
+	char *data;
 	int size;
 
 	if (!(vginfo = vginfo_from_vgid((const char *)&vg->id))) {

@@ -106,14 +107,22 @@ static void _store_metadata(struct volume_group *vg, unsigned precommitted)
 		return;
 	}
 
-	if (vginfo->vgmetadata)
-		_free_cached_vgmetadata(vginfo);
-
-	if (!(size = export_vg_to_buffer(vg, &vginfo->vgmetadata))) {
+	if (!(size = export_vg_to_buffer(vg, &data))) {
 		stack;
+		_free_cached_vgmetadata(vginfo);
 		return;
 	}
 
+	/* Avoid reparsing of the same data string */
+	if (vginfo->vgmetadata && vginfo->vgmetadata_size == size &&
+	    strcmp(vginfo->vgmetadata, data) == 0)
+		dm_free(data);
+	else {
+		_free_cached_vgmetadata(vginfo);
+		vginfo->vgmetadata_size = size;
+		vginfo->vgmetadata = data;
+	}
+
 	vginfo->precommitted = precommitted;
 
 	if (!id_write_format((const struct id *)vginfo->vgid, uuid, sizeof(uuid))) {

lib/cache/lvmcache.h +1 -0

index 080f3b5..f13cad2 100644
@@ -46,6 +46,7 @@ struct lvmcache_vginfo {
 	char _padding[7];
 	struct lvmcache_vginfo *next; /* Another VG with same name? */
 	char *creation_host;
+	size_t vgmetadata_size;
 	char *vgmetadata;	/* Copy of VG metadata as format_text string */
 	struct config_tree *cft; /* Config tree created from vgmetadata */
 				/* Lifetime is directly tied to vgmetadata */

Signature · 1 line

1.7.4.1

Zdenek Kabelac

author

2011-03-22 21:34 UTC #8 lore raw

PATCH 8/8 Few more files filtered from memory locking

In reply to Zdenek Kabelac

Signed-off-by Zdenek Kabelac <[email protected]>

---
 lib/mm/memlock.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

lib/mm/memlock.c +2 -0

index 0e8f585..363b724 100644
@@ -89,9 +89,11 @@ static const char * const _ignore_maps[] = {
 /* default blacklist for maps */
 static const char * const _blacklist_maps[] = {
     "locale/locale-archive",
+    "/LC_MESSAGES/",
     "gconv/gconv-modules.cache",
     "/libreadline.so.",	/* not using readline during mlock */
     "/libncurses.so.",	/* not using readline during mlock */
+    "/libtinfo.so.",	/* not using readline during mlock */
     "/libdl-",		/* not using dlopen,dlsym during mlock */
     /* "/libdevmapper-event.so" */
 };

Signature · 1 line

1.7.4.1