[sheepdog] [PATCH v2] sheep: handle a case of small buffer in cinfo collection

Hitoshi Mitake mitake.hitoshi at lab.ntt.co.jp
Mon Dec 15 05:54:02 CET 2014


At Thu, 11 Dec 2014 11:15:27 +0900,
Hitoshi Mitake wrote:
> 
> Current cinfo_collection_work() and SD_OP_VDI_STATE_SNAPSHOT_CTL
> doesn't handle a case of small buffer. This patch solves it.
> 
> Cc: Saeki Masaki <saeki.masaki at po.ntts.co.jp>
> Cc: Yuka Kawasaki <kawasaki.yuka at po.ntts.co.jp>
> Signed-off-by: Hitoshi Mitake <mitake.hitoshi at lab.ntt.co.jp>
> ---
>  sheep/group.c      | 56 +++++++++++++++++++++++++++++++++++++++---------------
>  sheep/ops.c        | 21 ++++++++++----------
>  sheep/sheep_priv.h |  3 ++-
>  sheep/vdi.c        | 24 ++++++++++++++++-------
>  4 files changed, 71 insertions(+), 33 deletions(-)
> 
> v2: calculate correct length of buffer

Sorry, this version also has a bug. I'll post v3 later.

Thanks,
Hitoshi

