tabled: fix bugs in streaming of data

PATCH Applied Pete Zaitcev 2 participants 5 messages Jan 5, 2010 – Jan 7, 2010

lore

Share & tools

lore

Pete Zaitcev

author

2010-01-05 07:27 UTC #0 lore raw

PATCH 1/2 tabled: fix bugs in streaming of data

This patch fixes 3 plain coding bugs in the streaming mechanism.

1. Whenever we loop back to "restart" label, we have to clear n_iov. Otherwise, the loop that fills iov[] writes beyond the end of the array. The result is binary garbage on console and a hang.

Behold the evils of initialized variables. Fix is obvious.

BTW, array indexes are signed.

2. When we consume a queue element partially, we ought to advance its buffer pointer when we decrement the length. Otherwise, we resend the data that was sent already, causing a corruption in transfer.

The fix is obvious (add tmp->buf += sz).

3. The accounting (through unfortunately named write_cnt) was incorrect because wr->len may be decremented by the time the element is freed.

Since we're at it, we implement a couple of safety improvements. First, we change the API to the callback a little. From now on, it does not have access to the struct client_write anymore, and cannot accidentially rely on wr->buf (which can be changed now). Also, we renamed "len" into "togo" and named the new member "length", to make sure that all use instances for "len" were reviewed.

Signed-off-by Pete Zaitcev <[email protected]>

---
 server/object.c |    8 +++-----
 server/server.c |   31 ++++++++++++++++---------------
 server/tabled.h |   11 ++++++-----
 3 files changed, 25 insertions(+), 25 deletions(-)

commit c3671739c217b4b7a63d82543d94318f368a109f

Author: Master <[email protected]>
Date:   Tue Jan 5 00:04:15 2010 -0700

Bugfixes for the object streaming to client.

server/object.c +3 -5

index 103b36e..d61a446 100644
@@ -983,8 +983,7 @@ void cli_in_end(struct client *cli)
 	stor_close(&cli->in_ce);
 }
 
-static bool object_get_more(struct client *cli, struct client_write *wr,
-			    bool done);
+static bool object_get_more(struct client *cli, void *cb_data, bool done);
 
 /*
  * Return true iff cli_writeq was called. This is compatible with the

@@ -1036,12 +1035,11 @@ err_out:
 }
 
 /* callback from the client side: a queued write is being disposed */
