[PATCH 1/6] chunkd: Don't hardcode the prefix length of object pathname

[PATCH 1/6] chunkd: Don't hardcode the prefix length of object pathname

[Akinobu Mita]

Define PREFIX_LEN instead of hardcoded prefix length of object pathname
in fs_obj_pathname()

This makes easier to change the prefix length.

Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
 server/be-fs.c |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/server/be-fs.c b/server/be-fs.c
index 8f72da5..aa8f008 100644
--- a/server/be-fs.c
+++ b/server/be-fs.c
@@ -204,10 +204,12 @@ static struct fs_obj *fs_obj_alloc(void)
 	return obj;
 }
 
+#define PREFIX_LEN 4
+
 static char *fs_obj_pathname(uint32_t table_id,const void *key, size_t key_len)
 {
 	char *s = NULL;
-	char prefix[5] = "";
+	char prefix[PREFIX_LEN + 1] = "";
 	struct stat st;
 	size_t slen;
 	unsigned char md[SHA256_DIGEST_LENGTH];
@@ -219,7 +221,7 @@ static char *fs_obj_pathname(uint32_t table_id,const void *key, size_t key_len)
 	SHA256(key, key_len, md);
 	hexstr(md, SHA256_DIGEST_LENGTH, mdstr);
 
-	memcpy(prefix, mdstr, 4);
+	memcpy(prefix, mdstr, PREFIX_LEN);
 
 	slen = strlen(chunkd_srv.vol_path) + 1 +	/* volume */
 	       16 +					/* table id */
@@ -253,7 +255,7 @@ static char *fs_obj_pathname(uint32_t table_id,const void *key, size_t key_len)
 	}
 
 	sprintf(s, MDB_TPATH_FMT "/%s/%s", chunkd_srv.vol_path, table_id,
-		prefix, mdstr + 4);
+		prefix, mdstr + PREFIX_LEN);
 
 	return s;
 
@@ -725,7 +727,7 @@ again:
 
 		if (de->d_name[0] == '.')
 			goto again;
-		if (strlen(de->d_name) != 4)
+		if (strlen(de->d_name) != PREFIX_LEN)
 			goto again;
 
 		if (asprintf(&t->sub, "%s/%s", t->table_path, de->d_name) < 0)
-- 
1.6.0.6

[PATCH 2/6] chunkd: change the prefix length of object pathname from 4 to 3

[Akinobu Mita]

The volume directory may have 2^16 (=65536) directories because
prefix length of object pathname is 4 bytes. But ext3 filesystem can
only have EXT_LINK_MAX (32000) directories.

This changes the prefix length from 4 bytes to 3 bytes and breaks
the volume data format compatibility with older version.

Now the maximum count of links in volume directory is 2^12 (=4096)
and it doesn't exceed ext3's upper limit of max links.

Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
 server/be-fs.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/server/be-fs.c b/server/be-fs.c
index aa8f008..512e33f 100644
--- a/server/be-fs.c
+++ b/server/be-fs.c
@@ -204,7 +204,7 @@ static struct fs_obj *fs_obj_alloc(void)
 	return obj;
 }
 
