[sheepdog] [PATCH 5/6] sheep: refactor vdi lookup, create, delete operation

[MORITA Kazutaka]

> diff --git a/sheep/ops.c b/sheep/ops.c
> index 3e0a9d3..5526ca8 100644
> --- a/sheep/ops.c
> +++ b/sheep/ops.c
> @@ -92,21 +92,21 @@ static int cluster_new_vdi(struct request *req)
>  {
>  	const struct sd_req *hdr = &req->rq;
>  	struct sd_rsp *rsp = &req->rp;
> -	uint32_t vid = 0;
> -	struct vdi_iocb iocb;
> +	uint32_t vid;
> +	struct vdi_iocb iocb = {};
>  	int ret;
>  
> +	if (hdr->data_length != SD_MAX_VDI_LEN)
> +		return SD_RES_INVALID_PARMS;
> +
>  	iocb.name = req->data;
>  	iocb.data_len = hdr->data_length;
>  	iocb.size = hdr->vdi.vdi_size;
>  	iocb.base_vid = hdr->vdi.base_vdi_id;
>  	iocb.create_snapshot = !!hdr->vdi.snapid;
> -	iocb.nr_copies = hdr->vdi.copies;
> +	iocb.nr_copies = hdr->vdi.copies ? hdr->vdi.copies : sys->nr_copies;

Initializing iocb as follows looks cleaner?

struct vdi_iocb iocb = {
       .name = req->data,
       .data_len = hdr->data_length,
       ...
}

> diff --git a/sheep/vdi.c b/sheep/vdi.c
> index 33a5943..8ec7b9b 100644
> --- a/sheep/vdi.c
> +++ b/sheep/vdi.c
> @@ -165,6 +165,13 @@ int fill_vdi_copy_list(void *data)
>  	return nr * sizeof(*vc);
>  }
>  
> +static inline bool is_deleted(struct sheepdog_inode *inode)

I think the function name should be 'vdi_is_deleted' or
'sd_vdi_is_deleted'.

> +{
> +	if (*inode->name == '\0')
> +		return true;
> +	return false;
> +}
> +

return *inode->name == '\0'; looks simpler.


>  int vdi_exist(uint32_t vid)
>  {
>  	struct sheepdog_inode *inode;
> @@ -182,7 +189,7 @@ int vdi_exist(uint32_t vid)
>  		goto out;
>  	}
>  
> -	if (*inode->name == '\0') {
> +	if (is_deleted(inode)) {
>  		ret = 0;
>  		goto out;
>  	}
> @@ -299,131 +306,159 @@ out:
>  	return ret;
>  }
>  
> -static int find_first_vdi(unsigned long start, unsigned long end,
> -			  const char *name, const char *tag, uint32_t snapid,
> -			  uint32_t *vid, unsigned long *deleted_nr,
> -			  uint32_t *next_snap, uint64_t *create_time)
> +/*
> + * Return SUCCESS (range of bits set):
> + * Iff we get a bitmap range [left, right) that VDI might be set between. if
> + * right < start, this means a wrap around case where we should examine the
> + * two split ranges, [left, SD_NR_VDIS - 1] and [0, right). 'Right' is the free
> + * bit that might be used by newly created VDI.
> + *
> + * Otherwise:
> + * Return NO_VDI (bit not set) or FULL_VDI (bitmap fully set)
> + */
> +static int get_vdi_bitmap_range(const char *name, unsigned long *left,
> +				unsigned long *right)
>  {
> -	struct sheepdog_inode *inode = NULL;
> -	unsigned long i;
> -	int ret = SD_RES_NO_MEM;
> +	*left = fnv_64a_buf(name, strlen(name), FNV1A_64_INIT) &
> +		(SD_NR_VDIS - 1);

I think we should add a helper function to calculate the first vdi id.
cluster_get_vdi_attr() also has the same calculation.

> +	*right = find_next_zero_bit(sys->vdi_inuse, SD_NR_VDIS, *left);
> +	if (*left == *right)
> +		return SD_RES_NO_VDI;
> +
> +	if (*right == SD_NR_VDIS) {
> +		/* Wrap around */
> +		*right = find_next_zero_bit(sys->vdi_inuse, SD_NR_VDIS, 0);
> +		if (*right == SD_NR_VDIS)
> +			return SD_RES_FULL_VDI;
> +	}
> +	return SD_RES_SUCCESS;
> +}
> +
> +static inline bool is_snapshot(struct sheepdog_inode *inode)
> +{
> +	return !!inode->snap_ctime;
> +}