-static bool object_get_more(struct client *cli, struct client_write *wr,
-			    bool done)
+static bool object_get_more(struct client *cli, void *cb_data, bool done)
 {
 
 	/* free now-written buffer */
-	free(wr->cb_data);
+	free(cb_data);
 
 	/* do not queue more, if !completion or fd was closed early */
 	if (!done)	/* FIXME We used to test for input errors here. */

server/server.c +16 ~~-15~~

index 32bb8a4..f16d2ab 100644
@@ -386,10 +386,10 @@ static bool cli_write_free(struct client *cli, struct client_write *tmp,
 {
 	bool rcb = false;
 
-	cli->write_cnt -= tmp->len;
+	cli->write_cnt -= tmp->length;
 	list_del(&tmp->node);
 	if (tmp->cb)
-		rcb = tmp->cb(cli, tmp, done);
+		rcb = tmp->cb(cli, tmp->cb_data, done);
 	free(tmp);
 
 	return rcb;

@@ -487,7 +487,7 @@ static bool cli_evt_recycle(struct client *cli, unsigned int events)
 
 static void cli_writable(struct client *cli)
 {
-	unsigned int n_iov = 0;
+	int n_iov;
 	struct client_write *tmp;
 	ssize_t rc;
 	struct iovec iov[CLI_MAX_WR_IOV];

@@ -497,13 +497,14 @@ restart:
 	more_work = false;
 
 	/* accumulate pending writes into iovec */
+	n_iov = 0;
 	list_for_each_entry(tmp, &cli->write_q, node) {
+		if (n_iov == CLI_MAX_WR_IOV)
+			break;
 		/* bleh, struct iovec should declare iov_base const */
 		iov[n_iov].iov_base = (void *) tmp->buf;
-		iov[n_iov].iov_len = tmp->len;
+		iov[n_iov].iov_len = tmp->togo;
 		n_iov++;
-		if (n_iov == CLI_MAX_WR_IOV)
-			break;
 	}
 
 	/* execute non-blocking write */

@@ -527,14 +528,15 @@ do_write:
 		tmp = list_entry(cli->write_q.next, struct client_write, node);
 
 		/* mark data consumed by decreasing tmp->len */
-		sz = (tmp->len < rc) ? tmp->len : rc;
-		tmp->len -= sz;
+		sz = (tmp->togo < rc) ? tmp->togo : rc;
+		tmp->togo -= sz;
+		tmp->buf += sz;
 		rc -= sz;
 
 		/* if tmp->len reaches zero, write is complete,
 		 * call callback and clean up
 		 */
-		if (tmp->len == 0)
+		if (tmp->togo == 0)
 			if (cli_write_free(cli, tmp, true))
 				more_work = true;
 	}

@@ -600,11 +602,12 @@ int cli_writeq(struct client *cli, const void *buf, unsigned int buflen,
 		return -ENOMEM;
 
 	wr->buf = buf;
-	wr->len = buflen;
+	wr->togo = buflen;
+	wr->length = buflen;
 	wr->cb = cb;
 	wr->cb_data = cb_data;
 	list_add_tail(&wr->node, &cli->write_q);
-	cli->write_cnt += wr->len;
+	cli->write_cnt += buflen;
 
 	return 0;
 }

@@ -645,11 +648,9 @@ do_read:
 	return 0;
 }
 
-bool cli_cb_free(struct client *cli, struct client_write *wr,
-			bool done)
+bool cli_cb_free(struct client *cli, void *cb_data, bool done)
 {
-	free(wr->cb_data);
-
+	free(cb_data);
 	return false;
 }

server/tabled.h +6 -5

index 59c474a..a0d3400 100644
@@ -103,11 +103,13 @@ struct storage_node {
 };
 
 typedef bool (*cli_evt_func)(struct client *, unsigned int);
-typedef bool (*cli_write_func)(struct client *, struct client_write *, bool);
+typedef bool (*cli_write_func)(struct client *, void *, bool);
 
 struct client_write {
-	const void		*buf;		/* write buffer */
-	int			len;		/* write buffer length */
+	const void		*buf;		/* write buffer pointer */
+	int			togo;		/* write buffer remainder */
+
+	int			length;		/* length for accounting */
 	cli_write_func		cb;		/* callback */
 	void			*cb_data;	/* data passed to cb */

@@ -314,8 +316,7 @@ extern bool cli_resp_xml(struct client *cli, int http_status,
 extern int cli_writeq(struct client *cli, const void *buf, unsigned int buflen,
 		     cli_write_func cb, void *cb_data);
 extern size_t cli_wqueued(struct client *cli);
-extern bool cli_cb_free(struct client *cli, struct client_write *wr,
-			bool done);
+extern bool cli_cb_free(struct client *cli, void *cb_data, bool done);
 extern bool cli_write_start(struct client *cli);
 extern int cli_req_avail(struct client *cli);
 extern void applog(int prio, const char *fmt, ...);

Jeff Garzik

2010-01-05 09:13 UTC #1 lore raw

Quoted 26 lines · Pete Zaitcev

This patch fixes 3 plain coding bugs in the streaming mechanism.
1. Whenever we loop back to "restart" label, we have to clear n_iov. Otherwise, the loop that fills iov[] writes beyond the end of the array. The result is binary garbage on console and a hang.
Behold the evils of initialized variables. Fix is obvious.
BTW, array indexes are signed.
2. When we consume a queue element partially, we ought to advance its buffer pointer when we decrement the length. Otherwise, we resend the data that was sent already, causing a corruption in transfer.
The fix is obvious (add tmp->buf += sz).
3. The accounting (through unfortunately named write_cnt) was incorrect
    because wr->len may be decremented by the time the element is freed.
Since we're at it, we implement a couple of safety improvements. First, we change the API to the callback a little. From now on, it does not have access to the struct client_write anymore, and cannot accidentially rely on wr->buf (which can be changed now). Also, we renamed "len" into "togo" and named the new member "length", to make sure that all use instances for "len" were reviewed.

Quoted 7 lines

Signed-off-by Pete Zaitcev<[email protected]>

---
  server/object.c |    8 +++-----
  server/server.c |   31 ++++++++++++++++---------------
  server/tabled.h |   11 ++++++-----
  3 files changed, 25 insertions(+), 25 deletions(-)

applied... chunkd needs these fixes also, yes?

Pete Zaitcev

author

2010-01-06 04:02 UTC #2 lore raw

Quoted 1 line · On Tue, 05 Jan 2010 04:13:36 -0500 Jeff Garzik <[email protected]> wrote:

applied... chunkd needs these fixes also, yes?

No, it appears correct as-is. Although, the large-object test in the suite does not check the data it transmits, so if chunkd were broken, we might not know it. On the other hand, the fact that tabled stores test objects in chunkservers assures us that it works at least as far as tabled's tests verify.

-- Pete

Jeff Garzik

2010-01-07 22:10 UTC #3 lore raw

Quoted 10 lines · Pete Zaitcev

Quoted 1 line · On Tue, 05 Jan 2010 04:13:36 -0500 Jeff Garzik<[email protected]> wrote:
applied... chunkd needs these fixes also, yes?
No, it appears correct as-is. Although, the large-object test in the suite does not check the data it transmits, so if chunkd were broken, we might not know it. On the other hand, the fact that tabled stores test objects in chunkservers assures us that it works at least as far as tabled's tests verify.

I was thinking specifically that chunkd may want the in-progress xfer codes changes in your patch. chunkd, tabled, nfs4d (and ancient storaged) all share the same basic write-in-progress and TCP data xfer code.

I'll take a look, no need to worry about it yourself.

Jeff

Pete Zaitcev

author

2010-01-07 22:27 UTC #4 lore raw

Quoted 5 lines · On Thu, 07 Jan 2010 17:10:49 -0500 Jeff Garzik <[email protected]> wrote:

I was thinking specifically that chunkd may want the in-progress xfer codes changes in your patch. chunkd, tabled, nfs4d (and ancient storaged) all share the same basic write-in-progress and TCP data xfer code.
I'll take a look, no need to worry about it yourself.

At least 1 of the 3 that I fixed definitely was a corruption that I introduced to your design when I adapted it to simultaneous streaming on write. Not sure about other two, but maybe I just screwed tabled.
But sure, if you have a moment to re-check chunkd, be my guest.

-- Pete