> 
> diff --git a/sheep/group.c b/sheep/group.c
> index 2b98a9b..e633bc3 100644
> --- a/sheep/group.c
> +++ b/sheep/group.c
> @@ -694,43 +694,69 @@ struct cinfo_collection_work {
>  
>  static struct cinfo_collection_work *collect_work;
>  
> -static void cinfo_collection_work(struct work *work)
> +static struct vdi_state *do_cinfo_collection_work(uint32_t epoch,
> +						  struct sd_node *n,
> +						  int *nr_vdi_states)
>  {
> +	struct vdi_state *vs = NULL;
> +	unsigned int rlen = 512;
>  	struct sd_req hdr;
>  	struct sd_rsp *rsp = (struct sd_rsp *)&hdr;
> +	int ret;
> +
> +	vs = xcalloc(rlen, sizeof(*vs));
> +
> +retry:
> +	sd_init_req(&hdr, SD_OP_VDI_STATE_SNAPSHOT_CTL);
> +	hdr.vdi_state_snapshot.get = 1;
> +	hdr.vdi_state_snapshot.tgt_epoch = epoch;
> +	hdr.data_length = rlen;
> +
> +	ret = sheep_exec_req(&n->nid, &hdr, (char *)vs);
> +	if (ret == SD_RES_SUCCESS) {
> +		sd_debug("succeed to obtain snapshot of vdi states");
> +		*nr_vdi_states = rsp->data_length / sizeof(*vs);
> +		return vs;
> +	} else if (ret == SD_RES_BUFFER_SMALL) {
> +		sd_debug("buffer is small for obtaining snapshot of vdi states,"
> +			 " doubling it (%lu -> %lu)", rlen * sizeof(*vs),
> +			 rlen * 2 * sizeof(*vs));
> +		rlen *= 2;
> +		vs = xrealloc(vs, sizeof(*vs) * rlen);
> +		goto retry;
> +	}
> +
> +	sd_err("failed to obtain snapshot of vdi states from node %s",
> +	       node_to_str(n));
> +	return NULL;
> +}
> +
> +static void cinfo_collection_work(struct work *work)
> +{
> +	struct sd_req hdr;
>  	struct vdi_state *vs = NULL;
> -	unsigned int rlen;
>  	struct cinfo_collection_work *w =
>  		container_of(work, struct cinfo_collection_work, work);
>  	struct sd_node *n;
> -	int ret;
> +	int ret, nr_vdi_states = 0;
>  
>  	sd_debug("start collection of cinfo...");
>  
>  	assert(w == collect_work);
>  
> -	rlen = SD_DATA_OBJ_SIZE; /* FIXME */
> -	vs = xzalloc(rlen);
> -
>  	rb_for_each_entry(n, &w->members->nroot, rb) {
>  		if (node_is_local(n))
>  			continue;
>  
> -		sd_init_req(&hdr, SD_OP_VDI_STATE_SNAPSHOT_CTL);
> -		hdr.vdi_state_snapshot.get = 1;
> -		hdr.vdi_state_snapshot.tgt_epoch = w->epoch;
> -		hdr.data_length = rlen;
> -
> -		ret = sheep_exec_req(&n->nid, &hdr, (char *)vs);
> -		if (ret == SD_RES_SUCCESS)
> +		vs = do_cinfo_collection_work(w->epoch, n, &nr_vdi_states);
> +		if (vs)
>  			goto get_succeed;
>  	}
>  
>  	panic("getting a snapshot of vdi state at epoch %d failed", w->epoch);
>  
>  get_succeed:
> -
> -	w->nr_vdi_states = rsp->data_length / sizeof(*vs);
> +	w->nr_vdi_states = nr_vdi_states;
>  	w->result = vs;
>  
>  	sd_debug("collecting cinfo done, freeing from remote nodes");
> diff --git a/sheep/ops.c b/sheep/ops.c
> index a617a83..3991060 100644
> --- a/sheep/ops.c
> +++ b/sheep/ops.c
> @@ -1391,21 +1391,22 @@ static int local_vdi_state_snapshot_ctl(const struct sd_req *req,
>  {
>  	bool get = !!req->vdi_state_snapshot.get;
>  	int epoch = req->vdi_state_snapshot.tgt_epoch;
> -	int ret;
> +	int ret, length = 0;
>  
>  	sd_info("%s vdi state snapshot at epoch %d",
>  		get ? "getting" : "freeing", epoch);
>  
>  	if (get) {
> -		/*
> -		 * FIXME: assuming request has enough space for storing
> -		 * the snapshot
> -		 */
> -		ret = get_vdi_state_snapshot(epoch, data);
> -		if (0 <= ret)
> -			rsp->data_length = ret;
> -		else
> -			return SD_RES_AGAIN;
> +		ret = get_vdi_state_snapshot(epoch, data, req->data_length,
> +					     &length);
> +		if (ret == SD_RES_SUCCESS)
> +			rsp->data_length = length;
> +		else {
> +			sd_err("failed to get vdi state snapshot: %s",
> +			       sd_strerror(ret));
> +
> +			return ret;
> +		}
>  	} else
>  		free_vdi_state_snapshot(epoch);
>  
> diff --git a/sheep/sheep_priv.h b/sheep/sheep_priv.h
> index 5fc6b90..72817b9 100644
> --- a/sheep/sheep_priv.h
> +++ b/sheep/sheep_priv.h
> @@ -344,7 +344,8 @@ bool vdi_lock(uint32_t vid, const struct node_id *owner, int type);
>  bool vdi_unlock(uint32_t vid, const struct node_id *owner, int type);
>  void apply_vdi_lock_state(struct vdi_state *vs);
>  void take_vdi_state_snapshot(int epoch);
> -int get_vdi_state_snapshot(int epoch, void *data);
> +int get_vdi_state_snapshot(int epoch, void *data, int data_len_max,
> +			   int *data_len_result);
>  void free_vdi_state_snapshot(int epoch);
>  void log_vdi_op_lock(uint32_t vid, const struct node_id *owner, int type);
>  void log_vdi_op_unlock(uint32_t vid, const struct node_id *owner, int type);
> diff --git a/sheep/vdi.c b/sheep/vdi.c
> index 1c8fb36..a85bbf5 100644
> --- a/sheep/vdi.c
> +++ b/sheep/vdi.c
> @@ -1793,21 +1793,31 @@ main_fn void take_vdi_state_snapshot(int epoch)
>  	sd_debug("a number of vdi state: %d", snapshot->nr_vs);
>  }
>  
> -main_fn int get_vdi_state_snapshot(int epoch, void *data)
> +main_fn int get_vdi_state_snapshot(int epoch, void *data, int data_len_max,
> +				   int *data_len_result)
>  {
>  	struct vdi_state_snapshot *snapshot;
> +	int len;
>  
>  	list_for_each_entry(snapshot, &vdi_state_snapshot_list, list) {
> -		if (snapshot->epoch == epoch) {
> -			memcpy(data, snapshot->vs,
> -			       sizeof(*snapshot->vs) * snapshot->nr_vs);
> -			return sizeof(*snapshot->vs) * snapshot->nr_vs;
> -		}
> +		if (snapshot->epoch == epoch)
> +			goto found;
>  	}
>  
>  	sd_info("get request for not prepared vdi state snapshot, epoch: %d",
>  		epoch);
> -	return -1;
> +	return SD_RES_AGAIN;
> +
> +found:
> +	len = sizeof(*snapshot->vs) * snapshot->nr_vs;
> +	if (data_len_max < len) {
> +		sd_info("maximum allowed length: %d, required length: %d",
> +			data_len_max, len);
> +		return SD_RES_BUFFER_SMALL;
> +	}
> +
> +	memcpy(data, snapshot->vs, len);
> +	return len;
>  }
>  
>  main_fn void free_vdi_state_snapshot(int epoch)
> -- 
> 1.8.3.2
> 



More information about the sheepdog mailing list