[Sheepdog] [PATCH 1/2] store: split store_queue_request_local

Liu Yuan namei.unix at gmail.com
Fri Nov 11 14:04:02 CET 2011


On 11/11/2011 08:49 PM, Christoph Hellwig wrote:

> Split store_queue_request_local into one function for each command.  While
> this leads to a small amount of duplication it keeps the code nicely
> separated and helps with adding new commands.
> 
> Signed-off-by: Christoph Hellwig <hch at lst.de>
> 
> Index: sheepdog/sheep/store.c
> ===================================================================
> --- sheepdog.orig/sheep/store.c	2011-11-11 13:30:05.000000000 +0100
> +++ sheepdog/sheep/store.c	2011-11-11 13:33:37.650025624 +0100
> @@ -574,163 +574,227 @@ int read_object_local(uint64_t oid, char
>  	return rsp_data_length;
>  }
>  
> -static int store_queue_request_local(struct request *req, uint32_t epoch)
> +static int store_remove_obj(struct request *req, uint32_t epoch)
> +{
> +	struct sd_obj_req *hdr = (struct sd_obj_req *)&req->rq;
> +	char path[1024];
> +
> +	snprintf(path, sizeof(path), "%s%08u/%016" PRIx64, obj_path,
> +		 epoch, hdr->oid);
> +
> +	if (unlink(path) < 0) {
> +		if (errno == ENOENT)
> +			return SD_RES_NO_OBJ;
> +		eprintf("%m\n");
> +		return SD_RES_EIO;
> +	}
> +
> +	return SD_RES_SUCCESS;
> +}
> +
> +static int store_read_obj(struct request *req, uint32_t epoch)
>  {
> -	int fd = -1;
> -	int ret = SD_RES_SUCCESS;
>  	struct sd_obj_req *hdr = (struct sd_obj_req *)&req->rq;
>  	struct sd_obj_rsp *rsp = (struct sd_obj_rsp *)&req->rp;
> -	uint64_t oid = hdr->oid;
> -	uint32_t opcode = hdr->opcode;
> -	char path[1024], *buf = NULL;
> +	int ret = SD_RES_SUCCESS;
> +	int fd = -1;
>  
> -	dprintf("%x, %" PRIx64" , %u\n", opcode, oid, epoch);
> +	fd = ob_open(epoch, hdr->oid, 0, &ret);
> +	if (fd < 0)
> +		return ret;
> +
> +	ret = pread64(fd, req->data, hdr->data_length, hdr->offset);
> +	if (ret < 0) {
> +		eprintf("%m\n");
> +		ret = SD_RES_EIO;
> +		goto out;
> +	}
>  
> -	switch (opcode) {
> -	case SD_OP_WRITE_OBJ:
> -	case SD_OP_READ_OBJ:
> -		fd = ob_open(epoch, oid, 0, &ret);
> -		if (fd < 0)
> -			goto out;
> -		break;
> -	case SD_OP_CREATE_AND_WRITE_OBJ:
> -		if (!hdr->copies) {
> -			eprintf("the number of copies cannot be zero\n");
> -			ret = SD_RES_INVALID_PARMS;
> -			goto out;
> -		}
> +	rsp->data_length = ret;
> +	rsp->copies = sys->nr_sobjs;
>  
> -		fd = ob_open(epoch, oid, O_CREAT|O_TRUNC, &ret);
> -		if (fd < 0)
> -			goto out;
> -
> -		if (hdr->flags & SD_FLAG_CMD_COW) {
> -			dprintf("%" PRIu64 ", %" PRIx64 "\n", oid, hdr->cow_oid);
> -
> -			buf = valloc(SD_DATA_OBJ_SIZE);
> -			if (!buf) {
> -				eprintf("failed to allocate memory\n");
> -				ret = SD_RES_NO_MEM;
> -				goto out;
> -			}
> -			ret = read_from_other_sheep(req, hdr->epoch, hdr->cow_oid, buf,
> -						     hdr->copies);
> -			if (ret) {
> -				eprintf("failed to read old object\n");
> -				ret = SD_RES_EIO;
> -				goto out;
> -			}
> -			ret = pwrite64(fd, buf, SD_DATA_OBJ_SIZE, 0);
> -			if (ret != SD_DATA_OBJ_SIZE) {
> -				if (errno == ENOSPC)
> -					ret = SD_RES_NO_SPACE;
> -				else {
> -					eprintf("%m\n");
> -					ret = SD_RES_EIO;
> -				}
> -				goto out;
> -			}
> -			free(buf);
> -			buf = NULL;
> -		} else {
> -			int size = SECTOR_SIZE;
> -			buf = valloc(size);
> -			if (!buf) {
> -				eprintf("failed to allocate memory\n");
> -				ret = SD_RES_NO_MEM;
> -				goto out;
> -			}
> -			memset(buf, 0, size);
> -			ret = pwrite64(fd, buf, size, SD_DATA_OBJ_SIZE - size);
> -			free(buf);
> -			buf = NULL;
> -
> -			if (ret != size) {
> -				if (errno == ENOSPC)
> -					ret = SD_RES_NO_SPACE;
> -				else {
> -					eprintf("%m\n");
> -					ret = SD_RES_EIO;
> -				}
> -				goto out;
> -			}
> -		}
> +	ret = SD_RES_SUCCESS;
> +out:
> +	close(fd);
> +	return ret;
> +}
>  
> -	default:
> -		break;
> +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;
> +
> +	if (hdr->flags & SD_FLAG_CMD_TRUNCATE) {
> +		ret = ftruncate(fd, hdr->offset + hdr->data_length);
> +		if (ret) {
> +			eprintf("%m\n");
> +			return SD_RES_EIO;
> +		}
>  	}
>  
> -	switch (opcode) {
> -	case SD_OP_REMOVE_OBJ:
> +	if (is_vdi_obj(oid)) {
> +		char path[1024];
> +
>  		snprintf(path, sizeof(path), "%s%08u/%016" PRIx64, obj_path,
>  			 epoch, oid);
> -		ret = unlink(path);
> -		if (ret) {
> -			if (errno == ENOENT)
> -				ret = SD_RES_NO_OBJ;
> -			else {
> -				eprintf("%m\n");
> -				ret = SD_RES_EIO;
> -			}
> -		}
> -		break;
> -	case SD_OP_READ_OBJ:
> -		ret = pread64(fd, req->data, hdr->data_length, hdr->offset);
> -		if (ret < 0) {
> +		ret = jrnl_perform(fd, req->data, hdr->data_length,
> +				   hdr->offset, path, jrnl_path);
> +		if (ret)
> +			return ret;
> +	} else {
> +		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");
> -			ret = SD_RES_EIO;
> -			goto out;
> +			return SD_RES_EIO;
>  		}
> +	}
>  
> -		rsp->data_length = ret;
> -		rsp->copies = sys->nr_sobjs;
> +	return SD_RES_SUCCESS;
> +}
>  
> -		ret = SD_RES_SUCCESS;
> -		break;
> -	case SD_OP_WRITE_OBJ:
> -	case SD_OP_CREATE_AND_WRITE_OBJ:
> -		if (hdr->flags & SD_FLAG_CMD_TRUNCATE) {
> -			ret = ftruncate(fd, hdr->offset + hdr->data_length);
> -			if (ret) {
> -				eprintf("%m\n");
> -				ret = SD_RES_EIO;
> -				goto out;
> -			}
> -		}
> +static int store_write_obj(struct request *req, uint32_t epoch)
> +{
> +	struct sd_obj_req *hdr = (struct sd_obj_req *)&req->rq;
> +	int ret = SD_RES_SUCCESS;
> +	int fd = -1;
>  
> -		if (is_vdi_obj(oid)) {
> -			snprintf(path, sizeof(path), "%s%08u/%016" PRIx64, obj_path,
> -				 epoch, oid);
> -			ret = jrnl_perform(fd, req->data, hdr->data_length,
> -					   hdr->offset, path, jrnl_path);
> -			if (ret)
> -				goto out;
> -		} else {
> -			ret = pwrite64(fd, req->data, hdr->data_length,
> -				       hdr->offset);
> -			if (ret != hdr->data_length) {
> -				if (errno == ENOSPC)
> -					ret = SD_RES_NO_SPACE;
> -				else {
> -					eprintf("%m\n");
> -					ret = SD_RES_EIO;
> -				}
> -				goto out;
> -			}
> -		}
> +	fd = ob_open(epoch, hdr->oid, 0, &ret);
> +	if (fd < 0)
> +		return ret;
>  
> -		ret = SD_RES_SUCCESS;
> -		break;
> +	ret = store_write_obj_fd(fd, req, epoch);
> +
> +	close(fd);
> +	return ret;
> +}
> +
> +static int store_create_and_write_obj_cow(struct request *req, uint32_t epoch)
> +{
> +	struct sd_obj_req *hdr = (struct sd_obj_req *)&req->rq;
> +	int ret = SD_RES_SUCCESS;
> +	int fd = -1;
> +	char *buf = NULL;
> +
> +	if (!hdr->copies) {
> +		eprintf("the number of copies cannot be zero\n");
> +		return SD_RES_INVALID_PARMS;
> +	}
> +
> +	fd = ob_open(epoch, hdr->oid, O_CREAT|O_TRUNC, &ret);
> +	if (fd < 0)
> +		return ret;
> +
> +	dprintf("%" PRIu64 ", %" PRIx64 "\n", hdr->oid, hdr->cow_oid);
> +
> +	buf = valloc(SD_DATA_OBJ_SIZE);
> +	if (!buf) {
> +		eprintf("failed to allocate memory\n");
> +		ret = SD_RES_NO_MEM;
> +		goto out;
> +	}
> +	ret = read_from_other_sheep(req, hdr->epoch, hdr->cow_oid, buf,
> +				     hdr->copies);
> +	if (ret) {
> +		eprintf("failed to read old object\n");
> +		ret = SD_RES_EIO;
> +		goto out;
>  	}
> +	ret = pwrite64(fd, buf, SD_DATA_OBJ_SIZE, 0);
> +	if (ret != SD_DATA_OBJ_SIZE) {
> +		if (errno == ENOSPC)
> +			ret = SD_RES_NO_SPACE;
> +		else {
> +			eprintf("%m\n");
> +			ret = SD_RES_EIO;
> +		}
> +		goto out;
> +	}
> +	free(buf);
> +	buf = NULL;
> +
> +	ret = store_write_obj_fd(fd, req, epoch);
>  out:
>  	if (buf)
>  		free(buf);
> +	close(fd);
> +	return ret;
> +}
> +
> +
> +static int store_create_and_write_obj(struct request *req, uint32_t epoch)
> +{
> +	struct sd_obj_req *hdr = (struct sd_obj_req *)&req->rq;
> +	const int size = SECTOR_SIZE;
> +	int ret = SD_RES_SUCCESS;
> +	int fd = -1;
> +	char *buf = NULL;
> +
> +	if (!hdr->copies) {
> +		eprintf("the number of copies cannot be zero\n");
> +		return SD_RES_INVALID_PARMS;
> +	}
> +
> +	fd = ob_open(epoch, hdr->oid, O_CREAT|O_TRUNC, &ret);
> +	if (fd < 0)
> +		return ret;
> +
> +	buf = valloc(size);
> +	if (!buf) {
> +		eprintf("failed to allocate memory\n");
> +		ret = SD_RES_NO_MEM;
> +		goto out;
> +	}
> +	memset(buf, 0, size);
> +	ret = pwrite64(fd, buf, size, SD_DATA_OBJ_SIZE - size);
> +	free(buf);
> +
> +	if (ret != size) {
> +		if (errno == ENOSPC)
> +			ret = SD_RES_NO_SPACE;
> +		else {
> +			eprintf("%m\n");
> +			ret = SD_RES_EIO;
> +		}
> +		goto out;
> +	}
>  
> -	if (fd != -1)
> -		close(fd);
> +	ret = store_write_obj_fd(fd, req, epoch);
> +out:
> +	close(fd);
> +	return ret;
> +}
> +
> +static int store_queue_request_local(struct request *req, uint32_t epoch)
> +{
> +	struct sd_obj_req *hdr = (struct sd_obj_req *)&req->rq;
> +	int ret = SD_RES_SUCCESS;
> +
> +	dprintf("%x, %" PRIx64" , %u\n", hdr->opcode, hdr->oid, epoch);
> +
> +	switch (hdr->opcode) {
> +	case SD_OP_WRITE_OBJ:
> +		ret = store_write_obj(req, epoch);
> +		break;
> +	case SD_OP_REMOVE_OBJ:
> +		ret = store_remove_obj(req, epoch);
> +		break;
> +	case SD_OP_READ_OBJ:
> +		ret = store_read_obj(req, epoch);
> +		break;
> +	case SD_OP_CREATE_AND_WRITE_OBJ:
> +		if (hdr->flags & SD_FLAG_CMD_COW)
> +			ret = store_create_and_write_obj_cow(req, epoch);
> +		else
> +			ret = store_create_and_write_obj(req, epoch);
> +		break;
> +	}
>  
>  	if (ret == SD_RES_NO_OBJ && hdr->flags & SD_FLAG_CMD_RECOVERY) {
> -		int len  = epoch_log_read(epoch - 1, req->data, hdr->data_length);
> +		struct sd_obj_rsp *rsp = (struct sd_obj_rsp *)&req->rp;
> +		int len = epoch_log_read(epoch - 1, req->data, hdr->data_length);
>  		if (len < 0)
>  			len = 0;
>  		rsp->data_length = len;


Since we'er already here with the messy IO code, how about further
cleaning up the code by replace the switch case with function array that
is indexed by hdr->opcode? Would be even nicer if you try to re-factor
the IO logic.

Thanks,
Yuan



More information about the sheepdog mailing list