-#define PREFIX_LEN 4
+#define PREFIX_LEN 3
 
 static char *fs_obj_pathname(uint32_t table_id,const void *key, size_t key_len)
 {
-- 
1.6.0.6

Re: [PATCH 2/6] chunkd: change the prefix length of object pathname from 4 to 3

[Jeff Garzik]

On 01/11/2010 07:25 AM, Akinobu Mita wrote:
> The volume directory may have 2^16 (=65536) directories because
> prefix length of object pathname is 4 bytes. But ext3 filesystem can
> only have EXT_LINK_MAX (32000) directories.
>
> This changes the prefix length from 4 bytes to 3 bytes and breaks
> the volume data format compatibility with older version.
>
> Now the maximum count of links in volume directory is 2^12 (=4096)
> and it doesn't exceed ext3's upper limit of max links.
>
> Signed-off-by: Akinobu Mita<akinobu.mita@gmail.com>
> ---
>   server/be-fs.c |    2 +-
>   1 files changed, 1 insertions(+), 1 deletions(-)

hey, welcome, new contributor!

This patch makes sense, but it also raises the question of whether or 
not we should move to a two-level directory scheme, eg.

	123/456/7890ABCDEF
     rather than
	123/4567890ABCDEF

to limit the size of the top-level directories.  It really depends on 
the object counts a typical chunkd node will be seeing.  As with the 
other patch, I will give this some thought after sleep.

Comments welcome!

	Jeff

Re: [PATCH 2/6] chunkd: change the prefix length of object pathname from 4 to 3

[Akinobu Mita]

2010/1/12 Jeff Garzik <jeff@garzik.org>:
> On 01/11/2010 07:25 AM, Akinobu Mita wrote:
>>
>> The volume directory may have 2^16 (=65536) directories because
>> prefix length of object pathname is 4 bytes. But ext3 filesystem can
>> only have EXT_LINK_MAX (32000) directories.
>>
>> This changes the prefix length from 4 bytes to 3 bytes and breaks
>> the volume data format compatibility with older version.
>>
>> Now the maximum count of links in volume directory is 2^12 (=4096)
>> and it doesn't exceed ext3's upper limit of max links.
>>
>> Signed-off-by: Akinobu Mita<akinobu.mita@gmail.com>
>> ---
>>  server/be-fs.c |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> hey, welcome, new contributor!
>
> This patch makes sense, but it also raises the question of whether or not we
> should move to a two-level directory scheme, eg.
>
>        123/456/7890ABCDEF
>    rather than
>        123/4567890ABCDEF
>
> to limit the size of the top-level directories.  It really depends on the
> object counts a typical chunkd node will be seeing.  As with the other
> patch, I will give this some thought after sleep.

Two-level directory scheme looks good.

I will do it unless someone thinks 536,870,912,000(=4096*4096*32000)
objects in one table is not enough :)

Re: [PATCH 2/6] chunkd: change the prefix length of object pathname from 4 to 3

[Jeff Garzik]

On 01/12/2010 03:28 AM, Akinobu Mita wrote:
> 2010/1/12 Jeff Garzik<jeff@garzik.org>:
>> On 01/11/2010 07:25 AM, Akinobu Mita wrote:
>>>
>>> The volume directory may have 2^16 (=65536) directories because
>>> prefix length of object pathname is 4 bytes. But ext3 filesystem can
>>> only have EXT_LINK_MAX (32000) directories.
>>>
>>> This changes the prefix length from 4 bytes to 3 bytes and breaks
>>> the volume data format compatibility with older version.
>>>
>>> Now the maximum count of links in volume directory is 2^12 (=4096)
>>> and it doesn't exceed ext3's upper limit of max links.
>>>
>>> Signed-off-by: Akinobu Mita<akinobu.mita@gmail.com>
>>> ---
>>>   server/be-fs.c |    2 +-
>>>   1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> hey, welcome, new contributor!
>>
>> This patch makes sense, but it also raises the question of whether or not we
>> should move to a two-level directory scheme, eg.
>>
>>         123/456/7890ABCDEF
>>     rather than
>>         123/4567890ABCDEF
>>
>> to limit the size of the top-level directories.  It really depends on the
>> object counts a typical chunkd node will be seeing.  As with the other
>> patch, I will give this some thought after sleep.
>
> Two-level directory scheme looks good.
>
> I will do it unless someone thinks 536,870,912,000(=4096*4096*32000)
> objects in one table is not enough :)

FWIW, 32000 is only the limit on directories-with-a-directory.  You can 
easily have millions of regular files in a single ext3 directory.  So it 
is really 4096*4096*millions.

	Jeff

Re: [PATCH 2/6] chunkd: change the prefix length of object pathname from 4 to 3

[Akinobu Mita]

