[PATCHES] ctdb - slightly refactor the vacuum_fetch loop

Michael Adam obnox at samba.org
Thu Jun 4 23:53:13 MDT 2015


Hi Amitay,

Thanks for your comments.
At

On 2015-06-05 at 10:57 +1000, Amitay Isaacs wrote:
> Hi Michael,
> 
> On Thu, Jun 4, 2015 at 4:26 PM, Michael Adam <obnox at samba.org> wrote:
> 
> > Review etc appreciated!
> >
> > Cheers - Michael
> >
> 
> The reworking looks good.
> 
> For the second last patch that converts while loop into for
> loop, I would prefer not to overload the for loop assignments
> and conditions.  Keep only iterator variables in the for
> statement and move other assignments and conditions in the body
> of the loop.

Hmm I had thought about this back and forth, and actually
liked the way, but I just realized that I even missed the
in-loop initialization of r = v->r ... ;) So I now kept the
while-loop and just moved the advancing code to the end.

What do you think?

> Also, if you don't mind can you use "ctdb-foobar:" instead of
> "ctdb:foobar:" in the commit messages? It makes easier to import the
> patches to 2.5 with a script. :-)

done.

Updated version attached.

Thanks - Michael
-------------- next part --------------
From d84b22ca6935461b954022d1861b1e799510d4ac 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 817085a96ce4cf63bb23d89471bcb8d0d66c0cd9 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 5ea61eb0912426376ec0761062e110f38543cc4a 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 | 114 +++++++++++++++++++++++++-------------------
 1 file changed, 66 insertions(+), 48 deletions(-)

diff --git a/ctdb/server/ctdb_recoverd.c b/ctdb/server/ctdb_recoverd.c
index e5cb766..8af252e 100644
--- a/ctdb/server/ctdb_recoverd.c
+++ b/ctdb/server/ctdb_recoverd.c
@@ -1008,67 +1008,85 @@ 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;
-
-		ZERO_STRUCT(call);
-		call.call_id = CTDB_NULL_FUNC;
-		call.flags = CTDB_IMMEDIATE_MIGRATION;
-		call.flags |= CTDB_CALL_FLAG_VACUUM_MIGRATION;
+		bool ok;
 
 		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 040474a5391c93b350a85fd745cfc96842c855bc 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 8af252e..51dcdcd 100644
--- a/ctdb/server/ctdb_recoverd.c
+++ b/ctdb/server/ctdb_recoverd.c
@@ -1125,8 +1125,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;
@@ -1134,8 +1133,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;
 		}
 	}
 
@@ -1143,8 +1141,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++) {
@@ -1155,30 +1152,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;
@@ -1188,8 +1181,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];
 
@@ -1198,6 +1190,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 3ff8b6e59b30c94f5e29685a856a8fb6225b56ab 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: slightly reorder the vacuum fetch
 loop

Reads more naturally this way, imho.

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

diff --git a/ctdb/server/ctdb_recoverd.c b/ctdb/server/ctdb_recoverd.c
index 51dcdcd..e5a4a40 100644
--- a/ctdb/server/ctdb_recoverd.c
+++ b/ctdb/server/ctdb_recoverd.c
@@ -1080,13 +1080,14 @@ static void vacuum_fetch_next(struct vacuum_info *v)
 		bool ok;
 
 		r = v->r;
-		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;
 		}
+
+		v->r = (struct ctdb_rec_data *)(r->length + (uint8_t *)r);
+		v->recs->count--;
 	}
 
 	talloc_free(v);
-- 
2.4.2


From f2a2aa389c75ad1ce015265acc2a5909cb0fe405 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. This also makes the talloc/free
bracked around the loop more obvious.

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

diff --git a/ctdb/server/ctdb_recoverd.c b/ctdb/server/ctdb_recoverd.c
index e5a4a40..002191f 100644
--- a/ctdb/server/ctdb_recoverd.c
+++ b/ctdb/server/ctdb_recoverd.c
@@ -1070,29 +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)
-{
-	while (v->recs->count) {
-		struct ctdb_rec_data *r;
-		bool ok;
-
-		r = v->r;
-
-		ok = vacuum_fetch_process_one(v->ctdb_db, v->rec->ctdb->pnn, r);
-		if (!ok) {
-			break;
-		}
-
-		v->r = (struct ctdb_rec_data *)(r->length + (uint8_t *)r);
-		v->recs->count--;
-	}
-
-	talloc_free(v);
-}
-
 
 /*
   destroy a vacuum info structure
@@ -1190,7 +1167,21 @@ static void vacuum_fetch_handler(struct ctdb_context *ctdb, uint64_t srvid,
 
 	talloc_set_destructor(v, vacuum_info_destructor);
 
-	vacuum_fetch_next(v);
+	while (v->recs->count) {
+		bool ok;
+
+		r = v->r;
+
+		ok = vacuum_fetch_process_one(v->ctdb_db, v->rec->ctdb->pnn, r);
+		if (!ok) {
+			break;
+		}
+
+		v->r = (struct ctdb_rec_data *)(r->length + (uint8_t *)r);
+		v->recs->count--;
+	}
+
+	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/20150605/99185d4f/attachment.pgp>


More information about the samba-technical mailing list