[PATCHES] ctdb - slightly refactor the vacuum_fetch loop

Michael Adam obnox at samba.org
Thu Jun 4 00:26:10 MDT 2015


Hi,

attached is a patchset that slightly reworks the loop in the
vacuum fetch handler of ctdb.  So that the result reads much
more naturally imho, than the original code. (reducing
indentation and makin the talloc/free bracket around the
loop more obvious.  It does not change the behaviour.

(Note: The original patchset first renamed vacuum_fetch_next
to vacuum_fetch_loop and amended the function header comment,
but since I finally removed the function, moving the loop back
into vacuum_fetch_handler, I removed these changes.)

Review etc appreciated!

Cheers - Michael
-------------- next part --------------
From 1422f1990935fa49193cfab357ae6df2895099c2 Mon Sep 17 00:00:00 2001
From: Michael Adam <obnox at samba.org>
Date: Thu, 28 May 2015 01:06:25 +0200
Subject: [PATCH 1/6] ctdb:recoverd/vacuum: remove unneeded prototype.

Signed-off-by: Michael Adam <obnox at samba.org>
---
 ctdb/server/ctdb_recoverd.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/ctdb/server/ctdb_recoverd.c b/ctdb/server/ctdb_recoverd.c
index e76a0d0..1b4ac50 100644
--- a/ctdb/server/ctdb_recoverd.c
+++ b/ctdb/server/ctdb_recoverd.c
@@ -998,7 +998,6 @@ struct vacuum_info {
 	struct ctdb_rec_data *r;
 };
 
-static void vacuum_fetch_next(struct vacuum_info *v);
 
 /*
   called when a vacuum fetch has completed - just free it and do the next one
-- 
2.4.2


From 7e0815cd8861c775b16b4777eae7b312acc75af8 Mon Sep 17 00:00:00 2001
From: Michael Adam <obnox at samba.org>
Date: Tue, 2 Jun 2015 21:27:16 +0200
Subject: [PATCH 2/6] ctdb:recoverd/vacuum: move two variables into scope.

Signed-off-by: Michael Adam <obnox at samba.org>
---
 ctdb/server/ctdb_recoverd.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/ctdb/server/ctdb_recoverd.c b/ctdb/server/ctdb_recoverd.c
index 1b4ac50..e5cb766 100644
--- a/ctdb/server/ctdb_recoverd.c
+++ b/ctdb/server/ctdb_recoverd.c
@@ -1013,13 +1013,12 @@ static void vacuum_fetch_callback(struct ctdb_client_call_state *state)
 */
 static void vacuum_fetch_next(struct vacuum_info *v)
 {
-	struct ctdb_call call;
-	struct ctdb_rec_data *r;
-
 	while (v->recs->count) {
 		struct ctdb_client_call_state *state;
 		TDB_DATA data;
 		struct ctdb_ltdb_header *hdr;
+		struct ctdb_call call;
+		struct ctdb_rec_data *r;
 
 		ZERO_STRUCT(call);
 		call.call_id = CTDB_NULL_FUNC;
-- 
2.4.2


From 519406dbaf2890f85931bef79136aaec66f80c2e Mon Sep 17 00:00:00 2001
From: Michael Adam <obnox at samba.org>
Date: Tue, 2 Jun 2015 21:39:00 +0200
Subject: [PATCH 3/6] ctdb:recoverd/vacuum: factor vacuum_fetch_process_one out
 of vacuum_fetch_loop

Signed-off-by: Michael Adam <obnox at samba.org>
---
 ctdb/server/ctdb_recoverd.c | 113 ++++++++++++++++++++++++++------------------
 1 file changed, 66 insertions(+), 47 deletions(-)

diff --git a/ctdb/server/ctdb_recoverd.c b/ctdb/server/ctdb_recoverd.c
index e5cb766..bbc2d78 100644
--- a/ctdb/server/ctdb_recoverd.c
+++ b/ctdb/server/ctdb_recoverd.c
@@ -1008,67 +1008,86 @@ static void vacuum_fetch_callback(struct ctdb_client_call_state *state)
 }
 
 
+/**
+ * Process one elements of the vacuum fetch list:
+ * Migrate it over to us with the special flag
+ * CTDB_CALL_FLAG_VACUUM_MIGRATION.
+ */
+static bool vacuum_fetch_process_one(struct ctdb_db_context *ctdb_db,
+				     uint32_t pnn,
+				     struct ctdb_rec_data *r)
+{
+	struct ctdb_client_call_state *state;
+	TDB_DATA data;
+	struct ctdb_ltdb_header *hdr;
+	struct ctdb_call call;
+
+	ZERO_STRUCT(call);
+	call.call_id = CTDB_NULL_FUNC;
+	call.flags = CTDB_IMMEDIATE_MIGRATION;
+	call.flags |= CTDB_CALL_FLAG_VACUUM_MIGRATION;
+
+	call.key.dptr = &r->data[0];
+	call.key.dsize = r->keylen;
+
+	/* ensure we don't block this daemon - just skip a record if we can't get
+	   the chainlock */
+	if (tdb_chainlock_nonblock(ctdb_db->ltdb->tdb, call.key) != 0) {
+		return true;
+	}
+
+	data = tdb_fetch(ctdb_db->ltdb->tdb, call.key);
+	if (data.dptr == NULL) {
+		tdb_chainunlock(ctdb_db->ltdb->tdb, call.key);
+		return true;
+	}
+
+	if (data.dsize < sizeof(struct ctdb_ltdb_header)) {
+		free(data.dptr);
+		tdb_chainunlock(ctdb_db->ltdb->tdb, call.key);
+		return true;
+	}
+
+	hdr = (struct ctdb_ltdb_header *)data.dptr;
+	if (hdr->dmaster == pnn) {
+		/* its already local */
+		free(data.dptr);
+		tdb_chainunlock(ctdb_db->ltdb->tdb, call.key);
+		return true;
+	}
+
+	free(data.dptr);
+
+	state = ctdb_call_send(ctdb_db, &call);
+	tdb_chainunlock(ctdb_db->ltdb->tdb, call.key);
+	if (state == NULL) {
+		DEBUG(DEBUG_ERR,(__location__ " Failed to setup vacuum fetch call\n"));
+		return false;
+	}
+	state->async.fn = vacuum_fetch_callback;
+	state->async.private_data = NULL;
+
+	return true;
+}
+
 /*
   process the next element from the vacuum list
 */
 static void vacuum_fetch_next(struct vacuum_info *v)
 {
 	while (v->recs->count) {
-		struct ctdb_client_call_state *state;
-		TDB_DATA data;
-		struct ctdb_ltdb_header *hdr;
-		struct ctdb_call call;
 		struct ctdb_rec_data *r;
+		bool ok;
 
-		ZERO_STRUCT(call);
-		call.call_id = CTDB_NULL_FUNC;
-		call.flags = CTDB_IMMEDIATE_MIGRATION;
-		call.flags |= CTDB_CALL_FLAG_VACUUM_MIGRATION;
 
 		r = v->r;
 		v->r = (struct ctdb_rec_data *)(r->length + (uint8_t *)r);
 		v->recs->count--;
 
-		call.key.dptr = &r->data[0];
-		call.key.dsize = r->keylen;
-
-		/* ensure we don't block this daemon - just skip a record if we can't get
-		   the chainlock */
-		if (tdb_chainlock_nonblock(v->ctdb_db->ltdb->tdb, call.key) != 0) {
-			continue;
-		}
-
-		data = tdb_fetch(v->ctdb_db->ltdb->tdb, call.key);
-		if (data.dptr == NULL) {
-			tdb_chainunlock(v->ctdb_db->ltdb->tdb, call.key);
-			continue;
-		}
-
-		if (data.dsize < sizeof(struct ctdb_ltdb_header)) {
-			free(data.dptr);
-			tdb_chainunlock(v->ctdb_db->ltdb->tdb, call.key);
-			continue;
-		}
-		
-		hdr = (struct ctdb_ltdb_header *)data.dptr;
-		if (hdr->dmaster == v->rec->ctdb->pnn) {
-			/* its already local */
-			free(data.dptr);
-			tdb_chainunlock(v->ctdb_db->ltdb->tdb, call.key);
-			continue;
-		}
-
-		free(data.dptr);
-
-		state = ctdb_call_send(v->ctdb_db, &call);
-		tdb_chainunlock(v->ctdb_db->ltdb->tdb, call.key);
-		if (state == NULL) {
-			DEBUG(DEBUG_ERR,(__location__ " Failed to setup vacuum fetch call\n"));
-			talloc_free(v);
-			return;
+		ok = vacuum_fetch_process_one(v->ctdb_db, v->rec->ctdb->pnn, r);
+		if (!ok) {
+			break;
 		}
-		state->async.fn = vacuum_fetch_callback;
-		state->async.private_data = NULL;
 	}
 
 	talloc_free(v);
-- 
2.4.2


From 50d975880822c0ca4d93dedaa8131bc3e3c479d8 Mon Sep 17 00:00:00 2001
From: Michael Adam <obnox at samba.org>
Date: Tue, 2 Jun 2015 21:57:54 +0200
Subject: [PATCH 4/6] ctdb:recoverd/vacuum: add common exit point to
 vacuum_fetch_handler

Signed-off-by: Michael Adam <obnox at samba.org>
---
 ctdb/server/ctdb_recoverd.c | 26 ++++++++++----------------
 1 file changed, 10 insertions(+), 16 deletions(-)

diff --git a/ctdb/server/ctdb_recoverd.c b/ctdb/server/ctdb_recoverd.c
index bbc2d78..098dde3 100644
--- a/ctdb/server/ctdb_recoverd.c
+++ b/ctdb/server/ctdb_recoverd.c
@@ -1126,8 +1126,7 @@ static void vacuum_fetch_handler(struct ctdb_context *ctdb, uint64_t srvid,
 	r = (struct ctdb_rec_data *)&recs->data[0];
 
 	if (recs->count == 0) {
-		talloc_free(tmp_ctx);
-		return;
+		goto done;
 	}
 
 	srcnode = r->reqid;
@@ -1135,8 +1134,7 @@ static void vacuum_fetch_handler(struct ctdb_context *ctdb, uint64_t srvid,
 	for (v=rec->vacuum_info;v;v=v->next) {
 		if (srcnode == v->srcnode && recs->db_id == v->ctdb_db->db_id) {
 			/* we're already working on records from this node */
-			talloc_free(tmp_ctx);
-			return;
+			goto done;
 		}
 	}
 
@@ -1144,8 +1142,7 @@ static void vacuum_fetch_handler(struct ctdb_context *ctdb, uint64_t srvid,
 	ret = ctdb_ctrl_getdbmap(ctdb, CONTROL_TIMEOUT(), CTDB_CURRENT_NODE, tmp_ctx, &dbmap);
 	if (ret != 0) {
 		DEBUG(DEBUG_ERR, (__location__ " Unable to get dbids from local node\n"));
-		talloc_free(tmp_ctx);
-		return;
+		goto done;
 	}
 
 	for (i=0;i<dbmap->num;i++) {
@@ -1156,30 +1153,26 @@ static void vacuum_fetch_handler(struct ctdb_context *ctdb, uint64_t srvid,
 	}
 	if (i == dbmap->num) {
 		DEBUG(DEBUG_ERR, (__location__ " Unable to find db_id 0x%x on local node\n", recs->db_id));
-		talloc_free(tmp_ctx);
-		return;		
+		goto done;
 	}
 
 	/* find the name of this database */
 	if (ctdb_ctrl_getdbname(ctdb, CONTROL_TIMEOUT(), CTDB_CURRENT_NODE, recs->db_id, tmp_ctx, &name) != 0) {
 		DEBUG(DEBUG_ERR,(__location__ " Failed to get name of db 0x%x\n", recs->db_id));
-		talloc_free(tmp_ctx);
-		return;
+		goto done;
 	}
 
 	/* attach to it */
 	ctdb_db = ctdb_attach(ctdb, CONTROL_TIMEOUT(), name, persistent, 0);
 	if (ctdb_db == NULL) {
 		DEBUG(DEBUG_ERR,(__location__ " Failed to attach to database '%s'\n", name));
-		talloc_free(tmp_ctx);
-		return;
+		goto done;
 	}
 
 	v = talloc_zero(rec, struct vacuum_info);
 	if (v == NULL) {
 		DEBUG(DEBUG_CRIT,(__location__ " Out of memory\n"));
-		talloc_free(tmp_ctx);
-		return;
+		goto done;
 	}
 
 	v->rec = rec;
@@ -1189,8 +1182,7 @@ static void vacuum_fetch_handler(struct ctdb_context *ctdb, uint64_t srvid,
 	if (v->recs == NULL) {
 		DEBUG(DEBUG_CRIT,(__location__ " Out of memory\n"));
 		talloc_free(v);
-		talloc_free(tmp_ctx);
-		return;		
+		goto done;
 	}
 	v->r = 	(struct ctdb_rec_data *)&v->recs->data[0];
 
@@ -1199,6 +1191,8 @@ static void vacuum_fetch_handler(struct ctdb_context *ctdb, uint64_t srvid,
 	talloc_set_destructor(v, vacuum_info_destructor);
 
 	vacuum_fetch_next(v);
+
+done:
 	talloc_free(tmp_ctx);
 }
 
-- 
2.4.2


From 785d10d619d8fae2d186f104cc27d80f2cfcfbc6 Mon Sep 17 00:00:00 2001
From: Michael Adam <obnox at samba.org>
Date: Tue, 2 Jun 2015 22:16:17 +0200
Subject: [PATCH 5/6] ctdb:recoverd/vacuum: improve vacuum fetch loop.

Reads more naturally this way, imho.

Signed-off-by: Michael Adam <obnox at samba.org>
---
 ctdb/server/ctdb_recoverd.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/ctdb/server/ctdb_recoverd.c b/ctdb/server/ctdb_recoverd.c
index 098dde3..e292efc 100644
--- a/ctdb/server/ctdb_recoverd.c
+++ b/ctdb/server/ctdb_recoverd.c
@@ -1075,15 +1075,14 @@ static bool vacuum_fetch_process_one(struct ctdb_db_context *ctdb_db,
 */
 static void vacuum_fetch_next(struct vacuum_info *v)
 {
-	while (v->recs->count) {
-		struct ctdb_rec_data *r;
-		bool ok;
-
-
-		r = v->r;
-		v->r = (struct ctdb_rec_data *)(r->length + (uint8_t *)r);
-		v->recs->count--;
+	struct ctdb_rec_data *r;
+	bool ok;
 
+	for (ok = true, r = v->r;
+	     ok && (v->recs->count != 0);
+	     v->r = (struct ctdb_rec_data *)(r->length + (uint8_t *)r),
+	     v->recs->count--)
+	{
 		ok = vacuum_fetch_process_one(v->ctdb_db, v->rec->ctdb->pnn, r);
 		if (!ok) {
 			break;
-- 
2.4.2


From 41bfabe754cfff625bc965f38b3d7f21a9ea0a30 Mon Sep 17 00:00:00 2001
From: Michael Adam <obnox at samba.org>
Date: Tue, 2 Jun 2015 22:17:03 +0200
Subject: [PATCH 6/6] ctdb:recoverd/vacuum: move fetch loop back into fetch
 handler.

With the processing of one element factored out,
it is more natural to have the actual loop inside the
handler function.

Signed-off-by: Michael Adam <obnox at samba.org>
---
 ctdb/server/ctdb_recoverd.c | 33 ++++++++++-----------------------
 1 file changed, 10 insertions(+), 23 deletions(-)

diff --git a/ctdb/server/ctdb_recoverd.c b/ctdb/server/ctdb_recoverd.c
index e292efc..be0b695 100644
--- a/ctdb/server/ctdb_recoverd.c
+++ b/ctdb/server/ctdb_recoverd.c
@@ -1070,28 +1070,6 @@ static bool vacuum_fetch_process_one(struct ctdb_db_context *ctdb_db,
 	return true;
 }
 
-/*
-  process the next element from the vacuum list
-*/
-static void vacuum_fetch_next(struct vacuum_info *v)
-{
-	struct ctdb_rec_data *r;
-	bool ok;
-
-	for (ok = true, r = v->r;
-	     ok && (v->recs->count != 0);
-	     v->r = (struct ctdb_rec_data *)(r->length + (uint8_t *)r),
-	     v->recs->count--)
-	{
-		ok = vacuum_fetch_process_one(v->ctdb_db, v->rec->ctdb->pnn, r);
-		if (!ok) {
-			break;
-		}
-	}
-
-	talloc_free(v);
-}
-
 
 /*
   destroy a vacuum info structure
@@ -1120,6 +1098,7 @@ static void vacuum_fetch_handler(struct ctdb_context *ctdb, uint64_t srvid,
 	struct ctdb_rec_data *r;
 	uint32_t srcnode;
 	struct vacuum_info *v;
+	bool ok;
 
 	recs = (struct ctdb_marshall_buffer *)data.dptr;
 	r = (struct ctdb_rec_data *)&recs->data[0];
@@ -1189,7 +1168,15 @@ static void vacuum_fetch_handler(struct ctdb_context *ctdb, uint64_t srvid,
 
 	talloc_set_destructor(v, vacuum_info_destructor);
 
-	vacuum_fetch_next(v);
+	for (ok = true, r = v->r;
+	     ok && (v->recs->count != 0);
+	     v->r = (struct ctdb_rec_data *)(r->length + (uint8_t *)r),
+	     v->recs->count--)
+	{
+		ok = vacuum_fetch_process_one(v->ctdb_db, v->rec->ctdb->pnn, r);
+	}
+
+	talloc_free(v);
 
 done:
 	talloc_free(tmp_ctx);
-- 
2.4.2

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20150604/c24bd587/attachment.pgp>


More information about the samba-technical mailing list