>>> This patch makes sense, but it also raises the question of whether or not
>>> we
>>> should move to a two-level directory scheme, eg.
>>>
>>>        123/456/7890ABCDEF
>>>    rather than
>>>        123/4567890ABCDEF
>>>
>>> to limit the size of the top-level directories.  It really depends on the
>>> object counts a typical chunkd node will be seeing.  As with the other
>>> patch, I will give this some thought after sleep.
>>
>> Two-level directory scheme looks good.
>>
>> I will do it unless someone thinks 536,870,912,000(=4096*4096*32000)
>> objects in one table is not enough :)
>
> FWIW, 32000 is only the limit on directories-with-a-directory.  You can
> easily have millions of regular files in a single ext3 directory.  So it is
> really 4096*4096*millions.

Oops, how embarrassing... so 1-level directory scheme with 3-bytes prefix
is nealy unlimited in maximum count of objects.

BTW, chunkd cannot have more than 32000 tables on ext3 by the same reason
(EXT3_MAX_LINK). So, should we use two or three-level directory scheme
for table_id in object pathname ?

Re: [PATCH 2/6] chunkd: change the prefix length of object pathname from 4 to 3

[Jeff Garzik]

On 01/12/2010 10:50 PM, Akinobu Mita wrote:
>>>> This patch makes sense, but it also raises the question of whether or not
>>>> we
>>>> should move to a two-level directory scheme, eg.
>>>>
>>>>         123/456/7890ABCDEF
>>>>     rather than
>>>>         123/4567890ABCDEF
>>>>
>>>> to limit the size of the top-level directories.  It really depends on the
>>>> object counts a typical chunkd node will be seeing.  As with the other
>>>> patch, I will give this some thought after sleep.
>>>
>>> Two-level directory scheme looks good.
>>>
>>> I will do it unless someone thinks 536,870,912,000(=4096*4096*32000)
>>> objects in one table is not enough :)
>>
>> FWIW, 32000 is only the limit on directories-with-a-directory.  You can
>> easily have millions of regular files in a single ext3 directory.  So it is
>> really 4096*4096*millions.
>
> Oops, how embarrassing... so 1-level directory scheme with 3-bytes prefix
> is nealy unlimited in maximum count of objects.

Yes.  It mainly becomes a question of balancing lookup costs, at that point:

With a 1-level directory scheme, millions of objects could imply 
prohibitively long directory-lookup times as those directories [although 
super-large directories are better handled in ext3+htree, ext4, btrfs 
and XFS].

On the other hand, a 2-level directory scheme would reduce or eliminate 
the occurrence of large directories, with the cost of having to perform 
many more mkdir(2) calls during object creation.  Additional costs 
include larger dcache footprint, and added fs_list_objs() complexity.


> BTW, chunkd cannot have more than 32000 tables on ext3 by the same reason
> (EXT3_MAX_LINK). So, should we use two or three-level directory scheme
> for table_id in object pathname ?

At this point, I think it is unlikely that people will create more than 
32000 tables on a single server.  If I am wrong, we can eliminate this 
limit at a later date.

	Jeff

[PATCH 3/6] chunkd: Don't return positive value on stc_get_recv failure

[Akinobu Mita]

stc_get_recv may return positive errno value on failure.

Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
 lib/chunkdc.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/lib/chunkdc.c b/lib/chunkdc.c
index 2ed976b..91c42dc 100644
--- a/lib/chunkdc.c
+++ b/lib/chunkdc.c
@@ -431,7 +431,7 @@ size_t stc_get_recv(struct st_client *stc, void *data, size_t data_len)
 			if (done_cnt == data_len)
 				break;
 			if (ioctl(stc->fd, FIONREAD, &avail))