'vdi_is_snapshot' is better?

> +
> +static inline bool has_tag(struct vdi_iocb *iocb)

vdi_has_tag is better?

> +{
> +	if ((iocb->tag && iocb->tag[0]) || iocb->snapid)
> +		return true;
> +	return false;
> +}

return ((iocb->tag && iocb->tag[0]) || iocb->snapid) looks simpler.

> +
> +static inline bool tag_match(struct vdi_iocb *iocb,
> +			     struct sheepdog_inode *inode)
> +{
> +	const char *tag = iocb->tag;
> +
> +	if (inode->tag[0] && !strncmp(inode->tag, tag, sizeof(inode->tag)))
> +		return true;
> +	if (iocb->snapid == inode->snap_id)
> +		return true;
> +	return false;
> +}

vdi_tag_match is better?

> +
> +static int fill_vdi_info_range(uint32_t left, uint32_t right,
> +			      struct vdi_iocb *iocb, struct vdi_info *info)
> +{
> +	struct sheepdog_inode *inode;
>  	bool vdi_found = false;
> -	int nr_copies;
> +	int nr_copies, ret;
> +	uint32_t i;
> +	const char *name = iocb->name;
>  
>  	inode = xmalloc(SD_INODE_HEADER_SIZE);
> -	for (i = start; i >= end; i--) {
> +	for (i = right - 1; i >= left; i--) {
>  		nr_copies = get_vdi_copy_number(i);
>  		ret = read_object(vid_to_vdi_oid(i), (char *)inode,
>  				  SD_INODE_HEADER_SIZE, 0, nr_copies);
> -		if (ret != SD_RES_SUCCESS) {
> -			ret = SD_RES_EIO;
> -			goto out_free_inode;
> -		}
> +		if (ret != SD_RES_SUCCESS)
> +			goto out;
>  
> -		if (inode->name[0] == '\0') {
> -			*deleted_nr = i;
> -			continue; /* deleted */
> +		if (is_deleted(inode)) {
> +			/* Recycle the deleted inode for fresh vdi create */
> +			if (!iocb->create_snapshot)
> +				info->free_bit = i;
> +			continue;
>  		}
>  
>  		if (!strncmp(inode->name, name, strlen(inode->name))) {
> -			*next_snap = inode->snap_id + 1;
> -
> -			if (!(tag && tag[0]) && !snapid && inode->snap_ctime)
> -				break;
> -
> -			vdi_found = true;
> -			if (tag && tag[0] &&
> -			    strncmp(inode->tag, tag, sizeof(inode->tag)) != 0)
> -				continue;
> -			if (snapid) {
> -				if (snapid != inode->snap_id)
> +			sd_dprintf("%s = %s, %u = %u", iocb->tag, inode->tag,
> +				   iocb->snapid, inode->snap_id);
> +			if (has_tag(iocb)) {
> +				/* Read, delete, clone on snapshots */
> +				if (!is_snapshot(inode)) {
> +					vdi_found = true;
>  					continue;
> -
> -				if (inode->snap_ctime == 0) {
> -					sd_dprintf("vdi %" PRIx32 " is not a "
> -						   "snapshot\n", inode->vdi_id);
> -					ret = SD_RES_INVALID_PARMS;
> -					goto out_free_inode;
>  				}
> +				if (!tag_match(iocb, inode))
> +					continue;
> +			} else {
> +				/*
> +				 * Rollback & snap create, read, delete on
> +				 * current working VDI
> +				 */
> +				iocb->snapid = inode->snap_id + 1;
> +				if (is_snapshot(inode))
> +					/* return NO_VDI for rollback create */
> +					break;

The comment looks strange to me.  The current VDI must be the right
side, so we should break here for efficiency even if we don't support
vdi rollback.

Thanks,

Kazutaka