[Sheepdog] [PATCH v4 1/2] journel: move data commiting out of jrnl_perform()

[MORITA Kazutaka]

At Thu, 17 Nov 2011 22:26:47 +0800,
Liu Yuan wrote:
> 
> From: Liu Yuan <tailai.ly at taobao.com>
> 
> Let jrnl_perform just concentrate on journeling stuff, not intrude in store IO.
> This would make store IO interface abstracting easier and cleaner.
> 
> Signed-off-by: Liu Yuan <tailai.ly at taobao.com>
> ---
>  sheep/journal.c    |   70 ++++++++++++++++------------------------
>  sheep/sheep_priv.h |    5 ++-
>  sheep/store.c      |   89 ++++++++++++++++++++++++++++++++++++++--------------
>  3 files changed, 96 insertions(+), 68 deletions(-)
> 
> diff --git a/sheep/journal.c b/sheep/journal.c
> index beff25c..790ebb6 100644
> --- a/sheep/journal.c
> +++ b/sheep/journal.c
> @@ -189,68 +189,54 @@ static int jrnl_apply_to_target_object(struct jrnl_descriptor *jd)
>  	return res;
>  }
>  
> -static int jrnl_commit_data(struct jrnl_descriptor *jd)
> -{
> -	int ret = 0;
> -	ssize_t retsize;
> -	struct jrnl_head *head = (struct jrnl_head *) &jd->head;
> -
> -	retsize = pwrite64(jd->target_fd, jd->data, head->size, head->offset);
> -	if (retsize != head->size) {
> -		if (errno == ENOSPC)
> -			ret = SD_RES_NO_SPACE;
> -		else
> -			ret = SD_RES_EIO;
> -	}
> -
> -	return ret;
> -}
> -
>  /*
>   * We cannot use this function for concurrent write operations
>   */
> -int jrnl_perform(int fd, void *buf, size_t count, off_t offset,
> +struct jrnl_descriptor *jrnl_begin(void *buf, size_t count, off_t offset,
>  		 const char *path, const char *jrnl_dir)
>  {
>  	int ret;
> -	struct jrnl_descriptor jd;
> -
> -	memset(&jd, 0, sizeof(jd));
> -	jd.target_fd = fd;
> +	struct jrnl_descriptor *jd = xzalloc(sizeof(*jd));
>  
> -	jd.head.offset = offset;
> -	jd.head.size = count;
> -	strcpy(jd.head.target_path, path);
> +	jd->head.offset = offset;
> +	jd->head.size = count;
> +	strcpy(jd->head.target_path, path);
>  
> -	jd.data = buf;
> +	jd->data = buf;
>  
> -	ret = jrnl_create(&jd, jrnl_dir);
> +	ret = jrnl_create(jd, jrnl_dir);
>  	if (ret)
> -		goto out;
> +		goto err;
>  
> -	ret = jrnl_write_header(&jd);
> +	ret = jrnl_write_header(jd);
>  	if (ret)
> -		goto out;
> +		goto err;
>  
> -	ret = jrnl_write_data(&jd);
> +	ret = jrnl_write_data(jd);
>  	if (ret)
> -		goto out;
> +		goto err;
>  
> -	ret = jrnl_write_end_mark(&jd);
> +	ret = jrnl_write_end_mark(jd);
>  	if (ret)
> -		goto out;
> +		goto err;
> +	return jd;
> +err:
> +	free(jd);
> +	return NULL;
> +}
>  
> -	ret = jrnl_commit_data(&jd);
> -	if (ret)
> -		goto out;
> +int jrnl_end(struct jrnl_descriptor * jd)
> +{
> +	int ret = 0;
> +	if (!jd)
> +		return ret;
>  
> -	ret = jrnl_close(&jd);
> +	ret = jrnl_close(jd);
>  	if (ret)
> -		goto out;
> -
> -	ret = jrnl_remove(&jd);
> +		goto err;
>  
> -out:
> +	ret = jrnl_remove(jd);
> +err:
>  	return ret;
>  }
>  
> diff --git a/sheep/sheep_priv.h b/sheep/sheep_priv.h
> index 56bcbd1..cbacab3 100644
> --- a/sheep/sheep_priv.h
> +++ b/sheep/sheep_priv.h
> @@ -260,8 +260,9 @@ int do_process_main(struct sd_op_template *op, const struct sd_req *req,
>  		    struct sd_rsp *rsp, void *data);
>  
>  /* Journal */
> -int jrnl_perform(int fd, void *buf, size_t count, off_t offset,
> -		 const char *path, const char *jrnl_dir);
> +struct jrnl_descriptor *jrnl_begin(void *buf, size_t count, off_t offset,
> +				   const char *path, const char *jrnl_dir);
> +int jrnl_end(struct jrnl_descriptor * jd);
>  int jrnl_recover(const char *jrnl_dir);
>  
>  static inline int is_myself(uint8_t *addr, uint16_t port)
> diff --git a/sheep/store.c b/sheep/store.c
> index c142bc3..f022d55 100644
> --- a/sheep/store.c
> +++ b/sheep/store.c
> @@ -624,6 +624,7 @@ static int store_write_obj_fd(int fd, struct request *req, uint32_t epoch)
>  	struct sd_obj_req *hdr = (struct sd_obj_req *)&req->rq;
>  	uint64_t oid = hdr->oid;
>  	int ret = SD_RES_SUCCESS;
> +	void *jd = NULL;
>  
>  	if (hdr->flags & SD_FLAG_CMD_TRUNCATE) {
>  		ret = ftruncate(fd, hdr->offset + hdr->data_length);
> @@ -638,10 +639,19 @@ static int store_write_obj_fd(int fd, struct request *req, uint32_t epoch)
>  
>  		snprintf(path, sizeof(path), "%s%08u/%016" PRIx64, obj_path,
>  			 epoch, oid);
> -		ret = jrnl_perform(fd, req->data, hdr->data_length,
> +		jd = jrnl_begin(req->data, hdr->data_length,
>  				   hdr->offset, path, jrnl_path);
> -		if (ret)
> -			return ret;
> +		if (!jd)
> +			return SD_RES_EIO;
> +		ret = pwrite64(fd, req->data, hdr->data_length, hdr->offset);
> +		if (ret != hdr->data_length) {
> +			if (errno == ENOSPC)
> +				return SD_RES_NO_SPACE;
> +			eprintf("%m\n");
> +			jrnl_end(jd);
> +			return SD_RES_EIO;
> +		}
> +		jrnl_end(jd);
>  	} else {
>  		ret = pwrite64(fd, req->data, hdr->data_length, hdr->offset);
>  		if (ret != hdr->data_length) {
> @@ -651,7 +661,6 @@ static int store_write_obj_fd(int fd, struct request *req, uint32_t epoch)
>  			return SD_RES_EIO;
>  		}
>  	}
> -
>  	return SD_RES_SUCCESS;
>  }
>  
> @@ -1090,20 +1099,30 @@ int remove_epoch(int epoch)
>  int set_cluster_ctime(uint64_t ct)
>  {
>  	int fd, ret;
> +	void *jd;
>  
>  	fd = open(config_path, O_DSYNC | O_WRONLY);
>  	if (fd < 0)
> -		return -1;
> +		return SD_RES_EIO;
>  
> -	ret = jrnl_perform(fd, &ct, sizeof(ct),
> -			   offsetof(struct sheepdog_config, ctime),
> -			   config_path, jrnl_path);
> +	jd = jrnl_begin(&ct, sizeof(ct),
> +			offsetof(struct sheepdog_config, ctime),
> +			config_path, jrnl_path);
> +	if (!jd) {
> +		ret = SD_RES_EIO;
> +		goto err;
> +	}
> +	ret = pwrite64(fd, &ct, sizeof(ct), offsetof(struct sheepdog_config, ctime));
> +	if (ret != sizeof(ct)) {
> +		ret = SD_RES_EIO;
> +		jrnl_end(jd);
> +		goto err;
> +	}
> +	ret = SD_RES_SUCCESS;
> +	jrnl_end(jd);
> +err:

The following code may be a bit cleaner.

==
	if (ret != sizeof(ct))
		ret = SD_RES_EIO;
	else
		ret = SD_RES_SUCCESS;

	jrnl_end(jd);
==

>  	close(fd);
> -
> -	if (ret)
> -		return -1;
> -
> -	return 0;
> +	return ret;
>  }
>  
>  uint64_t get_cluster_ctime(void)
> @@ -2125,20 +2144,31 @@ int read_epoch(uint32_t *epoch, uint64_t *ct,
>  int set_cluster_copies(uint8_t copies)
>  {
>  	int fd, ret;
> +	void *jd;
>  
>  	fd = open(config_path, O_DSYNC | O_WRONLY);
>  	if (fd < 0)
>  		return SD_RES_EIO;
>  
> -	ret = jrnl_perform(fd, &copies, sizeof(copies),
> -			   offsetof(struct sheepdog_config, copies),
> -			   config_path, jrnl_path);
> -	close(fd);
> -
> -	if (ret != 0)
> -		return SD_RES_EIO;
> +	jd = jrnl_begin(&copies, sizeof(copies),
> +			offsetof(struct sheepdog_config, copies),
> +			config_path, jrnl_path);
> +	if (!jd) {
> +		ret = SD_RES_EIO;
> +		goto err;
> +	}
>  
> -	return SD_RES_SUCCESS;
> +	ret = pwrite64(fd, &copies, sizeof(copies), offsetof(struct sheepdog_config, copies));
> +	if (ret != sizeof(copies)) {
> +		ret = SD_RES_EIO;
> +		jrnl_end(jd);
> +		goto err;
> +	}
> +	jrnl_end(jd);
> +	ret = SD_RES_SUCCESS;
> +err:

Same here.

> +	close(fd);
> +	return ret;
>  }
>  
>  int get_cluster_copies(uint8_t *copies)
> @@ -2162,17 +2192,28 @@ int get_cluster_copies(uint8_t *copies)
>  int set_cluster_flags(uint16_t flags)
>  {
>  	int fd, ret = SD_RES_EIO;
> +	void *jd;
>  
>  	fd = open(config_path, O_DSYNC | O_WRONLY);
>  	if (fd < 0)
>  		goto out;
>  
> -	ret = jrnl_perform(fd, &flags, sizeof(flags),
> +	jd = jrnl_begin(&flags, sizeof(flags),
>  			offsetof(struct sheepdog_config, flags),
>  			config_path, jrnl_path);
> -	if (ret != 0)
> +	if (!jd) {
>  		ret = SD_RES_EIO;
> -
> +		goto err;
> +	}
> +	ret = pwrite64(fd, &flags, sizeof(flags), offsetof(struct sheepdog_config, flags));
> +	if (ret != sizeof(flags)) {
> +		ret = SD_RES_EIO;
> +		jrnl_end(jd);
> +		goto err;
> +	}
> +	jrnl_end(jd);
> +	ret = SD_RES_SUCCESS;
> +err:

And here.

Thanks,

Kazutaka