-				return errno;
+				return -errno;
 			if (avail == 0) {
 				if ((avail = SSL_pending(stc->ssl)) == 0)
 					break;
@@ -457,14 +457,14 @@ size_t stc_get_recv(struct st_client *stc, void *data, size_t data_len)
 		}
 	} else {
 		if (ioctl(stc->fd, FIONREAD, &avail))
-			return errno;
+			return -errno;
 		if (avail) {
 			if ((xfer_len = avail) > data_len)
 				xfer_len = data_len;
 
 			rc = read(stc->fd, data, xfer_len);
 			if (rc < 0)
-				return errno;
+				return -errno;
 
 			done_cnt += rc;
 		}
-- 
1.6.0.6

[PATCH 4/6] chcli: Add -i option to read value from file

[Akinobu Mita]

Add -i option to read value from FILE, rather than command line.
This enables to test GET/PUT huge values.

Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
 tools/chcli.c |   92 ++++++++++++++++++++++++++++++++++++++++++++++++---------
 1 files changed, 78 insertions(+), 14 deletions(-)

diff --git a/tools/chcli.c b/tools/chcli.c
index a74b8cb..51ec697 100644
--- a/tools/chcli.c
+++ b/tools/chcli.c
@@ -18,6 +18,7 @@
  */
 
 #define _GNU_SOURCE
+#define _FILE_OFFSET_BITS 64
 #include "chunkd-config.h"
 
 #include <assert.h>
@@ -51,6 +52,8 @@ static struct argp_option options[] = {
 	  "Connect to remote chunkd at specified HOST:PORT" },
 	{ "key", 'k', "FILE", 0,
 	  "Read key from FILE, rather than command line" },
+	{ "input", 'i', "FILE", 0,
+	  "Read value from FILE, rather than command line" },
 	{ "output", 'o', "FILE", 0,
 	  "Send GET output to FILE, rather than stdout" },
 	{ "ssl", 'S', NULL, 0,
@@ -101,6 +104,9 @@ static char *password;
 static char *output_fn;
 static char *key_data;
 static gsize key_data_len;
+static bool key_in_file;
+static char *input_fn;
+static bool value_in_file;
 static char *table_name;
 static size_t table_name_len;
 static bool table_create;
@@ -271,6 +277,11 @@ static error_t parse_opt (int key, char *arg, struct argp_state *state)
 			fprintf(stderr, "failed to read key file %s\n", arg);
 			argp_usage(state);
 		}
+		key_in_file = true;
+		break;
+	case 'i':
+		input_fn = arg;
+		value_in_file = true;
 		break;
 	case 'o':
 		output_fn = arg;
@@ -403,27 +414,74 @@ static int cmd_del(void)
 	return 0;
 }
 
+static size_t read_file_cb(void *ptr, size_t size, size_t nmemb,
+			void *user_data)
+{
+	int *fdp = user_data;
+
+	return read(*fdp, ptr, size * nmemb);
+}
+
+static bool stc_put_file(struct st_client *stc, const void *key, size_t key_len,
+			const char *filename, uint32_t flags)
+{
+	bool rcb;
+	int fd;
+	struct stat stat;
+	off64_t content_len;
+	int rc;
+
+	fd = open(filename, O_RDONLY);
+	if (fd < 0)
+		return false;
+
+	rc = fstat(fd, &stat);
+	if (rc) {
+		close(fd);
+		return false;
+	}
+	content_len = stat.st_size;
+	rcb = stc_put(stc, key, key_len, read_file_cb, content_len, &fd, flags);
+	close(fd);
+
+	return rcb;
+}
+
 static int cmd_put(void)
 {
 	struct st_client *stc;
+	bool rcb;
+	char *value_data;
+	gsize value_data_len;
 
-	/* if key data not supplied via file, absorb first cmd arg */
-	if (!key_data) {
-		if (!n_cmd_args) {
+	if (key_in_file && value_in_file) {
+		if (n_cmd_args) {
+			fprintf(stderr, "PUT invalid arg\n");
+			return 1;
+		}
+	} else if (key_in_file) {
+		if (n_cmd_args != 1) {
+			fprintf(stderr, "PUT requires value arg\n");
+			return 1;
+		}
+		value_data = cmd_args[0];
+		value_data_len = strlen(cmd_args[0]);
+	} else if (value_in_file) {
+		if (n_cmd_args != 1) {
 			fprintf(stderr, "PUT requires key arg\n");
 			return 1;
 		}
-
 		key_data = cmd_args[0];
 		key_data_len = strlen(cmd_args[0]) + 1;
-
-		cmd_args++;
-		n_cmd_args--;
-	}
-
-	if (n_cmd_args != 1) {
-		fprintf(stderr, "PUT requires value arg\n");
-		return 1;
+	} else {
+		if (n_cmd_args != 2) {
+			fprintf(stderr, "PUT requires key arg\n");
+			return 1;
+		}
+		key_data = cmd_args[0];
+		key_data_len = strlen(cmd_args[0]) + 1;
+		value_data = cmd_args[1];
+		value_data_len = strlen(cmd_args[1]);
 	}
 
 	if (key_data_len < 1 || key_data_len > CHD_KEY_SZ) {
@@ -436,8 +494,14 @@ static int cmd_put(void)
 	if (!stc)
 		return 1;
 
-	if (!stc_put_inline(stc, key_data, key_data_len,
-			    cmd_args[0], strlen(cmd_args[0]), 0)) {
+	if (value_in_file) {
+		rcb = stc_put_file(stc, key_data, key_data_len, input_fn, 0);
+	} else {
+		rcb = stc_put_inline(stc, key_data, key_data_len,
+				value_data, value_data_len, 0);
+	}
+
+	if (!rcb) {
 		fprintf(stderr, "PUT failed\n");
 		return 1;
 	}
-- 
1.6.0.6

[PATCH 5/6] chcli: fix error handling for write()

[Akinobu Mita]

The number of bytes written may be less than the count bytes
specified by write(2) call.

Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
 tools/chcli.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/tools/chcli.c b/tools/chcli.c
index 51ec697..2c9ac23 100644
--- a/tools/chcli.c
+++ b/tools/chcli.c
@@ -595,6 +595,7 @@ static int cmd_get(void)
 
 	while (get_len > 0) {
 		size_t need_len;
+		ssize_t rc;
 
 		need_len = MIN(GET_BUFSZ, get_len);
 
@@ -603,14 +604,15 @@ static int cmd_get(void)
 			return 1;
 		}
 
-		if (write(wfd, get_buf, need_len) != need_len) {
+		rc = write(wfd, get_buf, need_len);
+		if (rc < 0) {
 			fprintf(stderr, "GET write to output failed: %s\n",
 				strerror(errno));
 			unlink(output_fn);
 			return 1;
 		}
 
-		get_len -= need_len;
+		get_len -= rc;
 	}
 
 	if (wfd != STDOUT_FILENO)
-- 
1.6.0.6

[PATCH 6/6] chunkd: fix PUT/GET for more than 2GB values

[Akinobu Mita]

- Define _FILE_OFFSET_BITS

- Use uint64_t to hold object length and buffer length

- Don't pass too much buffer to writev. writev returns -1 if the sum of
  the iov_len values overflows an ssize_t value

Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
 server/be-fs.c  |    1 +
 server/chunkd.h |    6 +++---
 server/object.c |    2 +-
 server/server.c |    9 ++++++++-
 4 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/server/be-fs.c b/server/be-fs.c
index 512e33f..a2b23d3 100644
--- a/server/be-fs.c
+++ b/server/be-fs.c
@@ -18,6 +18,7 @@
  */
 
 #define _GNU_SOURCE
+#define _FILE_OFFSET_BITS 64
 #include "chunkd-config.h"
 
 #include <sys/types.h>
diff --git a/server/chunkd.h b/server/chunkd.h
index a97088d..9b3eeb9 100644
--- a/server/chunkd.h
+++ b/server/chunkd.h
@@ -67,7 +67,7 @@ struct timer {
 
 struct client_write {
 	const void		*buf;		/* write buffer */
-	int			len;		/* write buffer length */
+	uint64_t		len;		/* write buffer length */
 	cli_write_func		cb;		/* callback */
 	void			*cb_data;	/* data passed to cb */
 	bool			sendfile;	/* using sendfile? */
@@ -115,11 +115,11 @@ struct client {
 
 	char			*out_user;
 	SHA_CTX			out_hash;
-	long			out_len;
+	uint64_t		out_len;
 
 	struct backend_obj	*out_bo;
 
-	long			in_len;
+	uint64_t		in_len;
 	struct backend_obj	*in_obj;
 
 	/* we put the big arrays and objects at the end... */
diff --git a/server/object.c b/server/object.c
index a1205f5..ad1d98a 100644
--- a/server/object.c
+++ b/server/object.c
@@ -140,7 +140,7 @@ bool cli_evt_data_in(struct client *cli, unsigned int events)
 	read_sz = MIN(cli->out_len, CLI_DATA_BUF_SZ);
 
 	if (debugging)
-		applog(LOG_DEBUG, "REQ(data-in) seq %x, out_len %ld, read_sz %u",
+		applog(LOG_DEBUG, "REQ(data-in) seq %x, out_len %llu, read_sz %u",
 		       cli->creq.nonce, cli->out_len, read_sz);
 
 	if (cli->ssl) {
diff --git a/server/server.c b/server/server.c
index 3f38cca..b04178b 100644
--- a/server/server.c
+++ b/server/server.c
@@ -413,14 +413,21 @@ static int cli_wr_iov(struct client *cli, struct iovec *iov, int max_iov)
 {
 	struct client_write *tmp;
 	int n_iov = 0;
+	ssize_t total =  0;
 
 	/* accumulate pending writes into iovec */
 	list_for_each_entry(tmp, &cli->write_q, node) {
 		if (n_iov >= max_iov)
 			break;
 
+		if (tmp->len > (sizeof(ssize_t) == 8 ? LONG_MAX : INT_MAX))
+			break;
+		if (total + tmp->len < total)
+			break;
+
 		iov[n_iov].iov_base = (void *) tmp->buf;
 		iov[n_iov].iov_len = tmp->len;
+		total += tmp->len;
 
 		n_iov++;
 	}
@@ -436,7 +443,7 @@ static void cli_wr_completed(struct client *cli, ssize_t rc, bool *more_work)
 	 * amount of data written
 	 */
 	while (rc > 0) {
-		int sz;
+		ssize_t sz;
 
 		/* get pointer to first record on list */
 		tmp = list_entry(cli->write_q.next, struct client_write, node);
-- 
1.6.0.6

Re: [PATCH 6/6] chunkd: fix PUT/GET for more than 2GB values

[Jeff Garzik]

On 01/11/2010 07:25 AM, Akinobu Mita wrote:
> - Define _FILE_OFFSET_BITS
>
> - Use uint64_t to hold object length and buffer length
>
> - Don't pass too much buffer to writev. writev returns -1 if the sum of
>    the iov_len values overflows an ssize_t value
>
> Signed-off-by: Akinobu Mita<akinobu.mita@gmail.com>
> ---
>   server/be-fs.c  |    1 +
>   server/chunkd.h |    6 +++---
>   server/object.c |    2 +-
>   server/server.c |    9 ++++++++-
>   4 files changed, 13 insertions(+), 5 deletions(-)

applied

Re: [PATCH 1/6] chunkd: Don't hardcode the prefix length of object pathname

[Jeff Garzik]

On 01/11/2010 07:25 AM, Akinobu Mita wrote:
> Define PREFIX_LEN instead of hardcoded prefix length of object pathname
> in fs_obj_pathname()
>
> This makes easier to change the prefix length.
>
> Signed-off-by: Akinobu Mita<akinobu.mita@gmail.com>

applied 1-5